Skip to content
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.

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

needs-restarting: Get boot time from systemd UnitsLoadStartTimestamp #560

Conversation

evan-goode
Copy link
Member

Resolves https://issues.redhat.com/browse/RHEL-35577.

Get the boot time from UnitsLoadStartTimestamp if systemd is available, but make sure to use the kernel boot time for calculating process start times using data from procfs. The previous attempt [1] at this failed to do so and introduced a regression [2].

Also, get the kernel boot time from the btime field of /proc/stat instead of calculating it from /proc/uptime, to be consistent with what procps-ng does.

[1] #527
[2] https://issues.redhat.com/browse/RHEL-39775

@pep8speaks
Copy link

pep8speaks commented Oct 7, 2024

Hello @evan-goode! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-10-11 22:41:01 UTC

@evan-goode evan-goode force-pushed the evan-goode/needs-restarting-boot-time branch from 148048a to 6391132 Compare October 7, 2024 21:39
@evan-goode
Copy link
Member Author

@bajertom said:

https://issues.redhat.com/browse/RHEL-14900 fails with current dnf-plugins-core.
https://issues.redhat.com/browse/RHEL-39775 passes with the current dnf-plugins-core.
I have rebuild dnf-plugins-core with the patch from PR#560 and installed it on a system and then both test cases PASS, therefore no regression is introduced.
I have also tried it on RHEL-10 and the result is the same. (Actually the latter fails on an --exclude-services option being unknown to needs-restarting command, however I see that it is going to get implemented by you 🙂 )

Marking this as RFR. @ppisar I'll request your review since you investigated the regression last time :)

@evan-goode evan-goode marked this pull request as ready for review October 9, 2024 22:11
@evan-goode evan-goode requested a review from ppisar October 9, 2024 22:11
@ppisar ppisar self-assigned this Oct 11, 2024
Copy link
Contributor

@ppisar ppisar left a comment

Choose a reason for hiding this comment

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

Otherwise, the code looks good. I will try to do some tests.

plugins/needs_restarting.py Outdated Show resolved Hide resolved
plugins/needs_restarting.py Outdated Show resolved Hide resolved
@ppisar
Copy link
Contributor

ppisar commented Oct 11, 2024

Manual testing did not find any problem.

@evan-goode evan-goode force-pushed the evan-goode/needs-restarting-boot-time branch from 6391132 to fb8c919 Compare October 11, 2024 22:35
Resolves https://issues.redhat.com/browse/RHEL-35577.

Get the boot time from UnitsLoadStartTimestamp if systemd is available,
but make sure to use the kernel boot time for calculating process start
times using data from procfs. The previous attempt [1] at this failed to
do so and introduced a regression [2].

Also, get the kernel boot time from the btime field of /proc/stat instead of
calculating it from /proc/uptime, to be consistent with what procps-ng
does.

[1] rpm-software-management#527
[2] https://issues.redhat.com/browse/RHEL-39775
@evan-goode evan-goode force-pushed the evan-goode/needs-restarting-boot-time branch from fb8c919 to 17b38c8 Compare October 11, 2024 22:40
@evan-goode evan-goode requested a review from ppisar October 11, 2024 22:42
Copy link
Contributor

@ppisar ppisar left a comment

Choose a reason for hiding this comment

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

The Ansible test failure (mismatched libstdc++.so.6(CXXABI_1.3.15)) is unrelated.

@ppisar ppisar merged commit f685c89 into rpm-software-management:master Oct 14, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants