o/snapstate: reorder tasks such that snap and component downloads now…

… happen consecutively

overlord/snapstate/component.go

@@ -253,15 +253,17 @@ type ComponentInstallFlags struct {
 }
 
 type componentInstallTaskSet struct {
-	compSetupTaskID        string
-	beforeLinkTasks        []*state.Task
-	maybeLinkTask          *state.Task
-	postHookToDiscardTasks []*state.Task
-	maybeDiscardTask       *state.Task
+	compSetupTaskID               string
+	beforeLocalModificationsTasks []*state.Task
+	beforeLinkTasks               []*state.Task
+	maybeLinkTask                 *state.Task
+	postHookToDiscardTasks        []*state.Task
+	maybeDiscardTask              *state.Task
 }
 
 func (c *componentInstallTaskSet) taskSet() *state.TaskSet {
-	tasks := make([]*state.Task, 0, len(c.beforeLinkTasks)+1+len(c.postHookToDiscardTasks)+1)
+	tasks := make([]*state.Task, 0, len(c.beforeLocalModificationsTasks)+len(c.beforeLinkTasks)+1+len(c.postHookToDiscardTasks)+1)
+	tasks = append(tasks, c.beforeLocalModificationsTasks...)
 	tasks = append(tasks, c.beforeLinkTasks...)
 	if c.maybeLinkTask != nil {
 		tasks = append(tasks, c.maybeLinkTask)
@@ -344,13 +346,13 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS
 		compSetupTaskID: prepare.ID(),
 	}
 
-	componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, prepare)
+	componentTS.beforeLocalModificationsTasks = append(componentTS.beforeLocalModificationsTasks, prepare)
 
 	if fromStore {
 		validate := st.NewTask("validate-component", fmt.Sprintf(
 			i18n.G("Fetch and check assertions for component %q%s"), compSetup.ComponentName(), revisionStr),
 		)
-		componentTS.beforeLinkTasks = append(componentTS.beforeLinkTasks, validate)
+		componentTS.beforeLocalModificationsTasks = append(componentTS.beforeLocalModificationsTasks, validate)
 		addTask(validate)
 	}
 

overlord/snapstate/component_install_test.go

@@ -61,15 +61,15 @@ const (
 
 // opts is a bitset with compOpt* as possible values.
 func expectedComponentInstallTasks(opts int) []string {
-	beforeLink, link, postOpHooksAndAfter, discard := expectedComponentInstallTasksSplit(opts)
-	return append(append(append(beforeLink, link...), postOpHooksAndAfter...), discard...)
+	beforeMount, beforeLink, link, postOpHooksAndAfter, discard := expectedComponentInstallTasksSplit(opts)
+	return append(append(append(append(beforeMount, beforeLink...), link...), postOpHooksAndAfter...), discard...)
 }
 
-func expectedComponentInstallTasksSplit(opts int) (beforeLink, link, postOpHooksAndAfter, discard []string) {
+func expectedComponentInstallTasksSplit(opts int) (beforeMount, beforeLink, link, postOpHooksAndAfter, discard []string) {
 	if opts&compOptIsLocal != 0 {
-		beforeLink = []string{"prepare-component"}
+		beforeMount = []string{"prepare-component"}
 	} else {
-		beforeLink = []string{"download-component", "validate-component"}
+		beforeMount = []string{"download-component", "validate-component"}
 	}
 
 	// Revision is not the same as the current one installed
@@ -109,7 +109,7 @@ func expectedComponentInstallTasksSplit(opts int) (beforeLink, link, postOpHooks
 		discard = append(discard, "discard-component")
 	}
 
-	return beforeLink, link, postOpHooksAndAfter, discard
+	return beforeMount, beforeLink, link, postOpHooksAndAfter, discard
 }
 
 func checkSetupTasks(c *C, ts *state.TaskSet) {

overlord/snapstate/snapstate.go

@@ -489,11 +489,26 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 		prev = tasks[len(tasks)-1]
 	}
 
+	compTaskSets, compSetupIDs, err := splitComponentTasksForInstall(
+		compsups, st, snapst, snapsup, prepare.ID(), fromChange,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	finalBeforeLocalMod := prepare
+
 	var checkAsserts *state.Task
 	if fromStore {
 		// fetch and check assertions
 		checkAsserts = st.NewTask("validate-snap", fmt.Sprintf(i18n.G("Fetch and check assertions for snap %q%s"), snapsup.InstanceName(), revisionStr))
 		addTask(checkAsserts)
+		finalBeforeLocalMod = checkAsserts
+	}
+
+	for _, t := range compTaskSets.afterValidateTasks {
+		finalBeforeLocalMod = t
+		addTask(t)
 	}
 
 	// mount
@@ -522,16 +537,9 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 		addTask(t)
 	}
 
-	tasksBeforePreRefreshHook, tasksAfterLinkSnap, tasksAfterPostOpHook, tasksBeforeDiscard, compSetupIDs, err := splitComponentTasksForInstall(
-		compsups, st, snapst, snapsup, prepare.ID(), fromChange,
-	)
-	if err != nil {
-		return nil, err
-	}
+	compTaskSets.beforeDiscardTasks = append(compTaskSets.beforeDiscardTasks, discardExtraComps...)
 
-	tasksBeforeDiscard = append(tasksBeforeDiscard, discardExtraComps...)
-
-	for _, t := range tasksBeforePreRefreshHook {
+	for _, t := range compTaskSets.beforePreRefreshHookTasks {
 		addTask(t)
 	}
 
@@ -619,7 +627,7 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 	linkSnap := st.NewTask("link-snap", fmt.Sprintf(i18n.G("Make snap %q%s available to the system"), snapsup.InstanceName(), revisionStr))
 	addTask(linkSnap)
 
-	for _, t := range tasksAfterLinkSnap {
+	for _, t := range compTaskSets.afterLinkSnapTasks {
 		addTask(t)
 	}
 
@@ -678,7 +686,7 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 		addTask(installHook)
 	}
 
-	for _, t := range tasksAfterPostOpHook {
+	for _, t := range compTaskSets.afterPostOpHookTasks {
 		addTask(t)
 	}
 
@@ -708,7 +716,7 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 	startSnapServices := st.NewTask("start-snap-services", fmt.Sprintf(i18n.G("Start snap %q%s services"), snapsup.InstanceName(), revisionStr))
 	addTask(startSnapServices)
 
-	for _, t := range tasksBeforeDiscard {
+	for _, t := range compTaskSets.beforeDiscardTasks {
 		addTask(t)
 	}
 
@@ -789,14 +797,7 @@ func doInstall(st *state.State, snapst *SnapState, snapsup SnapSetup, compsups [
 	if installHook != nil {
 		installSet.MarkEdge(installHook, HooksEdge)
 	}
-	// if snap is being installed from the store, then the last task before
-	// any system modifications are done is check validate-snap, otherwise
-	// it's the prepare-snap
-	if checkAsserts != nil {
-		installSet.MarkEdge(checkAsserts, LastBeforeLocalModificationsEdge)
-	} else {
-		installSet.MarkEdge(prepare, LastBeforeLocalModificationsEdge)
-	}
+	installSet.MarkEdge(finalBeforeLocalMod, LastBeforeLocalModificationsEdge)
 	if flags&noRestartBoundaries == 0 {
 		if err := SetEssentialSnapsRestartBoundaries(st, nil, []*state.TaskSet{installSet}); err != nil {
 			return nil, err
@@ -840,6 +841,14 @@ func requiresKmodSetup(snapst *SnapState, compsups []ComponentSetup) bool {
 	return false
 }
 
+type componentTaskSetsForSnapInstall struct {
+	afterValidateTasks        []*state.Task
+	beforePreRefreshHookTasks []*state.Task
+	afterLinkSnapTasks        []*state.Task
+	afterPostOpHookTasks      []*state.Task
+	beforeDiscardTasks        []*state.Task
+}
+
 func splitComponentTasksForInstall(
 	compsups []ComponentSetup,
 	st *state.State,
@@ -848,28 +857,29 @@ func splitComponentTasksForInstall(
 	snapSetupTaskID string,
 	fromChange string,
 ) (
-	tasksBeforePreRefreshHook, tasksAfterLinkSnap, tasksAfterPostOpHook, tasksBeforeDiscard []*state.Task,
-	compSetupIDs []string,
-	err error,
+	componentTaskSetsForSnapInstall, []string, error,
 ) {
+	var compTaskSets componentTaskSetsForSnapInstall
+	var compSetupIDs []string
 	for _, compsup := range compsups {
 		componentTS, err := doInstallComponent(st, snapst, compsup, snapsup, snapSetupTaskID, nil, nil, fromChange)
 		if err != nil {
-			return nil, nil, nil, nil, nil, fmt.Errorf("cannot install component %q: %v", compsup.CompSideInfo.Component, err)
+			return componentTaskSetsForSnapInstall{}, nil, fmt.Errorf("cannot install component %q: %v", compsup.CompSideInfo.Component, err)
 		}
 
 		compSetupIDs = append(compSetupIDs, componentTS.compSetupTaskID)
 
-		tasksBeforePreRefreshHook = append(tasksBeforePreRefreshHook, componentTS.beforeLinkTasks...)
+		compTaskSets.afterValidateTasks = append(compTaskSets.afterValidateTasks, componentTS.beforeLocalModificationsTasks...)
+		compTaskSets.beforePreRefreshHookTasks = append(compTaskSets.beforePreRefreshHookTasks, componentTS.beforeLinkTasks...)
 		if componentTS.maybeLinkTask != nil {
-			tasksAfterLinkSnap = append(tasksAfterLinkSnap, componentTS.maybeLinkTask)
+			compTaskSets.afterLinkSnapTasks = append(compTaskSets.afterLinkSnapTasks, componentTS.maybeLinkTask)
 		}
-		tasksAfterPostOpHook = append(tasksAfterPostOpHook, componentTS.postHookToDiscardTasks...)
+		compTaskSets.afterPostOpHookTasks = append(compTaskSets.afterPostOpHookTasks, componentTS.postHookToDiscardTasks...)
 		if componentTS.maybeDiscardTask != nil {
-			tasksBeforeDiscard = append(tasksBeforeDiscard, componentTS.maybeDiscardTask)
+			compTaskSets.beforeDiscardTasks = append(compTaskSets.beforeDiscardTasks, componentTS.maybeDiscardTask)
 		}
 	}
-	return tasksBeforePreRefreshHook, tasksAfterLinkSnap, tasksAfterPostOpHook, tasksBeforeDiscard, compSetupIDs, nil
+	return compTaskSets, compSetupIDs, nil
 }
 
 func NeedsKernelSetup(model *asserts.Model) bool {

overlord/snapstate/snapstate_install_test.go

@@ -78,48 +78,53 @@ func expectedDoInstallTasks(typ snap.Type, opts, discards int, startTasks []stri
 			opts |= updatesBootConfig
 		}
 	}
+
+	var tasksAfterDownload, tasksBeforePreRefreshHook, tasksAfterLinkSnap, tasksAfterPostOpHook, tasksBeforeDiscard []string
+	for range components {
+		compOpts := compOptMultiCompInstall
+		if opts&localSnap != 0 {
+			compOpts |= compOptIsLocal
+		}
+		if opts&unlinkBefore != 0 {
+			compOpts |= compOptIsActive | compOptDuringSnapRefresh
+		}
+
+		beforeMount, beforeLink, link, hooksAndAfter, discard := expectedComponentInstallTasksSplit(compOpts)
+
+		tasksAfterDownload = append(tasksAfterDownload, beforeMount...)
+		tasksBeforePreRefreshHook = append(tasksBeforePreRefreshHook, beforeLink...)
+		tasksAfterLinkSnap = append(tasksAfterLinkSnap, link...)
+		tasksAfterPostOpHook = append(tasksAfterPostOpHook, hooksAndAfter...)
+		tasksBeforeDiscard = append(tasksBeforeDiscard, discard...)
+	}
+
 	if startTasks == nil {
 		switch {
 		case opts&localSnap != 0:
 			startTasks = []string{
 				"prerequisites",
 				"prepare-snap",
-				"mount-snap",
 			}
+			startTasks = append(startTasks, tasksAfterDownload...)
+			startTasks = append(startTasks, "mount-snap")
 		case opts&localRevision != 0:
 			startTasks = []string{
 				"prerequisites",
 				"prepare-snap",
 			}
+			startTasks = append(startTasks, tasksAfterDownload...)
 		default:
 			startTasks = []string{
 				"prerequisites",
 				"download-snap",
 				"validate-snap",
-				"mount-snap",
 			}
+			startTasks = append(startTasks, tasksAfterDownload...)
+			startTasks = append(startTasks, "mount-snap")
 		}
 	}
 	expected := startTasks
 
-	var tasksBeforePreRefreshHook, tasksAfterLinkSnap, tasksAfterPostOpHook, tasksBeforeDiscard []string
-	for range components {
-		compOpts := compOptMultiCompInstall
-		if opts&localSnap != 0 {
-			compOpts |= compOptIsLocal
-		}
-		if opts&unlinkBefore != 0 {
-			compOpts |= compOptIsActive | compOptDuringSnapRefresh
-		}
-
-		beforeLink, link, hooksAndAfter, discard := expectedComponentInstallTasksSplit(compOpts)
-
-		tasksBeforePreRefreshHook = append(tasksBeforePreRefreshHook, beforeLink...)
-		tasksAfterLinkSnap = append(tasksAfterLinkSnap, link...)
-		tasksAfterPostOpHook = append(tasksAfterPostOpHook, hooksAndAfter...)
-		tasksBeforeDiscard = append(tasksBeforeDiscard, discard...)
-	}
-
 	expected = append(expected, tasksBeforePreRefreshHook...)
 
 	if opts&unlinkBefore != 0 {
@@ -211,16 +216,24 @@ func verifyInstallTasksWithComponents(c *C, typ snap.Type, opts, discards int, c
 	kinds := taskKinds(ts.Tasks())
 
 	expected := expectedDoInstallTasks(typ, opts, discards, nil, components, nil)
-
 	c.Assert(kinds, DeepEquals, expected)
 
 	if opts&noLastBeforeModificationsEdge == 0 {
 		te := ts.MaybeEdge(snapstate.LastBeforeLocalModificationsEdge)
 		c.Assert(te, NotNil)
-		if opts&localSnap != 0 {
-			c.Assert(te.Kind(), Equals, "prepare-snap")
+
+		if len(components) == 0 {
+			if opts&localSnap != 0 {
+				c.Assert(te.Kind(), Equals, "prepare-snap")
+			} else {
+				c.Assert(te.Kind(), Equals, "validate-snap")
+			}
 		} else {
-			c.Assert(te.Kind(), Equals, "validate-snap")
+			if opts&localSnap != 0 {
+				c.Assert(te.Kind(), Equals, "prepare-component")
+			} else {
+				c.Assert(te.Kind(), Equals, "validate-component")
+			}
 		}
 	}
 
@@ -6636,7 +6649,30 @@ func (s *snapmgrTestSuite) testInstallComponentsRunThrough(c *C, snapName, insta
 		op:    "validate-snap:Doing",
 		name:  instanceName,
 		revno: snapRevision,
-	}, {
+	}}
+
+	// ops for downloading a component (but not yet mounting it)
+	for _, cs := range componentStates {
+		compName := cs.SideInfo.Component.ComponentName
+		compRev := cs.SideInfo.Revision
+		containerName := fmt.Sprintf("%s+%s", instanceName, compName)
+		filename := fmt.Sprintf("%s_%d.comp", containerName, compRev.N)
+
+		expected = append(expected, []fakeOp{{
+			op:   "storesvc-download",
+			name: cs.SideInfo.Component.String(),
+		}, {
+			op:                "validate-component:Doing",
+			name:              instanceName,
+			revno:             snapRevision,
+			componentName:     compName,
+			componentPath:     filepath.Join(dirs.SnapBlobDir, filename),
+			componentRev:      compRev,
+			componentSideInfo: *cs.SideInfo,
+		}}...)
+	}
+
+	expected = append(expected, []fakeOp{{
 		op:  "current",
 		old: "<no-current>",
 	}, {
@@ -6653,31 +6689,19 @@ func (s *snapmgrTestSuite) testInstallComponentsRunThrough(c *C, snapName, insta
 		name:  instanceName,
 		path:  filepath.Join(dirs.SnapBlobDir, snapFileName),
 		revno: snapRevision,
-	}}
+	}}...)
 
 	// ops for mounting a component (but not yet linking it)
 	for _, cs := range componentStates {
 		compName := cs.SideInfo.Component.ComponentName
 		compRev := cs.SideInfo.Revision
 		containerName := fmt.Sprintf("%s+%s", instanceName, compName)
-		filename := fmt.Sprintf("%s_%d.comp", containerName, compRev.N)
 
-		expected = append(expected, []fakeOp{{
-			op:   "storesvc-download",
-			name: cs.SideInfo.Component.String(),
-		}, {
-			op:                "validate-component:Doing",
-			name:              instanceName,
-			revno:             snapRevision,
-			componentName:     compName,
-			componentPath:     filepath.Join(dirs.SnapBlobDir, filename),
-			componentRev:      compRev,
-			componentSideInfo: *cs.SideInfo,
-		}, {
+		expected = append(expected, fakeOp{
 			op:                "setup-component",
 			containerName:     containerName,
-			containerFileName: filename,
-		}}...)
+			containerFileName: fmt.Sprintf("%s_%d.comp", containerName, compRev.N),
+		})
 	}
 
 	expected = append(expected, []fakeOp{{