o/snapstate: make snapd downgrading an exclusive change

Block new changes while a snapd downgrade is in progress and block snapd from downgrading while other changes are in-flight 

3 files changed, 275 insertions, 0 deletions

diff --git a/overlord/snapstate/conflict.go b/overlord/snapstate/conflict.go
index a6ca8f9051..5e89694398 100644
--- a/overlord/snapstate/conflict.go
+++ b/overlord/snapstate/conflict.go
@@ -25,6 +25,7 @@ import (
	"reflect"
 
	"github.com/snapcore/snapd/overlord/state"
+	"github.com/snapcore/snapd/strutil"
 )
 
 // FinalTasks are task kinds for final tasks in a change which means no further
@@ -98,6 +99,51 @@ func affectedSnaps(t *state.Task) ([]string, error) {
	return nil, nil
 }
 
+func snapSetupFromChange(chg *state.Change) (*SnapSetup, error) {
+	for _, t := range chg.Tasks() {
+	// Check a known task of each change that we know keep snap info.
+	if t.Kind() != "prerequisites" {
+	continue
+	}
+	return TaskSnapSetup(t)
+	}
+	return nil, nil
+}
+
+// changeIsSnapdDowngrade returns true if the change provided is a snapd
+// setup change with a version lower than what is currently installed. If a change
+// is not SnapSetup related this returns false.
+func changeIsSnapdDowngrade(st *state.State, chg *state.Change) (bool, error) {
+	snapsup, err := snapSetupFromChange(chg)
+	if err != nil {
+	return false, err
+	}
+	if snapsup == nil || snapsup.SnapName() != "snapd" {
+	return false, nil
+	}
+
+	var snapst SnapState
+	if err := Get(st, snapsup.InstanceName(), &snapst); err != nil {
+	return false, err
+	}
+
+	currentInfo, err := snapst.CurrentInfo()
+	if err != nil {
+	return false, fmt.Errorf("cannot retrieve snap info for current snapd: %v", err)
+	}
+
+	// On older snapd's 'Version' might be empty, and in this case we assume
+	// that snapd is downgrading as we cannot determine otherwise.
+	if snapsup.Version == "" {
+	return true, nil
+	}
+	res, err := strutil.VersionCompare(currentInfo.Version, snapsup.Version)
+	if err != nil {
+	return false, fmt.Errorf("cannot compare versions of snapd [cur: %s, new: %s]: %v", currentInfo.Version, snapsup.Version, err)
+	}
+	return res == 1, nil
+}
+
 func checkChangeConflictExclusiveKinds(st *state.State, newExclusiveChangeKind, ignoreChangeID string) error {
	for _, chg := range st.Changes() {
	if chg.Status().Ready() {
@@ -134,6 +180,21 @@ func checkChangeConflictExclusiveKinds(st *state.State, newExclusiveChangeKind,
	ChangeKind: "create-recovery-system",
	ChangeID: chg.ID(),
	}
+	case "revert-snap", "refresh-snap":
+	// Snapd downgrades are exclusive changes
+	if ignoreChangeID != "" && chg.ID() == ignoreChangeID {
+	continue
+	}
+	if downgrading, err := changeIsSnapdDowngrade(st, chg); err != nil {
+	return err
+	} else if !downgrading {
+	continue
+	}
+	return &ChangeConflictError{
+	Message: "snapd downgrade in progress, no other changes allowed until this is done",
+	ChangeKind: chg.Kind(),
+	ChangeID: chg.ID(),
+	}
	default:
	if newExclusiveChangeKind != "" {
	// we want to run a new exclusive change, but other
diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go
index 284cdfc244..e5501556de 100644
--- a/overlord/snapstate/snapstate.go
+++ b/overlord/snapstate/snapstate.go
@@ -399,6 +399,20 @@ func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int
	}
	snapsup.PlugsOnly = snapsup.PlugsOnly && (len(info.Slots) == 0)
 
+	// When downgrading snapd we want to make sure that it's an exclusive change.
+	if snapsup.SnapName() == "snapd" {
+	res, err := strutil.VersionCompare(info.Version, snapsup.Version)
+	if err != nil {
+	return nil, fmt.Errorf("cannot compare versions of snapd [cur: %s, new: %s]: %v", info.Version, snapsup.Version, err)
+	}
+	// If snapsup.Version was smaller, 1 is returned.
+	if res == 1 {
+	if err := CheckChangeConflictRunExclusively(st, "snapd downgrade"); err != nil {
+	return nil, err
+	}
+	}
+	}
+
	if experimentalRefreshAppAwareness && !excludeFromRefreshAppAwareness(snapsup.Type) && !snapsup.Flags.IgnoreRunning {
	// Note that because we are modifying the snap state inside
	// softCheckNothingRunningForRefresh, this block must be located
diff --git a/overlord/snapstate/snapstate_update_test.go b/overlord/snapstate/snapstate_update_test.go
index 385aa6e67d..07e87e9f3b 100644
--- a/overlord/snapstate/snapstate_update_test.go
+++ b/overlord/snapstate/snapstate_update_test.go
@@ -9925,3 +9925,203 @@ func (s *snapmgrTestSuite) TestMonitoringIsPersistedAndRestored(c *C) {
	c.Assert(s.state.Cached("monitored-snaps"), IsNil)
	c.Check(s.state.Cached("auto-refresh-continue-attempt"), Equals, 1)
 }
+
+func (s *snapmgrTestSuite) testUpdateDowngradeBlockedByOtherChanges(c *C, old, new string, revert bool) error {
+	si1 := snap.SideInfo{
+	RealName: "snapd",
+	SnapID: "snapd-id",
+	Channel: "latest",
+	Revision: snap.R(1),
+	}
+	si2 := snap.SideInfo{
+	RealName: "snapd",
+	SnapID: "snapd-id",
+	Channel: "latest",
+	Revision: snap.R(2),
+	}
+	si3 := snap.SideInfo{
+	RealName: "snapd",
+	SnapID: "snapd-id",
+	Channel: "latest",
+	Revision: snap.R(3),
+	}
+
+	restore := snapstate.MockSnapReadInfo(func(name string, si *snap.SideInfo) (*snap.Info, error) {
+	var version string
+	switch name {
+	case "snapd":
+	if (revert && si.Revision.N == 1) || (!revert && si.Revision.N == 2) {
+	version = old
+	} else if (revert && si.Revision.N == 2) || si.Revision.N == 3 {
+	version = new
+	} else {
+	return nil, fmt.Errorf("unexpected revision for test")
+	}
+	default:
+	version = "1.0"
+	}
+	return &snap.Info{
+	SuggestedName: name,
+	Version: version,
+	Architectures: []string{"all"},
+	SideInfo: *si,
+	}, nil
+	})
+	defer restore()
+
+	st := s.state
+	st.Lock()
+	defer st.Unlock()
+
+	chg := st.NewChange("unrelated", "...")
+	chg.AddTask(st.NewTask("task0", "..."))
+
+	snapstate.Set(s.state, "snapd", &snapstate.SnapState{
+	Active: true,
+	Sequence: []*snap.SideInfo{&si1, &si2, &si3},
+	TrackingChannel: "latest/stable",
+	Current: si2.Revision,
+	})
+
+	var err error
+	if revert {
+	_, err = snapstate.Revert(s.state, "snapd", snapstate.Flags{}, "")
+	} else {
+	_, err = snapstate.Update(s.state, "snapd", &snapstate.RevisionOptions{Revision: snap.R(3)}, s.user.ID, snapstate.Flags{})
+	}
+	return err
+}
+
+func (s *snapmgrTestSuite) TestUpdateDowngradeBlockedByOtherChanges(c *C) {
+	err := s.testUpdateDowngradeBlockedByOtherChanges(c, "2.57.1", "2.56", false)
+	c.Assert(err, ErrorMatches, `other changes in progress \(conflicting change "unrelated"\), change "snapd downgrade" not allowed until they are done`)
+}
+
+func (s *snapmgrTestSuite) TestUpdateDowngradeBlockedByOtherChangesAlsoWhenEmpty(c *C) {
+	err := s.testUpdateDowngradeBlockedByOtherChanges(c, "2.57.1", "", false)
+	c.Assert(err, ErrorMatches, `other changes in progress \(conflicting change "unrelated"\), change "snapd downgrade" not allowed until they are done`)
+}
+
+func (s *snapmgrTestSuite) TestUpdateDowngradeNotBlockedByOtherChanges(c *C) {
+	err := s.testUpdateDowngradeBlockedByOtherChanges(c, "2.57.1", "2.58", false)
+	c.Assert(err, IsNil)
+}
+
+func (s *snapmgrTestSuite) TestRevertBlockedByOtherChanges(c *C) {
+	// Swap values for revert case
+	err := s.testUpdateDowngradeBlockedByOtherChanges(c, "2.56", "2.57.1", true)
+	c.Assert(err, ErrorMatches, `other changes in progress \(conflicting change "unrelated"\), change "snapd downgrade" not allowed until they are done`)
+}
+
+func (s *snapmgrTestSuite) TestRevertBlockedByOtherChangesAlsoWhenEmpty(c *C) {
+	// Swap values for revert case
+	err := s.testUpdateDowngradeBlockedByOtherChanges(c, "2.58", "2.57.1", true)
+	c.Assert(err, IsNil)
+}
+
+func (s *snapmgrTestSuite) testUpdateNotAllowedWhileDowngrading(c *C, old, new string, revert bool) error {
+	si1 := snap.SideInfo{
+	RealName: "snapd",
+	SnapID: "snapd-id",
+	Channel: "latest",
+	Revision: snap.R(1),
+	}
+	si2 := snap.SideInfo{
+	RealName: "snapd",
+	SnapID: "snapd-id",
+	Channel: "latest",
+	Revision: snap.R(2),
+	}
+	si3 := snap.SideInfo{
+	RealName: "snapd",
+	SnapID: "snapd-id",
+	Channel: "latest",
+	Revision: snap.R(3),
+	}
+
+	si := snap.SideInfo{
+	RealName: "some-snap",
+	SnapID: "some-snap-id",
+	Revision: snap.R(7),
+	Channel: "channel-for-7",
+	}
+
+	restore := snapstate.MockSnapReadInfo(func(name string, si *snap.SideInfo) (*snap.Info, error) {
+	var version string
+	switch name {
+	case "snapd":
+	if (revert && si.Revision.N == 1) || (!revert && si.Revision.N == 2) {
+	version = old
+	} else if (revert && si.Revision.N == 2) || si.Revision.N == 3 {
+	version = new
+	} else {
+	return nil, fmt.Errorf("unexpected revision for test")
+	}
+	default:
+	version = "1.0"
+	}
+	return &snap.Info{
+	SuggestedName: name,
+	Version: version,
+	Architectures: []string{"all"},
+	SideInfo: *si,
+	}, nil
+	})
+	defer restore()
+
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	snapstate.Set(s.state, "snapd", &snapstate.SnapState{
+	Active: true,
+	Sequence: []*snap.SideInfo{&si1, &si2, &si3},
+	TrackingChannel: "latest/stable",
+	Current: si2.Revision,
+	})
+	snapstate.Set(s.state, "some-snap", &snapstate.SnapState{
+	Active: true,
+	Sequence: []*snap.SideInfo{&si},
+	TrackingChannel: "other-chanel/stable",
+	Current: si.Revision,
+	})
+
+	var err error
+	var ts *state.TaskSet
+	if revert {
+	ts, err = snapstate.Revert(s.state, "snapd", snapstate.Flags{}, "")
+	} else {
+	ts, err = snapstate.Update(s.state, "snapd", &snapstate.RevisionOptions{Revision: snap.R(3)}, s.user.ID, snapstate.Flags{})
+	}
+	c.Assert(err, IsNil)
+
+	chg := s.state.NewChange("refresh-snap", "refresh snapd")
+	chg.AddAll(ts)
+
+	_, err = snapstate.Update(s.state, "some-snap", &snapstate.RevisionOptions{Channel: "channel-for-7/stable"}, s.user.ID, snapstate.Flags{})
+	return err
+}
+
+func (s *snapmgrTestSuite) TestUpdateNotAllowedWhileDowngrading(c *C) {
+	err := s.testUpdateNotAllowedWhileDowngrading(c, "2.57.1", "2.56", false)
+	c.Assert(err, ErrorMatches, `snapd downgrade in progress, no other changes allowed until this is done`)
+}
+
+func (s *snapmgrTestSuite) TestUpdateNotAllowedWhileDowngradingAndWhenEmpty(c *C) {
+	err := s.testUpdateNotAllowedWhileDowngrading(c, "2.57.1", "", false)
+	c.Assert(err, ErrorMatches, `snapd downgrade in progress, no other changes allowed until this is done`)
+}
+
+func (s *snapmgrTestSuite) TestUpdateAllowedWhileUpgrading(c *C) {
+	err := s.testUpdateNotAllowedWhileDowngrading(c, "2.57.1", "2.58", false)
+	c.Assert(err, IsNil)
+}
+
+func (s *snapmgrTestSuite) TestUpdateNotAllowedWhileRevertDowngrading(c *C) {
+	err := s.testUpdateNotAllowedWhileDowngrading(c, "2.56", "2.57.1", true)
+	c.Assert(err, ErrorMatches, `snapd downgrade in progress, no other changes allowed until this is done`)
+}
+
+func (s *snapmgrTestSuite) TestUpdateAllowedWhileRevertUpgrading(c *C) {
+	err := s.testUpdateNotAllowedWhileDowngrading(c, "2.58", "2.57.1", true)
+	c.Assert(err, IsNil)
+}