From 43a5daff525d035d81fe2ca57b36e8fa609f9ee0 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 21 Jun 2023 16:50:46 +0200 Subject: [PATCH 1/2] testutil: support empty string cmd args 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. --- internals/testutil/exec.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internals/testutil/exec.go b/internals/testutil/exec.go index 113f7ceb..154127ca 100644 --- a/internals/testutil/exec.go +++ b/internals/testutil/exec.go @@ -92,9 +92,10 @@ type FakeCmd struct { // faking commands that need "\n" in their args (like zenity) // we use the following convention: // - generate \0 to separate args -// - generate \0\0 to separate commands +// - generate \0\f\n\r magic sequence to separate commands var scriptTpl = `#!/bin/bash printf "%%s" "$(basename "$0")" >> %[1]q + printf '\0' >> %[1]q for arg in "$@"; do @@ -102,7 +103,7 @@ for arg in "$@"; do printf '\0' >> %[1]q done -printf '\0' >> %[1]q +printf '\f\n\r' >> %[1]q %s ` @@ -176,12 +177,11 @@ func (cmd *FakeCmd) Calls() [][]string { if err != nil { panic(err) } - logContent := strings.TrimSuffix(string(raw), "\000") + logContent := strings.TrimSuffix(string(raw), "\000\f\n\r") allCalls := [][]string{} - calls := strings.Split(logContent, "\000\000") + calls := strings.Split(logContent, "\000\f\n\r") for _, call := range calls { - call = strings.TrimSuffix(call, "\000") allCalls = append(allCalls, strings.Split(call, "\000")) } return allCalls From 93859bb79cfcb03e6dffe11f62684eaeabe46b32 Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Wed, 21 Jun 2023 17:02:25 +0200 Subject: [PATCH 2/2] shutdown/notify: allow custom implementations 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. --- internals/cli/cmd_run.go | 3 +- internals/daemon/daemon.go | 61 ++++++++------ internals/daemon/daemon_test.go | 83 +++++++------------ internals/systemd/{sdnotify.go => notify.go} | 18 ++-- .../{sdnotify_test.go => notify_test.go} | 12 +-- internals/systemd/shutdown.go | 41 +++++++++ internals/systemd/shutdown_test.go | 68 +++++++++++++++ 7 files changed, 195 insertions(+), 91 deletions(-) rename internals/systemd/{sdnotify.go => notify.go} (80%) rename internals/systemd/{sdnotify_test.go => notify_test.go} (86%) create mode 100644 internals/systemd/shutdown.go create mode 100644 internals/systemd/shutdown_test.go diff --git a/internals/cli/cmd_run.go b/internals/cli/cmd_run.go index 2c1f2083..794be96e 100644 --- a/internals/cli/cmd_run.go +++ b/internals/cli/cmd_run.go @@ -28,7 +28,6 @@ import ( "github.com/canonical/pebble/cmd" "github.com/canonical/pebble/internals/daemon" "github.com/canonical/pebble/internals/logger" - "github.com/canonical/pebble/internals/systemd" ) var shortRunHelp = "Run the pebble environment" @@ -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") case <-d.Dying(): return } diff --git a/internals/daemon/daemon.go b/internals/daemon/daemon.go index b7405ee0..92542771 100644 --- a/internals/daemon/daemon.go +++ b/internals/daemon/daemon.go @@ -23,7 +23,6 @@ import ( "net" "net/http" "os" - "os/exec" "os/signal" "runtime" "strings" @@ -36,7 +35,6 @@ import ( "github.com/gorilla/mux" "github.com/canonical/pebble/internals/logger" - "github.com/canonical/pebble/internals/osutil" "github.com/canonical/pebble/internals/osutil/sys" "github.com/canonical/pebble/internals/overlord" "github.com/canonical/pebble/internals/overlord/checkstate" @@ -47,13 +45,40 @@ import ( "github.com/canonical/pebble/internals/systemd" ) +// notifySupervisor interface defines the required API for +// sending notifications to a supervisor daemon. +type notifySupervisor interface { + Available() bool + Send(state string) error +} + +// shutdown defines the required API for reboot, poweroff +// and halt control of the underlying system +type shutdown interface { + Reboot(delay time.Duration, msg string) error +} + var ( ErrRestartSocket = fmt.Errorf("daemon stop requested to wait for socket activation") - systemdSdNotify = systemd.SdNotify - sysGetuid = sys.Getuid + sysGetuid = sys.Getuid + + // The following functions should be modified depending on the + // underlying system in use. For example, some systems may + // not have systemd, and will use this system manager directly. + + // Notify provides a way to send a notification to a supervisor + // daemon, if one exists. The default implementation assumes + // systemd and should be replaced if not available. + Notify notifySupervisor = systemd.Notifier + + // Shutdown provides the system specific functionality to + // halt, poweroff or reboot. + Shutdown shutdown = systemd.Shutdown ) +var shutdownMsg = "reboot scheduled to update the system" + // Options holds the daemon setup required for the initialization of a new daemon. type Options struct { // Dir is the pebble directory where all setup is found. Defaults to /var/lib/pebble/default. @@ -441,7 +466,7 @@ func (ct *connTracker) trackConn(conn net.Conn, state http.ConnState) { } func (d *Daemon) CanStandby() bool { - return systemd.SocketAvailable() + return Notify.Available() } func (d *Daemon) initStandbyHandling() { @@ -457,7 +482,7 @@ func (d *Daemon) Start() { // we need to schedule and wait for a system restart d.tomb.Kill(nil) // avoid systemd killing us again while we wait - systemdSdNotify("READY=1") + Notify.Send("READY=1") return } if d.overlord == nil { @@ -504,7 +529,7 @@ func (d *Daemon) Start() { } // notify systemd that we are ready - systemdSdNotify("READY=1") + Notify.Send("READY=1") } // HandleRestart implements overlord.RestartBehavior. @@ -515,7 +540,7 @@ func (d *Daemon) HandleRestart(t restart.RestartType) { case restart.RestartSystem: // try to schedule a fallback slow reboot already here // in case we get stuck shutting down - if err := reboot(rebootWaitTimeout); err != nil { + if err := Shutdown.Reboot(rebootWaitTimeout, shutdownMsg); err != nil { logger.Noticef("%s", err) } @@ -593,7 +618,7 @@ func (d *Daemon) Stop(sigCh chan<- os.Signal) error { if !restartSystem { // tell systemd that we are stopping - systemdSdNotify("STOPPING=1") + Notify.Send("STOPPING=1") } if restartSocket { @@ -700,7 +725,7 @@ func (d *Daemon) doReboot(sigCh chan<- os.Signal, waitTimeout time.Duration) err } // ask for shutdown and wait for it to happen. // if we exit, pebble will be restarted by systemd - if err := reboot(rebootDelay); err != nil { + if err := Shutdown.Reboot(rebootDelay, shutdownMsg); err != nil { return err } // wait for reboot to happen @@ -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" - -func rebootImpl(rebootDelay time.Duration) error { - if rebootDelay < 0 { - rebootDelay = 0 - } - mins := int64(rebootDelay / time.Minute) - cmd := exec.Command("shutdown", "-r", fmt.Sprintf("+%d", mins), shutdownMsg) - if out, err := cmd.CombinedOutput(); err != nil { - return osutil.OutputErr(out, err) - } - return nil -} - -var reboot = rebootImpl - func (d *Daemon) Dying() <-chan struct{} { return d.tomb.Dying() } diff --git a/internals/daemon/daemon_test.go b/internals/daemon/daemon_test.go index c10d04d9..a717e1d8 100644 --- a/internals/daemon/daemon_test.go +++ b/internals/daemon/daemon_test.go @@ -56,23 +56,44 @@ type daemonSuite struct { authorized bool err error notified []string + delays []time.Duration restoreBackends func() } var _ = check.Suite(&daemonSuite{}) +type fakeNotify struct { + d *daemonSuite +} + +func (f fakeNotify) Available() bool { + return true +} + +func (f fakeNotify) Send(state string) error { + f.d.notified = append(f.d.notified, state) + return nil +} + +type fakeShutdown struct { + d *daemonSuite +} + +func (f fakeShutdown) Reboot(delay time.Duration, msg string) error { + f.d.delays = append(f.d.delays, delay) + return nil +} + func (s *daemonSuite) SetUpTest(c *check.C) { s.pebbleDir = c.MkDir() s.statePath = filepath.Join(s.pebbleDir, ".pebble.state") - systemdSdNotify = func(notif string) error { - s.notified = append(s.notified, notif) - return nil - } + Notify = &fakeNotify{d: s} } func (s *daemonSuite) TearDownTest(c *check.C) { - systemdSdNotify = systemd.SdNotify + Notify = systemd.Notifier s.notified = nil + s.delays = nil s.authorized = false s.err = nil } @@ -665,18 +686,14 @@ func (s *daemonSuite) TestRestartSystemWiring(c *check.C) { oldRebootNoticeWait := rebootNoticeWait oldRebootWaitTimeout := rebootWaitTimeout defer func() { - reboot = rebootImpl + Shutdown = systemd.Shutdown rebootNoticeWait = oldRebootNoticeWait rebootWaitTimeout = oldRebootWaitTimeout }() rebootWaitTimeout = 100 * time.Millisecond rebootNoticeWait = 150 * time.Millisecond - var delays []time.Duration - reboot = func(d time.Duration) error { - delays = append(delays, d) - return nil - } + Shutdown = &fakeShutdown{d: s} st.Lock() restart.Request(st, restart.RestartSystem) @@ -700,8 +717,8 @@ func (s *daemonSuite) TestRestartSystemWiring(c *check.C) { c.Check(rs, check.Equals, true) - c.Check(delays, check.HasLen, 1) - c.Check(delays[0], check.DeepEquals, rebootWaitTimeout) + c.Check(s.delays, check.HasLen, 1) + c.Check(s.delays[0], check.DeepEquals, rebootWaitTimeout) now := time.Now() @@ -709,8 +726,8 @@ func (s *daemonSuite) TestRestartSystemWiring(c *check.C) { c.Check(err, check.ErrorMatches, "expected reboot did not happen") - c.Check(delays, check.HasLen, 2) - c.Check(delays[1], check.DeepEquals, 1*time.Minute) + c.Check(s.delays, check.HasLen, 2) + c.Check(s.delays[1], check.DeepEquals, 1*time.Minute) // we are not stopping, we wait for the reboot instead c.Check(s.notified, check.DeepEquals, []string{"READY=1"}) @@ -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) { - cmd := testutil.FakeCommand(c, "shutdown", "", true) - defer cmd.Restore() - - tests := []struct { - delay time.Duration - delayArg string - }{ - {-1, "+0"}, - {0, "+0"}, - {time.Minute, "+1"}, - {10 * time.Minute, "+10"}, - {30 * time.Second, "+0"}, - } - - for _, t := range tests { - err := reboot(t.delay) - c.Assert(err, check.IsNil) - c.Check(cmd.Calls(), check.DeepEquals, [][]string{ - {"shutdown", "-r", t.delayArg, "reboot scheduled to update the system"}, - }) - - cmd.ForgetCalls() - } -} - func makeDaemonListeners(c *check.C, d *Daemon) { generalL, err := net.Listen("tcp", "127.0.0.1:0") c.Assert(err, check.IsNil) @@ -926,10 +917,6 @@ func (s *daemonSuite) TestRestartExpectedRebootGiveUp(c *check.C) { } func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) { - notifySocket := filepath.Join(c.MkDir(), "notify.socket") - os.Setenv("NOTIFY_SOCKET", notifySocket) - defer os.Setenv("NOTIFY_SOCKET", "") - restore := standby.FakeStandbyWait(5 * time.Millisecond) defer restore() @@ -944,9 +931,6 @@ func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) { time.Sleep(5 * time.Millisecond) } - c.Assert(d.standbyOpinions.CanStandby(), check.Equals, false) - f, _ := os.Create(notifySocket) - f.Close() c.Assert(d.standbyOpinions.CanStandby(), check.Equals, true) select { @@ -961,9 +945,6 @@ func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) { } func (s *daemonSuite) TestRestartIntoSocketModePendingChanges(c *check.C) { - os.Setenv("NOTIFY_SOCKET", c.MkDir()) - defer os.Setenv("NOTIFY_SOCKET", "") - restore := standby.FakeStandbyWait(5 * time.Millisecond) defer restore() diff --git a/internals/systemd/sdnotify.go b/internals/systemd/notify.go similarity index 80% rename from internals/systemd/sdnotify.go rename to internals/systemd/notify.go index 5f1279cf..3ce67abf 100644 --- a/internals/systemd/sdnotify.go +++ b/internals/systemd/notify.go @@ -23,18 +23,24 @@ import ( "github.com/canonical/pebble/internals/osutil" ) -var osGetenv = os.Getenv +type notifySystemd struct{} -func SocketAvailable() bool { +var ( + osGetenv = os.Getenv + Notifier = ¬ifySystemd{} +) + +// Available determines if the systemd unit supports notifications. +func (n notifySystemd) Available() bool { notifySocket := osGetenv("NOTIFY_SOCKET") return notifySocket != "" && osutil.CanStat(notifySocket) } -// SdNotify sends the given state string notification to systemd. +// Send the given state string notification to systemd. // // inspired by libsystemd/sd-daemon/sd-daemon.c from the systemd source -func SdNotify(notifyState string) error { - if notifyState == "" { +func (n notifySystemd) Send(state string) error { + if state == "" { return fmt.Errorf("cannot use empty notify state") } @@ -56,6 +62,6 @@ func SdNotify(notifyState string) error { } defer conn.Close() - _, err = conn.Write([]byte(notifyState)) + _, err = conn.Write([]byte(state)) return err } diff --git a/internals/systemd/sdnotify_test.go b/internals/systemd/notify_test.go similarity index 86% rename from internals/systemd/sdnotify_test.go rename to internals/systemd/notify_test.go index ad19dabe..2926803e 100644 --- a/internals/systemd/sdnotify_test.go +++ b/internals/systemd/notify_test.go @@ -44,16 +44,16 @@ func (sd *sdNotifyTestSuite) TearDownTest(c *C) { func (sd *sdNotifyTestSuite) TestSocketAvailable(c *C) { socketPath := filepath.Join(c.MkDir(), "notify.socket") - c.Assert(systemd.SocketAvailable(), Equals, false) + c.Assert(systemd.Notifier.Available(), Equals, false) sd.env["NOTIFY_SOCKET"] = socketPath - c.Assert(systemd.SocketAvailable(), Equals, false) + c.Assert(systemd.Notifier.Available(), Equals, false) f, _ := os.Create(socketPath) f.Close() - c.Assert(systemd.SocketAvailable(), Equals, true) + c.Assert(systemd.Notifier.Available(), Equals, true) } func (sd *sdNotifyTestSuite) TestSdNotifyMissingNotifyState(c *C) { - c.Check(systemd.SdNotify(""), ErrorMatches, "cannot use empty notify state") + c.Check(systemd.Notifier.Send(""), ErrorMatches, "cannot use empty notify state") } func (sd *sdNotifyTestSuite) TestSdNotifyWrongNotifySocket(c *C) { @@ -65,7 +65,7 @@ func (sd *sdNotifyTestSuite) TestSdNotifyWrongNotifySocket(c *C) { {"xxx", `cannot use \$NOTIFY_SOCKET value: "xxx"`}, } { sd.env["NOTIFY_SOCKET"] = t.env - c.Check(systemd.SdNotify("something"), ErrorMatches, t.errStr) + c.Check(systemd.Notifier.Send("something"), ErrorMatches, t.errStr) } } @@ -91,7 +91,7 @@ func (sd *sdNotifyTestSuite) TestSdNotifyIntegration(c *C) { ch <- string(buf[:n]) }() - err = systemd.SdNotify("something") + err = systemd.Notifier.Send("something") c.Assert(err, IsNil) c.Check(<-ch, Equals, "something") } diff --git a/internals/systemd/shutdown.go b/internals/systemd/shutdown.go new file mode 100644 index 00000000..5552775e --- /dev/null +++ b/internals/systemd/shutdown.go @@ -0,0 +1,41 @@ +// Copyright (c) 2014-2020 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package systemd + +import ( + "fmt" + "os/exec" + "time" + + "github.com/canonical/pebble/internals/osutil" +) + +type shutdown struct{} + +var Shutdown = &shutdown{} + +// Reboot the system after a specified duration of time, optionally +// displaying a wall message. +func (s shutdown) Reboot(delay time.Duration, msg string) error { + if delay < 0 { + delay = 0 + } + mins := int64(delay / time.Minute) + cmd := exec.Command("shutdown", "-r", fmt.Sprintf("+%d", mins), msg) + if out, err := cmd.CombinedOutput(); err != nil { + return osutil.OutputErr(out, err) + } + return nil +} diff --git a/internals/systemd/shutdown_test.go b/internals/systemd/shutdown_test.go new file mode 100644 index 00000000..11dd77ce --- /dev/null +++ b/internals/systemd/shutdown_test.go @@ -0,0 +1,68 @@ +// Copyright (c) 2014-2020 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package systemd_test + +import ( + "time" + + . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/reaper" + "github.com/canonical/pebble/internals/systemd" + "github.com/canonical/pebble/internals/testutil" +) + +type shutdownTestSuite struct{} + +var _ = Suite(&shutdownTestSuite{}) + +func (s *shutdownTestSuite) SetUpTest(c *C) { + // Needed for testutil.exec + reaper.Start() +} + +func (s *shutdownTestSuite) TearDownTest(c *C) { + reaper.Stop() +} + +// TestReboot checks that command construction match the +// expectation of the systemd shutdown command. +func (s *shutdownTestSuite) TestReboot(c *C) { + cmd := testutil.FakeCommand(c, "shutdown", "", true) + defer cmd.Restore() + + tests := []struct { + delay time.Duration + delayArg string + msg string + }{ + {0, "+0", ""}, + {0, "+0", "some msg"}, + {-1, "+0", "some msg"}, + {time.Minute, "+1", "some msg"}, + {10 * time.Minute, "+10", "some msg"}, + {30 * time.Second, "+0", "some msg"}, + } + + for _, t := range tests { + err := systemd.Shutdown.Reboot(t.delay, t.msg) + c.Assert(err, IsNil) + c.Check(cmd.Calls(), DeepEquals, [][]string{ + {"shutdown", "-r", t.delayArg, t.msg}, + }) + + cmd.ForgetCalls() + } +}