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

overlord/snapstate: on multi-snap refresh make sure bases and core are finished before dependent snaps #5018

Merged
merged 2 commits into from Apr 10, 2018
Merged

overlord/snapstate: on multi-snap refresh make sure bases and core are finished before dependent snaps #5018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e80fd9b
make sure non-base snaps wait for core (source of snapctl) and possib…
pedronis Apr 9, 2018
0c3862c
expand comments
pedronis Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions overlord/snapstate/backend_test.go
View file
Open in desktop
Expand Up @@ -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")
Expand All @@ -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))
}
Expand All @@ -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,
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions overlord/snapstate/export_test.go
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ package snapstate

import (
"errors"
"sort"
"time"

"gopkg.in/tomb.v2"
Expand Down Expand Up @@ -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
}
41 changes: 41 additions & 0 deletions overlord/snapstate/snapstate.go
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ import (
"encoding/json"
"fmt"
"reflect"
"sort"
"strings"

"golang.org/x/net/context"
Expand Down Expand Up @@ -744,6 +745,16 @@ func doUpdate(st *state.State, names []string, updates []*snap.Info, params func
reportUpdated[snapName] = true
}

// first core, bases, then rest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, this is a very good way to order snaps by kind.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought, do we need to put gadget ahead of apps here? Also kernel (probably)

Copy link
Collaborator Author

@pedronis pedronis Apr 10, 2018 •
edited

Choose a reason for hiding this comment

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

not for the current problem, gadget or kernel shouldn't block running hooks in other snaps. Kernel provokes a reboot but that's a different issue.

sort.Sort(byKind(updates))
prereqs := make(map[string]*state.TaskSet)
waitPrereq := func(ts *state.TaskSet, prereqName string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So waitPrereq is a helper that given snap name, adds the prerequisites of that snap, if any, to the given task set.

preTs := prereqs[prereqName]
if preTs != nil {
ts.WaitAll(preTs)
}
}

for _, update := range updates {
channel, flags, snapst := params(update)

Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a comment that this loop iterates over updates ordered by the priority and this makes this newly added code safe, that is, that we always have prerequisites by the time we need to read them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's was the intention of the comments inside the two if sides, but it seems they are not clear enough

// 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)
}
Expand All @@ -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"
Expand Down
79 changes: 79 additions & 0 deletions overlord/snapstate/snapstate_test.go
View file
Open in desktop
Expand Up @@ -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()
Expand Down