From 29054489517cb3d60e680ee883431128a99ed54e Mon Sep 17 00:00:00 2001 From: Zeyad Yasser Date: Tue, 12 Nov 2024 12:43:40 +0200 Subject: [PATCH] many: add state unlocker to runinhibit helpers (#14671) * many: add state unlocker to runinhibit.LockWithHint Signed-off-by: Zeyad Gouda * many: add state unlocker to runinhibit.Unlock Signed-off-by: Zeyad Gouda * many: add state unlocker to runinhibit.IsLocked Signed-off-by: Zeyad Gouda * many: add state unlocker to runinhibit.RemoveLockFile Signed-off-by: Zeyad Gouda * cmd/snaplock/runinhibit: update doc comment to mention unlocker Signed-off-by: Zeyad Gouda * tests: add a regression spread test for lp-2084730 Signed-off-by: Zeyad Gouda * o/snapstate/backend: add unit tests for forbidden nil state unlocker Signed-off-by: Zeyad Gouda * tests: address review comments Signed-off-by: Zeyad Gouda * tests: fix restore for tests/regression/lp-2084730 Signed-off-by: Zeyad Gouda * tests: skip lsof command if not found Signed-off-by: Zeyad Gouda * cmd/snaplock: add comment about potential deadlock using snap lock The snap lock is only accessible to root and is only intended to synchronize operations between snapd and snap-confine (and snap-update-ns in some cases). Any process holding the snap lock must not do any interactions with snapd to avoid deadlocks due to locked snap state. Signed-off-by: Zeyad Gouda * o/hookstate: avoid unlocking/locking state twice quickly Signed-off-by: Zeyad Gouda --------- Signed-off-by: Zeyad Gouda --- cmd/snap/cmd_run.go | 2 +- cmd/snap/cmd_run_test.go | 20 ++--- cmd/snap/inhibit_test.go | 12 +-- cmd/snaplock/lock.go | 5 ++ cmd/snaplock/runinhibit/inhibit.go | 54 ++++++++++++- cmd/snaplock/runinhibit/inhibit_test.go | 90 ++++++++++++++------- overlord/hookstate/ctlcmd/refresh.go | 2 +- overlord/hookstate/ctlcmd/refresh_test.go | 2 +- overlord/hookstate/hooks.go | 16 ++-- overlord/hookstate/hooks_test.go | 32 ++++---- overlord/snapstate/backend.go | 4 +- overlord/snapstate/backend/link.go | 18 ++++- overlord/snapstate/backend/link_test.go | 93 ++++++++++++++++++---- overlord/snapstate/backend/locking.go | 11 ++- overlord/snapstate/backend/locking_test.go | 31 +++++++- overlord/snapstate/backend/setup.go | 8 +- overlord/snapstate/backend/setup_test.go | 17 ++++ overlord/snapstate/backend_test.go | 4 +- overlord/snapstate/handlers.go | 41 ++++++---- overlord/snapstate/handlers_link_test.go | 8 +- overlord/snapstate/refresh.go | 2 +- overlord/snapstate/refresh_test.go | 4 +- tests/regression/lp-2084730/task.yaml | 63 +++++++++++++++ 23 files changed, 412 insertions(+), 127 deletions(-) create mode 100644 tests/regression/lp-2084730/task.yaml diff --git a/cmd/snap/cmd_run.go b/cmd/snap/cmd_run.go index 4418e53fa24..cc096a67e8c 100644 --- a/cmd/snap/cmd_run.go +++ b/cmd/snap/cmd_run.go @@ -501,7 +501,7 @@ func (x *cmdRun) straceOpts() (opts []string, raw bool, err error) { func checkSnapRunInhibitionConflict(app *snap.AppInfo) error { // Remove hint check takes precedence because we want to exit early snapName := app.Snap.InstanceName() - hint, _, err := runinhibit.IsLocked(snapName) + hint, _, err := runinhibit.IsLocked(snapName, nil) if err != nil { return err } diff --git a/cmd/snap/cmd_run_test.go b/cmd/snap/cmd_run_test.go index 5df4a2cc5c0..ee0b5eeb22d 100644 --- a/cmd/snap/cmd_run_test.go +++ b/cmd/snap/cmd_run_test.go @@ -281,7 +281,7 @@ func (s *RunSuite) TestSnapRunAppRunsChecksRefreshInhibitionLock(c *check.C) { defer restorer() inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil) c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil) c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) @@ -321,7 +321,7 @@ func (s *RunSuite) TestSnapRunAppRunsChecksRefreshInhibitionLock(c *check.C) { func (s *RunSuite) testSnapRunAppRunsChecksRemoveInhibitionLock(c *check.C, svc bool) { inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo, nil), check.IsNil) cmd := "snapname.app" if svc { @@ -355,7 +355,7 @@ func (s *RunSuite) TestSnapRunAppRefreshAppAwarenessUnsetSkipsInhibitionLockWait // mark snap as inhibited inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil) c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil) // unset refresh-app-awareness flag c.Assert(os.RemoveAll(features.RefreshAppAwareness.ControlFile()), check.IsNil) @@ -379,7 +379,7 @@ func (s *RunSuite) TestSnapRunAppNewRevisionAfterInhibition(c *check.C) { // mark snap as inhibited inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil) // unset refresh-app-awareness flag c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil) c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) @@ -498,7 +498,7 @@ func (s *RunSuite) TestSnapRunHookNoRuninhibit(c *check.C) { defer restore() inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(42)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil) c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil) c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) @@ -532,7 +532,7 @@ func (s *RunSuite) TestSnapRunAppRuninhibitSkipsServices(c *check.C) { defer restorer() inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R("x2")} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), check.IsNil) c.Assert(os.MkdirAll(dirs.FeaturesDir, 0755), check.IsNil) c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) @@ -718,7 +718,7 @@ func (s *RunSuite) testSnapRunAppRetryNoInhibitHintFileThenOngoingRefresh(c *che // mock snap inhibited to trigger race condition detection // i.e. we started without a hint lock file (snap on first install) // then a refresh started which created the hint lock file. - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil) // nil FileLock means no inhibit file exists return nil, nil @@ -847,7 +847,7 @@ func (s *RunSuite) testSnapRunAppRetryNoInhibitHintFileThenOngoingRemove(c *chec // mock snap inhibited to trigger race condition detection // i.e. we started without a hint lock file (snap on first install) // then a remove started which created the hint lock file. - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil) // nil FileLock means no inhibit file exists return nil, nil @@ -955,7 +955,7 @@ func (s *RunSuite) TestSnapRunAppRetryNoInhibitHintFileThenOngoingRefreshMissing // and we have an ongoing refresh which removed current symlink. c.Assert(err, testutil.ErrorIs, snaprun.ErrSnapRefreshConflict) // and created the inhibition hint lock file. - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil) return nil, err } else { var err error @@ -1054,7 +1054,7 @@ func (s *RunSuite) TestSnapRunAppMaxRetry(c *check.C) { // mock snap inhibited to trigger race condition detection // i.e. we started without a hint lock file (snap on first install) // then a refresh started which created the hint lock file. - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}), check.IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R("x2")}, nil), check.IsNil) // nil FileLock means no inhibit file exists return nil, nil diff --git a/cmd/snap/inhibit_test.go b/cmd/snap/inhibit_test.go index ce1b6f76759..dbf7ca9d361 100644 --- a/cmd/snap/inhibit_test.go +++ b/cmd/snap/inhibit_test.go @@ -71,7 +71,7 @@ func (s *RunSuite) TestWaitWhileInhibitedRunThrough(c *C) { c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil) var waitWhileInhibitedCalled int restore := snaprun.MockWaitWhileInhibited(func(ctx context.Context, snapName string, notInhibited func(ctx context.Context) error, inhibited func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error), interval time.Duration) (flock *osutil.FileLock, retErr error) { @@ -130,7 +130,7 @@ func (s *RunSuite) TestWaitWhileInhibitedErrorOnStartNotification(c *C) { c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil) var startCalled, finishCalled int inhibitionFlow := fakeInhibitionFlow{ @@ -166,7 +166,7 @@ func (s *RunSuite) TestWaitWhileInhibitedErrorOnFinishNotification(c *C) { c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil) var waitWhileInhibitedCalled int restore := snaprun.MockWaitWhileInhibited(func(ctx context.Context, snapName string, notInhibited func(ctx context.Context) error, inhibited func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error), interval time.Duration) (flock *osutil.FileLock, retErr error) { @@ -226,7 +226,7 @@ func (s *RunSuite) TestWaitWhileInhibitedContextCancellationOnError(c *C) { c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil) originalCtx, cancel := context.WithCancel(context.Background()) inhibitionFlow := fakeInhibitionFlow{ @@ -259,7 +259,7 @@ func (s *RunSuite) TestWaitWhileInhibitedGateRefreshNoNotification(c *C) { c.Assert(os.WriteFile(features.RefreshAppAwareness.ControlFile(), []byte(nil), 0644), check.IsNil) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedGateRefresh, inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedGateRefresh, inhibitInfo, nil), IsNil) var called int restore := snaprun.MockWaitWhileInhibited(func(ctx context.Context, snapName string, notInhibited func(ctx context.Context) error, inhibited func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error), interval time.Duration) (flock *osutil.FileLock, retErr error) { @@ -310,7 +310,7 @@ func (s *RunSuite) testWaitWhileInhibitedRemoveInhibition(c *C, svc bool) { snaptest.MockSnapCurrent(c, string(mockYaml), &snap.SideInfo{Revision: snap.R(11)}) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(11)} - c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("snapname", runinhibit.HintInhibitedForRemove, inhibitInfo, nil), IsNil) inhibitionFlow := fakeInhibitionFlow{ start: func(ctx context.Context) error { diff --git a/cmd/snaplock/lock.go b/cmd/snaplock/lock.go index d6ac6471414..f059db5be54 100644 --- a/cmd/snaplock/lock.go +++ b/cmd/snaplock/lock.go @@ -36,6 +36,11 @@ func lockFileName(snapName string) string { } // OpenLock creates and opens a lock file associated with a particular snap. +// +// NOTE: The snap lock is only accessible to root and is only intended to +// synchronize operations between snapd and snap-confine (and snap-update-ns +// in some cases). Any process holding the snap lock must not do any +// interactions with snapd to avoid deadlocks due to locked snap state. func OpenLock(snapName string) (*osutil.FileLock, error) { if err := os.MkdirAll(dirs.SnapRunLockDir, 0700); err != nil { return nil, fmt.Errorf("cannot create lock directory: %s", err) diff --git a/cmd/snaplock/runinhibit/inhibit.go b/cmd/snaplock/runinhibit/inhibit.go index 09223f46804..3bec5a86d7f 100644 --- a/cmd/snaplock/runinhibit/inhibit.go +++ b/cmd/snaplock/runinhibit/inhibit.go @@ -124,6 +124,12 @@ func removeInhibitInfoFiles(snapName string) error { return nil } +// Unlocker functions are passed from code using runinhibit to indicate that global +// state should be unlocked during file lock operations to avoid having deadlocks where +// both inhibition hint file and state are locked and waiting for each other. Unlocker +// being nil indicates not to do this. +type Unlocker func() (relock func()) + // LockWithHint sets a persistent "snap run" inhibition lock, for the given snap, with a given hint // and saves given info that will be needed by "snap run" during inhibition (e.g. snap revision). // @@ -132,7 +138,17 @@ func removeInhibitInfoFiles(snapName string) error { // start and will block, presenting a user interface if possible. Also // info.Previous corresponding to the snap revision that was installed must be // provided and cannot be unset. -func LockWithHint(snapName string, hint Hint, info InhibitInfo) error { +// +// If unlocker is passed it indicates that the global state needs to be unlocked +// before taking the inhibition hint file lock. It is the responsibility of the +// caller to make sure state is locked if a non-nil unlocker is passed. +func LockWithHint(snapName string, hint Hint, info InhibitInfo, unlocker Unlocker) error { + if unlocker != nil { + // unlock/relock global state + relock := unlocker() + defer relock() + } + if err := hint.validate(); err != nil { return err } @@ -179,7 +195,17 @@ func LockWithHint(snapName string, hint Hint, info InhibitInfo) error { // Unlock truncates the run inhibition lock, for the given snap. // // An empty inhibition lock means uninhibited "snap run". -func Unlock(snapName string) error { +// +// If unlocker is passed it indicates that the global state needs to be unlocked +// before taking the inhibition hint file lock. It is the responsibility of the +// caller to make sure state is locked if a non-nil unlocker is passed. +func Unlock(snapName string, unlocker Unlocker) error { + if unlocker != nil { + // unlock/relock global state + relock := unlocker() + defer relock() + } + flock, err := openHintFileLock(snapName) if os.IsNotExist(err) { return nil @@ -214,7 +240,17 @@ func Unlock(snapName string) error { // // It returns the current, non-empty hint if inhibition is in place. Otherwise // it returns an empty hint. -func IsLocked(snapName string) (Hint, InhibitInfo, error) { +// +// If unlocker is passed it indicates that the global state needs to be unlocked +// before taking the inhibition hint file lock. It is the responsibility of the +// caller to make sure state is locked if a non-nil unlocker is passed. +func IsLocked(snapName string, unlocker Unlocker) (Hint, InhibitInfo, error) { + if unlocker != nil { + // unlock/relock global state + relock := unlocker() + defer relock() + } + hintFlock, err := osutil.OpenExistingLockForReading(HintFile(snapName)) if os.IsNotExist(err) { return "", InhibitInfo{}, nil @@ -254,7 +290,17 @@ func IsLocked(snapName string) (Hint, InhibitInfo, error) { // it. // // The function does not fail if the inhibition lock does not exist. -func RemoveLockFile(snapName string) error { +// +// If unlocker is passed it indicates that the global state needs to be unlocked +// before taking the inhibition hint file lock. It is the responsibility of the +// caller to make sure state is locked if a non-nil unlocker is passed. +func RemoveLockFile(snapName string, unlocker Unlocker) error { + if unlocker != nil { + // unlock/relock global state + relock := unlocker() + defer relock() + } + hintFlock, err := osutil.OpenExistingLockForReading(HintFile(snapName)) if os.IsNotExist(err) { return nil diff --git a/cmd/snaplock/runinhibit/inhibit_test.go b/cmd/snaplock/runinhibit/inhibit_test.go index 841b191d904..7a51a808b92 100644 --- a/cmd/snaplock/runinhibit/inhibit_test.go +++ b/cmd/snaplock/runinhibit/inhibit_test.go @@ -61,7 +61,7 @@ func (s *runInhibitSuite) TestLockWithEmptyHint(c *C) { _, err := os.Stat(runinhibit.InhibitDir) c.Assert(os.IsNotExist(err), Equals, true) - err = runinhibit.LockWithHint("pkg", runinhibit.HintNotInhibited, s.inhibitInfo) + err = runinhibit.LockWithHint("pkg", runinhibit.HintNotInhibited, s.inhibitInfo, nil) c.Assert(err, ErrorMatches, "lock hint cannot be empty") _, err = os.Stat(runinhibit.InhibitDir) @@ -73,7 +73,7 @@ func (s *runInhibitSuite) TestLockWithUnsetRevision(c *C) { _, err := os.Stat(runinhibit.InhibitDir) c.Assert(os.IsNotExist(err), Equals, true) - err = runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R(0)}) + err = runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, runinhibit.InhibitInfo{Previous: snap.R(0)}, nil) c.Assert(err, ErrorMatches, "snap revision cannot be unset") _, err = os.Stat(runinhibit.InhibitDir) @@ -95,9 +95,16 @@ func (s *runInhibitSuite) TestLockWithHint(c *C) { _, err := os.Stat(runinhibit.InhibitDir) c.Assert(os.IsNotExist(err), Equals, true) + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } expectedInfo := runinhibit.InhibitInfo{Previous: snap.R(42)} - err = runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, expectedInfo) + err = runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, expectedInfo, fakeUnlocker) c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) fi, err := os.Stat(runinhibit.InhibitDir) c.Assert(err, IsNil) @@ -112,19 +119,19 @@ func (s *runInhibitSuite) TestLockWithHint(c *C) { // The lock can be re-acquired to present a different hint. func (s *runInhibitSuite) TestLockLocked(c *C) { expectedInfo := runinhibit.InhibitInfo{Previous: snap.R(42)} - err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, expectedInfo) + err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, expectedInfo, nil) c.Assert(err, IsNil) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FileEquals, "refresh") testInhibitInfo(c, "pkg", "refresh", expectedInfo) expectedInfo = runinhibit.InhibitInfo{Previous: snap.R(43)} - err = runinhibit.LockWithHint("pkg", runinhibit.Hint("just-testing"), expectedInfo) + err = runinhibit.LockWithHint("pkg", runinhibit.Hint("just-testing"), expectedInfo, nil) c.Assert(err, IsNil) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FileEquals, "just-testing") testInhibitInfo(c, "pkg", "just-testing", expectedInfo) expectedInfo = runinhibit.InhibitInfo{Previous: snap.R(44)} - err = runinhibit.LockWithHint("pkg", runinhibit.Hint("short"), expectedInfo) + err = runinhibit.LockWithHint("pkg", runinhibit.Hint("short"), expectedInfo, nil) c.Assert(err, IsNil) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FileEquals, "short") testInhibitInfo(c, "pkg", "short", expectedInfo) @@ -132,18 +139,25 @@ func (s *runInhibitSuite) TestLockLocked(c *C) { // Unlocking an unlocked lock doesn't break anything. func (s *runInhibitSuite) TestUnlockUnlocked(c *C) { - err := runinhibit.Unlock("pkg") + err := runinhibit.Unlock("pkg", nil) c.Assert(err, IsNil) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FileAbsent) } // Unlocking an locked lock truncates the hint and removes inhibit info file. func (s *runInhibitSuite) TestUnlockLocked(c *C) { - err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo) + err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil) c.Assert(err, IsNil) - err = runinhibit.Unlock("pkg") + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + err = runinhibit.Unlock("pkg", fakeUnlocker) c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FileEquals, "") c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.refresh"), testutil.FileAbsent) @@ -154,26 +168,36 @@ func (s *runInhibitSuite) TestIsLockedMissing(c *C) { _, err := os.Stat(runinhibit.InhibitDir) c.Assert(os.IsNotExist(err), Equals, true) - hint, info, err := runinhibit.IsLocked("pkg") + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + + hint, info, err := runinhibit.IsLocked("pkg", fakeUnlocker) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) err = os.MkdirAll(runinhibit.InhibitDir, 0755) c.Assert(err, IsNil) - hint, info, err = runinhibit.IsLocked("pkg") + hint, info, err = runinhibit.IsLocked("pkg", fakeUnlocker) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) + c.Check(unlockerCalled, Equals, 2) + c.Check(relockCalled, Equals, 2) } // IsLocked returns the previously set hint/info. func (s *runInhibitSuite) TestIsLockedLocked(c *C) { - err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo) + err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil) c.Assert(err, IsNil) - hint, info, err := runinhibit.IsLocked("pkg") + hint, info, err := runinhibit.IsLocked("pkg", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedForRefresh) c.Check(info, Equals, s.inhibitInfo) @@ -181,27 +205,37 @@ func (s *runInhibitSuite) TestIsLockedLocked(c *C) { // IsLocked returns not-inhibited after unlocking. func (s *runInhibitSuite) TestIsLockedUnlocked(c *C) { - err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo) + err := runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil) c.Assert(err, IsNil) - err = runinhibit.Unlock("pkg") + err = runinhibit.Unlock("pkg", nil) c.Assert(err, IsNil) - hint, info, err := runinhibit.IsLocked("pkg") + hint, info, err := runinhibit.IsLocked("pkg", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) } func (s *runInhibitSuite) TestRemoveLockFile(c *C) { - c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil), IsNil) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FilePresent) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.refresh"), testutil.FilePresent) - c.Assert(runinhibit.RemoveLockFile("pkg"), IsNil) + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + + c.Assert(runinhibit.RemoveLockFile("pkg", fakeUnlocker), IsNil) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.lock"), testutil.FileAbsent) c.Check(filepath.Join(runinhibit.InhibitDir, "pkg.refresh"), testutil.FileAbsent) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) // Removing an absent lock file is not an error. - c.Assert(runinhibit.RemoveLockFile("pkg"), IsNil) + c.Assert(runinhibit.RemoveLockFile("pkg", fakeUnlocker), IsNil) + c.Check(unlockerCalled, Equals, 2) + c.Check(relockCalled, Equals, 2) } func checkFileLocked(c *C, path string) { @@ -219,7 +253,7 @@ func checkFileNotLocked(c *C, path string) { } func (s *runInhibitSuite) TestWaitWhileInhibitedWalkthrough(c *C) { - c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil), IsNil) notInhibitedCalled := 0 inhibitedCalled := 0 @@ -232,7 +266,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedWalkthrough(c *C) { if inhibitedCalled == 3 { // let's remove run inhibtion - c.Assert(runinhibit.Unlock("pkg"), IsNil) + c.Assert(runinhibit.Unlock("pkg", nil), IsNil) } return waitCh @@ -242,7 +276,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedWalkthrough(c *C) { notInhibited := func(ctx context.Context) error { notInhibitedCalled++ - hint, _, err := runinhibit.IsLocked("pkg") + hint, _, err := runinhibit.IsLocked("pkg", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) @@ -279,7 +313,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedWalkthrough(c *C) { } func (s *runInhibitSuite) TestWaitWhileInhibitedContextCancellation(c *C) { - c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil), IsNil) ctx, cancel := context.WithCancel(context.Background()) @@ -324,7 +358,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedContextCancellation(c *C) { } func (s *runInhibitSuite) TestWaitWhileInhibitedNilCallbacks(c *C) { - c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil), IsNil) waitCalled := 0 // closed channel returns immediately @@ -335,7 +369,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedNilCallbacks(c *C) { // lock should be released during wait interval checkFileNotLocked(c, runinhibit.HintFile("pkg")) // let's remove run inhibtion - c.Assert(runinhibit.Unlock("pkg"), IsNil) + c.Assert(runinhibit.Unlock("pkg", nil), IsNil) return waitCh } @@ -366,7 +400,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedCallbackError(c *C) { // lock must be released on error checkFileNotLocked(c, runinhibit.HintFile("pkg")) - c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil), IsNil) inhibited := func(ctx context.Context, hint runinhibit.Hint, inhibitInfo *runinhibit.InhibitInfo) (cont bool, err error) { return false, fmt.Errorf("inhibited error") } @@ -378,7 +412,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedCallbackError(c *C) { } func (s *runInhibitSuite) TestWaitWhileInhibitedCont(c *C) { - c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo), IsNil) + c.Assert(runinhibit.LockWithHint("pkg", runinhibit.HintInhibitedForRefresh, s.inhibitInfo, nil), IsNil) notInhibitedCalled := 0 inhibitedCalled := 0 @@ -406,7 +440,7 @@ func (s *runInhibitSuite) TestWaitWhileInhibitedCont(c *C) { c.Check(notInhibitedCalled, Equals, 0) c.Check(inhibitedCalled, Equals, 1) - hint, inhibitInfo, err := runinhibit.IsLocked("pkg") + hint, inhibitInfo, err := runinhibit.IsLocked("pkg", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedForRefresh) c.Check(inhibitInfo, Equals, s.inhibitInfo) diff --git a/overlord/hookstate/ctlcmd/refresh.go b/overlord/hookstate/ctlcmd/refresh.go index 91af2d8ec31..8c33a75a93c 100644 --- a/overlord/hookstate/ctlcmd/refresh.go +++ b/overlord/hookstate/ctlcmd/refresh.go @@ -365,7 +365,7 @@ func (c *refreshCommand) printInhibitLockHint() error { } defer lock.Unlock() - hint, _, err := runinhibit.IsLocked(snapName) + hint, _, err := runinhibit.IsLocked(snapName, nil) if err != nil { return err } diff --git a/overlord/hookstate/ctlcmd/refresh_test.go b/overlord/hookstate/ctlcmd/refresh_test.go index 0e72badf4e6..f9713ed162e 100644 --- a/overlord/hookstate/ctlcmd/refresh_test.go +++ b/overlord/hookstate/ctlcmd/refresh_test.go @@ -511,7 +511,7 @@ func (s *refreshSuite) TestRefreshPrintInhibitHint(c *C) { err = lock.Lock() c.Assert(err, IsNil) inhibitInfo := runinhibit.InhibitInfo{Previous: snap.R(1)} - c.Check(runinhibit.LockWithHint("snap1", runinhibit.HintInhibitedForRefresh, inhibitInfo), IsNil) + c.Check(runinhibit.LockWithHint("snap1", runinhibit.HintInhibitedForRefresh, inhibitInfo, nil), IsNil) lock.Unlock() stdout, stderr, err := ctlcmd.Run(mockContext, []string{"refresh", "--show-lock"}, 0) diff --git a/overlord/hookstate/hooks.go b/overlord/hookstate/hooks.go index b2dd9844f00..d13a0298538 100644 --- a/overlord/hookstate/hooks.go +++ b/overlord/hookstate/hooks.go @@ -177,7 +177,7 @@ func (h *gateAutoRefreshHookHandler) Before() error { defer lock.Unlock() inhibitInfo := runinhibit.InhibitInfo{Previous: snapRev} - if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedGateRefresh, inhibitInfo); err != nil { + if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedGateRefresh, inhibitInfo, st.Unlocker()); err != nil { return err } @@ -214,7 +214,7 @@ func (h *gateAutoRefreshHookHandler) Done() (err error) { // invoking --hold/--proceed; this means proceed (except for respecting // refresh inhibit). if h.refreshAppAwareness { - if err := runinhibit.Unlock(snapName); err != nil { + if err := runinhibit.Unlock(snapName, st.Unlocker()); err != nil { return fmt.Errorf("cannot unlock inhibit lock for snap %s: %v", snapName, err) } } @@ -232,7 +232,7 @@ func (h *gateAutoRefreshHookHandler) Done() (err error) { case snapstate.GateAutoRefreshHold: // for action=hold the ctlcmd calls HoldRefresh; only unlock runinhibit. if h.refreshAppAwareness { - if err := runinhibit.Unlock(snapName); err != nil { + if err := runinhibit.Unlock(snapName, st.Unlocker()); err != nil { return fmt.Errorf("cannot unlock inhibit lock of snap %s: %v", snapName, err) } } @@ -243,14 +243,18 @@ func (h *gateAutoRefreshHookHandler) Done() (err error) { return err } if h.refreshAppAwareness { + // Unlock global state once for IsLocked and LockWithHint instead + // of unlocking/locking state twice quickly. + st.Unlock() + defer st.Lock() // we have HintInhibitedGateRefresh lock already when running the hook, change // it to HintInhibitedForRefresh. // Also let's reuse inhibit info that was saved in Before(). - _, inhibitInfo, err := runinhibit.IsLocked(snapName) + _, inhibitInfo, err := runinhibit.IsLocked(snapName, nil) if err != nil { return err } - if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRefresh, inhibitInfo); err != nil { + if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRefresh, inhibitInfo, nil); err != nil { return fmt.Errorf("cannot set inhibit lock for snap %s: %v", snapName, err) } } @@ -284,7 +288,7 @@ func (h *gateAutoRefreshHookHandler) Error(hookErr error) (ignoreHookErr bool, e } defer lock.Unlock() - if err := runinhibit.Unlock(snapName); err != nil { + if err := runinhibit.Unlock(snapName, st.Unlocker()); err != nil { return false, fmt.Errorf("cannot release inhibit lock of snap %s: %v", snapName, err) } } diff --git a/overlord/hookstate/hooks_test.go b/overlord/hookstate/hooks_test.go index 6530634d062..d162d5777b4 100644 --- a/overlord/hookstate/hooks_test.go +++ b/overlord/hookstate/hooks_test.go @@ -145,7 +145,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookProceedRuninhibitLock( defer ctx.Unlock() // check that runinhibit hint has been set by Before() hook handler. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedGateRefresh) c.Check(info, Equals, runinhibit.InhibitInfo{Previous: snap.R(1)}) @@ -178,7 +178,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookProceedRuninhibitLock( c.Assert(change.Err(), IsNil) c.Assert(change.Status(), Equals, state.DoneStatus) - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedForRefresh) c.Check(info, Equals, runinhibit.InhibitInfo{Previous: snap.R(1)}) @@ -192,7 +192,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookHoldUnlocksRuninhibit( defer ctx.Unlock() // check that runinhibit hint has been set by Before() hook handler. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedGateRefresh) c.Check(info, Equals, runinhibit.InhibitInfo{Previous: snap.R(1)}) @@ -226,7 +226,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookHoldUnlocksRuninhibit( c.Assert(change.Status(), Equals, state.DoneStatus) // runinhibit lock is released. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -237,7 +237,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookHoldUnlocksRuninhibit( func (s *gateAutoRefreshHookSuite) TestGateAutorefreshDefaultProceedUnlocksRuninhibit(c *C) { hookInvoke := func(ctx *hookstate.Context, tomb *tomb.Tomb) ([]byte, error) { // validity, refresh is inhibited for snap-a. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedGateRefresh) c.Check(info, Equals, runinhibit.InhibitInfo{Previous: snap.R(1)}) @@ -279,7 +279,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshDefaultProceedUnlocksRunin checkIsNotHeld(c, st, "snap-a") // runinhibit lock is released. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -290,7 +290,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshDefaultProceedUnlocksRunin func (s *gateAutoRefreshHookSuite) TestGateAutorefreshDefaultProceed(c *C) { hookInvoke := func(ctx *hookstate.Context, tomb *tomb.Tomb) ([]byte, error) { // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -327,7 +327,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshDefaultProceed(c *C) { checkIsNotHeld(c, st, "snap-b") // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -338,7 +338,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshDefaultProceed(c *C) { func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookError(c *C) { hookInvoke := func(ctx *hookstate.Context, tomb *tomb.Tomb) ([]byte, error) { // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -373,7 +373,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookError(c *C) { checkIsHeld(c, st, "snap-a", "snap-a") // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -384,7 +384,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookError(c *C) { func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorAfterProceed(c *C) { hookInvoke := func(ctx *hookstate.Context, tomb *tomb.Tomb) ([]byte, error) { // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -425,7 +425,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorAfterProceed(c *C checkIsHeld(c, st, "snap-a", "snap-a") // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -436,7 +436,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorAfterProceed(c *C func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorRuninhibitUnlock(c *C) { hookInvoke := func(ctx *hookstate.Context, tomb *tomb.Tomb) ([]byte, error) { // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedGateRefresh) c.Check(info, Equals, runinhibit.InhibitInfo{Previous: snap.R(1)}) @@ -476,7 +476,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorRuninhibitUnlock( checkIsHeld(c, st, "snap-a", "snap-a") // inhibit lock is unlocked - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -485,7 +485,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorRuninhibitUnlock( func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorHoldErrorLogged(c *C) { hookInvoke := func(ctx *hookstate.Context, tomb *tomb.Tomb) ([]byte, error) { // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) @@ -532,7 +532,7 @@ func (s *gateAutoRefreshHookSuite) TestGateAutorefreshHookErrorHoldErrorLogged(c c.Check(held, HasLen, 0) // no runinhibit because the refresh-app-awareness feature is disabled. - hint, info, err := runinhibit.IsLocked("snap-a") + hint, info, err := runinhibit.IsLocked("snap-a", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(info, Equals, runinhibit.InhibitInfo{}) diff --git a/overlord/snapstate/backend.go b/overlord/snapstate/backend.go index 5dc7dd5d647..86c5d6b94d5 100644 --- a/overlord/snapstate/backend.go +++ b/overlord/snapstate/backend.go @@ -106,7 +106,7 @@ type managerBackend interface { RemoveComponentDir(cpi snap.ContainerPlaceInfo) error RemoveContainerMountUnits(cpi snap.ContainerPlaceInfo, meter progress.Meter) error DiscardSnapNamespace(snapName string) error - RemoveSnapInhibitLock(snapName string) error + RemoveSnapInhibitLock(snapName string, stateUnlocker runinhibit.Unlocker) error RemoveAllSnapAppArmorProfiles() error RemoveKernelSnapSetup(instanceName string, rev snap.Revision, meter progress.Meter) error @@ -119,7 +119,7 @@ type managerBackend interface { Candidate(sideInfo *snap.SideInfo) // refresh related - RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, decision func() error) (*osutil.FileLock, error) + RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, stateUnlocker runinhibit.Unlocker, decision func() error) (*osutil.FileLock, error) // (not a backend method because doInstall cannot access the backend) // WithSnapLock(info *snap.Info, action func() error) error diff --git a/overlord/snapstate/backend/link.go b/overlord/snapstate/backend/link.go index 443f62b40ed..406b8d1841f 100644 --- a/overlord/snapstate/backend/link.go +++ b/overlord/snapstate/backend/link.go @@ -59,6 +59,9 @@ type LinkContext struct { // establish run inhibition lock for refresh operations. RunInhibitHint runinhibit.Hint + // StateUnlocker is passed to inhibition lock operations. + StateUnlocker runinhibit.Unlocker + // RequireMountedSnapdSnap indicates that the apps and services // generated when linking need to use tooling from the snapd snap mount. RequireMountedSnapdSnap bool @@ -155,6 +158,12 @@ func updateCurrentSymlinks(info *snap.Info) (revert func(), e error) { // LinkSnap makes the snap available by generating wrappers and setting the current symlinks. func (b Backend) LinkSnap(info *snap.Info, dev snap.Device, linkCtx LinkContext, tm timings.Measurer) (rebootRequired boot.RebootInfo, e error) { + // explicitly prevent passing nil state unlocker to avoid internal errors of + // forgeting to pass the unlocker leading to deadlocks. + if linkCtx.StateUnlocker == nil { + return boot.RebootInfo{}, errors.New("internal error: LinkContext.StateUnlocker cannot be nil") + } + if info.Revision.Unset() { return boot.RebootInfo{}, fmt.Errorf("cannot link snap %q with unset revision", info.InstanceName()) } @@ -220,7 +229,7 @@ func (b Backend) LinkSnap(info *snap.Info, dev snap.Device, linkCtx LinkContext, } // Stop inhibiting application startup by removing the inhibitor file. - if err := runinhibit.Unlock(info.InstanceName()); err != nil { + if err := runinhibit.Unlock(info.InstanceName(), linkCtx.StateUnlocker); err != nil { return boot.RebootInfo{}, err } @@ -379,9 +388,14 @@ func removeGeneratedSnapdWrappers(s *snap.Info, firstInstall bool, meter progres func (b Backend) UnlinkSnap(info *snap.Info, linkCtx LinkContext, meter progress.Meter) error { var err0 error if hint := linkCtx.RunInhibitHint; hint != runinhibit.HintNotInhibited { + // explicitly prevent passing nil state unlocker to avoid internal errors of + // forgeting to pass the unlocker leading to deadlocks. + if linkCtx.StateUnlocker == nil { + return errors.New("internal error: LinkContext.StateUnlocker cannot be nil if LinkContext.RunInhibitHint is set") + } // inhibit startup of new programs inhibitInfo := runinhibit.InhibitInfo{Previous: info.SnapRevision()} - err0 = runinhibit.LockWithHint(info.InstanceName(), hint, inhibitInfo) + err0 = runinhibit.LockWithHint(info.InstanceName(), hint, inhibitInfo, linkCtx.StateUnlocker) } // remove generated services, binaries etc diff --git a/overlord/snapstate/backend/link_test.go b/overlord/snapstate/backend/link_test.go index bb705e64397..7d0ef616e75 100644 --- a/overlord/snapstate/backend/link_test.go +++ b/overlord/snapstate/backend/link_test.go @@ -78,6 +78,13 @@ type linkSuite struct { var _ = Suite(&linkSuite{}) +func mockLinkContextWithStateUnlocker() backend.LinkContext { + return backend.LinkContext{ + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, + } +} + func (s *linkSuite) TestLinkDoUndoGenerateWrappers(c *C) { const yaml = `name: hello version: 1.0 @@ -110,7 +117,7 @@ apps: ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - _, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*")) @@ -177,7 +184,7 @@ Exec=foo c.Assert(os.WriteFile(filepath.Join(iconsDir, "snap.hello.png"), []byte(""), 0644), IsNil) c.Assert(os.WriteFile(filepath.Join(iconsDir, "snap.hello.svg"), []byte(""), 0644), IsNil) - _, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*")) @@ -232,7 +239,7 @@ Exec=foo c.Assert(os.WriteFile(filepath.Join(iconsDir, "snap.hello.png"), []byte(""), 0644), IsNil) c.Assert(os.WriteFile(filepath.Join(iconsDir, "snap.hello.svg"), []byte(""), 0644), IsNil) - _, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*")) @@ -269,7 +276,7 @@ version: 1.0 ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - reboot, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + reboot, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) c.Check(reboot, Equals, boot.RebootInfo{RebootRequired: false}) @@ -303,6 +310,8 @@ version: 1.0 reboot, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{ HasOtherInstances: false, + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, }, s.perfTimings) c.Assert(err, IsNil) @@ -360,7 +369,7 @@ type: base ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - reboot, err := s.be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings) + reboot, err := s.be.LinkSnap(info, coreDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) c.Check(reboot, Equals, boot.RebootInfo{RebootRequired: true}) } @@ -382,7 +391,7 @@ type: kernel ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - reboot, err := be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings) + reboot, err := be.LinkSnap(info, coreDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) c.Check(reboot, DeepEquals, boot.RebootInfo{}) } @@ -412,7 +421,7 @@ type: snapd ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - _, err := be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings) + _, err := be.LinkSnap(info, coreDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) c.Assert(called, Equals, true) } @@ -433,11 +442,23 @@ apps: ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - _, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + linkCtx := backend.LinkContext{StateUnlocker: fakeUnlocker} + + _, err := s.be.LinkSnap(info, mockDev, linkCtx, s.perfTimings) c.Assert(err, IsNil) + // no hint file, no locking needed + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) - _, err = s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err = s.be.LinkSnap(info, mockDev, linkCtx, s.perfTimings) c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 2) + c.Check(relockCalled, Equals, 2) l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*")) c.Assert(err, IsNil) @@ -475,7 +496,7 @@ apps: ` info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)}) - _, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) err = s.be.UnlinkSnap(info, backend.LinkContext{}, progress.Null) @@ -506,7 +527,7 @@ func (s *linkSuite) TestLinkFailsForUnsetRevision(c *C) { info := &snap.Info{ SuggestedName: "foo", } - _, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, ErrorMatches, `cannot link snap "foo" with unset revision`) } @@ -545,7 +566,7 @@ func (s *linkSuite) TestLinkSnapdSnapOnCore(c *C) { info, _ := mockSnapdSnapForLink(c) - reboot, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + reboot, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) c.Assert(reboot, Equals, boot.RebootInfo{RebootRequired: false}) @@ -654,7 +675,7 @@ func (s *linkCleanupSuite) testLinkCleanupDirOnFail(c *C, dir string) { c.Assert(os.Chmod(dir, 0555), IsNil) defer os.Chmod(dir, 0755) - _, err := s.be.LinkSnap(s.info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(s.info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, NotNil) _, isPathError := err.(*os.PathError) _, isLinkError := err.(*os.LinkError) @@ -699,7 +720,7 @@ func (s *linkCleanupSuite) TestLinkCleanupOnSystemctlFail(c *C) { }) defer r() - _, err := s.be.LinkSnap(s.info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(s.info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, ErrorMatches, "ouchie") for _, d := range []string{dirs.SnapBinariesDir, dirs.SnapDesktopFilesDir, dirs.SnapServicesDir} { @@ -719,7 +740,7 @@ func (s *linkCleanupSuite) TestLinkCleansUpDataDirAndSymlinksOnSymlinkFail(c *C) c.Assert(os.Chmod(d, 0555), IsNil) defer os.Chmod(d, 0755) - _, err := s.be.LinkSnap(s.info, mockDev, backend.LinkContext{}, s.perfTimings) + _, err := s.be.LinkSnap(s.info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, ErrorMatches, `(?i).*symlink.*permission denied.*`) c.Check(s.info.DataDir(), testutil.FileAbsent) @@ -756,6 +777,8 @@ func (s *linkCleanupSuite) testLinkCleanupFailedSnapdSnapOnCorePastWrappers(c *C linkCtx := backend.LinkContext{ FirstInstall: firstInstall, + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, } reboot, err := s.be.LinkSnap(info, mockDev, linkCtx, s.perfTimings) c.Assert(err, ErrorMatches, fmt.Sprintf("symlink %s /.*/snapd/current.*: permission denied", info.Revision)) @@ -817,6 +840,8 @@ version: 1.0 _, err := s.be.LinkSnap(snapinstance, mockDev, backend.LinkContext{ HasOtherInstances: tc.otherInstances, + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, }, s.perfTimings) c.Assert(err, NotNil) @@ -871,7 +896,7 @@ func (s *snapdOnCoreUnlinkSuite) TestUndoGeneratedWrappers(c *C) { return filepath.Join(dirs.SnapServicesDir, filepath.Base(p)) } - reboot, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings) + reboot, err := s.be.LinkSnap(info, mockDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, IsNil) c.Assert(reboot, Equals, boot.RebootInfo{RebootRequired: false}) @@ -885,12 +910,20 @@ func (s *snapdOnCoreUnlinkSuite) TestUndoGeneratedWrappers(c *C) { // linked snaps do not have a run inhibition lock c.Check(filepath.Join(runinhibit.InhibitDir, "snapd.lock"), testutil.FileAbsent) + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } linkCtx := backend.LinkContext{ FirstInstall: true, RunInhibitHint: runinhibit.HintInhibitedForRefresh, + StateUnlocker: fakeUnlocker, } err = s.be.UnlinkSnap(info, linkCtx, nil) c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) // generated wrappers should be gone now for _, entry := range generatedSnapdUnits { @@ -903,6 +936,8 @@ func (s *snapdOnCoreUnlinkSuite) TestUndoGeneratedWrappers(c *C) { // unlink is idempotent err = s.be.UnlinkSnap(info, linkCtx, nil) c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 2) + c.Check(relockCalled, Equals, 2) c.Check(filepath.Join(runinhibit.InhibitDir, "snapd.lock"), testutil.FilePresent) c.Check(filepath.Join(runinhibit.InhibitDir, "snapd.refresh"), testutil.FilePresent) } @@ -928,12 +963,20 @@ func (s *snapdOnCoreUnlinkSuite) TestUnlinkNonFirstSnapdOnCoreDoesNothing(c *C) } // content list uses absolute paths already snaptest.PopulateDir("/", units) + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } linkCtx := backend.LinkContext{ FirstInstall: false, RunInhibitHint: runinhibit.HintInhibitedForRefresh, + StateUnlocker: fakeUnlocker, } err = s.be.UnlinkSnap(info, linkCtx, nil) c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) for _, unit := range units { c.Check(unit[0], testutil.FileEquals, "precious") } @@ -963,6 +1006,8 @@ apps: linkCtxWithTooling := backend.LinkContext{ RequireMountedSnapdSnap: true, + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, } _, err := s.be.LinkSnap(info, mockDev, linkCtxWithTooling, s.perfTimings) c.Assert(err, IsNil) @@ -977,6 +1022,8 @@ After=usr-lib-snapd.mount`) linkCtxNoTooling := backend.LinkContext{ RequireMountedSnapdSnap: false, + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, } _, err = s.be.LinkSnap(info, mockDev, linkCtxNoTooling, s.perfTimings) c.Assert(err, IsNil) @@ -1001,6 +1048,8 @@ apps: ServiceOptions: &wrappers.SnapServiceOptions{ QuotaGroup: grp, }, + // This is required for LinkSnap + StateUnlocker: func() (relock func()) { return func() {} }, } _, err = s.be.LinkSnap(info, mockDev, linkCtxWithGroup, s.perfTimings) c.Assert(err, IsNil) @@ -1055,7 +1104,7 @@ type: snapd be := backend.NewForPreseedMode() coreDev := boottest.MockUC20Device("run", nil) - _, err = be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings) + _, err = be.LinkSnap(info, coreDev, mockLinkContextWithStateUnlocker(), s.perfTimings) c.Assert(err, ErrorMatches, `BROKEN`) c.Assert(restartDone, Equals, true) @@ -1194,3 +1243,13 @@ func (s *linkSuite) TestKillSnapApps(c *C) { c.Assert(err, IsNil) c.Assert(called, Equals, 1) } + +func (s *linkSuite) TestLinkSnapNilStateUnlockerError(c *C) { + _, err := s.be.LinkSnap(nil, nil, backend.LinkContext{}, nil) + c.Assert(err, ErrorMatches, "internal error: LinkContext.StateUnlocker cannot be nil") +} + +func (s *linkSuite) TestUnlinkSnapNilStateUnlockerError(c *C) { + err := s.be.UnlinkSnap(nil, backend.LinkContext{RunInhibitHint: "not-nil"}, nil) + c.Assert(err, ErrorMatches, "internal error: LinkContext.StateUnlocker cannot be nil if LinkContext.RunInhibitHint is set") +} diff --git a/overlord/snapstate/backend/locking.go b/overlord/snapstate/backend/locking.go index 0bdb128e199..95064e45a8a 100644 --- a/overlord/snapstate/backend/locking.go +++ b/overlord/snapstate/backend/locking.go @@ -19,13 +19,19 @@ package backend import ( + "errors" + "github.com/snapcore/snapd/cmd/snaplock" "github.com/snapcore/snapd/cmd/snaplock/runinhibit" "github.com/snapcore/snapd/osutil" "github.com/snapcore/snapd/snap" ) -func (b Backend) RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, decision func() error) (lock *osutil.FileLock, err error) { +func (b Backend) RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, stateUnlocker runinhibit.Unlocker, decision func() error) (lock *osutil.FileLock, err error) { + if stateUnlocker == nil { + return nil, errors.New("internal error: stateUnlocker cannot be nil") + } + // A process may be created after the soft refresh done upon // the request to refresh a snap. If such process is alive by // the time this code is reached the refresh process is stopped. @@ -65,7 +71,7 @@ func (b Backend) RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, // and hard checks, as it would effectively make hard check a no-op, // but it might provide a nicer user experience. inhibitInfo := runinhibit.InhibitInfo{Previous: info.SnapRevision()} - if err := runinhibit.LockWithHint(info.InstanceName(), hint, inhibitInfo); err != nil { + if err := runinhibit.LockWithHint(info.InstanceName(), hint, inhibitInfo, stateUnlocker); err != nil { return nil, err } return lock, nil @@ -81,6 +87,7 @@ func (b Backend) RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, // Note that this is not a method of the Backend type, so that it can be // invoked from doInstall, which does not have access to a backend object. func WithSnapLock(info *snap.Info, action func() error) error { + // XXX: Should we unlock state while holding snap lock? (ie. pass runinhibit.Unlocker) lock, err := snaplock.OpenLock(info.InstanceName()) if err != nil { return err diff --git a/overlord/snapstate/backend/locking_test.go b/overlord/snapstate/backend/locking_test.go index 350b5b9def0..982c9dc40f1 100644 --- a/overlord/snapstate/backend/locking_test.go +++ b/overlord/snapstate/backend/locking_test.go @@ -48,18 +48,27 @@ func (s *lockingSuite) TestRunInhibitSnapForUnlinkPositiveDecision(c *C) { version: 1 ` info := snaptest.MockInfo(c, yaml, &snap.SideInfo{Revision: snap.R(1)}) - lock, err := s.be.RunInhibitSnapForUnlink(info, "hint", func() error { + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + lock, err := s.be.RunInhibitSnapForUnlink(info, "hint", fakeUnlocker, func() error { // This decision function returns nil so the lock is established and // the inhibition hint is set. return nil }) c.Assert(err, IsNil) c.Assert(lock, NotNil) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) lock.Close() - hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName()) + hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName(), fakeUnlocker) c.Assert(err, IsNil) c.Check(string(hint), Equals, "hint") c.Check(inhibitInfo, Equals, runinhibit.InhibitInfo{Previous: snap.R(1)}) + c.Check(unlockerCalled, Equals, 2) + c.Check(relockCalled, Equals, 2) } func (s *lockingSuite) TestRunInhibitSnapForUnlinkNegativeDecision(c *C) { @@ -67,17 +76,31 @@ func (s *lockingSuite) TestRunInhibitSnapForUnlinkNegativeDecision(c *C) { version: 1 ` info := snaptest.MockInfo(c, yaml, nil) - lock, err := s.be.RunInhibitSnapForUnlink(info, "hint", func() error { + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + lock, err := s.be.RunInhibitSnapForUnlink(info, "hint", fakeUnlocker, func() error { // This decision function returns an error so the lock is not // established and the inhibition hint is not set. return errors.New("propagated") }) c.Assert(err, ErrorMatches, "propagated") c.Assert(lock, IsNil) - hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName()) + c.Check(unlockerCalled, Equals, 0) + c.Check(relockCalled, Equals, 0) + hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName(), fakeUnlocker) c.Assert(err, IsNil) c.Check(string(hint), Equals, "") c.Check(inhibitInfo, Equals, runinhibit.InhibitInfo{}) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) +} + +func (s *linkSuite) TestRunInhibitSnapForUnlinkNilStateUnlockerError(c *C) { + _, err := s.be.RunInhibitSnapForUnlink(nil, "not-nil", nil, nil) + c.Assert(err, ErrorMatches, "internal error: stateUnlocker cannot be nil") } func (s *lockingSuite) TestWithSnapLock(c *C) { diff --git a/overlord/snapstate/backend/setup.go b/overlord/snapstate/backend/setup.go index e845694f955..a37949565ee 100644 --- a/overlord/snapstate/backend/setup.go +++ b/overlord/snapstate/backend/setup.go @@ -20,6 +20,7 @@ package backend import ( + "errors" "fmt" "os" "path/filepath" @@ -302,8 +303,11 @@ func (b Backend) UndoSetupComponent(cpi snap.ContainerPlaceInfo, installRecord * } // RemoveSnapInhibitLock removes the file controlling inhibition of "snap run". -func (b Backend) RemoveSnapInhibitLock(instanceName string) error { - return runinhibit.RemoveLockFile(instanceName) +func (b Backend) RemoveSnapInhibitLock(instanceName string, stateUnlocker runinhibit.Unlocker) error { + if stateUnlocker == nil { + return errors.New("internal error: stateUnlocker cannot be nil") + } + return runinhibit.RemoveLockFile(instanceName, stateUnlocker) } // SetupKernelModulesComponents changes kernel-modules configuration by diff --git a/overlord/snapstate/backend/setup_test.go b/overlord/snapstate/backend/setup_test.go index 63a4f6634f4..eed8d07b896 100644 --- a/overlord/snapstate/backend/setup_test.go +++ b/overlord/snapstate/backend/setup_test.go @@ -1057,3 +1057,20 @@ func (s *setupSuite) TestRemoveKernelModulesComponentsFails(c *C) { s.testRemoveKernelModulesComponents(c, newComps, firstInstalled, ksnap, kernRev, "cannot remove mount in .*: cannot disable comp3-32") } + +func (s *linkSuite) TestRemoveSnapInhibitLock(c *C) { + var unlockerCalled, relockCalled int + fakeUnlocker := func() (relock func()) { + unlockerCalled++ + return func() { relockCalled++ } + } + err := s.be.RemoveSnapInhibitLock("some-snap", fakeUnlocker) + c.Assert(err, IsNil) + c.Check(unlockerCalled, Equals, 1) + c.Check(relockCalled, Equals, 1) +} + +func (s *linkSuite) TestRemoveSnapInhibitLockNilStateUnlockerError(c *C) { + err := s.be.RemoveSnapInhibitLock("some-snap", nil) + c.Assert(err, ErrorMatches, "internal error: stateUnlocker cannot be nil") +} diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index 58ab52ea88b..62bd3caea43 100644 --- a/overlord/snapstate/backend_test.go +++ b/overlord/snapstate/backend_test.go @@ -1551,7 +1551,7 @@ func (f *fakeSnappyBackend) RemoveAllSnapAppArmorProfiles() error { return f.maybeErrForLastOp() } -func (f *fakeSnappyBackend) RemoveSnapInhibitLock(snapName string) error { +func (f *fakeSnappyBackend) RemoveSnapInhibitLock(snapName string, stateUnlocker runinhibit.Unlocker) error { f.appendOp(&fakeOp{ op: "remove-inhibit-lock", name: snapName, @@ -1632,7 +1632,7 @@ func (f *fakeSnappyBackend) RemoveSnapAliases(snapName string) error { return f.maybeErrForLastOp() } -func (f *fakeSnappyBackend) RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, decision func() error) (lock *osutil.FileLock, err error) { +func (f *fakeSnappyBackend) RunInhibitSnapForUnlink(info *snap.Info, hint runinhibit.Hint, stateUnlocker runinhibit.Unlocker, decision func() error) (lock *osutil.FileLock, err error) { f.appendOp(&fakeOp{ op: "run-inhibit-snap-for-unlink", name: info.InstanceName(), diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go index e3b387c9870..cb2235ff4ac 100644 --- a/overlord/snapstate/handlers.go +++ b/overlord/snapstate/handlers.go @@ -1344,6 +1344,8 @@ func (m *SnapManager) restoreUnlinkOnError(t *state.Task, info *snap.Info, other IsUndo: true, ServiceOptions: opts, HasOtherInstances: otherInstances, + // passed state must be locked + StateUnlocker: st.Unlocker(), } _, err = m.backend.LinkSnap(info, deviceCtx, linkCtx, tm) return err @@ -1461,6 +1463,7 @@ func (m *SnapManager) doUnlinkCurrentSnap(t *state.Task, _ *tomb.Tomb) (err erro // This task is only used for unlinking a snap during refreshes so we // can safely hard-code this condition here. RunInhibitHint: runinhibit.HintInhibitedForRefresh, + StateUnlocker: st.Unlocker(), SkipBinaries: skipBinaries, HasOtherInstances: otherInstances, } @@ -1607,6 +1610,7 @@ func (m *SnapManager) undoUnlinkCurrentSnap(t *state.Task, _ *tomb.Tomb) error { IsUndo: true, ServiceOptions: opts, HasOtherInstances: otherInstances, + StateUnlocker: st.Unlocker(), } reboot, err := m.backend.LinkSnap(oldInfo, deviceCtx, linkCtx, perfTimings) if err != nil { @@ -2213,6 +2217,7 @@ func (m *SnapManager) doLinkSnap(t *state.Task, _ *tomb.Tomb) (err error) { FirstInstall: firstInstall, ServiceOptions: opts, HasOtherInstances: otherInstances, + StateUnlocker: st.Unlocker(), } // on UC18+, snap tooling comes from the snapd snap so we need generated // mount units to depend on the snapd snap mount units @@ -2839,6 +2844,7 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error { FirstInstall: firstInstall, IsUndo: true, HasOtherInstances: otherInstances, + StateUnlocker: st.Unlocker(), } var backendErr error @@ -3388,20 +3394,6 @@ func (m *SnapManager) doKillSnapApps(t *state.Task, _ *tomb.Tomb) (err error) { lock.Lock() defer lock.Unlock() - inhibitInfo := runinhibit.InhibitInfo{Previous: snapsup.Revision()} - if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRemove, inhibitInfo); err != nil { - return err - } - // Note: The snap hint lock file is completely removed in “discard-snap” - // so we only need to unlock it in case of an error here or during undo. - defer func() { - // Unlock snap inhibition if anything goes wrong afterwards to - // avoid keeping the snap stuck at this inhibited state. - if err != nil { - runinhibit.Unlock(snapName) - } - }() - var reason snap.AppKillReason if err := t.Get("kill-reason", &reason); err != nil && !errors.Is(err, state.ErrNoState) { return err @@ -3410,11 +3402,27 @@ func (m *SnapManager) doKillSnapApps(t *state.Task, _ *tomb.Tomb) (err error) { perfTimings := state.TimingsForTask(t) defer perfTimings.Save(st) + inhibitInfo := runinhibit.InhibitInfo{Previous: snapsup.Revision()} + if err := runinhibit.LockWithHint(snapName, runinhibit.HintInhibitedForRemove, inhibitInfo, st.Unlocker()); err != nil { + return err + } + // State lock is not needed for killing apps or stopping services and since those // can take some time, let's unlock the state st.Unlock() defer st.Lock() + // Note: The snap hint lock file is completely removed in “discard-snap” + // so we only need to unlock it in case of an error here or during undo. + defer func() { + // Unlock snap inhibition if anything goes wrong afterwards to + // avoid keeping the snap stuck at this inhibited state. + if err != nil { + // state is unlocked, it is okay to pass nil here + runinhibit.Unlock(snapName, nil) + } + }() + if err := m.backend.KillSnapApps(snapName, reason, perfTimings); err != nil { // Snap processes termination is best-effort and task should continue // without returning an error. This is to avoid a maliciously crafted snap @@ -3455,7 +3463,7 @@ func (m *SnapManager) undoKillSnapApps(t *state.Task, _ *tomb.Tomb) error { return err } - if err := runinhibit.Unlock(snapsup.InstanceName()); err != nil { + if err := runinhibit.Unlock(snapsup.InstanceName(), st.Unlocker()); err != nil { return err } @@ -3568,6 +3576,7 @@ func (m *SnapManager) undoUnlinkSnap(t *state.Task, _ *tomb.Tomb) error { IsUndo: true, ServiceOptions: opts, HasOtherInstances: otherInstances, + StateUnlocker: st.Unlocker(), } reboot, err := m.backend.LinkSnap(info, deviceCtx, linkCtx, perfTimings) if err != nil { @@ -3720,7 +3729,7 @@ func (m *SnapManager) doDiscardSnap(t *state.Task, _ *tomb.Tomb) error { t.Errorf("cannot discard snap namespace %q, will retry in 3 mins: %s", snapsup.InstanceName(), err) return &state.Retry{After: 3 * time.Minute} } - err = m.backend.RemoveSnapInhibitLock(snapsup.InstanceName()) + err = m.backend.RemoveSnapInhibitLock(snapsup.InstanceName(), st.Unlocker()) if err != nil { return err } diff --git a/overlord/snapstate/handlers_link_test.go b/overlord/snapstate/handlers_link_test.go index 4a1361de39f..903141e4c98 100644 --- a/overlord/snapstate/handlers_link_test.go +++ b/overlord/snapstate/handlers_link_test.go @@ -2683,7 +2683,7 @@ func (s *linkSnapSuite) testDoKillSnapApps(c *C, svc bool) { c.Assert(s.fakeBackend.ops.Ops(), DeepEquals, expected.Ops()) c.Check(s.fakeBackend.ops, DeepEquals, expected) - hint, _, err := runinhibit.IsLocked("some-snap") + hint, _, err := runinhibit.IsLocked("some-snap", nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintInhibitedForRemove) @@ -2737,7 +2737,7 @@ func (s *linkSnapSuite) TestDoKillSnapAppsUnlocksOnError(c *C) { c.Assert(task.Status(), Equals, state.ErrorStatus) - hint, _, err := runinhibit.IsLocked("some-snap") + hint, _, err := runinhibit.IsLocked("some-snap", nil) c.Assert(err, IsNil) // On error hint inhibition file is unlocked c.Check(hint, Equals, runinhibit.HintNotInhibited) @@ -2784,7 +2784,7 @@ func (s *linkSnapSuite) TestDoKillSnapAppsTerminateBestEffort(c *C) { c.Assert(task.Status(), Equals, state.DoneStatus) - hint, _, err := runinhibit.IsLocked("some-snap") + hint, _, err := runinhibit.IsLocked("some-snap", nil) c.Assert(err, IsNil) // Error is ignored, inhibition lock is held c.Check(hint, Equals, runinhibit.HintInhibitedForRemove) @@ -2863,7 +2863,7 @@ func (s *linkSnapSuite) testDoUndoKillSnapApps(c *C, svc bool) { c.Assert(s.fakeBackend.ops.Ops(), DeepEquals, expected.Ops()) c.Check(s.fakeBackend.ops, DeepEquals, expected) - hint, _, err := runinhibit.IsLocked("some-snap") + hint, _, err := runinhibit.IsLocked("some-snap", nil) c.Assert(err, IsNil) // On undo hint inhibition file is unlocked c.Check(hint, Equals, runinhibit.HintNotInhibited) diff --git a/overlord/snapstate/refresh.go b/overlord/snapstate/refresh.go index 86663ee1eeb..c722070419c 100644 --- a/overlord/snapstate/refresh.go +++ b/overlord/snapstate/refresh.go @@ -175,7 +175,7 @@ func (err BusySnapError) Pids() []int { // the refresh change and continue running existing app processes. func hardEnsureNothingRunningDuringRefresh(backend managerBackend, st *state.State, snapst *SnapState, snapsup *SnapSetup, info *snap.Info) (bool, *osutil.FileLock, error) { var inhibitionTimeout bool - lock, err := backend.RunInhibitSnapForUnlink(info, runinhibit.HintInhibitedForRefresh, func() error { + lock, err := backend.RunInhibitSnapForUnlink(info, runinhibit.HintInhibitedForRefresh, st.Unlocker(), func() error { // In case of successful refresh inhibition the snap state is modified // to indicate when the refresh was first inhibited. If the first // refresh inhibition is outside of a grace period then refresh diff --git a/overlord/snapstate/refresh_test.go b/overlord/snapstate/refresh_test.go index 7fab8ce2bb4..8bddfd12afa 100644 --- a/overlord/snapstate/refresh_test.go +++ b/overlord/snapstate/refresh_test.go @@ -177,7 +177,7 @@ func (s *refreshSuite) TestDoSoftRefreshCheckAllowed(c *C) { c.Assert(err, IsNil) // In addition, the inhibition lock is not set. - hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName()) + hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName(), nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(inhibitInfo, Equals, runinhibit.InhibitInfo{}) @@ -201,7 +201,7 @@ func (s *refreshSuite) TestDoSoftRefreshCheckDisallowed(c *C) { c.Assert(err, ErrorMatches, `snap "pkg" has running apps or hooks, pids: 123`) // Validity check: the inhibition lock was not set. - hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName()) + hint, inhibitInfo, err := runinhibit.IsLocked(info.InstanceName(), nil) c.Assert(err, IsNil) c.Check(hint, Equals, runinhibit.HintNotInhibited) c.Check(inhibitInfo, Equals, runinhibit.InhibitInfo{}) diff --git a/tests/regression/lp-2084730/task.yaml b/tests/regression/lp-2084730/task.yaml new file mode 100644 index 00000000000..b40dec348a1 --- /dev/null +++ b/tests/regression/lp-2084730/task.yaml @@ -0,0 +1,63 @@ +summary: Ensure that snapd state doesn't get stuck if inhibition hint file is locked. + +details: | + Check that snapd state will not stay locked when inhibition hint file lock is held. + For context: In LP #2084730, A problem was discovered related to refresh app awareness + feature which, in a very specific scenario, can cause snapd to enter a deadlock state + because snapd was tring to hold an exclusive lock on the inhibition hint lock file + while locking global state which was already locked by snap run. This results in all API + calls hanging which was made worse by snap run trying to call the snapd API causing + a deadlock. + +environment: + INHIBITION_LOCK_FILE: /var/lib/snapd/inhibit/test-snapd-sh.lock + +prepare: | + snap install --stable test-snapd-sh + mkdir -p /var/lib/snapd/inhibit + touch "$INHIBITION_LOCK_FILE" + + snap set core experimental.parallel-instances=true + +restore: | + # release inhibition hint lock + systemctl stop inhibition-file-locker.service + + # Wait for refresh to finish to be able to remove the snap + snap debug ensure-state-soon + retry -n 10 sh -c "snap changes | NOMATCH Doing" + + snap set core experimental.parallel-instances! + +execute: | + systemd-run --unit inhibition-file-locker.service flock "$INHIBITION_LOCK_FILE" --command "sleep 10000" + # Wait for the inhibition file lock to be held + retry -n 10 not flock --timeout 0 "$INHIBITION_LOCK_FILE" --command "true" + + # This refresh will block because it tries to hold the inhibition file lock for test-snapd-sh + snap refresh --no-wait --edge test-snapd-sh > locked-change-id + + # wait until snapd is blocked in link-snap + # avoid using the API directly to not take the state lock + locked_id="$(cat locked-change-id)" + retry -n 50 --wait 1 sh -c "snap debug state /var/lib/snapd/state.json --change=$locked_id | MATCH 'Doing .* Make current revision for snap .* unavailable'" + + # and still waiting + snap debug state /var/lib/snapd/state.json --change="$locked_id" | MATCH 'Doing .* Make current revision for snap .* unavailable' + + # Check snapd has the inhibition hint lock file open + if command -v lsof; then + lsof "$INHIBITION_LOCK_FILE" | MATCH "snapd" + fi + + # Check that snapd state is not locked when trying to hold the inhibition file lock above + timeout 5 snap debug api /v2/snaps + + # Check that operations changing the state of the snap (remove, or another refresh) fail the conflict check + snap refresh --beta test-snapd-sh 2>&1 | MATCH 'snap "test-snapd-sh" has "refresh-snap" change in progress' + snap remove test-snapd-sh 2>&1 | MATCH 'snap "test-snapd-sh" has "refresh-snap" change in progress' + + # Check that parallel instance of the blocked snap is not affected + snap install --stable test-snapd-sh_instance + snap refresh --edge test-snapd-sh_instance + snap remove test-snapd-sh_instance