Disconnect hooks, howto 'undo' connect hooks

snapd
1

With the changes that recently (May 9th) landed in master and the changes proposed in followup branches we’re finally getting support for interface hooks. New hooks currently available in master are:

  • prepare-plug-plugname
  • prepare-slot-slotname
  • connect-plug-plugname
  • connect-slot-slotname

These hooks (collectively named as “connect hooks” below) are executed on snap connect ... and when snap is installed (on autoconnect) or refreshed (upcoming PR 5120).

The PR https://github.com/snapcore/snapd/pull/4767 adds two more hooks:

  • disconnect-plug-plugname
  • disconnect-slot-slotname

These hooks will be executed on snap disconnect or removal with the connection still active (security not lifted yet, that is before actual disconnect_ task), so that they have required permissions to do any cleanups.

The problem

I’d like to discuss how to undo connect hooks (the first four hooks at the top) in case of failure. Currently, we don’t have any concept of undo for any hooks in the system.

  • If we just call disconnect hooks whenever any of the four connect hooks fail, the disconnect hooks will likely fail too since the connection wasn’t really fully created.

  • Since all hooks need to be defined upfront when we define a set of tasks to perform given change, there is no good way currently to add extra hooks and run them only in case of the need for undo.

The plan

Therefore we probably should extend hook manager machinery to support running hooks on undo
and make interface hooks more stateful, e.g. make them aware of what succeeded/failed (making the logic for hooks authors more complicated, unfortunately).

  • HookSetup structure would get extened with UndoHook name (optional, only for hooks that need undo).
  • We would use disconnect hooks on undo for all prepare-* and connect-*_ hook failures; we will expose some extra info to them about what failed (open question: how do we do that? Via snapctl or envrionment variables?
  • HookManager would grow undoRunHook method for the generic run-hook task kind; it would execute the undo hook defined in the HookSetup structure of the task if present, or would be a no-op if no undo hook was defined.
  • note, that because IgnoreError flag can be set on hooks, an undo hook would only get executed if the hook task as a whole fails, i.e. if IgnoreError is *not set.

Any comments to the issue and proposed plan?

2

a different approach (harder to realize I fear) would be to call disconnect hooks only correspondingly to undoing the connect-* ones and call something else corresponding to prepare-* possibly only if we didn’t get as far as completing the connect-* ones

3

Agreed, except the simplest to implement and understand might be to just have counterparts on the disconnect side, and establish that on errors we’ll go backwards as far as we went forwards.

So when going in:

  • prepare-plug-…
  • prepare-slot-…
  • <grant permissions>
  • connect-plug-…
  • connect-slot-…

Then, when taking it down:

  • disconnect-slot-…
  • disconnect-plug-…
  • <drop permissions>
  • unprepare-slot-…
  • unprepare-plug-…

With all hooks being optional.

Internally, we’d just set up the undo hook on the same task of the do hook, which means we’ll naturally call the undo side when going backwards only if we did call the do side when going forwards.

Sounds reasonable?

1 Like
4

yes, I think just being fully symmetric may be the easiest approach to explain, and should not let cases be uncovered.

5

Yes, looks very reasonable, I’ll do this. Thanks!

6

https://github.com/snapcore/snapd/pull/4767 seems to still just call the disconnect-* hooks. How can the invocations of disconnect-* distinguish that case from the undo situation? should we also on pure disconnect call unprepare-* ?

7

I think that calling unprepare-* hooks on normal disconnect sounds reasonable, thanks for raising this.

PR #4767 only introduces disconnect- hooks and I do not to inted to update this PR. I’m going to propose undo handling in separate PR if that’s ok (should be up for review soon).