Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
hooks: commands for controlling own services from snapctl #3852
Conversation
stolowski
added some commits
Sep 1, 2017
All checks have failed
pedronis
requested a review
from
niemeyer
Sep 5, 2017
|
The errors from spread look real. I wonder if it could be appamror releated? But the logs do not indicate this. |
|
@mvo5 Indeed, thanks for heads up. No, I think I actually broke it by seemingly innocent changes I made before pushing... It was passing shortly before that, now I can reproduce the failure locally. Investigating. |
stolowski
added some commits
Sep 6, 2017
Some checks were not successful
Some checks haven’t completed yet
codecov-io
commented
Sep 6, 2017
•
Edited 1 time
-
codecov-io
Sep 8, 2017
-
Sep 8, 2017
Codecov Report
@@ Coverage Diff @@
## master #3852 +/- ##
==========================================
+ Coverage 75.91% 75.91% +<.01%
==========================================
Files 416 420 +4
Lines 35994 36034 +40
==========================================
+ Hits 27324 27355 +31
- Misses 6753 6759 +6
- Partials 1917 1920 +3
Continue to review full report at Codecov.
|
stolowski
requested a review
from
chipaca
Sep 6, 2017
|
FWIW I think we should keep the argument as |
| } | ||
| + return BadRequest(err.Error()) |
chipaca
Sep 7, 2017
Member
what's the second if for here? You've written
if foo() { return bar }
if baz() { return quux }
return quux
chipaca
Sep 7, 2017
Member
(if there is a reason for the second if, then my criticism is: use a type switch)
stolowski
Sep 7, 2017
Contributor
There is really no good argument for that other than my oversight. Thanks for spotting.
| + addCommand("stop", shortStopHelp, "", func() command { return &stopCommand{} }) | ||
| +} | ||
| + | ||
| +func getServiceInfos(st *state.State, snapName string, serviceNames []string) ([]*snap.AppInfo, error) { |
chipaca
Sep 7, 2017
Member
rather strange to have these helpers in stop.go. By “strange” I mean “arbitrary and confusing”. I don't mind having one file for each op (even though they're all fairly compact and really similar, meaning I myself would've probably plopped them all in a single file), but that means the generic bits need to live elsewhere.
chipaca
Sep 7, 2017
Member
this function is very similar to daemon.appInfosFor, except that it's subtly different: in this version the arguments need to be snap.service (not snap[.service]), so you can't restart all the services with just snap. I think this negates the benefits of keeping things prefixed with the snap name. Why have it this way?
chipaca
Sep 7, 2017
Member
If you're going to keep it in a separate func (and that might be fine, as this version can be much simpler than the other one which is more generic), why not make use of the fact that it's simpler? You've made it needlessly convoluted, I think.
stolowski
Sep 7, 2017
Contributor
The generic version of daemon.appInfosFor is not easily reusable, unfortunately. It's generic, but also specific to the deamon (e.g. it's tied to http responses), and also it's it lives in the deamon which would create a circular dependency.
+1 for the comment re stop.go, I'll move these functions to helpers.go or alike. The reason for having stop, start and restart separately is for consistency, as the existing snaptcl subcommands were implemented like that.
stolowski
Sep 7, 2017
Contributor
I wasn't aware of the simplified syntax where you can only give snap name to stop all services so yes, need to handle that. Thanks for spotting.
stolowski
added some commits
Sep 7, 2017
All checks have failed
|
@chipaca I think I've addressed all your comments. |
Some checks were not successful
chipaca
approved these changes
Sep 12, 2017
LGTM.
If you ever revisit this, I'm thinking maybe renaming AppInstruction to either Instruction or Command makes sense.
niemeyer
requested changes
Sep 13, 2017
Looks very nice overall, thanks for sorting that one out!
A few comments:
| - if snapName != lastName { | ||
| - snapNames = append(snapNames, snapName) | ||
| - lastName = snapName | ||
| + chg, err := servicectl.ServiceControl(st, appInfos, &inst) |
niemeyer
Sep 13, 2017
Contributor
Per existing conventions, this should be something like
ts, err := servicestate.Change(st, ...)`
and it should return a task set instead of a change, just as usual everywhere else.
For example, see configstate.Configure or cmdstate.Exec or snapstate.Install, etc.
niemeyer
Sep 13, 2017
Contributor
Please feel free to postpone to Change to TaskSet modification. The reason why we do this, for the record, is because task sets are composable. We can have those same tasks next to other tasks.
stolowski
Sep 14, 2017
Contributor
Allright, postponing this for now. Yes, I'm aware of the distinction between tasksets and changes, it was just more handy to have the change right away for the two use cases we currently have.
niemeyer
Sep 19, 2017
Contributor
Can you please do the rename suggested, though? There's no reason for this to be completely different, per prior comment.
chipaca
Sep 19, 2017
Member
hmm... hopping on late, but it seems to me that getting something back from a function called Change, and then having to pass it to a different function called NewChange is going to be a little bit weird for casual code readers.
stolowski
Sep 19, 2017
Contributor
@chipaca For now though this name makes sense. If we later change it to return taskset then indeed it might make sense to rename it.
| + chg, err := servicectl.ServiceControl(st, appInfos, &inst) | ||
| + if err != nil { | ||
| + if _, ok := err.(servicectl.ServiceActionConflict); ok { | ||
| + return InternalError(err.Error()) |
niemeyer
Sep 13, 2017
Contributor
An internal error is a bug in our logic: it should never happen, but we failed somehow. A change conflict is a completely reasonable error that we need to report back, but that can happen very naturally.
Luckily, there's a good error type for conflicts.. it's called Conflict.
| @@ -24,6 +24,14 @@ import "fmt" | ||
| var AttributesTask = attributesTask | ||
| var CopyAttributes = copyAttributes | ||
| +func SetServiceControlFunc(f ServiceControlFunc) { |
niemeyer
Sep 13, 2017
Contributor
The way we usually do this is:
func MockServiceChangeFunc(f <big type>) (restore func()) {
old := servicechangeFunc
servicechangeFunc = f
return func() { servicechangeFunc = old }
}
| + return svcs, nil | ||
| +} | ||
| + | ||
| +type ServiceControlFunc func(st *state.State, appInfos []*snap.AppInfo, inst *servicectl.AppInstruction) (*state.Change, error) |
| + | ||
| +type ServiceControlFunc func(st *state.State, appInfos []*snap.AppInfo, inst *servicectl.AppInstruction) (*state.Change, error) | ||
| + | ||
| +var runService ServiceControlFunc = servicectl.ServiceControl |
niemeyer
Sep 13, 2017
Contributor
This is the usual convention I believe:
var servicestateChange = servicestate.Change
No need for it to have a type declared.
| + defer st.Unlock() | ||
| + return chg.Err() | ||
| + case <-time.After(configstate.ConfigureHookTimeout() / 2): | ||
| + return fmt.Errorf("%s command timed out", inst.Action) |
niemeyer
Sep 13, 2017
•
Edited 1 time
-
niemeyer
Sep 13, 2017
Contributor
-
Sep 13, 2017
This is misleading. The command didn't time out.. we simply got tired of waiting and decided to return. It wasn't cancelled, and most likely it's still running in the background. Not sure about what's the best thing to do here, but for now let's at least be more honest:
"%s command is taking too long"
| +) | ||
| + | ||
| +var ( | ||
| + shortRestartHelp = i18n.G("Start services") |
| +) | ||
| + | ||
| +func init() { | ||
| + addCommand("restart", shortStartHelp, "", func() command { return &restartCommand{} }) |
| +type restartCommand struct { | ||
| + baseCommand | ||
| + Positional struct { | ||
| + ServiceNames []string `positional-arg-name:"<service>" required:"1"` |
niemeyer
Sep 13, 2017
Contributor
requires:"yes" would make it more consistent with the next line. Relevant for the other commands too.
| + "github.com/snapcore/snapd/snap" | ||
| +) | ||
| + | ||
| +type AppInstruction struct { |
| +} | ||
| + | ||
| +type UnknownAction struct{ error } | ||
| +type ServiceActionConflict struct{ error } |
| + argv[1] = "reload-or-restart" | ||
| + } | ||
| + default: | ||
| + return nil, UnknownAction{fmt.Errorf("unknown action %q", inst.Action)} |
niemeyer
Sep 13, 2017
Contributor
Error types are always returned as pointers. If we break that rule, we will create very hard to debug errors because we won't be sure, will try to catch errors the wrong way, and unless there's explicit test coverage for the branch, which unfortunately isn't always true for errors, the code compiles fine and never hits.
With that said, why is that a special type? In general we wait until we have a use case for the special type before introducing one.
stolowski
Sep 14, 2017
Contributor
Ok, fixed. You're right, there is no need for special error type.
| + st.Lock() | ||
| + defer st.Unlock() | ||
| + if err := snapstate.CheckChangeConflictMany(st, snapNames, nil); err != nil { | ||
| + return nil, ServiceActionConflict{err} |
| +COMMAND=$(snapctl get command) | ||
| +if [ "$COMMAND" != "" ]; | ||
| +then | ||
| + snapctl $COMMAND test-snapd-service.test-snapd-service |
| + echo "And then restart" | ||
| + snap set test-snapd-service command=restart | ||
| + | ||
| + echo "It's is running" |
stolowski
added some commits
Sep 14, 2017
All checks have passed
|
@niemeyer Thanks for the review! I've adressed all your comments. |
stolowski
added some commits
Sep 19, 2017
stolowski commentedSep 5, 2017
Support start/stop/restart commands from snapctl. I've moved part of the implementation of 'snap start/stop/restart' implemented by John to overlord/servicectl package to make it reusable from hookstate/ctlcmd. The only change done to the original code moved from api.go is the introducion of custom error type.
Open question: currently
snapctl stop|start|restartrequires "." argument, which is consistent withsnap stop|start|restart, even though<snapname>part could actually be optional, because it needs to be the current snap whose hook is being called. Any thoughs on making snap name implicit in this cotext, or shall I leave it as is?