config: add StopSignal #544
Conversation
|
LGTM I am not sure if we should add |
|
On Mon, Jan 30, 2017 at 12:26:08PM -0800, Antonio Murdaca wrote:
M config.md (4)
M specs-go/v1/config.go (3)
This also needs a JSON Schema change and fs.go rebuild, although that
can happen as a follow-up PR if folks prefer.
I'd rather address this with an annotation [1], because then we don't
have to add property-specific documentation for how this should be
translated into the runtime config (in flight as #492). But
documenting translation for another property isn't that hard, so it's
not a big deal either way.
[1]: #541 (comment)
|
|
Updated schema-fs as well |
what exactly are the criteria here? this seems a bit loose; I thought we were going from a versioned docker config type? |
|
@runcom can you please list the valid strings in this field? And ideally write a test case? |
|
@philips sure |
|
@jonboulle Portability is just a measure of whether config is made with respect to the internals of the image or the host environment. Specifying the stop signal for a process is going to operate regardless of host machine (on the same os), whereas stop timeout may be very specific to how a machine is configured or what it is running. From a docker perspective, stop timeout exists in the image config, but there could be an argument that this is host specific, hence not included here automatically. Granted, I am favoring caution here, as it is easy to add but impossible to remove. It would be good for someone else to lodge an informed opinion here. |
|
@philips updated |
config.md
Outdated
| @@ -124,6 +124,10 @@ Note: Any OPTIONAL field MAY also be set to null, which is equivalent to being a | |||
| If there are no labels then this property MAY either be absent or an empty map. | |||
| Implementations that are reading/processing this configuration file MUST NOT generate an error if they encounter an unknown labels key. | |||
|
|
|||
| - **StopSignal** *string*, OPTIONAL | |||
|
|
|||
| The field contains the system call signal that will be sent to the container to exit. The signal can be a valid unsigned number that matches a position in the kernel’s syscall table, for instance `9`, or a signal name in the format `SIGNAME`, for instance `SIGKILL`. | |||
There was a problem hiding this comment.
Can we require the named version (e.g. SIGKILL) and not allow numbers (e.g. 9)? A fair number of names are portable across POSIX systems (and a handful are required by ISO C), but POSIX has very little to say about the semantics for a particular signal number (e.g. it does not require SIGKILL to be 9). And the Linux man page has:
Because the range of available real-time signals varies according to the glibc threading implementation… programs should never refer to real-time signals using hard-coded numbers, but instead should always refer to real-time signals using the notation
SIGRTMIN+n…
And more generally
Several signal numbers are architecture-dependent, as indicated in the "Value" column.
So using numbers is less portable for no obvious benefit (or maybe there is a benefit, and I'm just not seeing it?).
There was a problem hiding this comment.
I'm fine using named signals - @stevvooe wdyt?
There was a problem hiding this comment.
We use the posix constants: https://github.com/docker/docker/blob/dad8cbfc2d6db45331be2cbd8f9997ab0851fccf/container/container_unit_test.go#L29, as far as I can tell. These may be slightly platform specific, so I am not sure what constraints to apply, if any at all.
|
@stevvooe sorry, my question was imprecise - I meant criteria with respect to what fields we add to this config object at all, not about which are portable and which aren't. e.g. I thought this was basically https://github.com/docker/docker/blob/master/image/spec/v1.md#image-json-description , except now we're extending beyond that. |
|
god i hate this Can you rebase please? |
|
@vbatts done! |
|
LGTM |
|
@runcom Please rebase! |
|
@stevvooe done |
|
I still feel we shouldn't allow numbers unless there is a really good reason. |
@runcom Is this easy to do? If not, let me know and we can merge this and open an issue. |
I think it's fine, this is the spec and it's just wording afaict. I can drop the number stuff in the stop signal paragraph I guess. We can add numbers if there's a need to add them. |
|
BTW, this is the original conversation when stopsignal was added in docker moby/moby#15307 (comment) and Dan raised the issue that matching syscall names against a table is often outdated. Not sure that's a real argument though, but still, something to consider when deciding to use numbers as well. |
I wonder if we could look these up dynamically. |
|
@runcom Looks like we require another rebase. Sorry. |
runcom
force-pushed
the
stop-signal
branch
2 times, most recently
from
66d9a11 to
d5ab0e4
Compare
Signed-off-by: Antonio Murdaca <[email protected]>
|
Rebased |
|
LGTM |
|
LGTM |
Close #541
@stevvooe @vbatts PTAL I probably forgot something
Signed-off-by: Antonio Murdaca [email protected]