Add signals API to send a signal to one or more services #92

benhoyt · 2021-12-02T04:53:23Z

This implements spec JU029 Pebble: sending signals to services. Specifically:

An API to send a signal to one or more running services:
- POST /v1/signals {"signal": "SIGHUP", "services": ["s1", "s2"]}
A Go client to send a signal:
- c.SendSignal(&client.SendSignalOptions{Signal: "SIGHUP", Services: []string{"s1, "s2"})
A CLI command to send a signal:
- pebble signal HUP s1 s2 ...

One question I had was whether the API should return an error if the service being signaled is not running. After discussion with Harry we decided to mimic the behavior of the Unix kill command, which gives an error if the pid is not active. But that's open for debate -- it will mean if you call send_signal() in a charm it'll raise an exception if the service hasn't been started yet (maybe that's a good thing).

benhoyt · 2021-12-03T01:22:06Z

cmd/pebble/cmd_help.go

@@ -168,11 +168,15 @@ type helpCategory struct {
 var helpCategories = []helpCategory{{
 	Label:       "Basics",
 	Description: "basic management",
-	Commands:    []string{"run", "autostart", "start", "stop", "exec", "services", "logs"},
+	Commands:    []string{"run", "exec"},


A couple of things here:

I added the new signal command for this commit.

I'm adding in restart and replan, missed here when those were added.

"basic management" is getting too long, especially with the checks command that will be added in another PR. I think the category "service management" is good, though I'm not sure about the "basic management" name anymore -- suggestions for a better name/grouping?

Thanks for fixing my mistake with restart/replan

Also I like this grouping

benhoyt · 2021-12-03T01:23:36Z

internal/daemon/api_services_test.go

@@ -321,15 +320,3 @@ func (s *apiSuite) TestServicesReplan(c *C) {
 	c.Check(tasks[0].Summary(), Equals, `Start service "test1"`)
 	c.Check(tasks[1].Summary(), Equals, `Start service "test2"`)
 }
-
-type wrappedServiceManager struct {


This wasn't being used anywhere, deleting.

Thanks. I don't even know why this was here.

benhoyt · 2021-12-03T01:24:45Z

internal/overlord/servstate/handlers.go

@@ -499,14 +499,18 @@ func getAction(config *plan.Service, success bool) (action plan.ServiceAction, o
 func (s *serviceData) sendSignal(signal string) error {
 	switch s.state {
 	case stateStarting, stateRunning:
+		sig := unix.SignalNum(signal)


The send-signal functionality already existed via the restart/backoff work; I'm just tweaking the error behavior here.

Just to double check, what's the API we intend to offer here? "HUP", or "SIGHUP"? I suggest enforcing one of them only, and perhaps only "SIGHUP" here as it's the more formal name, which we're opting not to use in the CLI as we have "signal HUP" already.

hpidcock · 2021-12-03T02:40:04Z

internal/daemon/api_signals.go

+	if err := decoder.Decode(&payload); err != nil {
+		return statusBadRequest("cannot decode request body: %v", err)
+	}
+	if len(payload.Services) <= 0 {


In other APIs do we not deal with the wildcard case (might just be autostart?)? Here it probably doesn't make sense just wondering if we should seek consistency.

Yeah, replan and autostart error if you do provide service names, and start, stop, and restart error if you don't (which makes sense for each of those, I think). I don't think it's a good idea (or makes a lot of sense) to send a signal to all services. If someone really wants to they can do something like this:

service_names = [s.name for s in container.get_services()] container.send_signal('SIGHUP', service_names)

hpidcock · 2021-12-03T02:45:22Z

internal/daemon/api_signals.go

+
+type signalsPayload struct {
+	Signal   string   `json:"signal"`
+	Services []string `json:"services"`


I think we should look at adding a ProcessGroup/Group/SignalGroup bool, where if true the signal is sent to the process group of the service rather than just the pid of the process.

That way a SIGUSR1 can just be sent to the service process and not children in the same process group.

That's a good idea. I was wondering whether it should send to children or not. I guess there are two questions:

What should the default be, send to just the service, or send to group? I think your approach of sending to just the service is a reasonable default (least potentially destructive).

What should the field be named? I think group is not great, as that indicates a group name (and is used as a group name in other Pebble APIs and the plan config). Maybe SendToGroup bool -- longer but nice and clear?

I'm not entirely sure if we need to signal children or not, but I'm pretty sure we should not have that as the default behavior. We have no idea about what sub-processes the given service will have, and signaling the entire group seems somewhat of a bad practice. The one exception being SIGKILL, probably, which we don't want to be sent this way in any case.

As such, I suggest just limiting to the process itself for now. As we learn why people want to signal the group, we can design the API and implement it accordingly.

Okay, I've limited it to just the process for now, not the group.

hpidcock · 2021-12-03T02:50:45Z

internal/overlord/servstate/handlers.go

 		logger.Noticef("Sending %s to service %q", signal, s.config.Name)
-		err := syscall.Kill(-s.cmd.Process.Pid, unix.SignalNum(signal))
+		err := syscall.Kill(-s.cmd.Process.Pid, sig)


This is incorrect for your usage, this signals the process group (-pid is a process group), which I think only makes sense in service stop.

I think we need to review this anyway for service stop:

Stop should first signal SIGTERM to the service process (not the group, the process should SIGTERM its child processes)

Wait the grace time

Always send SIGKILL to the process group (even if the process exits, we need to signal the whole process group to ensure all child processes are killed).

@niemeyer does this sound right to you?

This is a good call -- I fairly new to this signal handling stuff and hadn't considered that. What you propose sounds reasonable. I'll try to do a bit more reading on it, and probably add this to the agenda for tomorrow night's spec review meeting.

We discussed this on video and have a solution -- will follow up in a separate PR as it's not related to the send-signal API.

niemeyer

Thanks for this!

Assuming these comments make sense, LGTM once they are sorted. Otherwise, happy to chat further.

client/signals.go

niemeyer · 2021-12-06T18:05:14Z

cmd/pebble/cmd_help.go

+}, {
+	Label:       "Services",
+	Description: "service management",
+	Commands:    []string{"autostart", "start", "stop", "restart", "services", "logs", "signal"},


It feels worth reorganizing indeed, but the groups feels a bit blurred, in the sense that it's not clear what goes inside which given their descriptions. Also, as a previous problem we already had, the descriptions are currently not worded evenly: some of them are actions ("view changes") while others are passive ("basic/service management"). Finally, "basic" now has "run" and "exec", which are completely unlike each other, and the proximity of their names doesn't help either.

So maybe we should dedicate a further moment to try to help the situation here and have more distinctive "buckets".

Here is an initial attempt:

"Run", "run pebble" {"run", "help", "version"} "Plan", "view and change configuration" {"add", "plan", "replan"} "Services", "manage services" {"services", "logs", "start", "restart", "signal", "stop", "replan"} "Changes", "manage changes and their tasks" {"changes", "tasks"} "Warnings", "manage warnings" {"warnings", "okay"}

Some notes:

Isn't "autostart" the old version of "replan"? That means we can drop the old one.

If "autostart" and "replan" are indeed friends, it's curious we had them in different groups. What's the right one?

I've tried to order services inside the groups using some subjective logic while taking into account their meaning, relevance, and proximity

The above is missing "exec", which doesn't really fit anywhere. I've updated suggesting adding "Operations" which will have "exec" and (soon) "checks". Let's discuss tonight.

For now I've added a "Files" group for files and exec.

cmd/pebble/cmd_signal.go

niemeyer · 2021-12-06T18:08:55Z

cmd/pebble/cmd_signal.go

+name must be uppercase to distinguish it from the service names, for example:
+
+pebble signal HUP mysql nginx
+pebble signal SIGHUP mysql nginx    # full signal name is fine too


I'm not strongly against the optional SIG prefix, but can we please wait a bit before we support it? It feels like consistency generally wins, and from the two lines above the first one reads the best. This way we also don't even need to document the exception.

I've removed it from the docs. However, it's still supported as we have to add the SIG prefix for the API anyway, so it would be explicit code to prevent it from working. I think consistently using HUP (for example) in the docs but allowing the alternative is fine.

niemeyer · 2021-12-06T18:11:58Z

internal/daemon/api_services_test.go

@@ -321,15 +320,3 @@ func (s *apiSuite) TestServicesReplan(c *C) {
 	c.Check(tasks[0].Summary(), Equals, `Start service "test1"`)
 	c.Check(tasks[1].Summary(), Equals, `Start service "test2"`)
 }
-
-type wrappedServiceManager struct {


Thanks. I don't even know why this was here.

niemeyer · 2021-12-06T18:15:06Z

internal/daemon/api_signals.go

+
+type signalsPayload struct {
+	Signal   string   `json:"signal"`
+	Services []string `json:"services"`


I'm not entirely sure if we need to signal children or not, but I'm pretty sure we should not have that as the default behavior. We have no idea about what sub-processes the given service will have, and signaling the entire group seems somewhat of a bad practice. The one exception being SIGKILL, probably, which we don't want to be sent this way in any case.

As such, I suggest just limiting to the process itself for now. As we learn why people want to signal the group, we can design the API and implement it accordingly.

niemeyer · 2021-12-06T18:18:11Z

internal/overlord/servstate/handlers.go

@@ -499,14 +499,18 @@ func getAction(config *plan.Service, success bool) (action plan.ServiceAction, o
 func (s *serviceData) sendSignal(signal string) error {
 	switch s.state {
 	case stateStarting, stateRunning:
+		sig := unix.SignalNum(signal)


Just to double check, what's the API we intend to offer here? "HUP", or "SIGHUP"? I suggest enforcing one of them only, and perhaps only "SIGHUP" here as it's the more formal name, which we're opting not to use in the CLI as we have "signal HUP" already.

niemeyer · 2021-12-06T18:20:48Z

internal/overlord/servstate/handlers.go

 		if err != nil {
 			return err
 		}

 	case stateBackoff, stateTerminating, stateKilling, stateStopped:
-		return fmt.Errorf("cannot send signal while service is stopped or stopping")
+		return fmt.Errorf("service is not running")


This seems to lack the context ("cannot send signal ...").

The message below seems to lack a verb as well ("is invalid" or similar).

Yeah, given the context added below this is okay.

internal/overlord/servstate/manager.go

benhoyt · 2021-12-07T04:23:50Z

Just to double check, what's the API we intend to offer here? "HUP", or "SIGHUP"? I suggest enforcing one of them only, and perhaps only "SIGHUP" here as it's the more formal name, which we're opting not to use in the CLI as we have "signal HUP" already.

Yep, it's always the full SIGHUP in the API (also in the existing exec API).

#13558 Includes the following Pebble PRs: * canonical/pebble#92 * canonical/pebble#88 * canonical/pebble#87 * canonical/pebble#90

benhoyt added 5 commits December 2, 2021 17:52

Add /v1/signals API to send a signal to one or more services

7780ea5

Add client SendSignals method and test

9b76c72

Add "pebble signal HUP svc1 svc2 ..." command and tests

de00707

Rename SignalsOptions to SendSignalOptions

5cffd69

Add restart and replan commands; Move "run" and "exec" to new category

f3e9474

benhoyt commented Dec 3, 2021

View reviewed changes

benhoyt marked this pull request as ready for review December 3, 2021 01:31

benhoyt mentioned this pull request Dec 3, 2021

Add send_signal method to pebble.Client and model.Container canonical/operator#669

Merged

hpidcock reviewed Dec 3, 2021

View reviewed changes

Weird to check for len() < 0

b2bfe5d

niemeyer approved these changes Dec 6, 2021

View reviewed changes

benhoyt added 2 commits December 7, 2021 16:07

Code review tweaks

265bc50

Make SendSignal API only send to process, not group

6bfbec9

Replace Operations command group with Files

3168197

benhoyt merged commit 4d9bc7c into canonical:master Dec 8, 2021

benhoyt deleted the signals-api branch December 8, 2021 23:24

This was referenced Dec 8, 2021

Update Pebble: send signal API, restart from snapd, simplify client.Logs "N" juju/juju#13558

Merged

Need a way to send signals to services (eg: SIGHUP to reload config) #16

Closed

jujubot added a commit to juju/juju that referenced this pull request Dec 9, 2021

Merge pull request #13558 from benhoyt/update-pebble-13

24dfee1

#13558 Includes the following Pebble PRs: * canonical/pebble#92 * canonical/pebble#88 * canonical/pebble#87 * canonical/pebble#90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add signals API to send a signal to one or more services #92

Add signals API to send a signal to one or more services #92

benhoyt commented Dec 2, 2021 •

edited

Loading

benhoyt Dec 3, 2021 •

edited

Loading

hpidcock Dec 3, 2021

hpidcock Dec 3, 2021

benhoyt Dec 3, 2021

niemeyer Dec 6, 2021

benhoyt Dec 3, 2021

niemeyer Dec 6, 2021

hpidcock Dec 3, 2021

benhoyt Dec 5, 2021

hpidcock Dec 3, 2021

hpidcock Dec 3, 2021

benhoyt Dec 5, 2021

niemeyer Dec 6, 2021

benhoyt Dec 7, 2021

hpidcock Dec 3, 2021

benhoyt Dec 5, 2021 •

edited

Loading

benhoyt Dec 8, 2021

niemeyer left a comment

niemeyer Dec 6, 2021

benhoyt Dec 7, 2021

benhoyt Dec 8, 2021

niemeyer Dec 6, 2021

benhoyt Dec 7, 2021

niemeyer Dec 6, 2021

niemeyer Dec 6, 2021

niemeyer Dec 6, 2021

niemeyer Dec 6, 2021

benhoyt Dec 7, 2021

benhoyt commented Dec 7, 2021

Add signals API to send a signal to one or more services #92

Add signals API to send a signal to one or more services #92

Conversation

benhoyt commented Dec 2, 2021 • edited Loading

benhoyt Dec 3, 2021 • 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

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

benhoyt Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer 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

benhoyt commented Dec 7, 2021

benhoyt commented Dec 2, 2021 •

edited

Loading

benhoyt Dec 3, 2021 •

edited

Loading

benhoyt Dec 5, 2021 •

edited

Loading