From e80fd9bb71f5d055536e9d57d526b2de56ac764a Mon Sep 17 00:00:00 2001 From: Samuele Pedroni Date: Mon, 9 Apr 2018 20:17:26 +0200 Subject: [PATCH 1/2] make sure non-base snaps wait for core (source of snapctl) and possibly their base if those are being updated in the same change, otherwise the current link of core or the base could go away and break running hooks or services of the snap --- overlord/snapstate/backend_test.go | 8 +++ overlord/snapstate/export_test.go | 6 +++ overlord/snapstate/snapstate.go | 41 +++++++++++++++ overlord/snapstate/snapstate_test.go | 79 ++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+) diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index f51190454fa..dec7ee0d0c4 100644 --- a/overlord/snapstate/backend_test.go +++ b/overlord/snapstate/backend_test.go @@ -218,6 +218,7 @@ type refreshCand struct { func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) { var name string + typ := snap.TypeApp switch cand.snapID { case "": panic("store refresh APIs expect snap-ids") @@ -231,12 +232,16 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) { name = "some-snap" case "core-snap-id": name = "core" + typ = snap.TypeOS case "snap-with-snapd-control-id": name = "snap-with-snapd-control" case "producer-id": name = "producer" case "consumer-id": name = "consumer" + case "some-base-id": + name = "some-base" + typ = snap.TypeBase default: panic(fmt.Sprintf("refresh: unknown snap-id: %s", cand.snapID)) } @@ -256,6 +261,7 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) { } info := &snap.Info{ + Type: typ, SideInfo: snap.SideInfo{ RealName: name, Channel: cand.channel, @@ -278,6 +284,8 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) { Symlink: "$SNAP/usr", }, } + case "channel-for-base": + info.Base = "some-base" } var hit snap.Revision diff --git a/overlord/snapstate/export_test.go b/overlord/snapstate/export_test.go index 7237b814c24..243fe768982 100644 --- a/overlord/snapstate/export_test.go +++ b/overlord/snapstate/export_test.go @@ -21,6 +21,7 @@ package snapstate import ( "errors" + "sort" "time" "gopkg.in/tomb.v2" @@ -172,3 +173,8 @@ func MockRefreshRetryDelay(d time.Duration) func() { refreshRetryDelay = origRefreshRetryDelay } } + +func ByKindOrder(snaps ...*snap.Info) []*snap.Info { + sort.Sort(byKind(snaps)) + return snaps +} diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 612f4a48d99..273ced20524 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -24,6 +24,7 @@ import ( "encoding/json" "fmt" "reflect" + "sort" "strings" "golang.org/x/net/context" @@ -744,6 +745,16 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func reportUpdated[snapName] = true } + // first core, bases, then rest + sort.Sort(byKind(updates)) + prereqs := make(map[string]*state.TaskSet) + waitPrereq := func(ts *state.TaskSet, prereqName string) { + preTs := prereqs[prereqName] + if preTs != nil { + ts.WaitAll(preTs) + } + } + for _, update := range updates { channel, flags, snapst := params(update) @@ -786,6 +797,20 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func } ts.JoinLane(st.NewLane()) + if update.Type == snap.TypeOS || update.Type == snap.TypeBase { + // prereq types come first in the updates, we + // also assume bases don't have hooks, otherwise + // they would need to wait on core + prereqs[update.Name()] = ts + } else { + // prereqs were processed, wait for them as necessary + // for the other kind of snaps + waitPrereq(ts, defaultCoreSnapName) + if update.Base != "" { + waitPrereq(ts, update.Base) + } + } + scheduleUpdate(update.Name(), ts) tasksets = append(tasksets, ts) } @@ -806,6 +831,22 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func return updated, tasksets, nil } +type byKind []*snap.Info + +func (bk byKind) Len() int { return len(bk) } +func (bk byKind) Swap(i, j int) { bk[i], bk[j] = bk[j], bk[i] } + +var kindRevOrder = map[snap.Type]int{ + snap.TypeOS: 2, + snap.TypeBase: 1, +} + +func (bk byKind) Less(i, j int) bool { + iRevOrd := kindRevOrder[bk[i].Type] + jRevOrd := kindRevOrder[bk[j].Type] + return iRevOrd >= jRevOrd +} + func applyAutoAliasesDelta(st *state.State, delta map[string][]string, op string, refreshAll bool, linkTs func(snapName string, ts *state.TaskSet)) (*state.TaskSet, error) { applyTs := state.NewTaskSet() kind := "refresh-aliases" diff --git a/overlord/snapstate/snapstate_test.go b/overlord/snapstate/snapstate_test.go index cf03e33fccf..a70c6c2a3f2 100644 --- a/overlord/snapstate/snapstate_test.go +++ b/overlord/snapstate/snapstate_test.go @@ -829,6 +829,85 @@ func (s *snapmgrTestSuite) TestUpdateAllDevMode(c *C) { c.Check(updates, HasLen, 0) } +func (s *snapmgrTestSuite) TestByKindOrder(c *C) { + core := &snap.Info{Type: snap.TypeOS} + base := &snap.Info{Type: snap.TypeBase} + app := &snap.Info{Type: snap.TypeApp} + + c.Check(snapstate.ByKindOrder(base, core), DeepEquals, []*snap.Info{core, base}) + c.Check(snapstate.ByKindOrder(app, core), DeepEquals, []*snap.Info{core, app}) + c.Check(snapstate.ByKindOrder(app, base), DeepEquals, []*snap.Info{base, app}) + c.Check(snapstate.ByKindOrder(app, base, core), DeepEquals, []*snap.Info{core, base, app}) + c.Check(snapstate.ByKindOrder(app, core, base), DeepEquals, []*snap.Info{core, base, app}) +} + +func (s *snapmgrTestSuite) TestUpdateManyWaitForBases(c *C) { + s.state.Lock() + defer s.state.Unlock() + + snapstate.Set(s.state, "core", &snapstate.SnapState{ + Active: true, + Sequence: []*snap.SideInfo{ + {RealName: "core", SnapID: "core-snap-id", Revision: snap.R(1)}, + }, + Current: snap.R(1), + SnapType: "os", + }) + + snapstate.Set(s.state, "some-base", &snapstate.SnapState{ + Active: true, + Sequence: []*snap.SideInfo{ + {RealName: "some-base", SnapID: "some-base-id", Revision: snap.R(1)}, + }, + Current: snap.R(1), + SnapType: "base", + }) + + snapstate.Set(s.state, "some-snap", &snapstate.SnapState{ + Active: true, + Sequence: []*snap.SideInfo{ + {RealName: "some-snap", SnapID: "some-snap-id", Revision: snap.R(1)}, + }, + Current: snap.R(1), + SnapType: "app", + Channel: "channel-for-base", + }) + + updates, tts, err := snapstate.UpdateMany(context.TODO(), s.state, []string{"some-snap", "core", "some-base"}, 0) + c.Assert(err, IsNil) + c.Assert(tts, HasLen, 3) + c.Check(updates, HasLen, 3) + + // to make TaskSnapSetup work + chg := s.state.NewChange("refresh", "...") + for _, ts := range tts { + chg.AddAll(ts) + } + + prereqTotal := len(tts[0].Tasks()) + len(tts[1].Tasks()) + prereqs := map[string]bool{} + for i, task := range tts[2].Tasks() { + waitTasks := task.WaitTasks() + if i == 0 { + c.Check(len(waitTasks), Equals, prereqTotal) + } else if task.Kind() == "link-snap" { + c.Check(len(waitTasks), Equals, prereqTotal+1) + for _, pre := range waitTasks { + if pre.Kind() == "link-snap" { + snapsup, err := snapstate.TaskSnapSetup(pre) + c.Assert(err, IsNil) + prereqs[snapsup.Name()] = true + } + } + } + } + + c.Check(prereqs, DeepEquals, map[string]bool{ + "core": true, + "some-base": true, + }) +} + func (s *snapmgrTestSuite) TestUpdateManyValidateRefreshes(c *C) { s.state.Lock() defer s.state.Unlock() From 0c3862cd4f2fd905cee7ccb97c24f084792a7679 Mon Sep 17 00:00:00 2001 From: Samuele Pedroni Date: Tue, 10 Apr 2018 11:27:10 +0200 Subject: [PATCH 2/2] expand comments --- overlord/snapstate/snapstate.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 273ced20524..dedee0c4ade 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -755,6 +755,8 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func } } + // updates is sorted by kind so this will process first core + // and bases and then other snaps for _, update := range updates { channel, flags, snapst := params(update) @@ -797,14 +799,18 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func } ts.JoinLane(st.NewLane()) + // because of the sorting of updates we fill prereqs + // first (if branch) and only then use it to setup + // waits (else branch) if update.Type == snap.TypeOS || update.Type == snap.TypeBase { - // prereq types come first in the updates, we + // prereq types come first in updates, we // also assume bases don't have hooks, otherwise // they would need to wait on core prereqs[update.Name()] = ts } else { - // prereqs were processed, wait for them as necessary - // for the other kind of snaps + // prereqs were processed already, wait for + // them as necessary for the other kind of + // snaps waitPrereq(ts, defaultCoreSnapName) if update.Base != "" { waitPrereq(ts, update.Base)