I think this needs some validation of StopSignal, probably in swarmkit's controlapi, and maybe also in the CLI.

Design LGTM, @aaronlehmann not sure how much validation we should do, or leave it to Linux?

@yongtang I think we can do --stop-timeout as a follow up

@thaJeztah: I think docker service create --stop-signal gordon ... should fail instead of creating a service that fails immediately.

Thanks @aaronlehmann @thaJeztah for the review. The PR has been updated. Now the signal is validated in containerToGRPC(). Please take a look.

For --stop-timeout, I noticed that in docker it is defined as a pointer:
https://github.com/docker/docker/blob/v1.13.0/api/types/container/config.go#L60

type Config struct {
...
	StopTimeout     *int                `json:",omitempty"` // Timeout (in seconds) to stop a container
...
}

That might be an issue to define it in protobuf. I will take a look and see how to solve it.

@aaronlehmann It looks like

--stop-grace-period duration       Time to wait before force killing a container (ns|us|ms|s|m|h)

is kind of --stop-timeout in docker run. I think it might have been covered?

Also, --stop-grace-period actually does not setup the value StopTimeout in container's config. Instead, it is called explicitly in:
https://github.com/docker/docker/blob/v1.13.1/daemon/cluster/executor/container/adapter.go#L338-L347

func (c *containerAdapter) shutdown(ctx context.Context) error {
	// Default stop grace period to nil (daemon will use the stopTimeout of the container)
	var stopgrace *int
	spec := c.container.spec()
	if spec.StopGracePeriod != nil {
		stopgraceValue := int(spec.StopGracePeriod.Seconds)
		stopgrace = &stopgraceValue
	}
	return c.backend.ContainerStop(c.container.name(), stopgrace)
}

I am wondering if --stop-signal could do the same thing to call it explicitly in shutdown? (instead of set the value in container's config)

I think it might have been covered?

Yeah, this is covered by --stop-grace-period.

Also, --stop-grace-period actually does not setup the value StopTimeout in container's config.

I'm not sure whether this is intentional. It may make sense to also apply this value in the container config. The initial purpose of --stop-grace-period was to handle this shutdown use case, but I don't see why it shouldn't set the StopTimeout for other cases as well.

I'm not completely sure how the inner workings are w.r.t. --stop-grace-period; is it currently only used when updating a service? --stop-timeout is used when shutting down the daemon; does swarmkit perform actions before the daemon shuts down?

Slightly orthogonal, but should we rename --stop-timeout on docker run to --stop-grace-period? It's a pity we're not consistent in naming here if they serve the same purpose

My understanding is that --stop-grace-period is currently only honored when swarmkit takes explicit action to shut down a container; i.e. during a service update. However, I think that honoring it in other cases makes more sense than adding a new parameter that would be confusingly similar.

Agreed; if they both do the same, we should have a single flag (and think of unifying the docker run and docker service create naming)

@htc2u if you have a bot running on your account; would it be possible to turn it off? It's generating a lot of noise (keep in mind that each comment sends over 3000 e-mails / notifications to subscribers to this repository)

The PR has been updated with #31091 (SwarmKit vendoring) merged.

I still think some validation of StopSignal before creating the service is a good idea, but I guess it's fine this way if it's not feasible.

Thanks @aaronlehmann for the feedback. I wasn't sure previously but let me take a look and see if we could do something with validation.

@aaronlehmann Before a service is created, the Spec is converted and containerToGRPC then it will be passed to swarm. As the Signal validation is done inside containerToGRPC, now the validation happens before the service is actually created.

I created an additional test case in TestSwarmStopSignal so that invalid signal is covered. Please take a look and let me know if there are any issues.

As the Signal validation is done inside containerToGRPC, now the validation happens before the service is actually created.

It just occurred to me that this will validate against the set of signals that are valid on the manager. In a future mixed-OS cluster, it might cause problems if one, for example, connects to a Windows manager to set up a Linux service.

Ideally we would validate against the superset of signals defined on all platforms, but this might be making things complicated. I suppose we could move the SignalMap definitions out of the platform-specific files (as SignalMapLinux, SignalMapSolaris, etc.), and have platform-specific code that chooses the right map - and also a function that checks if a signal is defined in any map. Feel free to remove the validation if this is getting too complicated.

cc @friism

I think it would be better to remove the validation.

LGTM

Thanks @aaronlehmann @dnephin for the review. I think the mixture of workers with different OS might be a legitimate concern in the future. At least the platform related elements (like signals) probably could be considered as "constraints". In this case, tasks will not be deployed to those workers that cannot meet the constraint (like specific signals).

For now I just removed the validation. I think we could revisit it in the future.