daemon,overlord: add subcommand handling to snapctl #1718

kyrofa · 2016-08-22T17:52:59Z

Make some more progress on LP: #1586465 by adding some more logic and subcommands to snapctl. Stub out initial set command. This should allow other subcommands to be easily added (e.g. for interface hooks).

Add some more logic and subcommands to snapctl. Stub out initial set command. This should allow other subcommands to be easily added (e.g. for interface hooks). Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

zyga · 2016-08-23T14:20:07Z

overlord/hookstate/handler.go

+
+	// Generate a secure, random ID for this handler
+	idBytes := make([]byte, 32)
+	_, err := rand.Read(idBytes)


Can this block for any amount of time?

It uses getrandom(), which by default uses /dev/urandom, which shouldn't block.

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

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

niemeyer · 2016-08-26T14:57:35Z

overlord/hookstate/hookmgr.go

+		state:          s,
+		runner:         runner,
+		repository:     newRepository(),
+		activeHandlers: newHandlerCollection(),


s/activeHandlers/handlers/ (we don't have any inactive ones)

I've mentioned this in earlier reviews: it doesn't feel like the additional abstraction there is gaining us much. As a thought experiment, try to inline a map and a simple mutex here, and see how much code you need to write here, vs. how much code you drop.

That said, I believe what we really want here is a map of context, not handlers. The reason being that context is a concrete and common implementation, and it would be simple and convenient to introduce a HookHandler method on context to navigate from a context into its handler. Doing that in the opposite direction is boring, because every single handler will need to reimplement the same method to return its context.

…kmanager_snapctl

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

niemeyer · 2016-08-29T21:04:32Z

daemon/api.go

+		return BadRequest("cannot run snapctl: %s", err)
+	}
+
+	stdout, stderr, err := ctlcmd.RunCommand(context, snapctlRequest.Args)


This one might be just ctlcmd.Run now that we have cmd in the package name.

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

niemeyer · 2016-08-29T21:20:34Z

client/snapctl.go


 // RunSnapctl requests a snapctl run for the given options.
-func (client *Client) RunSnapctl(options SnapCtlOptions) (stdout, stderr []byte, err error) {
+func (client *Client) RunSnapctl(options hookstate.SnapCtlRequest) (stdout, stderr []byte, err error) {


Options was right on this end, per other similar methods. If the point is reusing the same type, it's okay to keep it as options everywhere.

This should also be received as a pointer, as the other option values in client.

Also note that, as we agreed before, when reusing such types we should have them inside client rather than inside the other internal package, so it's obvious when we're adding code that makes no sense for the client end.

Moved this back to client.SnapCtlOptions, and used that throughout for consistency.

niemeyer · 2016-08-29T21:22:30Z

LGTM with these last few points sorted.

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

…kmanager_snapctl

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

niemeyer · 2016-08-30T10:27:59Z

tests/main/snapctl/task.yaml

        exit 1
    fi

+    if [[ ! "$output" =~ .*"no context for ID: \"foo\"".* ]]; then


Spread test still broken here. I'd suggest the more usual:

echo "$output" | grep -q 'no context for ID: "foo"'

No need for ifs or the extra message here. Note that English sentence below is a direct translation of the code, which in this specific case is more clear.

Or alternatively, using the bash expression syntax, but simply:

[[ "$output" =~ 'no context for ID: "foo"' ]]

I've just confirmed that it traces properly as well, and actually better than echo+grep.

Ah, and the problem is actually on the former line:

error: cannot run snapctl: cannot run snapctl: no context for ID: "foo" + output='Running snapctl. It should be in my PATH and not cause denials.' + [[ ! Running snapctl. It should be in my PATH and not cause denials. =~ .*no context for ID: "foo".* ]]

Need something like output="$(snap ... 2>&1 >/dev/null)" to capture only stderr.

Oops, good catch, thanks! Passing now.

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

mvo5 · 2016-08-30T14:33:31Z

overlord/hookstate/hooktest/handler.go

+
+// NewMockHandler returns a new MockHandler.
+func NewMockHandler() *MockHandler {
+	return &MockHandler{


Pardon my ignorance here, but given goes default policy of zeroing values, why explicatively init them here? Or am I missing something?

No, you're not missing anything, I'm being dumb. Fixed now, thanks!

mvo5 · 2016-08-30T14:34:44Z

Looks good, just one question inline

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

daemon,overlord: add subcommand handling to snapctl.

7de317f

Add some more logic and subcommands to snapctl. Stub out initial set command. This should allow other subcommands to be easily added (e.g. for interface hooks). Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

zyga reviewed Aug 23, 2016
View reviewed changes

overlord/hookstate: add getHandler test.

4aaded3

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

kyrofa force-pushed the feature/1586465/hookmanager_snapctl branch from c708671 to 4aaded3 Compare August 23, 2016 17:12

Merge remote-tracking branch 'origin/master' into feature/1586465/hoo…

7b19c44

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

niemeyer reviewed Aug 26, 2016
View reviewed changes

niemeyer added the Reviewed label Aug 26, 2016

kyrofa added 5 commits August 29, 2016 09:47

Merge remote-tracking branch 'origin/master' into feature/1586465/hoo…

bcc6cbe

…kmanager_snapctl

move from a map of handlers to a map of contexts.

469c907

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

s/tools/ctlcmd/

485e347

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

ctlcmd: use context instead of handler.

0887dd2

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

ctlcmd: don't mutate global state.

91460cd

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

niemeyer reviewed Aug 29, 2016
View reviewed changes

tests: expect snapctl to fail with invalid context ID.

a9bccdd

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

niemeyer reviewed Aug 29, 2016
View reviewed changes

kyrofa added 5 commits August 29, 2016 14:34

remove NewHookContext().

efaa713

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

use client.SnapCtlOptions instead of hookstate.SnapCtlRequest.

8819b66

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

Merge remote-tracking branch 'origin/master' into feature/1586465/hoo…

34090d3

…kmanager_snapctl

s/request/options/

0b006f2

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

fix missing quotes in spread test.

9f4d759

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

niemeyer changed the title ~~daemon,overlord: add subcommand handling to snapctl.~~ daemon,overlord: add subcommand handling to snapctl Aug 30, 2016

niemeyer reviewed Aug 30, 2016
View reviewed changes

kyrofa and others added 3 commits August 30, 2016 06:25

match against stderr, redirect stdout.

5eac7a7

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

Merge branch 'master' into feature/1586465/hookmanager_snapctl

952afba

Merge branch 'master' into feature/1586465/hookmanager_snapctl

d326659

mvo5 reviewed Aug 30, 2016
View reviewed changes

kyrofa and others added 3 commits August 30, 2016 07:47

hooktest: no need to specify initial mock values.

dd286b1

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

Merge branch 'master' into feature/1586465/hookmanager_snapctl

3a9baa8

Merge branch 'master' into feature/1586465/hookmanager_snapctl

058fd8a

kyrofa merged commit 6a6234a into canonical:master Aug 30, 2016

kyrofa deleted the feature/1586465/hookmanager_snapctl branch August 30, 2016 17:02

daemon,overlord: add subcommand handling to snapctl #1718

daemon,overlord: add subcommand handling to snapctl #1718

Uh oh!

Conversation

kyrofa commented Aug 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyrofa Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvo5 commented Aug 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kyrofa Aug 30, 2016 •

edited

Loading