Review feedback (thanks Samuele).inject-autoconnect

2 files changed, 106 insertions, 18 deletions

diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go
index 6d0fc30d87..28f8677bef 100644
--- a/overlord/ifacestate/handlers.go
+++ b/overlord/ifacestate/handlers.go
@@ -124,12 +124,19 @@ func (m *InterfaceManager) doSetupProfiles(task *state.Task, tomb *tomb.Tomb) er
 
	// Compatibility with old snapd: check if we have auto-connect task and if not, inject it after self (setup-profiles).
	// In the older snapd versions interfaces were auto-connected as part of setupProfilesForSnap.
-	if task.Change() != nil {
+	if snapInfo.Type != snap.TypeOS || corePhase2 {
	var hasAutoConnect bool
	for _, t := range task.Change().Tasks() {
	if t.Kind() == "auto-connect" {
-	hasAutoConnect = true
-	break
+	otherSnapsup, err := snapstate.TaskSnapSetup(t)
+	if err != nil {
+	return err
+	}
+	// Check if this is auto-connect task for same snap
+	if snapsup.Name() == otherSnapsup.Name() {
+	hasAutoConnect = true
+	break
+	}
	}
	}
	if !hasAutoConnect {
@@ -807,4 +814,7 @@ func injectTasks(mainTask *state.Task, extraTasks *state.TaskSet) {
	for _, t := range ht {
	t.WaitAll(extraTasks)
	}
+
+	// make the extra tasks wait for main task
+	extraTasks.WaitFor(mainTask)
 }
diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go
index 0aa97c4451..74fd64029e 100644
--- a/overlord/ifacestate/ifacestate_test.go
+++ b/overlord/ifacestate/ifacestate_test.go
@@ -2562,6 +2562,12 @@ func (s *interfaceManagerSuite) TestInjectTasks(c *C) {
	ids := []string{t1.HaltTasks()[0].Kind(), t2.HaltTasks()[1].Kind()}
	sort.Strings(ids)
	c.Assert(ids, DeepEquals, []string{"task1-1", "task1-2"})
+
+	// verify that extra tasks wait for the main task
+	c.Assert(t1.WaitTasks(), HasLen, 1)
+	c.Assert(t1.WaitTasks()[0].Kind(), Equals, "task1")
+	c.Assert(t2.WaitTasks(), HasLen, 1)
+	c.Assert(t2.WaitTasks()[0].Kind(), Equals, "task1")
 }
 
 func (s *interfaceManagerSuite) TestInjectTasksWithNullChange(c *C) {
@@ -2589,21 +2595,33 @@ func (s *interfaceManagerSuite) TestInjectTasksWithNullChange(c *C) {
 func (s *interfaceManagerSuite) TestSetupProfilesInjectsAutoConnectIfMissing(c *C) {
	mgr := s.manager(c)
 
-	si := &snap.SideInfo{
-	RealName: "snap",
+	si1 := &snap.SideInfo{
+	RealName: "snap1",
	Revision: snap.R(1),
	}
-	sup := &snapstate.SnapSetup{SideInfo: si}
-	_ = snaptest.MockSnap(c, sampleSnapYaml, si)
+	sup1 := &snapstate.SnapSetup{SideInfo: si1}
+	_ = snaptest.MockSnap(c, sampleSnapYaml, si1)
+
+	si2 := &snap.SideInfo{
+	RealName: "snap2",
+	Revision: snap.R(1),
+	}
+	sup2 := &snapstate.SnapSetup{SideInfo: si2}
+	_ = snaptest.MockSnap(c, consumerYaml, si2)
 
	s.state.Lock()
	defer s.state.Unlock()
 
-	task := s.state.NewTask("setup-profiles", "")
-	task.Set("snap-setup", sup)
+	task1 := s.state.NewTask("setup-profiles", "")
+	task1.Set("snap-setup", sup1)
+
+	task2 := s.state.NewTask("setup-profiles", "")
+	task2.Set("snap-setup", sup2)
 
	chg := s.state.NewChange("test", "")
-	chg.AddTask(task)
+	chg.AddTask(task1)
+	task2.WaitFor(task1)
+	chg.AddTask(task2)
 
	s.state.Unlock()
 
@@ -2613,15 +2631,75 @@ func (s *interfaceManagerSuite) TestSetupProfilesInjectsAutoConnectIfMissing(c *
 
	// ensure all our tasks ran
	c.Assert(chg.Err(), IsNil)
-	c.Assert(chg.Tasks(), HasLen, 2)
+	c.Assert(chg.Tasks(), HasLen, 4)
 
-	// sanity check
-	c.Assert(chg.Tasks()[0].Kind(), Equals, "setup-profiles")
+	// sanity checks
+	t := chg.Tasks()[0]
+	c.Assert(t.Kind(), Equals, "setup-profiles")
+	t = chg.Tasks()[1]
+	c.Assert(t.Kind(), Equals, "setup-profiles")
+
+	// check that auto-connect tasks were added and have snap-setup
+	var autoconnectSup snapstate.SnapSetup
+	t = chg.Tasks()[2]
+	c.Assert(t.Kind(), Equals, "auto-connect")
+	c.Assert(t.Get("snap-setup", &autoconnectSup), IsNil)
+	c.Assert(autoconnectSup.Name(), Equals, "snap1")
 
-	// check that auto-connect task was added and has snap-setup
+	t = chg.Tasks()[3]
+	c.Assert(t.Kind(), Equals, "auto-connect")
+	c.Assert(t.Get("snap-setup", &autoconnectSup), IsNil)
+	c.Assert(autoconnectSup.Name(), Equals, "snap2")
+}
+
+func (s *interfaceManagerSuite) TestSetupProfilesInjectsAutoConnectIfCore(c *C) {
+	mgr := s.manager(c)
+
+	si1 := &snap.SideInfo{
+	RealName: "core",
+	Revision: snap.R(1),
+	}
+	sup1 := &snapstate.SnapSetup{SideInfo: si1}
+	_ = snaptest.MockSnap(c, ubuntuCoreSnapYaml, si1)
+
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	task1 := s.state.NewTask("setup-profiles", "")
+	task1.Set("snap-setup", sup1)
+
+	task2 := s.state.NewTask("setup-profiles", "")
+	task2.Set("snap-setup", sup1)
+	task2.Set("core-phase-2", true)
+	task2.WaitFor(task1)
+
+	chg := s.state.NewChange("test", "")
+	chg.AddTask(task1)
+	chg.AddTask(task2)
+
+	s.state.Unlock()
+
+	defer mgr.Stop()
+	s.settle(c)
+	s.state.Lock()
+
+	// ensure all our tasks ran
+	c.Assert(chg.Err(), IsNil)
+	c.Assert(chg.Tasks(), HasLen, 3)
+
+	// sanity checks
+	t := chg.Tasks()[0]
+	c.Assert(t.Kind(), Equals, "setup-profiles")
+	t = chg.Tasks()[1]
+	c.Assert(t.Kind(), Equals, "setup-profiles")
+	var phase2 bool
+	c.Assert(t.Get("core-phase-2", &phase2), IsNil)
+	c.Assert(t.HaltTasks(), HasLen, 1)
+
+	// check that auto-connect task was added after phase2
	var autoconnectSup snapstate.SnapSetup
-	ac := chg.Tasks()[1]
-	c.Assert(ac.Kind(), Equals, "auto-connect")
-	c.Assert(ac.Get("snap-setup", &autoconnectSup), IsNil)
-	c.Assert(autoconnectSup.Name(), Equals, "snap")
+	t = chg.Tasks()[2]
+	c.Assert(t.Kind(), Equals, "auto-connect")
+	c.Assert(t.Get("snap-setup", &autoconnectSup), IsNil)
+	c.Assert(autoconnectSup.Name(), Equals, "core")
 }