Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snapstate: support for pre-refresh hook #4281
Conversation
stolowski
added some commits
Sep 28, 2017
zyga
approved these changes
Nov 23, 2017
LGTM but important question about services and hooks in this context.
| + return HookTask(st, summary, hooksup, nil) | ||
| +} | ||
| + | ||
| +func SetupPreRefreshHook(st *state.State, snapName string) *state.Task { |
zyga
Nov 23, 2017
Contributor
As a general comment it'd be nice if we had a no-op thing here when the snap in question doesn't have a particular hook.
stolowski
Nov 23, 2017
Contributor
That applies to the hook machinery, although IMHO it's useful that we have the task logged and visible even if there is no hook ("Run foo hook of bar snap if present" - that's what we see in the tasks). In any case it's a material for separate discussion if we want that.
pedronis
Nov 23, 2017
•
Edited 1 time
-
pedronis
Nov 23, 2017
Contributor
-
Nov 23, 2017
in general we might not even know if the hook is there (because of implicit hooks) when we setup the tasks in a change
| + } | ||
| + | ||
| + if runRefreshHooks { | ||
| + preRefreshHook := SetupPreRefreshHook(st, snapsup.Name()) |
zyga
Nov 23, 2017
Contributor
Can the refresh hook restart the services because it runs after they are stopped and can poke their sockets and what not?
stolowski
Nov 23, 2017
Contributor
Yes, it can, there is nothing that prevents it currently. I see how this can be an issue that leads to a failure if service is started for the old revision of the snap in pre-refresh. We can easily prevent this and error out in snapctl based on hook name, if that's what we want.
niemeyer
Nov 27, 2017
Contributor
Why are we running this after the services are stopped? Part of the reason of running this before it stops is precisely so that we have the data and the old environment at hand. People will need the service running if we intend them to be able to dump data before the refresh, for example.
| @@ -336,7 +336,15 @@ func (r *TaskRunner) Ensure() { | ||
| } | ||
| continue | ||
| } | ||
| + | ||
| + if mustWait(t) { |
codecov-io
commented
Nov 23, 2017
•
Edited 17 times
-
codecov-io
Nov 29, 2017
-
codecov-io
Nov 29, 2017
-
codecov-io
Nov 29, 2017
-
codecov-io
Nov 29, 2017
-
codecov-io
Nov 28, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 27, 2017
-
codecov-io
Nov 24, 2017
-
codecov-io
Nov 23, 2017
-
Nov 29, 2017 -
Nov 29, 2017 -
Nov 29, 2017 -
Nov 29, 2017 -
Nov 28, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 27, 2017 -
Nov 24, 2017 -
Nov 23, 2017
Codecov Report
@@ Coverage Diff @@
## master #4281 +/- ##
==========================================
- Coverage 76.19% 76.18% -0.02%
==========================================
Files 445 445
Lines 38738 38759 +21
==========================================
+ Hits 29516 29528 +12
- Misses 7207 7216 +9
Partials 2015 2015
Continue to review full report at Codecov.
|
zyga
added
the
Blocked
label
Nov 23, 2017
|
I'm marking this as blocked as the prerequisite PR is still in a state where changes are requested. |
pedronis
removed
the
Blocked
label
Nov 24, 2017
stolowski
added some commits
Nov 24, 2017
| + } | ||
| + | ||
| + if runRefreshHooks { | ||
| + preRefreshHook := SetupPreRefreshHook(st, snapsup.Name()) |
zyga
Nov 23, 2017
Contributor
Can the refresh hook restart the services because it runs after they are stopped and can poke their sockets and what not?
stolowski
Nov 23, 2017
Contributor
Yes, it can, there is nothing that prevents it currently. I see how this can be an issue that leads to a failure if service is started for the old revision of the snap in pre-refresh. We can easily prevent this and error out in snapctl based on hook name, if that's what we want.
niemeyer
Nov 27, 2017
Contributor
Why are we running this after the services are stopped? Part of the reason of running this before it stops is precisely so that we have the data and the old environment at hand. People will need the service running if we intend them to be able to dump data before the refresh, for example.
| + if snapst.Active { |
niemeyer
Nov 27, 2017
Contributor
Also, this sounds suspect. Does it mean we're running the hook even when the snap is not active? In which context would this make sense, or otherwise in which ways will it create problems?
stolowski
Nov 27, 2017
•
Edited 1 time
-
stolowski
Nov 27, 2017
Contributor
-
Nov 27, 2017
You're right about running the hook before stopping rather than after. I missed this aspect as I considered pre- and post- refresh hooks to be about running before and after switching active revision. Will fix.
pedronis
added this to the 2.30 milestone
Nov 27, 2017
stolowski
added some commits
Nov 27, 2017
|
Fixed per latest agreements. |
| @@ -61,6 +61,10 @@ func needsMaybeCore(typ snap.Type) int { | ||
| } | ||
| func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int) (*state.TaskSet, error) { | ||
| + if snapst.IsInstalled() && !snapst.Active { | ||
| + return nil, fmt.Errorf("internal error: cannot update a disabled snap") |
niemeyer
Nov 29, 2017
Contributor
There's no reason for this to be an internal error, as this is a real situation that can happen by the user manipulating the snap state in sane ways. We just want to make sure that case is forbidden here.
| @@ -122,14 +126,23 @@ func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int | ||
| prev = checkAsserts | ||
| } | ||
| + // run refresh hooks when updating existing snap, otherwise run install | ||
| + runRefreshHooks := (snapst.IsInstalled() && !snapsup.Flags.Revert) |
niemeyer
Nov 29, 2017
Contributor
This should be moved right above the condition that uses it below. Otherwise this is lost between code that is completely unrelated to it.
stolowski
added some commits
Nov 29, 2017
| @@ -61,6 +61,10 @@ func needsMaybeCore(typ snap.Type) int { | ||
| } | ||
| func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int) (*state.TaskSet, error) { | ||
| + if snapst.IsInstalled() && !snapst.Active { | ||
| + return nil, fmt.Errorf("cannot update a disabled snap %q", snapsup.Name()) |
| @@ -122,14 +126,23 @@ func doInstall(st *state.State, snapst *SnapState, snapsup *SnapSetup, flags int | ||
| prev = checkAsserts | ||
| } | ||
| + runRefreshHooks := (snapst.IsInstalled() && !snapsup.Flags.Revert) |
pedronis
Nov 29, 2017
Contributor
I think Gustavo meant the flag should also be computed near first use
stolowski commentedNov 23, 2017
This branch completes the support for refresh hooks by providing pre-refresh hook that is run during refresh before switching to the new revision (as opposed to post-refresh hook).
Note: Depends on #4259.