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

Merge snapcore/snapd/state changes #259

Closed
wants to merge 1 commit into from
Closed

Merge snapcore/snapd/state changes #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion internals/cli/cmd_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,10 @@ func runDaemon(rcmd *cmdRun, ch chan os.Signal, ready chan<- func()) error {
}

d.Version = cmd.Version
d.Start()
err = d.Start()
if err != nil {
return err
}
Comment on lines +185 to +188
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify this for error only returns to mimic d.Init() ?

	if err := d.Start(); err != nil {
		return err
	}


watchdog, err := runWatchdog(d)
if err != nil {
Expand Down
16 changes: 8 additions & 8 deletions internals/daemon/api_changes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func setupChanges(st *state.State) []string {
}

func (s *apiSuite) TestStateChangesDefaultToInProgress(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down Expand Up @@ -75,7 +75,7 @@ func (s *apiSuite) TestStateChangesDefaultToInProgress(c *check.C) {
}

func (s *apiSuite) TestStateChangesInProgress(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down Expand Up @@ -104,7 +104,7 @@ func (s *apiSuite) TestStateChangesInProgress(c *check.C) {
}

func (s *apiSuite) TestStateChangesAll(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down Expand Up @@ -133,7 +133,7 @@ func (s *apiSuite) TestStateChangesAll(c *check.C) {
}

func (s *apiSuite) TestStateChangesReady(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down Expand Up @@ -161,7 +161,7 @@ func (s *apiSuite) TestStateChangesReady(c *check.C) {
}

func (s *apiSuite) TestStateChangesForServiceName(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down Expand Up @@ -192,7 +192,7 @@ func (s *apiSuite) TestStateChangesForServiceName(c *check.C) {
}

func (s *apiSuite) TestStateChange(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down Expand Up @@ -261,7 +261,7 @@ func (s *apiSuite) TestStateChange(c *check.C) {
}

func (s *apiSuite) TestStateChangeAbort(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

soon := 0
Expand Down Expand Up @@ -334,7 +334,7 @@ func (s *apiSuite) TestStateChangeAbort(c *check.C) {
}

func (s *apiSuite) TestStateChangeAbortIsReady(c *check.C) {
restore := state.FakeTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
restore := state.MockTime(time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC))
defer restore()

// Setup
Expand Down
12 changes: 6 additions & 6 deletions internals/daemon/api_files_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ func (s *filesSuite) TestRemoveSingle(c *C) {
c.Check(r.Result, HasLen, 1)
checkFileResult(c, r.Result[0], tmpDir+"/file", "", "")

c.Check(osutil.CanStat(tmpDir+"/file"), Equals, false)
c.Check(osutil.FileExists(tmpDir+"/file"), Equals, false)
}

func (s *filesSuite) TestRemoveMultiple(c *C) {
Expand Down Expand Up @@ -667,7 +667,7 @@ func (s *filesSuite) TestRemoveMultiple(c *C) {
checkFileResult(c, r.Result[2], tmpDir+"/non-empty", "generic-file-error", ".*directory not empty")
checkFileResult(c, r.Result[3], tmpDir+"/recursive", "", "")

c.Check(osutil.CanStat(tmpDir+"/file"), Equals, false)
c.Check(osutil.FileExists(tmpDir+"/file"), Equals, false)
c.Check(osutil.IsDir(tmpDir+"/empty"), Equals, false)
c.Check(osutil.IsDir(tmpDir+"/non-empty"), Equals, true)
c.Check(osutil.IsDir(tmpDir+"/recursive"), Equals, false)
Expand Down Expand Up @@ -1186,10 +1186,10 @@ group not found
checkFileResult(c, r.Result[4], pathUserNotFound, "generic-file-error", ".*unknown user.*")
checkFileResult(c, r.Result[5], pathGroupNotFound, "generic-file-error", ".*unknown group.*")

c.Check(osutil.CanStat(pathNoContent), Equals, false)
c.Check(osutil.CanStat(pathNotAbsolute), Equals, false)
c.Check(osutil.CanStat(pathNotFound), Equals, false)
c.Check(osutil.CanStat(pathPermissionDenied), Equals, false)
c.Check(osutil.FileExists(pathNoContent), Equals, false)
c.Check(osutil.FileExists(pathNotAbsolute), Equals, false)
c.Check(osutil.FileExists(pathNotFound), Equals, false)
c.Check(osutil.FileExists(pathPermissionDenied), Equals, false)
}

func assertFile(c *C, path string, perm os.FileMode, content string) {
Expand Down
1 change: 1 addition & 0 deletions internals/daemon/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ func (s *apiSuite) daemon(c *check.C) *Daemon {
}
d, err := New(&Options{Dir: s.pebbleDir})
c.Assert(err, check.IsNil)
c.Assert(d.Overlord().StartUp(), check.IsNil)
d.addRoutes()
s.d = d
return d
Expand Down
17 changes: 13 additions & 4 deletions internals/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,10 @@ func (d *Daemon) Init() error {
return nil
}

func (d *Daemon) Overlord() *overlord.Overlord {
return d.overlord
}

Comment on lines +388 to +391
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, daemon tests are bundled as part of the same package (not daemon_test). There are currently roughly 68 occurrences of d.overlord.xxx obtaining the overlord instance directly using the private struct field.

I think we should either stick to the earlier scheme, or update all the d.overlord references in tests.

// SetDegradedMode puts the daemon into a degraded mode which will the
// error given in the "err" argument for commands that are not marked
// as readonlyOK.
Expand Down Expand Up @@ -452,20 +456,24 @@ func (d *Daemon) initStandbyHandling() {
d.standbyOpinions.Start()
}

func (d *Daemon) Start() {
func (d *Daemon) Start() error {
if d.rebootIsMissing {
// 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")
return
return nil
}
if d.overlord == nil {
panic("internal error: no Overlord")
}

d.StartTime = time.Now()

// now perform expensive overlord/manages initialization
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment intends to read:
// now perform expensive overlord/manager initialization => "manager"

if err := d.overlord.StartUp(); err != nil {
return err
}
d.connTracker = &connTracker{conns: make(map[net.Conn]struct{})}
d.serve = &http.Server{
Handler: logit(d.router),
Expand Down Expand Up @@ -505,6 +513,7 @@ func (d *Daemon) Start() {

// notify systemd that we are ready
systemdSdNotify("READY=1")
return nil
}

// HandleRestart implements overlord.RestartBehavior.
Expand Down Expand Up @@ -673,7 +682,7 @@ func (d *Daemon) rebootDelay() (time.Duration, error) {
// see whether a reboot had already been scheduled
var rebootAt time.Time
err := d.state.Get("daemon-system-restart-at", &rebootAt)
if err != nil && err != state.ErrNoState {
if err != nil && !errors.Is(err, state.ErrNoState) {
return 0, err
}
rebootDelay := 1 * time.Minute
Expand Down Expand Up @@ -758,7 +767,7 @@ var errExpectedReboot = errors.New("expected reboot did not happen")
func (d *Daemon) RebootIsMissing(st *state.State) error {
var nTentative int
err := st.Get("daemon-system-restart-tentative", &nTentative)
if err != nil && err != state.ErrNoState {
if err != nil && !errors.Is(err, state.ErrNoState) {
return err
}
nTentative++
Expand Down
52 changes: 32 additions & 20 deletions internals/daemon/daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ func (s *daemonSuite) TestExplicitPaths(c *C) {

d := s.newDaemon(c)
d.Init()
d.Start()
err := d.Start()
c.Assert(err, check.IsNil)
defer d.Stop(nil)

info, err := os.Stat(s.socketPath)
Expand Down Expand Up @@ -459,7 +460,8 @@ func (s *daemonSuite) TestStartStop(c *check.C) {
untrustedAccept := make(chan struct{})
d.untrustedListener = &witnessAcceptListener{Listener: l2, accept: untrustedAccept}

d.Start()
err = d.Start()
c.Assert(err, check.IsNil)

generalDone := make(chan struct{})
go func() {
Expand Down Expand Up @@ -500,7 +502,8 @@ func (s *daemonSuite) TestRestartWiring(c *check.C) {
untrustedAccept := make(chan struct{})
d.untrustedListener = &witnessAcceptListener{Listener: l, accept: untrustedAccept}

d.Start()
err = d.Start()
c.Assert(err, check.IsNil)
defer d.Stop(nil)

generalDone := make(chan struct{})
Expand Down Expand Up @@ -567,7 +570,8 @@ func (s *daemonSuite) TestGracefulStop(c *check.C) {
untrustedAccept := make(chan struct{})
d.untrustedListener = &witnessAcceptListener{Listener: untrustedL, accept: untrustedAccept}

d.Start()
err = d.Start()
c.Assert(err, check.IsNil)

generalAccepting := make(chan struct{})
go func() {
Expand Down Expand Up @@ -634,7 +638,8 @@ func (s *daemonSuite) TestRestartSystemWiring(c *check.C) {
untrustedAccept := make(chan struct{})
d.untrustedListener = &witnessAcceptListener{Listener: l, accept: untrustedAccept}

d.Start()
err = d.Start()
c.Assert(err, check.IsNil)
defer d.Stop(nil)

st := d.overlord.State()
Expand Down Expand Up @@ -781,7 +786,8 @@ func (s *daemonSuite) TestRestartShutdownWithSigtermInBetween(c *check.C) {
d := s.newDaemon(c)
makeDaemonListeners(c, d)

d.Start()
err := d.Start()
c.Assert(err, check.IsNil)
st := d.overlord.State()

st.Lock()
Expand All @@ -791,7 +797,7 @@ func (s *daemonSuite) TestRestartShutdownWithSigtermInBetween(c *check.C) {
ch := make(chan os.Signal, 2)
ch <- syscall.SIGTERM
// stop will check if we got a sigterm in between (which we did)
err := d.Stop(ch)
err = d.Stop(ch)
c.Assert(err, check.IsNil)
}

Expand All @@ -813,7 +819,8 @@ func (s *daemonSuite) TestRestartShutdown(c *check.C) {
d := s.newDaemon(c)
makeDaemonListeners(c, d)

d.Start()
err := d.Start()
c.Assert(err, check.IsNil)
st := d.overlord.State()

st.Lock()
Expand Down Expand Up @@ -860,7 +867,8 @@ func (s *daemonSuite) TestRestartExpectedRebootIsMissing(c *check.C) {
c.Check(err, check.IsNil)
c.Check(n, check.Equals, 1)

d.Start()
err = d.Start()
c.Assert(err, check.IsNil)

c.Check(s.notified, check.DeepEquals, []string{"READY=1"})

Expand Down Expand Up @@ -896,8 +904,8 @@ func (s *daemonSuite) TestRestartExpectedRebootOK(c *check.C) {
defer st.Unlock()
var v interface{}
// these were cleared
c.Check(st.Get("daemon-system-restart-at", &v), check.Equals, state.ErrNoState)
c.Check(st.Get("system-restart-from-boot-id", &v), check.Equals, state.ErrNoState)
c.Check(st.Get("daemon-system-restart-at", &v), testutil.ErrorIs, state.ErrNoState)
Copy link
Contributor

Choose a reason for hiding this comment

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

Only in daemon tests an additional 170 occurrences of check.Equals remain after this change. My gut tells me I would either stick to the original way of doing things, or migrate the project to use the new method. The inconsistency of using both methods means people looking at the wrong part of the code may copy the old approach.

Furthermore, it really feels like having testutil changes contained in a separate commit of this PR (at least) will make impact easier to see and review. It seems like a clean standalone group of changes.

c.Check(st.Get("system-restart-from-boot-id", &v), testutil.ErrorIs, state.ErrNoState)
}

func (s *daemonSuite) TestRestartExpectedRebootGiveUp(c *check.C) {
Expand All @@ -920,9 +928,9 @@ func (s *daemonSuite) TestRestartExpectedRebootGiveUp(c *check.C) {
defer st.Unlock()
var v interface{}
// these were cleared
c.Check(st.Get("daemon-system-restart-at", &v), check.Equals, state.ErrNoState)
c.Check(st.Get("system-restart-from-boot-id", &v), check.Equals, state.ErrNoState)
c.Check(st.Get("daemon-system-restart-tentative", &v), check.Equals, state.ErrNoState)
c.Check(st.Get("daemon-system-restart-at", &v), testutil.ErrorIs, state.ErrNoState)
c.Check(st.Get("system-restart-from-boot-id", &v), testutil.ErrorIs, state.ErrNoState)
c.Check(st.Get("daemon-system-restart-tentative", &v), testutil.ErrorIs, state.ErrNoState)
}

func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) {
Expand All @@ -936,7 +944,8 @@ func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) {
d := s.newDaemon(c)
makeDaemonListeners(c, d)

d.Start()
err := d.Start()
c.Assert(err, check.IsNil)

// pretend some ensure happened
for i := 0; i < 5; i++ {
Expand All @@ -955,7 +964,7 @@ func (s *daemonSuite) TestRestartIntoSocketModeNoNewChanges(c *check.C) {
case <-time.After(15 * time.Second):
c.Errorf("daemon did not stop after 15s")
}
err := d.Stop(nil)
err = d.Stop(nil)
c.Check(err, check.Equals, ErrRestartSocket)
c.Check(d.restartSocket, check.Equals, true)
}
Expand All @@ -972,7 +981,8 @@ func (s *daemonSuite) TestRestartIntoSocketModePendingChanges(c *check.C) {

st := d.overlord.State()

d.Start()
err := d.Start()
c.Assert(err, check.IsNil)
// pretend some ensure happened
for i := 0; i < 5; i++ {
d.overlord.StateEngine().Ensure()
Expand All @@ -998,7 +1008,7 @@ func (s *daemonSuite) TestRestartIntoSocketModePendingChanges(c *check.C) {
c.Errorf("daemon did not stop after 5s")
}
// when the daemon got a pending change it just restarts
err := d.Stop(nil)
err = d.Stop(nil)
c.Check(err, check.IsNil)
c.Check(d.restartSocket, check.Equals, false)
}
Expand Down Expand Up @@ -1058,7 +1068,8 @@ func (s *daemonSuite) TestHTTPAPI(c *check.C) {
s.httpAddress = ":0" // Go will choose port (use listener.Addr() to find it)
d := s.newDaemon(c)
d.Init()
d.Start()
err := d.Start()
c.Assert(err, check.IsNil)
port := d.httpListener.Addr().(*net.TCPAddr).Port

request, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/v1/health", port), nil)
Expand Down Expand Up @@ -1101,7 +1112,8 @@ services:
d := s.newDaemon(c)
err := d.Init()
c.Assert(err, IsNil)
d.Start()
err = d.Start()
c.Assert(err, check.IsNil)

// Start the test service.
payload := bytes.NewBufferString(`{"action": "start", "services": ["test1"]}`)
Expand Down
Loading