overlord/snapstate: on multi-snap refresh make sure bases and core ar…

…e finished before dependent snaps (snapcore#5018)

This makes sure non-base snaps wait for core (source of snapctl etc) and possibly their base if those are being updated in the same multi-snap refresh change, otherwise the current link of core or the base could go away and break running hooks or services of the snap during the snap own refresh process.

The issue is more general than this, but this covers the auto-refresh case for example.

TODO: in general we should raise conflicts between changes where a snap is being operated on in one change, and their base or core is going through unlink-snap or unlink-current-snap in another.

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

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
+}

overlord/snapstate/snapstate.go

@@ -24,6 +24,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 
 	"golang.org/x/net/context"
@@ -741,6 +742,18 @@ 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)
+		}
+	}
+
+	// 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)
 
@@ -783,6 +796,24 @@ 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 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 already, 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)
 	}
@@ -803,6 +834,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"

overlord/snapstate/snapstate_test.go

@@ -822,6 +822,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()