Merge pull request #10812 from MiguelPires/fix-disc-fail-on-hook

o/ifacestate: don't fail remove if disconnect hook fails

5 files changed, 106 insertions, 23 deletions

diff --git a/overlord/ifacestate/export_test.go b/overlord/ifacestate/export_test.go
index bd0ff9efac..ad05a72aef 100644
--- a/overlord/ifacestate/export_test.go
+++ b/overlord/ifacestate/export_test.go
@@ -68,6 +68,10 @@ func NewConnectOptsWithAutoSet() connectOpts {
	return connectOpts{AutoConnect: true, ByGadget: false}
 }
 
+func NewDisconnectOptsWithAutoSet() disconnectOpts {
+	return disconnectOpts{AutoDisconnect: true}
+}
+
 func NewDisconnectOptsWithByHotplugSet() disconnectOpts {
	return disconnectOpts{ByHotplug: true}
 }
diff --git a/overlord/ifacestate/ifacestate.go b/overlord/ifacestate/ifacestate.go
index 882cc68ca9..8df590e16f 100644
--- a/overlord/ifacestate/ifacestate.go
+++ b/overlord/ifacestate/ifacestate.go
@@ -425,14 +425,16 @@ func disconnectTasks(st *state.State, conn *interfaces.Connection, flags disconn
	hookName := fmt.Sprintf("disconnect-slot-%s", slotName)
	if slotSnapInfo.Hooks[hookName] != nil {
	disconnectSlotHookSetup := &hookstate.HookSetup{
-	Snap: slotSnap,
-	Hook: hookName,
-	Optional: true,
+	Snap: slotSnap,
+	Hook: hookName,
+	Optional: true,
+	IgnoreError: flags.AutoDisconnect,
	}
	undoDisconnectSlotHookSetup := &hookstate.HookSetup{
-	Snap: slotSnap,
-	Hook: "connect-slot-" + slotName,
-	Optional: true,
+	Snap: slotSnap,
+	Hook: "connect-slot-" + slotName,
+	Optional: true,
+	IgnoreError: flags.AutoDisconnect,
	}
 
	summary := fmt.Sprintf(i18n.G("Run hook %s of snap %q"), disconnectSlotHookSetup.Hook, disconnectSlotHookSetup.Snap)
@@ -447,14 +449,16 @@ func disconnectTasks(st *state.State, conn *interfaces.Connection, flags disconn
	hookName := fmt.Sprintf("disconnect-plug-%s", plugName)
	if plugSnapInfo.Hooks[hookName] != nil {
	disconnectPlugHookSetup := &hookstate.HookSetup{
-	Snap: plugSnap,
-	Hook: hookName,
-	Optional: true,
+	Snap: plugSnap,
+	Hook: hookName,
+	Optional: true,
+	IgnoreError: flags.AutoDisconnect,
	}
	undoDisconnectPlugHookSetup := &hookstate.HookSetup{
-	Snap: plugSnap,
-	Hook: "connect-plug-" + plugName,
-	Optional: true,
+	Snap: plugSnap,
+	Hook: "connect-plug-" + plugName,
+	Optional: true,
+	IgnoreError: flags.AutoDisconnect,
	}
 
	summary := fmt.Sprintf(i18n.G("Run hook %s of snap %q"), disconnectPlugHookSetup.Hook, disconnectPlugHookSetup.Snap)
diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go
index c2b7cddf41..8bdf2187d2 100644
--- a/overlord/ifacestate/ifacestate_test.go
+++ b/overlord/ifacestate/ifacestate_test.go
@@ -4313,6 +4313,90 @@ plugs:
	})
 }
 
+func (s *interfaceManagerSuite) TestAutoDisconnectIgnoreHookError(c *C) {
+	s.mockIfaces(&ifacetest.TestInterface{InterfaceName: "test"})
+	mgr := s.hookManager(c)
+
+	// fail when running the disconnect hooks
+	hijackFunc := func(*hookstate.Context) error { return errors.New("test") }
+	mgr.RegisterHijack("disconnect-plug-plug", "consumer", hijackFunc)
+	mgr.RegisterHijack("disconnect-slot-slot", "producer", hijackFunc)
+
+	var consumerYaml = `
+name: consumer
+version: 1
+plugs:
+ plug:
+ interface: test
+hooks:
+ disconnect-plug-plug:
+`
+
+	var producerYaml = `
+name: producer
+version: 1
+slots:
+ slot:
+ interface: test
+hooks:
+ disconnect-slot-slot:
+`
+	consumerInfo := s.mockSnap(c, consumerYaml)
+	producerInfo := s.mockSnap(c, producerYaml)
+
+	s.state.Lock()
+	s.state.Set("conns", map[string]interface{}{
+	"consumer:plug producer:slot": map[string]interface{}{"interface": "test"},
+	})
+
+	s.state.Unlock()
+	s.manager(c)
+	s.state.Lock()
+
+	conn := &interfaces.Connection{
+	Plug: interfaces.NewConnectedPlug(consumerInfo.Plugs["plug"], nil, nil),
+	Slot: interfaces.NewConnectedSlot(producerInfo.Slots["slot"], nil, nil),
+	}
+
+	// call disconnect with the AutoDisconnect flag (used when removing a snap)
+	ts, err := ifacestate.DisconnectPriv(s.state, conn, ifacestate.NewDisconnectOptsWithAutoSet())
+	c.Assert(err, IsNil)
+
+	change := s.state.NewChange("disconnect", "")
+	change.AddAll(ts)
+
+	s.state.Unlock()
+	s.settle(c)
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	// the hook setup should be set to ignore errors
+	var hooksCount int
+	for _, t := range ts.Tasks() {
+	if t.Kind() != "run-hook" {
+	continue
+	}
+
+	var hooksetup hookstate.HookSetup
+	err = t.Get("hook-setup", &hooksetup)
+	c.Assert(err, IsNil)
+	c.Check(hooksetup.IgnoreError, Equals, true)
+
+	err = t.Get("undo-hook-setup", &hooksetup)
+	c.Assert(err, IsNil)
+	c.Check(hooksetup.IgnoreError, Equals, true)
+
+	hooksCount++
+	}
+
+	// should have two disconnection tasks
+	c.Assert(hooksCount, Equals, 2)
+
+	// the change should not have failed
+	c.Check(change.Err(), IsNil)
+	c.Assert(change.Status(), Equals, state.DoneStatus)
+}
+
 func (s *interfaceManagerSuite) TestManagerReloadsConnections(c *C) {
	s.mockIfaces(&ifacetest.TestInterface{InterfaceName: "test"}, &ifacetest.TestInterface{InterfaceName: "test2"})
	var consumerYaml = `
diff --git a/overlord/snapstate/autorefresh_gating_test.go b/overlord/snapstate/autorefresh_gating_test.go
index 1e58298aa1..4cdb5a3877 100644
--- a/overlord/snapstate/autorefresh_gating_test.go
+++ b/overlord/snapstate/autorefresh_gating_test.go
@@ -107,7 +107,7 @@ func (r *autoRefreshGatingStore) SnapAction(ctx context.Context, currentSnaps []
 }
 
 func mockInstalledSnap(c *C, st *state.State, snapYaml string, hasHook bool) *snap.Info {
-	snapInfo := snaptest.MockSnap(c, string(snapYaml), &snap.SideInfo{
+	snapInfo := snaptest.MockSnap(c, snapYaml, &snap.SideInfo{
	Revision: snap.R(1),
	})
 
diff --git a/tests/main/disconnect-undo/task.yaml b/tests/main/disconnect-undo/task.yaml
index 33144833ca..85b2e5a772 100644
--- a/tests/main/disconnect-undo/task.yaml
+++ b/tests/main/disconnect-undo/task.yaml
@@ -20,14 +20,5 @@ execute: |
 echo "And network plug remains connected"
 snap connections | MATCH "test-disconnect:network .*:network"
 
- echo "Snap removal fails because of failing disconnect hook"
- snap remove test-disconnect 2>&1 | MATCH "failure of disconnect hook"
-
- echo "And the plug is still connected"
- snap connections | MATCH "test-disconnect:network .*:network"
-
- echo "Making disconnect hook happy"
- touch /var/snap/test-disconnect/common/do-not-fail
-
- echo "And removing the snap is now possible"
+ echo "Snap removal succeeds despite failing disconnect hook"
 snap remove --purge test-disconnect