Implement dnf5 needs-restarting plugin #887

evan-goode · 2023-09-15T21:14:43Z

I'd like the DNF 5 needs-restarting command to behave a bit different from dnf4 needs-restarting. Here's what I propose:

dnf5 needs-restarting (no flag): I'd like this to simply report whether the system needs to be restarted. If there are "core packages" (such as kernel or glibc) that have been updated since boot, or if any advisory with the reboot_suggested flag applies to any of the packages that have been updated/installed since boot, dnf5 needs-restarting will list these packages and exit with code 1 to indicate a reboot is needed. Otherwise, it will exit with code 0. This proposed behavior is the same as the current behavior of dnf4 needs-restarting -r, but it would properly honor the reboot_suggested advisory rather than simply checking the hardcoded list of "core packages".

I think this should be the default behavior of needs-restarting (rather than being behind a flag) because it's probably the most common use case. Here's an example of a user running needs-restarting when they needed needs-restarting -r: https://bugzilla.redhat.com/show_bug.cgi?id=2237223.
Regarding the behavior of dnf4 needs-restarting (no flag): currently, this command lists processes with open outdated files and recommends the user restart them. I think we should leave this behavior out of dnf5 needs-restarting for two reasons. First, tracer and dnf-plugins-extras-tracer implement this approach well, and we don't need to duplicate their functionality. Second, this whole strategy of looking for open files is a bit of a hack and IMO shouldn't be included in the core DNF plugins. It might work most of the time, but there are nuances and edge cases here, e.g. when a process maps a library without opening it: https://bugzilla.redhat.com/show_bug.cgi?id=1131307, or if, hypothetically, a process reads a file into memory, closes it, then encrypts the copy in memory. There's just no way to tell that the process is "using" that outdated file.
dnf5 needs-restarting -s: I'd like this to list systemd services that need to be restarted, but rather than check whether the service has any open outdated files, check the dependencies of the package that provides the service. I haven't implemented this yet in this PR, but here's an outline:

for each running systemd $service
    find the $package that provides the unit file for $service, if any
        recursively get all $dependencies of $package
        if $package or any $dependency has been updated since $service started
            recommend restart of $service

For #743

Resolves #587

ppisar · 2023-09-18T09:52:16Z

dnf5 needs-restarting will list these packages and exit with code 1 to indicate a reboot is needed.

Would it be possible the reverse the exit codes? Now the codes match to "does-not-need-restart" question.

evan-goode · 2023-09-19T03:44:21Z

dnf5 needs-restarting will list these packages and exit with code 1 to indicate a reboot is needed.

Would it be possible the reverse the exit codes? Now the codes match to "does-not-need-restart" question.

Hmm, yes using code 0 (truthy) to mean "no restart" does make scripts read a bit awkward, I see your point:

if dnf5 needs-restarting; then
  echo The system DOES NOT need a reboot
fi

Similar tools like tracer, Debian's checkrestart, and dnf4 needs-restarting use these exit codes though, so I think still it would be best to keep consistent with them. checkrestart mentions the exit code may be consumed by monitoring tools like Nagios, where the convention is "exit zero is good".

I think of "no reboot needed" as the default, unsurprising, comfortable result and "reboot needed" as the less common result that needs to be handled. The exit codes make more sense from that perspective.

For rpm-software-management#743 Resolves rpm-software-management#587

Adjust list of core packages to include everything from https://access.redhat.com/solutions/27943

The new behavior of `dnf5 needs-restarting --services` still lists systemd services that should be restarted following an upgrade, but instead of checking open files like dnf4 needs-restarting, it checks whether any dependency of the package that provides the service has been updated since the service started.

dnf4 needs-restarting's logic to get the boot time doesn't work when the RTC exists but gives unreliable information (e.g. it's not in UTC). When systemd is available, we should just ask systemd when the system booted. When systemd is not available, we are likely in a container, and stat /proc/1 will probably yield a correct answer.

evan-goode · 2023-11-01T21:38:57Z

Alright, I think this is ready for review now.

dnf5-plugins/needs_restarting_plugin/CMakeLists.txt

dnf5-plugins/needs_restarting_plugin/needs_restarting.cpp

libdnf5/rpm/package_query.cpp

doc/dnf5_plugins/index.rst

dnf5-plugins/needs_restarting_plugin/CMakeLists.txt

dnf5-plugins/needs_restarting_plugin/needs_restarting.cpp

include/libdnf5/rpm/package_query.hpp

libdnf5/rpm/package_query.cpp

jan-kolarik · 2023-11-07T08:31:23Z

LGTM, thanks for the cooperation! 🙂

One more thing: as already mentioned, the existing tests in the CI stack do not cover all functionalities provided by this new DNF5 version of the needs-restarting plugin, particularly regarding the reboot suggested packages. I believe the updateinfo metadata can be mocked for this purpose, allowing us to create testing data and write the necessary tests using the current state of the CI stack.

Please create a follow-up issue in the CI stack to ensure we address this.

jan-kolarik · 2023-11-07T08:49:09Z

Oh, I just realized that the existing tests aren't being executed for dnf5 at the moment. We should attempt to reuse at least some of them.

Update the needs-restarting tests for DNF 5 and add a test for recommending a reboot when a package has an associated reboot_suggested advisory. Requires rpm-software-management/dnf5#887 For rpm-software-management/dnf5#743

evan-goode · 2023-11-08T22:23:32Z

CI PR: rpm-software-management/ci-dnf-stack#1403

kontura

Just a couple minor typos, otherwise LGTM.

Also I believe the changes in rpm/nevra.hpp are technically not longer needed but we can keep them if you think they might be useful in the future.

doc/changes.rst

dnf5-plugins/needs_restarting_plugin/needs_restarting.cpp

filter_reboot_suggested was binary searching the results of get_advisory_packages_sorted_by_name_arch_evr() assuming they were sorted according to cmp_naevr, however that function actually returns packages sorted by the libsolv ids of name, arch, and evr, not the string representations of name, arch, and evr like cmp_naevr does.

evan-goode · 2023-11-09T13:31:55Z

Just a couple minor typos, otherwise LGTM.

Also I believe the changes in rpm/nevra.hpp are technically not longer needed but we can keep them if you think they might be useful in the future.

True, I think we might as well keep them.

jan-kolarik · 2023-11-09T14:10:42Z

I just noticed the tmp commit present in the PR, probably an oversight... 🙂

DNF 4 needs-restarting -r has essentially the same behavior as DNF 5 needs-restarting with no flag, so the flag should be kept in as a no-op for backwards compatibility.

evan-goode · 2023-11-09T14:13:22Z

I just noticed the tmp commit present in the PR, probably an oversight... 🙂

Sigh... thank you. One more force push...

jan-kolarik · 2023-11-09T14:18:42Z

I just noticed the tmp commit present in the PR, probably an oversight... 🙂

Sigh... thank you. One more force push...

Great and thank you for your patience with such an exhaustive review 🙂 I believe we can proceed with the merge now, as Ales mentioned only minor typos.

Update the needs-restarting tests for DNF 5 and add a test for recommending a reboot when a package has an associated reboot_suggested advisory. Requires rpm-software-management/dnf5#887 For rpm-software-management/dnf5#743

evan-goode self-assigned this Sep 15, 2023

evan-goode added 2 commits October 18, 2023 20:40

Initial implementation of needs-restarting

56ee44e

For rpm-software-management#743 Resolves rpm-software-management#587

needs-restarting: Adjust list of core packages

78b976d

Adjust list of core packages to include everything from https://access.redhat.com/solutions/27943

evan-goode force-pushed the evan-goode/needs-restarting branch from cd8fddd to 0088b87 Compare October 18, 2023 20:40

evan-goode force-pushed the evan-goode/needs-restarting branch from cd366af to 29f83e5 Compare November 1, 2023 20:14

evan-goode added 3 commits November 1, 2023 21:03

doc: document needs-restarting plugin

5bef3f3

evan-goode force-pushed the evan-goode/needs-restarting branch from 29f83e5 to 167c0e9 Compare November 1, 2023 21:34

evan-goode marked this pull request as ready for review November 1, 2023 21:34

jan-kolarik assigned jan-kolarik and unassigned evan-goode Nov 2, 2023

jan-kolarik requested changes Nov 2, 2023

View reviewed changes

jan-kolarik reviewed Nov 3, 2023

View reviewed changes

dnf5-plugins/needs_restarting_plugin/needs_restarting.cpp Outdated Show resolved Hide resolved

jan-kolarik reviewed Nov 3, 2023

View reviewed changes

include/libdnf5/rpm/package_query.hpp Outdated Show resolved Hide resolved

evan-goode force-pushed the evan-goode/needs-restarting branch from 0cd91ea to b11d6c9 Compare November 3, 2023 17:22

evan-goode requested a review from jan-kolarik November 3, 2023 19:21

kontura requested changes Nov 6, 2023

View reviewed changes

libdnf5/rpm/package_query.cpp Outdated Show resolved Hide resolved

jan-kolarik approved these changes Nov 7, 2023

View reviewed changes

evan-goode mentioned this pull request Nov 8, 2023

dnf5: Enable needs-restarting tests rpm-software-management/ci-dnf-stack#1403

Merged

evan-goode requested a review from kontura November 8, 2023 22:23

kontura reviewed Nov 9, 2023

View reviewed changes

doc/changes.rst Outdated Show resolved Hide resolved

dnf5-plugins/needs_restarting_plugin/needs_restarting.cpp Show resolved Hide resolved

changes.rst: document changes to needs-restarting

474466f

evan-goode added 3 commits November 9, 2023 13:31

needs-restarting: docstring for filter_reboot_suggested

f8d3494

needs-restarting: Fixes and simplifications from code review

8cf7750

evan-goode force-pushed the evan-goode/needs-restarting branch from 48a643f to 4f459d8 Compare November 9, 2023 13:33

evan-goode requested a review from kontura November 9, 2023 13:33

needs-restarting: Add no-op -r for DNF 4 compat

1c0e75f

DNF 4 needs-restarting -r has essentially the same behavior as DNF 5 needs-restarting with no flag, so the flag should be kept in as a no-op for backwards compatibility.

evan-goode force-pushed the evan-goode/needs-restarting branch from 4f459d8 to 1c0e75f Compare November 9, 2023 14:12

jan-kolarik added this pull request to the merge queue Nov 9, 2023

Merged via the queue into rpm-software-management:main with commit 419fed5 Nov 9, 2023
7 checks passed

evan-goode mentioned this pull request Sep 25, 2024

needs-restarting should be able to list non-systemd processes that need restarting #1730

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dnf5 needs-restarting plugin #887

Implement dnf5 needs-restarting plugin #887

evan-goode commented Sep 15, 2023

ppisar commented Sep 18, 2023

evan-goode commented Sep 19, 2023

evan-goode commented Nov 1, 2023

jan-kolarik commented Nov 7, 2023

jan-kolarik commented Nov 7, 2023

evan-goode commented Nov 8, 2023

kontura left a comment

evan-goode commented Nov 9, 2023

jan-kolarik commented Nov 9, 2023

evan-goode commented Nov 9, 2023

jan-kolarik commented Nov 9, 2023

Implement dnf5 needs-restarting plugin #887

Implement dnf5 needs-restarting plugin #887

Conversation

evan-goode commented Sep 15, 2023

ppisar commented Sep 18, 2023

evan-goode commented Sep 19, 2023

evan-goode commented Nov 1, 2023

jan-kolarik commented Nov 7, 2023

jan-kolarik commented Nov 7, 2023

evan-goode commented Nov 8, 2023

kontura left a comment

Choose a reason for hiding this comment

evan-goode commented Nov 9, 2023

jan-kolarik commented Nov 9, 2023

evan-goode commented Nov 9, 2023

jan-kolarik commented Nov 9, 2023