ctr: make kill optionally use stop-signal by samuelkarp · Pull Request #2577 · containerd/containerd

samuelkarp · 2018-08-24T18:14:42Z

The OCI image specification includes a StopSignal field in the image configuration, denoting the system call signal to be sent to the container to exit. This commit adds an option to ctr task kill that reads the StopSignal from the image configuration and sends that signal.

crosbymichael · 2018-08-24T18:15:47Z

I kinda feel like if you are adding this, you are kinda relying on ctr to much ;)

samuelkarp · 2018-08-24T18:21:46Z

I'm mostly just trying to demonstrate how to extract data from the image configuration 😄. I was looking into this because I wanted to see what happened with health checks in the Docker image format (and they're not present in OCI); if I want to extract that data I'll need to go through this same sort of workflow.

(I don't rely on ctr in any production code, but I think it's a good place to stick example code that can be useful.)

samuelkarp · 2018-08-24T18:28:55Z

I also thought about putting something like this in the containerd client, but I wasn't sure where would make sense in terms of package organization. The oci package (where I cribbed most of this implementation from oci.WithImageConfig) seems to only contain oci.SpecOpts right now.

samuelkarp · 2018-08-24T18:33:09Z

Test Suite Failed
--- FAIL: TestCRISuite (56.64s)
	cri_test.go:122: Failed to run tests in paralllel: exit status 1
FAIL

This looks unrelated to me?

stevvooe · 2018-08-24T22:14:44Z

@samuelkarp It might be better to extract this from the actual OCI config on the task, rather than the image, as that might have changed.

For the cli, I think this should be the default for kill. I see no issue in changing this, as it doesn't break the compatibility guarantee.

stevvooe · 2018-08-24T22:19:27Z

Doesn't look like we can get at the current OCI config, so settling for the image is the best option, without an API change: https://github.com/containerd/containerd/blob/master/api/services/tasks/v1/tasks.proto#L112.

samuelkarp · 2018-08-27T20:34:52Z

@crosbymichael Any opinion on whether this should be the default? I'm happy to make the change if you concur with @stevvooe.

crosbymichael · 2018-09-04T15:32:09Z

Why not save this as a label/annotation on the runtime spec, the spec can then be fetched out

samuelkarp · 2018-09-24T20:42:37Z

@crosbymichael Would you suggest that for all containers that containerd runs? Or just an optional well-known annotation that can be set using the containerd client?

crosbymichael · 2018-09-24T20:46:20Z

Optional label. we could provide a const in containerd client for the key. What do you think?

samuelkarp · 2018-09-27T00:42:47Z

@crosbymichael Done.

The command "go get -u github.com/vbatts/git-validation" failed and exited with 1 during .

This failure appears to be unrelated to my commit.

crosbymichael · 2018-09-27T14:33:10Z

Should we add this to the containerd package and export it? It seems like people would have to re-implement this each time.

Yes, though moving this unmodified into the containerd package will create an import cycle; this code depends on commands.ParseSignal and commands imports containerd. Would you want to move commands.ParseSignal (and the associated commands.signalMap into containerd or should I just return the signal as a string and let the caller deal with it?

Why not just save the uint as the label value, then it's just a strconv.ParseUint?

StopSignal in the v1.ImageConfig is a string, so I'd still need to convert that string to a syscall.Signal somewhere, likely using commands.ParseSignal. It ends up being the same import cycle problem, but in storing the signal instead of retrieving it.

Ok, i see. I think it would be safe to move the signal map into the containerd client package. It's generally useful.

codecov-io · 2018-09-27T14:44:20Z

Codecov Report

Merging #2577 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2577   +/-   ##
=======================================
  Coverage   45.09%   45.09%           
=======================================
  Files          92       92           
  Lines       10132    10132           
=======================================
  Hits         4569     4569           
  Misses       4842     4842           
  Partials      721      721

Flag	Coverage Δ
#linux	`48.84% <ø> (ø)`	⬆️
#windows	`41.87% <ø> (ø)`	⬆️

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 df60d32...607888c. Read the comment docs.

Signed-off-by: Samuel Karp <[email protected]>

The OCI image specification includes a `StopSignal` field in the image configuration, denoting the system call signal to be sent to the container to exit. This commit adds a new `WithImageStopSignal` container option that can be used for storing the `StopSignal` field as a label on the container. This commit also adjusts `ctr run` to call `WithImageStopSignal` and `ctr tasks kill` to send the signal stored in that label by default. Signed-off-by: Samuel Karp <[email protected]>

samuelkarp · 2018-09-27T23:44:37Z

@crosbymichael Updated per your comments.

crosbymichael · 2018-09-28T15:43:14Z

LGTM

estesp

LGTM

samuelkarp force-pushed the stop-signal branch 2 times, most recently from a7537f7 to 4bb7fb4 Compare September 27, 2018 00:28

crosbymichael reviewed Sep 27, 2018

View reviewed changes

samuelkarp force-pushed the stop-signal branch from 4bb7fb4 to 5b46a61 Compare September 27, 2018 22:43

samuelkarp added 2 commits September 27, 2018 15:53

signals: move ParseSignal to containerd package

b392a3a

Signed-off-by: Samuel Karp <[email protected]>

samuelkarp force-pushed the stop-signal branch from 5b46a61 to 607888c Compare September 27, 2018 22:54

estesp approved these changes Sep 28, 2018

View reviewed changes

estesp merged commit ac01f20 into containerd:master Sep 28, 2018

thaJeztah mentioned this pull request Apr 2, 2019

[release/1.1 backport] travis configuration updates #3157

Closed

Conversation

samuelkarp commented Aug 24, 2018

Uh oh!

crosbymichael commented Aug 24, 2018

Uh oh!

samuelkarp commented Aug 24, 2018

Uh oh!

samuelkarp commented Aug 24, 2018

Uh oh!

samuelkarp commented Aug 24, 2018

Uh oh!

stevvooe commented Aug 24, 2018

Uh oh!

stevvooe commented Aug 24, 2018

Uh oh!

samuelkarp commented Aug 27, 2018

Uh oh!

crosbymichael commented Sep 4, 2018

Uh oh!

samuelkarp commented Sep 24, 2018

Uh oh!

crosbymichael commented Sep 24, 2018

Uh oh!

samuelkarp commented Sep 27, 2018

Uh oh!

crosbymichael Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

samuelkarp Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

crosbymichael Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

samuelkarp Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

crosbymichael Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samuelkarp commented Sep 27, 2018

Uh oh!

crosbymichael commented Sep 28, 2018

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Sep 27, 2018 •

edited

Loading