[RFC] shutdown/notify: allow for custom implementations #243

flotter · 2023-06-21T15:13:28Z

The current shutdown and notify logic in pebble is designed with the
assumption that it will run under systemd, or at least that some
systemd components, such as shutdown, is installed.

Allow custom flavours of pebble to change the default assumptions.

The daemon now defines a notification and shutdown interface, and
configures the default implementation to use systemd.
The shutdown logic (of which we currently only support reboot) is
moved to the systemd package, as its based on systemd.
Two globals daemon.Notify and daemon.Shutdown now provides a way
to override the defaults.

This is an alternative approach to what we have in #197

An hypothetical derivative project based on Pebble can now supply its own shutdown and notification implementations before starting the daemon:

daemon.Notify = custom.Notifier 
daemon.Shutdown = custom.Shutdown
:
d, err := daemon.New(&dopts)
:
d.Start()

FakeCommand allows unit tests to check which command-line arguments were passed, by recording a log of calls with their arguments. The following examples do not work: 1. shutdown -r "+0" "" 2. echo "" The last argument supplies the wall message, and an empty argument is valid. Having this support in place allows code to simply pass an empty argument for exec.Command(...). The issue is that both the argument and command delimiter uses null characters, which makes the command split operation split on the first occurrence of two null chars, which breaks the examples above. To add support for this: - Change the command delimiter from \000\000 => \000\f\n\r - Update the Calls() function to split the lines correctly.

The current shutdown and notify logic in pebble is designed with the assumption that it will run under systemd, or at least that some systemd components, such as shutdown, is installed. Allow custom flavours of pebble to change the default assumptions. - The daemon now defines an notification and shutdown interface, and configures the default implementation to use systemd. - The shutdown logic (of which we currently only support reboot) is moved to the systemd package, as its based on systemd. - Two globals daemon.Notify and daemon.Shutdown now provides a way to override the defaults.

anpep

Really clean approach!
Since I've seen pebble have platform-specific code, maybe we could consider moving internals/systemd/{notify,shutdown}.go -> internals/systemd/{notify,shutdown}_linux.go, then have _darwin.go return ErrDarwin.

flotter · 2023-06-21T15:17:35Z

internals/daemon/daemon.go

@@ -717,22 +742,6 @@ func (d *Daemon) doReboot(sigCh chan<- os.Signal, waitTimeout time.Duration) err
 	return fmt.Errorf("expected reboot did not happen")
 }

-var shutdownMsg = "reboot scheduled to update the system"


This has been moved to internals/systemd/shutdown.go. Note that I am reusing the shutdown command group name here, where the command offers halt, power-off and reboot functionality.

flotter · 2023-06-21T15:18:32Z

internals/daemon/daemon_test.go

@@ -724,32 +741,6 @@ func (s *daemonSuite) TestRestartSystemWiring(c *check.C) {
 	c.Check(rebootAt.After(approxAt) || rebootAt.Equal(approxAt), check.Equals, true)
 }

-func (s *daemonSuite) TestRebootHelper(c *check.C) {


This was moved to internals/systemd/shutdown_test.go.

flotter · 2023-06-21T15:21:20Z

internals/daemon/daemon_test.go

-	notifySocket := filepath.Join(c.MkDir(), "notify.socket")
-	os.Setenv("NOTIFY_SOCKET", notifySocket)
-	defer os.Setenv("NOTIFY_SOCKET", "")
-


The folllowing blocks of removals are deliberate, as they add very little value here apart for re-testing the systemd/notify.go functionalty. The exact same tests can already be found under systemd/notify_test.go and removing it here solves the problem that I want to use a mocked notifier when testing this package, not the real notifier as is required by these code snippets.

flotter · 2023-06-21T15:21:35Z

internals/daemon/daemon_test.go

-	c.Assert(d.standbyOpinions.CanStandby(), check.Equals, false)
-	f, _ := os.Create(notifySocket)
-	f.Close()


See comment above.

flotter · 2023-06-21T15:21:45Z

internals/daemon/daemon_test.go

-	os.Setenv("NOTIFY_SOCKET", c.MkDir())
-	defer os.Setenv("NOTIFY_SOCKET", "")
-


See comment above.

benhoyt · 2023-06-22T03:01:31Z

internals/cli/cmd_run.go

@@ -114,7 +113,7 @@ func runWatchdog(d *daemon.Daemon) (*time.Ticker, error) {
 			case <-wt.C:
 				// TODO: poke the API here and only report WATCHDOG=1 if it
 				//       replies with valid data.
-				systemd.SdNotify("WATCHDOG=1")
+				daemon.Notify.Send("WATCHDOG=1")


I'm not really sure what WATCHDOG=1 does, but it seems like it's pretty specific to systemd? In that case, could it not be a string in the Send interface, but just included in the systemd implementation? Or something like Send(daemon.SendWatchdog) and Send(daemon.SendReady) (not the best API, but spitballing here).

benhoyt · 2023-06-22T03:02:09Z

internals/daemon/daemon.go

+
+// shutdown defines the required API for reboot, poweroff
+// and halt control of the underlying system
+type shutdown interface {


Because external packages need to implement these interfaces (and might want to pass around the type), shouldn't they be exported/capitalised?

benhoyt · 2023-06-22T03:13:22Z

internals/daemon/daemon.go

+
+	// Shutdown provides the system specific functionality to
+	// halt, poweroff or reboot.
+	Shutdown shutdown = systemd.Shutdown


It seems really unfortunate that these need to be globals when the Daemon is created via the more typical daemon.New pattern (that is, not a global). Can these somehow be part of the Options struct and not be globals of the daemon package? But I guess the tricky part is then having the alternative CLI customize it.

One approach would be to add Notify and Shutdown to daemon.Options, but then have a ConfigureDaemon that's part of the CLI's "personality". The signature would be ConfigureDaemon func(opts *daemon.Options), and the alternative CLI would do something like:

cmd.Personality.ConfigureDaemon = func(opts *daemon.Options) { opts.Notify = mySpecialNotify opts.Shutdown = mySpecialShutdown }

I dunno, maybe it's not any better, but at least it would confine the messy globals to one place, the "personality" struct. Or we could put the globals in the internals/cli package -- putting them in the CLI still seems better to me than in the daemon package.

I like the idea of confinement but is this really a part of the Personality, I can see this behaviour growing with other options, so a new struct to embody them?

I don't think this is part of Personality, because (i) it does not concern other components apart from the daemon, and (ii) Termus will have its own initialization of the daemon during bootstrap.

I'm on board @benhoyt's idea of having those be inside daemon.Options 😃

flotter · 2023-06-22T12:19:22Z

Discussed with Gustavo.

One big issue is that even trying to move systemd out of the daemon, by refactoring shutdown and notify, lots of systemd assumptions remain, including the notification messages (as also pointed out by @benhoyt).

Gustavo helped me define a simpler approach. Thank you for everyone's review time. New PR to follow ...

flotter added 2 commits June 21, 2023 16:51

flotter requested review from anpep, benhoyt, barrettj12, atesburak and niemeyer June 21, 2023 15:27

anpep approved these changes Jun 21, 2023

View reviewed changes

flotter commented Jun 21, 2023

View reviewed changes

benhoyt reviewed Jun 22, 2023

View reviewed changes

flotter closed this Jun 22, 2023

flotter reopened this Jun 22, 2023

flotter closed this Jun 22, 2023

flotter mentioned this pull request Jun 22, 2023

testutil: support empty string args in fakecommand #244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] shutdown/notify: allow for custom implementations #243

[RFC] shutdown/notify: allow for custom implementations #243

flotter commented Jun 21, 2023 •

edited

Loading

anpep left a comment

flotter Jun 21, 2023 •

edited

Loading

flotter Jun 21, 2023

flotter Jun 21, 2023 •

edited

Loading

flotter Jun 21, 2023

flotter Jun 21, 2023

benhoyt Jun 22, 2023

benhoyt Jun 22, 2023

benhoyt Jun 22, 2023

atesburak Jun 22, 2023

anpep Jun 22, 2023

flotter commented Jun 22, 2023

		os.Setenv("NOTIFY_SOCKET", c.MkDir())
		defer os.Setenv("NOTIFY_SOCKET", "")

[RFC] shutdown/notify: allow for custom implementations #243

[RFC] shutdown/notify: allow for custom implementations #243

Conversation

flotter commented Jun 21, 2023 • edited Loading

anpep left a comment

Choose a reason for hiding this comment

flotter Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotter Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotter commented Jun 22, 2023

flotter commented Jun 21, 2023 •

edited

Loading

flotter Jun 21, 2023 •

edited

Loading

flotter Jun 21, 2023 •

edited

Loading