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

daemon: add ability to use syscall based reboot #250

Merged
merged 11 commits into from
Aug 11, 2023

Conversation

flotter
Copy link
Contributor

@flotter flotter commented Jun 29, 2023

Reboot in the daemon is currently relying on userspace having a shutdown command. This may not always be the case, as Pebble is used in a wide variety of environments.

Add a SetRebootMode() function to the daemon to allow the daemon to be initialised with a syscall based reboot implementation. The default reboot mode using systemd remains unchanged.

@flotter flotter requested review from anpep and benhoyt June 29, 2023 16:53
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
@flotter flotter force-pushed the syscall-reboot branch 3 times, most recently from 0d39fb2 to 0fddb08 Compare July 3, 2023 07:29
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks, Fred. Some notes and questions below.

internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
Reboot in the daemon is currently relying on userspace having a
shutdown command. This may not always be the case, as Pebble is
used in a wide variety of environments.

Add a SetSyscallReboot() function to the daemon to allow the daemon
to be initialised with a syscall based reboot implementation.
@flotter flotter added the High Priority Look at me first label Jul 6, 2023
@flotter flotter requested review from benhoyt and anpep July 6, 2023 14:03
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks fine overall to me, just a few really nitty comments.

internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Looks good, thank you. Just some last nitpicks for your consideration:

internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/daemon_test.go Outdated Show resolved Hide resolved
@flotter flotter requested review from atesburak and removed request for atesburak August 10, 2023 16:44
@canonical canonical deleted a comment from atesburak Aug 11, 2023
@flotter flotter removed the request for review from atesburak August 11, 2023 06:44
@flotter flotter requested a review from anpep August 11, 2023 07:05
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Perfect, thanks Fred!

@niemeyer niemeyer merged commit 423dba7 into canonical:master Aug 11, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants