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.

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

o/snapstate: support user-services when refreshing snaps #13905

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
41 changes: 30 additions & 11 deletions overlord/snapstate/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ type fakeOp struct {
unlinkSkipBinaries bool
skipKernelExtraction bool

services []string
disabledServices []string
services []string
disabledServices []string
disabledUserServices map[int][]string

vitalityRank int

Expand Down Expand Up @@ -800,7 +801,8 @@ type fakeSnappyBackend struct {
copySnapDataFailTrigger string
emptyContainer snap.Container

servicesCurrentlyDisabled []string
servicesCurrentlyDisabled []string
userServicesCurrentlyDisabled map[int][]string

lockDir string

Expand Down Expand Up @@ -1220,27 +1222,44 @@ func (f *fakeSnappyBackend) StopServices(svcs []*snap.AppInfo, reason snap.Servi
}

func (f *fakeSnappyBackend) QueryDisabledServices(info *snap.Info, meter progress.Meter) (*wrappers.DisabledServices, error) {
var l []string

// return the disabled services as disabled and nothing else
m := make(map[string]bool)
for _, svc := range f.servicesCurrentlyDisabled {
m[svc] = false
m[svc] = true
}

um := make(map[int]map[string]bool)
for uid, svcs := range f.userServicesCurrentlyDisabled {
umi := make(map[string]bool)
for _, svc := range svcs {
umi[svc] = true
}
um[uid] = umi
}

for name, enabled := range m {
if !enabled {
l = append(l, name)
var l []string
ul := make(map[int][]string)
for _, svc := range info.Services() {
if m[svc.Name] {
l = append(l, svc.Name)
} else {
for uid, umi := range um {
if umi[svc.Name] {
ul[uid] = append(ul[uid], svc.Name)
}
}
}
}

f.appendOp(&fakeOp{
op: "current-snap-service-states",
disabledServices: f.servicesCurrentlyDisabled,
op: "current-snap-service-states",
disabledServices: f.servicesCurrentlyDisabled,
disabledUserServices: f.userServicesCurrentlyDisabled,
})

return &wrappers.DisabledServices{
SystemServices: l,
UserServices: ul,
}, f.maybeErrForLastOp()
}

Expand Down
1 change: 1 addition & 0 deletions overlord/snapstate/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ var (
)

type AuxStoreInfo = auxStoreInfo
type DisabledServices = disabledServices

// link, misc handlers
var (
Expand Down
123 changes: 92 additions & 31 deletions overlord/snapstate/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -1954,33 +1954,48 @@ func writeSeqFile(name string, snapst *SnapState) error {
return osutil.AtomicWriteFile(p, b, 0644, 0)
}

// missingDisabledServices returns a list of services that are present in
// this snap info and should be disabled as well as a list of disabled
type disabledServices struct {
MissingSystemServices []string
FoundSystemServices []string
MissingUserServices map[int][]string
FoundUserServices map[int][]string
}

// missingDisabledServices returns lists of services that are present in
// this snap info and should be disabled as well as lists of disabled
// services that are currently missing (i.e. they were renamed).
// present in this snap info.
// the first arg is the disabled services when the snap was last active
func missingDisabledServices(svcs []string, info *snap.Info) ([]string, []string, error) {
// make a copy of all the previously disabled services that we will remove
// from, as well as an empty list to add to for the found services
missingSvcs := []string{}
foundSvcs := []string{}

// for all the previously disabled services, check if they are in the
// current snap info revision as services or not
for _, disabledSvcName := range svcs {
// check if the service is an app _and_ is a service
if app, ok := info.Apps[disabledSvcName]; ok && app.IsService() {
foundSvcs = append(foundSvcs, disabledSvcName)
} else {
missingSvcs = append(missingSvcs, disabledSvcName)
// the first arg is the disabled system services when the snap was last active
// the second arg is the disabled user services when the snap was last active
func missingDisabledServices(sysSvcs []string, userSvcs map[int][]string, info *snap.Info) (*disabledServices, error) {
overview := &disabledServices{
MissingUserServices: make(map[int][]string),
FoundUserServices: make(map[int][]string),
}

categorize := func(names []string) ([]string, []string) {
foundSvcs := []string{}
missingSvcs := []string{}
for _, name := range names {
// check if the service is an app _and_ is a service
if app, ok := info.Apps[name]; ok && app.IsService() {
foundSvcs = append(foundSvcs, name)
} else {
missingSvcs = append(missingSvcs, name)
}
}
sort.Strings(missingSvcs)
sort.Strings(foundSvcs)
return foundSvcs, missingSvcs
}

// sort the lists for easier testing
sort.Strings(missingSvcs)
sort.Strings(foundSvcs)

return foundSvcs, missingSvcs, nil
overview.FoundSystemServices, overview.MissingSystemServices = categorize(sysSvcs)
for uid, svcs := range userSvcs {
found, missing := categorize(svcs)
overview.FoundUserServices[uid] = found
overview.MissingUserServices[uid] = missing
}
return overview, nil
}

// LinkSnapParticipant is an interface for interacting with snap link/unlink
Expand Down Expand Up @@ -2998,6 +3013,11 @@ func (m *SnapManager) startSnapServices(t *state.Task, _ *tomb.Tomb) error {
return err
}

logger.Noticef("previously disabled system-services: %v", snapst.LastActiveDisabledServices)
for uid, svcs := range snapst.LastActiveDisabledUserServices {
logger.Noticef("previously disabled user-services [%d]: %v", uid, svcs)
Meulengracht marked this conversation as resolved.
Show resolved Hide resolved
}

// check if any previously disabled services are now no longer services and
// log messages about that
for _, svc := range snapst.LastActiveDisabledServices {
Expand All @@ -3013,29 +3033,39 @@ func (m *SnapManager) startSnapServices(t *state.Task, _ *tomb.Tomb) error {
// as well as the services which are not present in this revision, but were
// present and disabled in a previous one and as such should be kept inside
// snapst for persistent storage
svcsToDisable, svcsToSave, err := missingDisabledServices(snapst.LastActiveDisabledServices, currentInfo)
missingSvcsOverview, err := missingDisabledServices(
snapst.LastActiveDisabledServices,
snapst.LastActiveDisabledUserServices,
currentInfo)
if err != nil {
return err
}

// check what services with "InstallMode: disable" need to be disabled
svcsToDisableFromInstallMode, err := installModeDisabledServices(st, snapst, currentInfo)
svcsToDisable, err := installModeDisabledServices(st, snapst, currentInfo)
if err != nil {
return err
}
svcsToDisable = append(svcsToDisable, svcsToDisableFromInstallMode...)

// append services that were disabled by hooks (they should not get re-enabled)
svcsToDisable = append(svcsToDisable, snapst.ServicesDisabledByHooks...)

// Insert it into the overview, into the 'found' list. Re-use the entire
// list for all users (-1) as there won't be any difference between users
// in this specific case (and to allow us to keep a singular list of services
// that need to be disabled).
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment is not very clear, can we use a single list because hooks cannot target single users via snapctl commands?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your comment let me to a missing case of support for user-services. The service tracking code for hooks did not account for user-services - I added this in here, as it is required for completeness in the refresh handling.

missingSvcsOverview.FoundSystemServices = append(missingSvcsOverview.FoundSystemServices, svcsToDisable...)
missingSvcsOverview.FoundUserServices[-1] = svcsToDisable

// save the current last-active-disabled-services before we re-write it in case we
// need to undo this
t.Set("old-last-active-disabled-services", snapst.LastActiveDisabledServices)
t.Set("old-last-active-disabled-user-services", snapst.LastActiveDisabledUserServices)

// commit the missing services to state so when we unlink this revision and
// go to a different revision with potentially different service names, the
// currently missing service names will be re-disabled if they exist later
snapst.LastActiveDisabledServices = svcsToSave
snapst.LastActiveDisabledServices = missingSvcsOverview.MissingSystemServices
snapst.LastActiveDisabledUserServices = missingSvcsOverview.MissingUserServices

// reset services tracked by operations from hooks
snapst.ServicesDisabledByHooks = nil
Expand All @@ -3055,8 +3085,13 @@ func (m *SnapManager) startSnapServices(t *state.Task, _ *tomb.Tomb) error {
pb := NewTaskProgressAdapterUnlocked(t)

st.Unlock()
logger.Noticef("ignore system-services: %v", missingSvcsOverview.FoundSystemServices)
for uid, svcs := range missingSvcsOverview.FoundUserServices {
logger.Noticef("ignore user-services [%d]: %v", uid, svcs)
}
err = m.backend.StartServices(startupOrdered, &wrappers.DisabledServices{
SystemServices: svcsToDisable,
SystemServices: missingSvcsOverview.FoundSystemServices,
UserServices: missingSvcsOverview.FoundUserServices,
}, pb, perfTimings)
st.Lock()

Expand All @@ -3082,10 +3117,16 @@ func (m *SnapManager) undoStartSnapServices(t *state.Task, _ *tomb.Tomb) error {
}

var oldLastActiveDisabledServices []string
var oldLastActiveDisabledUserServices map[int][]string
if err := t.Get("old-last-active-disabled-services", &oldLastActiveDisabledServices); err != nil && !errors.Is(err, state.ErrNoState) {
return err
}
if err := t.Get("old-last-active-disabled-user-services", &oldLastActiveDisabledUserServices); err != nil && !errors.Is(err, state.ErrNoState) {
return err
}
snapst.LastActiveDisabledServices = oldLastActiveDisabledServices
snapst.LastActiveDisabledUserServices = oldLastActiveDisabledUserServices

Set(st, snapsup.InstanceName(), snapst)

svcs := currentInfo.Services()
Expand Down Expand Up @@ -3160,6 +3201,7 @@ func (m *SnapManager) stopSnapServices(t *state.Task, _ *tomb.Tomb) error {

// for undo
t.Set("old-last-active-disabled-services", snapst.LastActiveDisabledServices)
t.Set("old-last-active-disabled-user-services", snapst.LastActiveDisabledUserServices)
// undo could queryDisabledServices, but this avoids it
t.Set("disabled-services", disabledServices)

Expand All @@ -3173,6 +3215,20 @@ func (m *SnapManager) stopSnapServices(t *state.Task, _ *tomb.Tomb) error {
snapst.LastActiveDisabledServices,
disabledServices.SystemServices...,
)
// Merge the two user-services maps, ideally we would have one struct but we must
// preserve the backwards-compat with the json layout
Meulengracht marked this conversation as resolved.
Show resolved Hide resolved
if len(snapst.LastActiveDisabledUserServices) > 0 {
for uid, svcs := range disabledServices.UserServices {
snapst.LastActiveDisabledUserServices[uid] = append(snapst.LastActiveDisabledUserServices[uid], svcs...)
}
} else {
snapst.LastActiveDisabledUserServices = disabledServices.UserServices
}

logger.Noticef("queried disabled system-services: %v", disabledServices.SystemServices)
for uid, svcs := range disabledServices.UserServices {
logger.Noticef("queried disabled user-services [%d]: %v", uid, svcs)
}

// reset services tracked by operations from hooks
snapst.ServicesDisabledByHooks = nil
Expand Down Expand Up @@ -3210,11 +3266,16 @@ func (m *SnapManager) undoStopSnapServices(t *state.Task, _ *tomb.Tomb) error {
return err
}

var lastActiveDisabled []string
if err := t.Get("old-last-active-disabled-services", &lastActiveDisabled); err != nil && !errors.Is(err, state.ErrNoState) {
var oldLastActiveDisabledServices []string
var oldLastActiveDisabledUserServices map[int][]string
if err := t.Get("old-last-active-disabled-services", &oldLastActiveDisabledServices); err != nil && !errors.Is(err, state.ErrNoState) {
return err
}
snapst.LastActiveDisabledServices = lastActiveDisabled
if err := t.Get("old-last-active-disabled-user-services", &oldLastActiveDisabledUserServices); err != nil && !errors.Is(err, state.ErrNoState) {
return err
}
snapst.LastActiveDisabledServices = oldLastActiveDisabledServices
snapst.LastActiveDisabledUserServices = oldLastActiveDisabledUserServices
Set(st, snapsup.InstanceName(), snapst)

var disabledServices wrappers.DisabledServices
Expand Down