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

o/snapstate: move the reboot part of Backend.LinkSnap to a new method #14641

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+270 −71
Open

o/snapstate: move the reboot part of Backend.LinkSnap to a new method #14641

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
3 changes: 2 additions & 1 deletion overlord/snapstate/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,12 @@ type managerBackend interface {
SetupKernelModulesComponents(currentComps, finalComps []*snap.ComponentSideInfo, ksnapName string, ksnapRev snap.Revision, meter progress.Meter) (err error)
CopySnapData(newSnap, oldSnap *snap.Info, opts *dirs.SnapDirOptions, meter progress.Meter) error
SetupSnapSaveData(info *snap.Info, dev snap.Device, meter progress.Meter) error
LinkSnap(info *snap.Info, dev snap.Device, linkCtx backend.LinkContext, tm timings.Measurer) (rebootInfo boot.RebootInfo, err error)
LinkSnap(info *snap.Info, dev snap.Device, linkCtx backend.LinkContext, tm timings.Measurer) error
LinkComponent(cpi snap.ContainerPlaceInfo, snapRev snap.Revision) error
StartServices(svcs []*snap.AppInfo, disabledSvcs *wrappers.DisabledServices, meter progress.Meter, tm timings.Measurer) error
StopServices(svcs []*snap.AppInfo, reason snap.ServiceStopReason, meter progress.Meter, tm timings.Measurer) error
QueryDisabledServices(info *snap.Info, pb progress.Meter) (*wrappers.DisabledServices, error)
MaybePrepareReboot(info *snap.Info, dev snap.Device, isUndo bool) (boot.RebootInfo, error)

// the undoers for install
UndoSetupSnap(s snap.PlaceInfo, typ snap.Type, installRecord *backend.InstallRecord, dev snap.Device, meter progress.Meter) error
Expand Down
48 changes: 26 additions & 22 deletions overlord/snapstate/backend/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@
// installed
FirstInstall bool

// IsUndo is set when we are installing the previous snap while
// performing a revert of the latest one that was installed
IsUndo bool

// ServiceOptions is used to configure services.
ServiceOptions *wrappers.SnapServiceOptions

Expand Down Expand Up @@ -153,10 +149,28 @@
return revertFunc, nil
}

// MaybePrepareReboot configures the system for a reboot if necesssary because
// of a snap refresh. isUndo must be set when we are installing the previous
// snap while performing a revert of the latest one that was installed
func (b Backend) MaybePrepareReboot(info *snap.Info, dev snap.Device, isUndo bool) (boot.RebootInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Would using a boot.NextBootContext as the param rather than the isUndo boolean be an option? I guess it doesn't matter too much.

if b.preseed {
return boot.RebootInfo{}, nil
}

Check warning on line 158 in overlord/snapstate/backend/link.go

View check run for this annotation

Codecov / codecov/patch

overlord/snapstate/backend/link.go#L157-L158 

Added lines #L157 - L158 were not covered by tests

bootCtx := boot.NextBootContext{BootWithoutTry: isUndo}
rebootInfo, err := boot.Participant(
info, info.Type(), dev).SetNextBoot(bootCtx)
if err != nil {
return boot.RebootInfo{}, err
}

Check warning on line 165 in overlord/snapstate/backend/link.go

View check run for this annotation

Codecov / codecov/patch

overlord/snapstate/backend/link.go#L164-L165 

Added lines #L164 - L165 were not covered by tests

return rebootInfo, nil
}

// 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) {
func (b Backend) LinkSnap(info *snap.Info, dev snap.Device, linkCtx LinkContext, tm timings.Measurer) (e error) {
if info.Revision.Unset() {
return boot.RebootInfo{}, fmt.Errorf("cannot link snap %q with unset revision", info.InstanceName())
return fmt.Errorf("cannot link snap %q with unset revision", info.InstanceName())
}

osutil.MaybeInjectFault("link-snap")
Expand All @@ -167,7 +181,7 @@
restart, err = b.generateWrappers(info, linkCtx)
})
if err != nil {
return boot.RebootInfo{}, err
return err
}
defer func() {
if e == nil {
Expand All @@ -178,21 +192,11 @@
})
}()

var rebootInfo boot.RebootInfo
if !b.preseed {
bootCtx := boot.NextBootContext{BootWithoutTry: linkCtx.IsUndo}
rebootInfo, err = boot.Participant(
info, info.Type(), dev).SetNextBoot(bootCtx)
if err != nil {
return boot.RebootInfo{}, err
}
}

// only after link snap it will be possible to execute snap
// applications, so ensure that the shared snap directory exists for
// parallel installed snaps
if err := createSharedSnapDirForParallelInstance(info); err != nil {
return boot.RebootInfo{}, err
return err

Check warning on line 199 in overlord/snapstate/backend/link.go

View check run for this annotation

Codecov / codecov/patch

overlord/snapstate/backend/link.go#L199 

Added line #L199 was not covered by tests
}
cleanupSharedParallelInstanceDir := func() {
if !linkCtx.HasOtherInstances {
Expand All @@ -203,7 +207,7 @@
revertSymlinks, err := updateCurrentSymlinks(info)
if err != nil {
cleanupSharedParallelInstanceDir()
return boot.RebootInfo{}, err
return err
}
// if anything below here could return error, you need to
// somehow clean up whatever updateCurrentSymlinks did
Expand All @@ -214,17 +218,17 @@
revertSymlinks()
cleanupSharedParallelInstanceDir()

return boot.RebootInfo{}, err
return err
}

}

// Stop inhibiting application startup by removing the inhibitor file.
if err := runinhibit.Unlock(info.InstanceName()); err != nil {
return boot.RebootInfo{}, err
return err

Check warning on line 228 in overlord/snapstate/backend/link.go

View check run for this annotation

Codecov / codecov/patch

overlord/snapstate/backend/link.go#L228 

Added line #L228 was not covered by tests
}

return rebootInfo, nil
return nil
}

func (b Backend) LinkComponent(cpi snap.ContainerPlaceInfo, snapRev snap.Revision) error {
Expand Down
72 changes: 49 additions & 23 deletions overlord/snapstate/backend/link_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*"))
Expand Down Expand Up @@ -177,7 +177,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*"))
Expand Down Expand Up @@ -232,7 +232,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*"))
Expand Down Expand Up @@ -269,9 +269,12 @@ 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)
err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, mockDev, isUndo)
c.Assert(err, IsNil)
c.Check(reboot, Equals, boot.RebootInfo{RebootRequired: false})

mountDir := info.MountDir()
Expand Down Expand Up @@ -301,11 +304,14 @@ version: 1.0
`
info := snaptest.MockSnapInstance(c, "hello_foo", yaml, &snap.SideInfo{Revision: snap.R(11)})

reboot, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{
err := s.be.LinkSnap(info, mockDev, backend.LinkContext{
HasOtherInstances: false,
}, s.perfTimings)
c.Assert(err, IsNil)

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, mockDev, isUndo)
c.Assert(err, IsNil)
c.Check(reboot, Equals, boot.RebootInfo{RebootRequired: false})

mountDir := info.MountDir()
Expand Down Expand Up @@ -360,7 +366,11 @@ type: base
`
info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)})

reboot, err := s.be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings)
err := s.be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, coreDev, isUndo)
c.Assert(err, IsNil)
c.Check(reboot, Equals, boot.RebootInfo{RebootRequired: true})
}
Expand All @@ -382,7 +392,11 @@ type: kernel
`
info := snaptest.MockSnap(c, yaml, &snap.SideInfo{Revision: snap.R(11)})

reboot, err := be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings)
err := be.LinkSnap(info, coreDev, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, mockDev, isUndo)
c.Assert(err, IsNil)
c.Check(reboot, DeepEquals, boot.RebootInfo{})
}
Expand Down Expand Up @@ -412,7 +426,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)
c.Assert(called, Equals, true)
}
Expand All @@ -433,10 +447,10 @@ 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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

_, err = s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings)
err = s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

l, err := filepath.Glob(filepath.Join(dirs.SnapBinariesDir, "*"))
Expand Down Expand Up @@ -475,7 +489,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

err = s.be.UnlinkSnap(info, backend.LinkContext{}, progress.Null)
Expand Down Expand Up @@ -506,7 +520,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, ErrorMatches, `cannot link snap "foo" with unset revision`)
}

Expand Down Expand Up @@ -545,7 +559,11 @@ func (s *linkSuite) TestLinkSnapdSnapOnCore(c *C) {

info, _ := mockSnapdSnapForLink(c)

reboot, err := s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings)
err = s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, mockDev, isUndo)
c.Assert(err, IsNil)
c.Assert(reboot, Equals, boot.RebootInfo{RebootRequired: false})

Expand Down Expand Up @@ -654,7 +672,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, NotNil)
_, isPathError := err.(*os.PathError)
_, isLinkError := err.(*os.LinkError)
Expand Down Expand Up @@ -699,7 +717,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, ErrorMatches, "ouchie")

for _, d := range []string{dirs.SnapBinariesDir, dirs.SnapDesktopFilesDir, dirs.SnapServicesDir} {
Expand All @@ -719,7 +737,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, ErrorMatches, `(?i).*symlink.*permission denied.*`)

c.Check(s.info.DataDir(), testutil.FileAbsent)
Expand Down Expand Up @@ -757,8 +775,12 @@ func (s *linkCleanupSuite) testLinkCleanupFailedSnapdSnapOnCorePastWrappers(c *C
linkCtx := backend.LinkContext{
FirstInstall: firstInstall,
}
reboot, err := s.be.LinkSnap(info, mockDev, linkCtx, s.perfTimings)
err = s.be.LinkSnap(info, mockDev, linkCtx, s.perfTimings)
c.Assert(err, ErrorMatches, fmt.Sprintf("symlink %s /.*/snapd/current.*: permission denied", info.Revision))

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, mockDev, isUndo)
c.Assert(err, IsNil)
c.Assert(reboot, Equals, boot.RebootInfo{RebootRequired: false})

checker := testutil.FilePresent
Expand Down Expand Up @@ -814,7 +836,7 @@ version: 1.0

snapinstance := snaptest.MockSnapInstance(c, "instance-snap_foo", yaml, &snap.SideInfo{Revision: snap.R(11)})

_, err := s.be.LinkSnap(snapinstance, mockDev,
err := s.be.LinkSnap(snapinstance, mockDev,
backend.LinkContext{
HasOtherInstances: tc.otherInstances,
},
Expand Down Expand Up @@ -871,7 +893,11 @@ 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)
err = s.be.LinkSnap(info, mockDev, backend.LinkContext{}, s.perfTimings)
c.Assert(err, IsNil)

isUndo := false
reboot, err := s.be.MaybePrepareReboot(info, mockDev, isUndo)
c.Assert(err, IsNil)
c.Assert(reboot, Equals, boot.RebootInfo{RebootRequired: false})

Expand Down Expand Up @@ -964,7 +990,7 @@ apps:
linkCtxWithTooling := backend.LinkContext{
RequireMountedSnapdSnap: true,
}
_, err := s.be.LinkSnap(info, mockDev, linkCtxWithTooling, s.perfTimings)
err := s.be.LinkSnap(info, mockDev, linkCtxWithTooling, s.perfTimings)
c.Assert(err, IsNil)
c.Assert(filepath.Join(dirs.SnapServicesDir, "snap.hello.svc.service"), testutil.FileContains,
`Wants=usr-lib-snapd.mount
Expand All @@ -978,7 +1004,7 @@ After=usr-lib-snapd.mount`)
linkCtxNoTooling := backend.LinkContext{
RequireMountedSnapdSnap: false,
}
_, err = s.be.LinkSnap(info, mockDev, linkCtxNoTooling, s.perfTimings)
err = s.be.LinkSnap(info, mockDev, linkCtxNoTooling, s.perfTimings)
c.Assert(err, IsNil)
c.Assert(filepath.Join(dirs.SnapServicesDir, "snap.hello.svc.service"), Not(testutil.FileContains), `usr-lib-snapd.mount`)
}
Expand All @@ -1002,7 +1028,7 @@ apps:
QuotaGroup: grp,
},
}
_, err = s.be.LinkSnap(info, mockDev, linkCtxWithGroup, s.perfTimings)
err = s.be.LinkSnap(info, mockDev, linkCtxWithGroup, s.perfTimings)
c.Assert(err, IsNil)
c.Assert(filepath.Join(dirs.SnapServicesDir, "snap.hello.svc.service"), testutil.FileContains,
"\nSlice=snap.foogroup.slice\n")
Expand Down Expand Up @@ -1055,7 +1081,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, backend.LinkContext{}, s.perfTimings)
c.Assert(err, ErrorMatches, `BROKEN`)
c.Assert(restartDone, Equals, true)

Expand Down
29 changes: 20 additions & 9 deletions overlord/snapstate/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,24 @@ func (f *fakeSnappyBackend) SetupSnapSaveData(info *snap.Info, _ snap.Device, me
return f.maybeErrForLastOp()
}

func (f *fakeSnappyBackend) LinkSnap(info *snap.Info, dev snap.Device, linkCtx backend.LinkContext, tm timings.Measurer) (rebootInfo boot.RebootInfo, err error) {
func (f *fakeSnappyBackend) MaybePrepareReboot(
info *snap.Info, dev snap.Device, isUndo bool) (boot.RebootInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth capturing isUndo on the op to validate that it is set correctly?


op := &fakeOp{
op: "maybe-prepare-reboot",
}
f.appendOp(op)

reboot := false
if f.linkSnapMaybeReboot {
reboot = info.InstanceName() == dev.Base() ||
(f.linkSnapRebootFor != nil && f.linkSnapRebootFor[info.InstanceName()])
}

return boot.RebootInfo{RebootRequired: reboot}, nil
}

func (f *fakeSnappyBackend) LinkSnap(info *snap.Info, dev snap.Device, linkCtx backend.LinkContext, tm timings.Measurer) (err error) {
if info.MountDir() == f.linkSnapWaitTrigger {
f.linkSnapWaitCh <- 1
<-f.linkSnapWaitCh
Expand All @@ -1290,18 +1307,12 @@ func (f *fakeSnappyBackend) LinkSnap(info *snap.Info, dev snap.Device, linkCtx b
if info.MountDir() == f.linkSnapFailTrigger {
op.op = "link-snap.failed"
f.appendOp(op)
return boot.RebootInfo{RebootRequired: false}, errors.New("fail")
return errors.New("fail")
}

f.appendOp(op)

reboot := false
if f.linkSnapMaybeReboot {
reboot = info.InstanceName() == dev.Base() ||
(f.linkSnapRebootFor != nil && f.linkSnapRebootFor[info.InstanceName()])
}

return boot.RebootInfo{RebootRequired: reboot}, nil
return nil
}

func (f *fakeSnappyBackend) LinkComponent(cpi snap.ContainerPlaceInfo, snapRev snap.Revision) error {
Expand Down
Loading
Loading