overlord: actually run hooks. #1511

kyrofa · 2016-07-07T16:31:54Z

The basic machinery for running hooks is now in place, so it's time to make a bit more progress on LP: #1586465 and connect the pieces to enable them.

The basic machinery for running hooks is in place, so it's time to connect the pieces and enable them. Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

mvo5 · 2016-07-08T10:27:23Z

overlord/hookstate/hookmgr.go

@@ -143,13 +153,38 @@ func (m *HookManager) doRunHook(task *state.Task, tomb *tomb.Tomb) error {
 		return err
 	}

-	// TODO: Actually dispatch the hook.
+	_, err = RunHook(setup.Snap, setup.Revision, setup.Hook)
+	if err != nil {


I think it would be better if the runCommand error handling is done in the runCommand helper. Something like (if you don't need the output except for the error case which seems to be the case here):

func doRunCommand(name string, args...string) error { if output, err := exec.Command(name, args...).CombinedOuptut(); err != nil { return fmt.Errorf("failed to run command %q %q: %q (%s)", name, args, output, err) } return nil }

Alternatively https://github.com/snapcore/snapd/blob/master/partition/utils.go#L33 might be something worth putting in osutil if you need the stdout of the command.

Yeah agreed on the error handling. I'll investigate that method as well, thank you!

mvo5 · 2016-07-08T10:36:10Z

Thanks for this branch. Some suggestions around the command hanlding.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis · 2016-07-08T14:28:32Z

I think we likely want to tie the task tomb.Dying to calling cmd.Process.Kill() or cmd.Process.Signal combination

kyrofa · 2016-07-08T14:31:12Z

I think we likely want to tie the task tomb.Dying to calling cmd.Process.Kill() or cmd.Process.Signal combination

Oh, good catch, yes.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

mvo5 · 2016-07-08T15:25:02Z

overlord/hookstate/hookmgr.go

+	if err != nil {
+		// "exit code 1" is not particularly helpful. So rewrite the error
+		// using stdout and stderr instead.
+		err = fmt.Errorf("%s", strings.Trim(string(output), " \n"))


I think we want to give some more context to the error here. If for example a command just exits with exist-status 7 but no output the error would become "". Maybe more like: "fmt.Errorrf("failed to run command %q %q: %q (%s)", name, args, output, err) or similar, I don't mind the exact message, but I would like to see the original error (permission denied, command not found, exit status N etc) plus what command it actually was that run and the output.

Alright, done.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa · 2016-07-08T21:21:44Z

I think we likely want to tie the task tomb.Dying to calling cmd.Process.Kill() or cmd.Process.Signal combination.

Alright, I've done this. Please take a look when you're able.

…ofa/snapd into feature/1586465/enable_hooks

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

niemeyer · 2016-07-15T14:12:51Z

cmd/snap/cmd_run.go

@@ -67,7 +67,7 @@ func (x *cmdRun) Execute(args []string) error {
 	if x.Hook != "" && x.Command != "" {
 		return fmt.Errorf("cannot use --hook and --command together")
 	}
-	if x.Revision != "" && x.Hook == "" {
+	if x.Revision != "unset" && x.Revision != "" && x.Hook == "" {


Why do we need "unset" here?

For context, one of the reasons why I put "unset" there originally was because we were passing around unset revisions as if they were valid in awkward places, and it ended up as far as reaching the file system in some cases. So with that string we have a visual clue that something is not right. Typing in --revision=unset is one of those cases.. where's that string coming from?

Indeed, I don't envision anyone actually typing -r=unset, but it simplified a bit of the code that runs hooks from the HookManager. See this discussion for some background, and let me know if you'd like it changed back.

The use case here makes it look like the revision should never be unset. Is that not the case?

No. snap run accepts the -r parameter for the revision, which previously defaulted to an empty string, meaning "no revision, run on the active one." That required some custom logic in the HookManager to see if the requested hook had a revision set, and if not, to pass an empty string. This PR updates snap run to also accept "unset" to mean "no revision, run on the active one," which means HookManager can simply use snap.Revision.String() without any custom logic, and things just work.

Let me restate the question: do we have valid cases for having run-hook tasks without a revision?

Curious.. so where does --revision even come from? How about killing it all until we have a concrete reason?

This conversation.

Let me know if, after reading that discussion, you'd like it removed.

notice also that during installation of a snap itself we don't have an exposed concept of current until very late (but we know revisions)

After the discussion on telegram, I'll leave the revision handling the way it is in this PR, and we'll observe how it evolves as more hooks are added. If we run into issues with them they can be removed.

niemeyer · 2016-07-15T15:03:10Z

overlord/hookstate/processrunner.go

+
+// newProcessRunner starts a new process with the requested parameters and
+// returns its ProcessRunner.
+func newProcessRunner(name string, args ...string) (ProcessRunner, error) {


It feels like there's way too much indirection to do something quite straightforward. We have the exactly the same functionality available in exec.Cmd itself.

I bet you can comfortably kill all of that, including even doStartCommand (which is unused!), ProcessRunner, newProcessRunner, and all its friends, and replace all of it by simply:

var buf bytes.Buffer cmd := exec.Command("snap", "run", ...) cmd.Stdin = &buf cmd.Stdout = &buf cmd.Start()

Inside snapRun itself (old doRunHook).

For testing, we have a good mechanism for mocking actual commands (executables).

Note the comment on doStartCommand would then move there as well, as something along the lines of:

return fmt.Errorf("hook %q failed: %v", hookName, osutil.OutputErr(buf.Bytes(), err))

Right, but this was done so that I could actually test that flow. If I just use exec.Cmd, how do I verify that things are actually killed correctly?

testutil.MockCommand(c, "snap", "sleep 60000")

Very good, thank you-- that will indeed simplify things 😃 .

As an interesting philosophical point, note you're not actually testing that commands are killed correctly if you're mocking the thing that kills the commands out.

True. I expected that exec.Cmd.Process.Kill() would work as advertised-- I just wanted to test that it was called appropriately.

The logic also assumed that it was called at all, and on the right process, which was actually set rather than nil, etc. These additional layers that were created in order to enable mocking may also be broken.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

niemeyer · 2016-08-02T13:41:34Z

Thank you!

overlord: actually run hooks.

9fb7e1b

The basic machinery for running hooks is in place, so it's time to connect the pieces and enable them. Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa force-pushed the feature/1586465/enable_hooks branch from f720f2c to 9fb7e1b Compare July 7, 2016 16:34

mvo5 reviewed Jul 8, 2016
View reviewed changes

kyrofa and others added 2 commits July 8, 2016 13:36

travis: use Go 1.6.

bc83641

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Merge branch 'master' into feature/1586465/enable_hooks

b58a6d8

overlord/hookstate: include stdout if running hook fails.

a8c41ac

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

mvo5 reviewed Jul 8, 2016
View reviewed changes

kyrofa added 3 commits July 8, 2016 15:34

cmd/snap: handle revision of "unset" in snap run.

c12ee72

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

overlord/hookstate: make hook run failure more informative.

8f07e58

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

overlord/hookstate: allow hook to be aborted.

e8f771a

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa and others added 8 commits July 8, 2016 17:21

Merge branch 'master' into feature/1586465/enable_hooks

455a88d

Merge branch 'master' into feature/1586465/enable_hooks

c98aafd

Merge branch 'feature/1586465/enable_hooks' of https://github.com/kyr…

efc8c0a

…ofa/snapd into feature/1586465/enable_hooks

Merge branch 'master' into feature/1586465/enable_hooks

c2a8b83

cmd/snap: empty revision is okay.

6ddfd54

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Merge branch 'master' into feature/1586465/enable_hooks

9528896

Merge branch 'master' into feature/1586465/enable_hooks

1919fd5

Merge branch 'master' into feature/1586465/enable_hooks

3e8e84d

niemeyer reviewed Jul 15, 2016
View reviewed changes

kyrofa and others added 5 commits July 18, 2016 16:21

overlord/hookstate,osutil: use testutil.MockCommand.

46763bb

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Merge branch 'master' into feature/1586465/enable_hooks

259a8d8

Merge branch 'master' into feature/1586465/enable_hooks

47fcde3

Merge branch 'master' into feature/1586465/enable_hooks

d8998ef

Merge branch 'master' into feature/1586465/enable_hooks

4ee062a

kyrofa force-pushed the feature/1586465/enable_hooks branch from b35e2f4 to 4ee062a Compare July 30, 2016 16:14

Merge branch 'master' into feature/1586465/enable_hooks

1a1392a

niemeyer merged commit c92230c into snapcore:master Aug 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord: actually run hooks. #1511

overlord: actually run hooks. #1511

kyrofa commented Jul 7, 2016 •

edited

mvo5 Jul 8, 2016

kyrofa Jul 8, 2016

kyrofa Jul 8, 2016

mvo5 commented Jul 8, 2016

pedronis commented Jul 8, 2016

kyrofa commented Jul 8, 2016

mvo5 Jul 8, 2016

kyrofa Jul 8, 2016

kyrofa commented Jul 8, 2016

niemeyer Jul 15, 2016

kyrofa Jul 15, 2016 •

edited

niemeyer Jul 15, 2016

kyrofa Jul 15, 2016

niemeyer Jul 15, 2016

niemeyer Jul 15, 2016

kyrofa Jul 15, 2016

kyrofa Jul 15, 2016

pedronis Jul 15, 2016

kyrofa Jul 15, 2016 •

edited

niemeyer Jul 15, 2016 •

edited

niemeyer Jul 15, 2016

kyrofa Jul 15, 2016

niemeyer Jul 15, 2016

kyrofa Jul 15, 2016

niemeyer Jul 15, 2016

kyrofa Jul 15, 2016

niemeyer Jul 15, 2016

kyrofa Jul 18, 2016

niemeyer commented Aug 2, 2016

overlord: actually run hooks. #1511

overlord: actually run hooks. #1511

Conversation

kyrofa commented Jul 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Jul 8, 2016

pedronis commented Jul 8, 2016

kyrofa commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa commented Jul 8, 2016

Choose a reason for hiding this comment

kyrofa Jul 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jul 15, 2016 • edited

Choose a reason for hiding this comment

niemeyer Jul 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Aug 2, 2016

kyrofa commented Jul 7, 2016 •

edited

kyrofa Jul 15, 2016 •

edited

kyrofa Jul 15, 2016 •

edited

niemeyer Jul 15, 2016 •

edited