o/snapstate: refactor Install functions to use a singular implementat…

…ion that operates on a Target (#13949)

* snap: add method for checking if a revision is set to reduce double negatives in code

* o/snapstate, o/hookstate: make mocked snapstate.EnforcedValidationSets follow correct pattern when returning no error

* o/snapstate: add new function InstallTarget that operates a Target interfaces, which controls how snaps are found

* o/snapstate: implement Install family of functions with InstallTarget

InstallPathMany is omitted for now, since it is more of an update rather
than an installation.

* o/snapstate: update perquisites task handler to use new lane functionality in InstallWithDeviceContext

* o/snapstate: doc comment fixes

Co-authored-by: Miguel Pires <miguelpires94@gmail.com>

* o/snapstate: add license header to target.go

* o/snapstate: rename SnapstateOptions to Options

* o/snapstate: rename interface parameter to better indicate use

* o/snapstate: make laneGenerator a standalone function

* o/snapstate: fix redundant error message by calling singleActionResultErr when only one result is expected

* o/snapstate: doc comments on new target functions/types

* o/snapstate: rename StoreSnap.Name to StoreSnap.InstanceName

* o/snapstate: rename laneGenerator -> generateLane

* o/snapstate: test for error when installing an already installed snap

* o/snapstate: add test for invalid revision options

* o/snapstate: remove awkward installed snaps param from Target interface

* o/devicestate, o/snapstate: replace OptionInitializer with simpler boolean flag in Options that allows caller to skip seeded check

* o/devicestate, o/snapstate: rename new types introduced in refactor

* o/snapstate: rename InstallGoal.Install to InstallGoal.ToInstall

* o/snapstate: introduce Options.RequireOneSnap flag to require that we are only installing one snap AND allow us to choose how to generate errors

* o/snapstate: reduce number of exported fields/types related to InstallWithGroup

* o/snapstate: make functions that create goals return interfaces rather than concrete types

* o/snapstate: rename StoreGoal to StoreInstallGoal

* o/snapstate: rename RequireOneSnap to ExpectOneSnap

* o/snapstate: update doc commentsa on StoreInstallGoal and PathInstallGoal

* snap, o/snapstate: remove snap.R.Set helper method

* o/devicestate: replace unneeded param with empty string, which will default to SideInfo.RealName

* o/snapstate: manually re-lock lock earlier, avoid golangci-lint error

* o/snapstate: replace manually building SnapSetup with method on target

* o/snapstate: add comments to storeInstallGoal fields and clarify behavior of StoreInstallGoal

* o/snapstate: reword PathInstallGoal doc comment

* o/snapstate: re-export the installGoal interface

* o/snapstate: use slice inside of storeInstallGoal to maintain order of requested snap installs

* o/devicestate: make sure to set channel when seeding system, and add a test for it

---------

Co-authored-by: Miguel Pires <miguelpires94@gmail.com>

overlord/devicestate/firstboot.go

@@ -20,6 +20,7 @@
 package devicestate
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"runtime"
@@ -54,7 +55,19 @@ func installSeedSnap(st *state.State, sn *seed.Snap, flags snapstate.Flags, prqt
 		flags.DevMode = true
 	}
 
-	return snapstate.InstallPath(st, sn.SideInfo, sn.Path, "", sn.Channel, flags, prqt)
+	t := snapstate.PathInstallGoal("", sn.Path, sn.SideInfo, snapstate.RevisionOptions{
+		Channel: sn.Channel,
+	})
+	info, ts, err := snapstate.InstallOne(context.Background(), st, t, snapstate.Options{
+		Flags:         flags,
+		PrereqTracker: prqt,
+		Seed:          true,
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return ts, info, nil
 }
 
 func criticalTaskEdges(ts *state.TaskSet) (beginEdge, beforeHooksEdge, hooksEdge *state.Task, err error) {

overlord/devicestate/firstboot20_test.go

@@ -48,6 +48,7 @@ import (
 	"github.com/snapcore/snapd/release"
 	"github.com/snapcore/snapd/seed/seedtest"
 	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/snap/channel"
 	"github.com/snapcore/snapd/strutil"
 	"github.com/snapcore/snapd/systemd"
 	"github.com/snapcore/snapd/testutil"
@@ -416,13 +417,23 @@ func (s *firstBoot20Suite) testPopulateFromSeedCore20Happy(c *C, m *boot.Modeenv
 	// there either from ubuntu-image or from "install" mode.
 	c.Check(bloader.ExtractKernelAssetsCalls, HasLen, 0)
 
-	// ensure required flag is set on all essential snaps
+	namesToChannel := make(map[string]string)
+	for _, sn := range model.EssentialSnaps() {
+		ch, err := channel.Full(sn.DefaultChannel)
+		c.Assert(err, IsNil)
+		namesToChannel[sn.Name] = ch
+	}
+
+	// ensure required flag is set on all essential snaps and all of their
+	// channels got set properly
 	var snapst snapstate.SnapState
 	for _, reqName := range []string{"snapd", "core20", "pc-kernel", "pc"} {
 		err = snapstate.Get(state, reqName, &snapst)
 		c.Assert(err, IsNil)
 		c.Assert(snapst.Required, Equals, true, Commentf("required not set for %v", reqName))
 
+		c.Check(snapst.TrackingChannel, Equals, namesToChannel[reqName])
+
 		if m.Mode == "run" {
 			// also ensure that in run mode none of the snaps are installed as
 			// symlinks, they must be copied onto ubuntu-data

overlord/hookstate/ctlcmd/services_test.go

@@ -222,7 +222,7 @@ func (s *servicectlSuite) SetUpTest(c *C) {
 		snapstate.EnforcedValidationSets = old
 	})
 	snapstate.EnforcedValidationSets = func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	}
 }
 

overlord/snapstate/autorefresh_gating_test.go

@@ -86,7 +86,7 @@ func (s *autorefreshGatingSuite) SetUpTest(c *C) {
 	s.state.Set("refresh-privacy-key", "privacy-key")
 
 	restore := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	})
 	s.AddCleanup(restore)
 }

overlord/snapstate/autorefresh_test.go

@@ -152,7 +152,7 @@ func (s *autoRefreshTestSuite) SetUpTest(c *C) {
 	s.AddCleanup(snapstatetest.MockDeviceModel(DefaultModel()))
 
 	restore := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	})
 	s.AddCleanup(restore)
 }

overlord/snapstate/export_test.go

@@ -46,6 +46,8 @@ type (
 	ReRefreshSetup = reRefreshSetup
 
 	TooSoonError = tooSoonError
+
+	Target = target
 )
 
 var ComponentSetupTask = componentSetupTask
@@ -591,3 +593,13 @@ func MockAffectedSnapsByKind(value map[string]AffectedSnapsFunc) (restore func()
 		affectedSnapsByKind = old
 	}
 }
+
+// CustomInstallGoal allows us to define custom implementations of installGoal
+// to be used in tests.
+type CustomInstallGoal struct {
+	ToInstall func(context.Context, *state.State, Options) ([]Target, error)
+}
+
+func (c *CustomInstallGoal) toInstall(ctx context.Context, st *state.State, opts Options) ([]Target, error) {
+	return c.ToInstall(ctx, st, opts)
+}

overlord/snapstate/handlers.go

@@ -371,12 +371,17 @@ func (m *SnapManager) installOneBaseOrRequired(t *state.Task, snapName string, c
 	}
 
 	// not installed, nor queued for install -> install it
-	ts, err := InstallWithDeviceContext(context.TODO(), st, snapName, &RevisionOptions{Channel: channel}, userID, Flags{RequireTypeBase: requireTypeBase}, nil, deviceCtx, "")
+	ts, err := InstallWithDeviceContext(context.TODO(), st, snapName, &RevisionOptions{Channel: channel}, userID, Flags{
+		RequireTypeBase: requireTypeBase,
+		Transaction:     flags.Transaction,
+		Lane:            flags.Lane,
+	}, nil, deviceCtx, "")
 
 	// something might have triggered an explicit install while
 	// the state was unlocked -> deal with that here by simply
 	// retrying the operation.
-	if conflErr, ok := err.(*ChangeConflictError); ok {
+	var conflErr *ChangeConflictError
+	if errors.As(err, &conflErr) {
 		// conflicted with an install in the same change, just skip
 		if conflErr.ChangeID == t.Change().ID() {
 			return nil, nil
@@ -500,6 +505,21 @@ func hasAllContentAttrs(st *state.State, snapName string, requiredContentAttrs [
 func (m *SnapManager) installPrereqs(t *state.Task, base string, prereq map[string][]string, userID int, tm timings.Measurer, flags Flags) error {
 	st := t.State()
 
+	// If transactional, use a single lane for all tasks, so when
+	// one fails the changes for all affected snaps will be
+	// undone. Otherwise, have different lanes per snap so
+	// failures only affect the culprit snap.
+	if flags.Transaction == client.TransactionAllSnaps {
+		lanes := t.Lanes()
+		if len(lanes) != 1 {
+			return fmt.Errorf("internal error: more than one lane (%d) on a transactional action", len(lanes))
+		}
+
+		flags.Lane = lanes[0]
+	} else {
+		flags.Transaction = client.TransactionPerSnap
+	}
+
 	// We try to install all wanted snaps. If one snap cannot be installed
 	// because of change conflicts or similar we retry. Only if all snaps
 	// can be installed together we add the tasks to the change.
@@ -530,7 +550,10 @@ func (m *SnapManager) installPrereqs(t *state.Task, base string, prereq map[stri
 	if base != "none" {
 		timings.Run(tm, "install-prereq", fmt.Sprintf("install base %q", base), func(timings.Measurer) {
 			requireTypeBase := true
-			tsBase, err = m.installOneBaseOrRequired(t, base, nil, requireTypeBase, defaultBaseSnapsChannel(), onInFlightErr, userID, Flags{})
+			tsBase, err = m.installOneBaseOrRequired(t, base, nil, requireTypeBase, defaultBaseSnapsChannel(), onInFlightErr, userID, Flags{
+				Transaction: flags.Transaction,
+				Lane:        flags.Lane,
+			})
 		})
 		if err != nil {
 			return prereqError("snap base", base, err)
@@ -551,47 +574,31 @@ func (m *SnapManager) installPrereqs(t *state.Task, base string, prereq map[stri
 	if base != "core" && !snapdSnapInstalled && !coreSnapInstalled {
 		timings.Run(tm, "install-prereq", "install snapd", func(timings.Measurer) {
 			noTypeBaseCheck := false
-			tsSnapd, err = m.installOneBaseOrRequired(t, "snapd", nil, noTypeBaseCheck, defaultSnapdSnapsChannel(), onInFlightErr, userID, Flags{})
+			tsSnapd, err = m.installOneBaseOrRequired(t, "snapd", nil, noTypeBaseCheck, defaultSnapdSnapsChannel(), onInFlightErr, userID, Flags{
+				Transaction: flags.Transaction,
+				Lane:        flags.Lane,
+			})
 		})
 		if err != nil {
 			return prereqError("system snap", "snapd", err)
 		}
 	}
 
-	// If transactional, use a single lane for all tasks, so when
-	// one fails the changes for all affected snaps will be
-	// undone. Otherwise, have different lanes per snap so
-	// failures only affect the culprit snap.
-	var joinLane func(ts *state.TaskSet)
-	if flags.Transaction == client.TransactionAllSnaps {
-		lanes := t.Lanes()
-		if len(lanes) != 1 {
-			return fmt.Errorf("internal error: more than one lane (%d) on a transactional action", len(lanes))
-		}
-		transactionLane := lanes[0]
-		joinLane = func(ts *state.TaskSet) { ts.JoinLane(transactionLane) }
-	} else {
-		joinLane = func(ts *state.TaskSet) { ts.JoinLane(st.NewLane()) }
-	}
-
 	chg := t.Change()
 	// add all required snaps, no ordering, this will be done in the
 	// auto-connect task handler
 	for _, ts := range tss {
-		joinLane(ts)
 		chg.AddAll(ts)
 	}
 	// add the base if needed, prereqs else must wait on this
 	if tsBase != nil {
-		joinLane(tsBase)
 		for _, t := range chg.Tasks() {
 			t.WaitAll(tsBase)
 		}
 		chg.AddAll(tsBase)
 	}
 	// add snapd if needed, everything must wait on this
 	if tsSnapd != nil {
-		joinLane(tsSnapd)
 		for _, t := range chg.Tasks() {
 			t.WaitAll(tsSnapd)
 		}

overlord/snapstate/handlers_download_test.go

@@ -59,7 +59,7 @@ func (s *downloadSnapSuite) SetUpTest(c *C) {
 	s.AddCleanup(snapstatetest.UseFallbackDeviceModel())
 
 	restore := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	})
 	s.AddCleanup(restore)
 }

overlord/snapstate/handlers_prereq_test.go

@@ -78,7 +78,7 @@ func (s *prereqSuite) SetUpTest(c *C) {
 	s.AddCleanup(restoreInstallSize)
 
 	restore := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	})
 	s.AddCleanup(restore)
 

overlord/snapstate/handlers_rerefresh_test.go

@@ -501,7 +501,7 @@ func (s *reRefreshSuite) TestFilterReturnsFalseIfEpochEqualZero(c *C) {
 
 func (s *refreshSuite) TestMaybeRestoreValidationSetsAndRevertSnaps(c *C) {
 	restore := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	})
 	defer restore()
 

overlord/snapstate/refreshhints_test.go

@@ -116,7 +116,7 @@ func (s *refreshHintsTestSuite) SetUpTest(c *C) {
 	restoreModel := snapstatetest.MockDeviceModel(DefaultModel())
 	s.AddCleanup(restoreModel)
 	restoreEnforcedValidationSets := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) {
-		return nil, nil
+		return snapasserts.NewValidationSets(), nil
 	})
 	s.AddCleanup(restoreEnforcedValidationSets)
 	s.AddCleanup(func() {