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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43a5daf
testutil: support empty string cmd args
flotter Jun 21, 2023
93859bb
shutdown/notify: allow custom implementations
flotter Jun 21, 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
3 changes: 1 addition & 2 deletions internals/cli/cmd_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

case <-d.Dying():
return
}
Expand Down
61 changes: 35 additions & 26 deletions internals/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"net"
"net/http"
"os"
"os/exec"
"os/signal"
"runtime"
"strings"
Expand All @@ -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"
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 😃

)

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.
Expand Down Expand Up @@ -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() {
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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)
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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"
Copy link
Contributor Author

@flotter flotter Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


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()
}
Expand Down
83 changes: 32 additions & 51 deletions internals/daemon/daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand All @@ -700,17 +717,17 @@ 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()

err = d.Stop(nil)

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"})
Expand All @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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)
Expand Down Expand Up @@ -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", "")

Comment on lines -929 to -932
Copy link
Contributor Author

@flotter flotter Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

restore := standby.FakeStandbyWait(5 * time.Millisecond)
defer restore()

Expand All @@ -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()
Comment on lines -947 to -949
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

c.Assert(d.standbyOpinions.CanStandby(), check.Equals, true)

select {
Expand All @@ -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", "")

Comment on lines -964 to -966
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

restore := standby.FakeStandbyWait(5 * time.Millisecond)
defer restore()

Expand Down
18 changes: 12 additions & 6 deletions internals/systemd/sdnotify.go → internals/systemd/notify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = &notifySystemd{}
)

// 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")
}

Expand All @@ -56,6 +62,6 @@ func SdNotify(notifyState string) error {
}
defer conn.Close()

_, err = conn.Write([]byte(notifyState))
_, err = conn.Write([]byte(state))
return err
}
12 changes: 6 additions & 6 deletions internals/systemd/sdnotify_test.go → internals/systemd/notify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
}

Expand All @@ -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")
}
Expand Down
Loading