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.

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

Add reboot option to DNF Automatic #1879

Merged
merged 4 commits into from
Jan 26, 2023
Merged

Add reboot option to DNF Automatic #1879

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cafcd57
Add reboot option to DNF Automatic (RhBug:2124793)
evan-goode Jan 24, 2023
098de60
Add Evan Goode to AUTHORS
evan-goode Jan 24, 2023
9f83e90
DNF Automatic reboot: 5-minute delay and wall by default
evan-goode Jan 24, 2023
8d63b97
DNF Automatic: error message for failed reboot command
evan-goode Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions dnf/automatic/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import argparse
import logging
import os
import random
import socket
import time
Expand Down Expand Up @@ -179,6 +180,9 @@ def __init__(self):
libdnf.conf.VectorString(['default', 'security'])))
self.add_option('random_sleep', libdnf.conf.OptionNumberInt32(300))
self.add_option('network_online_timeout', libdnf.conf.OptionNumberInt32(60))
self.add_option('reboot', libdnf.conf.OptionEnumString('never',
libdnf.conf.VectorString(['never', 'when-changed', 'when-needed'])))
self.add_option('reboot_command', libdnf.conf.OptionString('shutdown -r'))

def imply(self):
if self.apply_updates:
Expand Down Expand Up @@ -340,6 +344,11 @@ def main(args):
base.do_transaction()
emitters.notify_applied()
emitters.commit()

if (conf.commands.reboot == 'when-changed' or
(conf.commands.reboot == 'when-needed' and base.reboot_needed())):
if os.waitstatus_to_exitcode(os.system(conf.commands.reboot_command)) != 0:
return 1
evan-goode marked this conversation as resolved.
Show resolved Hide resolved
except dnf.exceptions.Error as exc:
logger.error(_('Error: %s'), ucd(exc))
return 1
Expand Down
14 changes: 14 additions & 0 deletions dnf/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2794,6 +2794,20 @@ def _nevra(item):

return skipped_conflicts, skipped_dependency

def reboot_needed(self):
"""Check whether a system reboot is recommended following the transaction

:return: bool
"""
if not self.transaction:
return False

# List taken from DNF needs-restarting
need_reboot = frozenset(('kernel', 'kernel-rt', 'glibc',
'linux-firmware', 'systemd', 'dbus',
'dbus-broker', 'dbus-daemon'))
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to share this set with needs-restarting plugin somehow.
One option would be a new config option. In this case it should be implemented also for dnf5.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm true, would doing this via a new config option require a change in libdnf?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if we agree that it is useful, it will be spread across libdnf, dnf, dnf-plugins-core, and dnf5 :)
But I'm not gonna block this PR because of that. It could be done in subsequent patches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think we can hold off for now and err on the side of minimal changes. Hopefully this list doesn't change too much over the years. But it would be good for it to be configurable by the user.

changed_pkgs = self.transaction.install_set | self.transaction.remove_set
return any(pkg.name in need_reboot for pkg in changed_pkgs)

def _msg_installed(pkg):
name = ucd(pkg)
Expand Down
12 changes: 12 additions & 0 deletions doc/automatic.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,18 @@ Setting the mode of operation of the program.

What kind of upgrades to look at. ``default`` signals looking for all available updates, ``security`` only those with an issued security advisory.

``reboot``
either one of ``never``, ``when-changed``, ``when-needed``, default: ``never``

When the system should reboot following upgrades. ``never`` does not reboot the system. ``when-changed`` triggers a reboot after any upgrade. ``when-needed`` triggers a reboot only when rebooting is necessary to apply changes, such as when systemd or the kernel is upgraded.

``reboot_command``
string, default: ``shutdown -r``

Specify the command to run to trigger a reboot of the system. For example, add a 5-minute delay and a wall message by using ``shutdown -r +5 'Rebooting after upgrading packages'``



----------------------
``[emitters]`` section
----------------------
Expand Down
9 changes: 9 additions & 0 deletions etc/dnf/automatic.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ download_updates = yes
# install.timer override this setting.
apply_updates = no

# When the system should reboot following upgrades:
# never = don't reboot after upgrades
# when-changed = reboot after any changes
# when-needed = reboot when necessary to apply changes
reboot = never

# The command that is run to trigger a system reboot.
reboot_command = "shutdown -r"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was testing your code today, and I was thinking whether our ci is suitable or not for testing what you want. Probably, there is no simple way to trigger a reboot, but there are other options.
Since you set a custom command here, I would think of another way to test the reboot trigger.

Maybe, during behave tests, you could set an arbitrary command in etc/dnf/automatic.conf. Let's say that instead of shutdown -r you touch some file, then you can check if the file exists and have the trigger tested. Unfortunately this is the only partial solution I came up with.

Copy link

Choose a reason for hiding this comment

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

FTR: Cockpit's updates page delays the shutdown by a bit, so that logged in users get a warning and have some time to orderly stop their work. That's a good thing to do on servers IMHO.

That same thing also allows you to test this feature relatively painlessly: It's enough time to validate that a shutdown was scheduled and cancel it again (like in our tests). That avoids the costly (time) or impossible (Testing Farm/gating tests) actual reboot.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR: Cockpit's updates page delays the shutdown by a bit, so that logged in users get a warning and have some time to orderly stop their work. That's a good thing to do on servers IMHO

That's a good idea.

That same thing also allows you to test this feature relatively painlessly

That does not apply for dnf, since we run our tests in containers. But you cannot really test if a system reboots universally. Say that a system does not have the shutdown command, then the only thing you can test is to check if the hook triggers. It would be the user/sysadmin/distro responsibility to configure the conf file accordingly.

Said that, it's not a big deal imho since we are not testing the capability of shutdown -r of rebooting the system.

It's enough time to validate that a shutdown was scheduled and cancel it again

I think this would be the solution in most cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

FTR: Cockpit's updates page delays the shutdown by a bit, so that logged in users get a warning and have some time to orderly stop their work. That's a good thing to do on servers IMHO.

Thanks for the input, I've made the default reboot_command add a 5-minute delay and wall message.

@inknos how does rpm-software-management/ci-dnf-stack#1210 look? I set the reboot_command to echo a certain string, then check that stdout contains that string. Maybe touching a file is a more reliable way to make sure the command is actually run, but I think this should be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the stoud approach, I will post some comments about the CI PR and then I believe this is good to go. Thank you



[emitters]
# Name to use for this system in messages that are emitted. Default is the
Expand Down
4 changes: 4 additions & 0 deletions tests/automatic/test_main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,7 @@ def test_load(self):
conf = dnf.automatic.main.AutomaticConfig(FILE, downloadupdates=True, installupdates=False)
self.assertTrue(conf.commands.download_updates)
self.assertFalse(conf.commands.apply_updates)

# test that reboot is "never" by default
conf = dnf.automatic.main.AutomaticConfig(FILE)
self.assertEqual(conf.commands.reboot, 'never')