8000 Pr/fix uptime plugin boottime by marcin1j · Pull Request #2034 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Pr/fix uptime plugin boottime #2034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 27, 2017
Merged

Pr/fix uptime plugin boottime #2034

merged 1 commit into from
Sep 27, 2017

Conversation

marcin1j
Copy link
Contributor

Original uptime plugin implementation is susceptible to system date changes, which is likely to happen on embedded system without RTC.
The cause of the problem is a combination of two factors:

  1. the method used to calculate uptime (subtracting boottime from current time),
  2. reading boottime only once, during plugin initialization phase.

This set of commits changes the implementation in the following way:

  1. On Linux and AIX, uptime is read directly using a system call. A side effect is a simpler and faster code (no need to parse /proc filesystem, no double time() system call).
  2. On BSD and Solaris, uptime is calculated using original method (subtracting boottime from current time).

@rubenk
Copy link
Contributor
rubenk commented Nov 19, 2016

This breaks on Solaris:

uptime.c: In function ‘uptime_get_sys’:
uptime.c:88:11: error: implicit declaration of function ‘sysinfo’ [-Werror=implicit-function-declaration]
  status = sysinfo (&info);
           ^
uptime.c:97:24: error: ‘struct sysinfo’ has no member named ‘uptime’
  result = (time_t) info.uptime;
                        ^

@marcin1j marcin1j force-pushed the pr/fix-uptime-plugin-boottime branch from 56ee8be to 41d4647 Compare November 19, 2016 23:39
@marcin1j
Copy link
Contributor Author

rubenk,

I've changed last commit to depend on KERNEL_LINUX and avoid conflict with Solaris sys/sysinfo.h. Could you confirm this solves the problem you mentioned?

@marcin1j marcin1j force-pushed the pr/fix-uptime-plugin-boottime branch 7 times, most recently from d2a98fb to b5ce223 Compare July 11, 2017 13:20
@marcin1j
Copy link
Contributor Author

Please consider merging this commit. It's been waiting for almost a year. Since then, the source code was reformatted twice forcing me to update patch (I use this in a couple of production instances).

As for now, the PR is up-to-date with master branch.

@marcin1j marcin1j force-pushed the pr/fix-uptime-plugin-boottime branch from b5ce223 to af01dd6 Compare July 11, 2017 14:19
@hnyman hnyman mentioned this pull request Sep 14, 2017
Closed
hnyman added a commit to hnyman/packages that referenced this pull request Sep 14, 2017
@hnyman
collectd: uptime plugin: apply fix from upstream
63a45e6 
Uptime plugin fails to adjust for system time changes after boot.
As Openwrt/LEDE routers usually do not have a RTC, the system time
gets adjusted with NTP possibly after collectd has already started.
But collectd continues to use the initial time set by 'sysfixtime',
which can lead to incorrect uptime calculations.

Apply a proposed fix from upstream that uses /proc/uptime
Reference to collectd/collectd#2034

Fixes openwrt#4819

Signed-off-by: Hannu Nyman <hannu.nyman@iki.fi>
@rafwes
Copy link
rafwes commented Sep 24, 2017 •
edited
Loading

@octo @zerkms

This bug is causing issues in lede/openwrt, why is the fix not being picked up upstream?

@zerkms
Copy link
Contributor
zerkms commented Sep 24, 2017

I'm just a couple-times contributor here without write permissions 🤷

octo
octo approved these changes Sep 24, 2017
View reviewed changes
Copy link
Member
@octo octo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcin1j,

thank you very much for your patch. I'm sorry that it didn't get attention earlier; there's too much to do and too little time and we're all volunteers.

The code LGTM. Please fix the merge conflict, preferably by re-basing on master, then this is good to go.

Best regards,
—octo

@octo octo added the Bug A genuine bug label Sep 24, 2017
@marcin1j
Copy link
Contributor Author

@octo
I'll update it again (and hope this is the last time) but it needs to be tested first so give me a couple of hours.

@marcin1j
uptime plugin: don't cache boot time and 8000 simplify Linux code
4943510 
Caching boottime on startup yields incorrect uptime values if system
date changes after the daemon is started.
This is almost certain to happen on embedded systems without RTC, where
clock is set from NTP server at some point after boot process.

On Linux, we can retrieve uptime directly by either reading /proc/uptime
(it's sufficient to read a few bytes) or calling sysinfo() function.
Use the latter since it's the most efficient way in speed, memory
requirements and code simplicity terms.
@marcin1j marcin1j force-pushed the pr/fix-uptime-plugin-boottime branch from af01dd6 to 4943510 Compare September 25, 2017 10:30
@octo
Copy link
Member
octo commented Sep 25, 2017

@marcin1j Certainly, just let me know when you're ready.

@marcin1j
Copy link
Contributor Author

I've tested it. Everything seems OK.

@octo octo merged commit 4943510 into collectd:master Sep 27, 2017
@octo
Copy link
Member
octo commented Sep 27, 2017

Thanks @marcin1j!

@octo octo added this to the 5.8 milestone Oct 11, 2017
@augustollenz augustollenz mentioned this pull request Jan 7, 2018
Closed
salzmdan pushed a commit to salzmdan/packages that referenced this pull request Jan 8, 2018
@hnyman @salzmdan
collectd: uptime plugin: apply fix from upstream
98720c2 
Uptime plugin fails to adjust for system time changes after boot.
As Openwrt/LEDE routers usually do not have a RTC, the system time
gets adjusted with NTP possibly after collectd has already started.
But collectd continues to use the initial time set by 'sysfixtime',
which can lead to incorrect uptime calculations.

Apply a proposed fix from upstream that uses /proc/uptime
Reference to collectd/collectd#2034

Fixes openwrt#4819

Signed-off-by: Hannu Nyman <hannu.nyman@iki.fi>
@marcin1j marcin1j deleted the pr/fix-uptime-plugin-boottime branch November 17, 2020 13:18
@octo octo added Fix A pull request fixing a bug and removed Bug A genuine bug labels Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@octo octo octo approved these changes

Assignees
No one assigned
Labels
Fix A pull request fixing a bug Pending feedback
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
5 participants
@marcin1j @rubenk @rafwes @zerkms @octo
0