daemon, client, cmd/snap: implement snap start/stop/restart #3657

chipaca · 2017-08-04T11:05:21Z

This implements:

snap start [--enable] foo[.bar]...
snap stop [--disable] foo[.bar]...
snap restart [--reload] foo[.bar]...

Also it improves error messages for app-related commands by including
the snap name as the value of the snap-not-found error kind response.

This implements: * snap start [--enable] foo[.bar]... * snap stop [--disable] foo[.bar]... * snap restart [--reload] foo[.bar]... Also it improves error messages for app-related commands by including the snap name as the value of the snap-not-found error kind response.

codecov-io · 2017-08-04T11:35:17Z

Codecov Report

Merging #3657 into master will increase coverage by 0.39%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #3657      +/-   ##
==========================================
+ Coverage   75.16%   75.56%   +0.39%     
==========================================
  Files         388      391       +3     
  Lines       33628    33954     +326     
==========================================
+ Hits        25278    25657     +379     
+ Misses       6534     6471      -63     
- Partials     1816     1826      +10

Impacted Files	Coverage Δ
cmd/snap/error.go	`41.07% <0%> (-1.92%)`	⬇️
daemon/snap.go	`83.94% <100%> (ø)`	⬆️
daemon/response.go	`81.42% <100%> (+0.13%)`	⬆️
overlord/snapstate/snapstate.go	`80.72% <100%> (+0.25%)`	⬆️
client/apps.go	`88.77% <76.92%> (-7.84%)`	⬇️
cmd/snap/cmd_services.go	`49.54% <78.94%> (+43.77%)`	⬆️
daemon/api.go	`72.6% <91.3%> (+0.69%)`	⬆️
overlord/devicestate/handlers.go	`61.97% <0%> (-0.64%)`	⬇️
release/release.go	`89.18% <0%> (-0.15%)`	⬇️
interfaces/builtin/avahi_observe.go	`100% <0%> (ø)`	⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8880f59...8b0a3b8. Read the comment docs.

pedronis · 2017-08-04T12:32:18Z

daemon/api.go

+	st.Lock()
+	defer st.Unlock()
+	ts := cmdstate.Exec(st, desc, argv)
+	chg := st.NewChange("exec", desc)


s/exec/services-control/ or something?

should we check for conflicts with installing/refreshing the corresponding snap(s)?

Yeah, I forgot to change exec.

About conflicts, what would that look like?

usual snapstate.CheckChangeConflict or something similar?

it's a bit unusual to make task directly in api, but also don't know if inventing a place to put that makes sense

zyga

A few comments, looks good otherwise.

zyga · 2017-08-07T11:20:38Z

client/apps.go

+
+// RestartOptions represent the different options of the Restart call.
+type RestartOptions struct {
+	// Reload the services, if possible, instead of restarting.


It would be good to document the difference between the two.

zyga · 2017-08-07T11:22:27Z

client/apps_test.go

+func (cs *clientSuite) TestClientServiceStart(c *check.C) {
+	cs.rsp = `{"type": "async", "status-code": 202, "change": "24"}`
+
+	type tT struct {


zyga · 2017-08-07T11:25:09Z

client/apps_test.go

+			var reqOp map[string]interface{}
+			c.Assert(json.NewDecoder(cs.req.Body).Decode(&reqOp), check.IsNil, sc.comment)
+			if sc.opts.Enable {
+				c.Check(len(reqOp), check.Equals, 3, sc.comment)


What is 2 vs 3 below?

number of properties? maybe we should have testutil.SortedKeys(interface{} /*map*/) []string for cases like this

the boolean option isn't sent over the wire when false, so there's one less element in the decoded map.

zyga · 2017-08-07T11:26:30Z

client/apps_test.go

+			}
+			c.Check(reqOp["action"], check.Equals, "stop", sc.comment)
+			c.Check(reqOp["names"], check.DeepEquals, inames, sc.comment)
+		}


I think it would be nice to de-duplicate those tests.

I considered it, but couldn't find a way that wasn't too "magic" (either needing to fiddle with reflect, or passing around a bunch of functions.

If I missed an approach, let me know.

zyga · 2017-08-07T12:24:43Z

overlord/snapstate/snapstate.go

+//
+// It's like CheckChangeConflict, but for multiple snaps, and does not
+// check snapst.
+func CheckChangeConflictMulti(st *state.State, snapNames map[string]bool, checkConflictPredicate func(taskKind string) bool) error {


It would be great to add tests for those.

I think s/Multi/Many/ would follow the style around here more, and yes a test with a couple values in snapNames would be good

mvo5

Looks great, some small questions/suggestions/nitpick for your consideration. But no blockers.

mvo5 · 2017-08-08T15:04:28Z

cmd/snap/cmd_services.go

+	Enable bool `long:"enable"`
+}
+
+func (s *svcStart) Execute(args []string) error {


Are those commands tested ? Can probably be done in a similar style as cmd_snap_op_test.go

They're not! I'll cover them now.

mvo5 · 2017-08-08T15:05:57Z

cmd/snap/error.go

@@ -100,6 +100,12 @@ func errorToCmdMessage(snapName string, e error, opts *client.SnapOptions) (stri
 		// arch/channel/revision. Surface that here somehow!

 		msg = i18n.G("snap %q not found")
+		if snapName == "" {


I feel a little bit uneasy about this, its clever but snapName seems to be missleading now. But maybe I don't see the big picture. Maybe a comment where about err.Value and why it is used as snapName in this context and what sets it?

does it matter if we always believe the server instead? basically if Value != "" use that as snapName, also given that it can be "" it's a bit strange that snapName is the first argument to the function to me

I appreciate both these concerns. I think it needs a revamp, but if it's ok with you i'll leave that revamp for later? I don't think it's terrible to have it in master as is.

mvo5 · 2017-08-08T15:08:29Z

daemon/api.go

+		return InternalError("no services found")
+	}
+
+	argv := make([]string, 2, 3+len(appInfos))


Why 3+len(appInfos)? Maybe a small comment?

mvo5 · 2017-08-08T15:09:58Z

daemon/api_test.go

+
+func (s *appSuite) TestPostAppsStartOne(c *check.C) {
+	inst := appInstruction{Action: "start", Names: []string{"snap-a.svc2"}}
+	args := []string{"systemctl", "start", "snap.snap-a.svc2.service"}


Maybe expected or expectedCall or something here?

mvo5 · 2017-08-08T15:10:29Z

daemon/api_test.go

+func (s *appSuite) TestPostAppsStartOne(c *check.C) {
+	inst := appInstruction{Action: "start", Names: []string{"snap-a.svc2"}}
+	args := []string{"systemctl", "start", "snap.snap-a.svc2.service"}
+	s.testPostApps(c, inst, args)


Should we calso check the chg.Summary() here?

maybe... i'm still not happy with the summary though

mvo5 · 2017-08-08T15:11:08Z

daemon/api_test.go

+	c.Check(rsp.Result.(*errorResult).Message, check.Equals, `unknown action "discombobulate"`)
+}
+
+func (s *appSuite) TestPostAppsConflict(c *check.C) {


Nice set of tests!

pedronis

some comments

pedronis · 2017-08-09T10:06:40Z

client/apps_test.go

+func (cs *clientSuite) TestClientServiceStart(c *check.C) {
+	cs.rsp = `{"type": "async", "status-code": 202, "change": "24"}`
+
+	type tT struct {


pedronis · 2017-08-09T10:06:47Z

client/apps_test.go

+		comment check.CommentInterface
+	}
+
+	var scs []tT


pedronis · 2017-08-09T10:08:53Z

client/apps_test.go

+			var reqOp map[string]interface{}
+			c.Assert(json.NewDecoder(cs.req.Body).Decode(&reqOp), check.IsNil, sc.comment)
+			if sc.opts.Enable {
+				c.Check(len(reqOp), check.Equals, 3, sc.comment)


number of properties? maybe we should have testutil.SortedKeys(interface{} /*map*/) []string for cases like this

pedronis · 2017-08-09T10:15:50Z

cmd/snap/error.go

@@ -100,6 +100,12 @@ func errorToCmdMessage(snapName string, e error, opts *client.SnapOptions) (stri
 		// arch/channel/revision. Surface that here somehow!

 		msg = i18n.G("snap %q not found")
+		if snapName == "" {


does it matter if we always believe the server instead? basically if Value != "" use that as snapName, also given that it can be "" it's a bit strange that snapName is the first argument to the function to me

pedronis · 2017-08-09T10:17:50Z

daemon/api.go

+
+	argv := make([]string, 2, 3+len(appInfos))
+	argv[0] = "systemctl"
+	argv[1] = inst.Action


this is strange, shouldn't just we go over all cases in the switch?

We do! And the first line of each case other than default would be argv[1] = inst.Action. So...

can I make a silly request then? I would lay out the code differently

argv[0] = "systemctl" argv[1] = inst.Action switch inst.Action {

sure! done.

pedronis · 2017-08-09T10:19:09Z

daemon/api.go

+	st.Lock()
+	defer st.Unlock()
+	if err := snapstate.CheckChangeConflictMulti(st, snapNames, nil); err != nil {
+		return Conflict(err.Error())


this is a strange use of 409 I think

Hmm... perhaps. Let's see...

6.5.8. 409 Conflict

The 409 (Conflict) status code indicates that the request could not be completed due to a conflict with the current state of the target resource. This code is used in situations where the user might be able to resolve the conflict and resubmit the request. The server SHOULD generate a payload that includes enough information for a user to recognize the source of the conflict.

Conflicts are most likely to occur in response to a PUT request. For example, if versioning were being used and the representation being PUT included changes to a resource that conflict with those made by an earlier (third-party) request, the origin server might use a 409 response to indicate that it can't complete the request. In this case, the response representation would likely contain information useful for merging the differences based on the revision history.

To me this matches pretty closely with this usage: the request can't be completed because it clashes with the current state, so we let the user know what they can do (in this case, wait for the other change to finish). Do I have this wrong?

what's the resource? and what's state? the conflict here is transient while my reading of 409 is that it would be about a permanent state of the resource, so usually asking again wouldn't work either (unless there's some approach that is happy if the state is the same by different paths but that's not relevant here)

the target resource is the app (the service). The state's the State :-)

but, to go at this from the other end, what response do you think it should be? It's not a 400, as the request is fine. It's not a 5xx, there's nothing wrong with the server. We could use 422 Unprocessable Entity,

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity (hence a 415(Unsupported Media Type) status code is inappropriate), and the syntax of the request entity is correct (thus a 400 (Bad Request) status code is inappropriate) but was unable to process the contained instructions.

I think so far we probably use a mix of 400 or 500 for the situations, remember this is not the first place that uses those helper(s), the other are just buried in snapstate. So whatever we do we probably need to be consistent. We need to check if right now they are all 500 or 400.

pedronis · 2017-08-09T10:21:34Z

overlord/snapstate/snapstate.go

+//
+// It's like CheckChangeConflict, but for multiple snaps, and does not
+// check snapst.
+func CheckChangeConflictMulti(st *state.State, snapNames map[string]bool, checkConflictPredicate func(taskKind string) bool) error {


I think s/Multi/Many/ would follow the style around here more, and yes a test with a couple values in snapNames would be good

Biggest changes are the adding of tests.

pedronis · 2017-08-09T15:58:45Z

overlord/snapstate/snapstate.go

+//
+// It's like CheckChangeConflict, but for multiple snaps, and does not
+// check snapst.
+func CheckChangeConflictMany(st *state.State, snapNames map[string]bool, checkConflictPredicate func(taskKind string) bool) error {


I know it's convenient like this, but wouldn't the usual expectation for the signature here to have just:

snapNames []string

yes, and I went back and forth there a bit, but in the end the map made the most sense to me.
The second option was having it be a sorted list (easy to do because daemon has it sorted already), but I thought that a bit too obscure.

well, snapNames should be small enough that making a map first shouldn't be a problem, no?
you could even keep the map variant as the internal one and simply call it from Many and <NotMany>

…ead of a map[string]bool

pedronis

lgtm, last wondering about the change summary

pedronis · 2017-08-09T16:34:06Z

daemon/api.go

+		}
+	}
+
+	desc := fmt.Sprintf("%s of %v", inst.Action, names)


shouldn't this also differentiate enable vs start etc ?

probably, as i said elsewhere i'm not happy with the summaries and will revisit this. In a previous attempt at this functionality I invested a rather considerable amount of code in getting the summary right, and I still have that code :-D but it needs all sort of rejiggering to fit in this.

pedronis reviewed Aug 4, 2017

View reviewed changes

daemon: check for and abort on conflicts with ongoing tasks

4f27ccf

zyga approved these changes Aug 7, 2017

View reviewed changes

mvo5 reviewed Aug 8, 2017

View reviewed changes

pedronis reviewed Aug 9, 2017

View reviewed changes

chipaca added 3 commits August 9, 2017 16:34

daemon, client, cmd/snap: address review feedback

19e2be7

Biggest changes are the adding of tests.

daemon: missed one change requested in review (ops)

0e751f6

daemon: one more small legibility tweak

c8df78d

pedronis reviewed Aug 9, 2017

View reviewed changes

overlord/snapstate: make CheckChangeConflictMany take a []string inst…

8b0a3b8

…ead of a map[string]bool

pedronis approved these changes Aug 9, 2017

View reviewed changes

chipaca merged commit c2ef83c into canonical:master Aug 10, 2017

chipaca deleted the snap-services-ops branch September 4, 2017 18:26

daemon, client, cmd/snap: implement snap start/stop/restart #3657

daemon, client, cmd/snap: implement snap start/stop/restart #3657

Conversation

chipaca commented Aug 4, 2017

codecov-io commented Aug 4, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a 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

pedronis Aug 9, 2017 • edited Loading

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

6.5.8. 409 Conflict

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

pedronis Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 4, 2017 •

edited

Loading

pedronis Aug 9, 2017 •

edited

Loading

pedronis Aug 9, 2017 •

edited

Loading