Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add KillOpts for killing all processes #1434

Merged
merged 1 commit into from Aug 28, 2017

Conversation

crosbymichael
Copy link
Member

Fixes #1431

This adds KillOpts so that a client can specify when they want to kill a
single process or all the processes inside a container.

Signed-off-by: Michael Crosby crosbymichael@gmail.com

@codecov-io
Copy link

codecov-io commented Aug 28, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1434   +/-   ##
======================================
  Coverage    40.8%   40.8%           
======================================
  Files          23      23           
  Lines        2924    2924           
======================================
  Hits         1193    1193           
  Misses       1453    1453           
  Partials      278     278

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 a197618...ed6b8fb. Read the comment docs.

task_opts.go Outdated
All bool
}

type KillOpts func(context.Context, Process, *KillInfo) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could the options just take in the KillRequest type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we have been going away from exposing the protos in the client interfaces

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crosbymichael
Copy link
Member Author

ping @Random-Liu , can you take a look at this and see if it's what you had in mind?

@@ -51,3 +51,15 @@ func WithProcessKill(ctx context.Context, p Process) error {
<-s
return nil
}

type KillInfo struct {
// All kills all processes inside the task
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably comment that this only works for task. IIUC, exec.Kill will just ignore this field inside the shim.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Random-Liu
Copy link
Member

Random-Liu commented Aug 28, 2017

LGTM with a nit. @crosbymichael Thanks for the quick fix!

Fixes containerd#1431

This adds KillOpts so that a client can specify when they want to kill a
single process or all the processes inside a container.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mlaventure mlaventure merged commit 31b5bb9 into containerd:master Aug 28, 2017
@crosbymichael crosbymichael deleted the kill-all branch August 28, 2017 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants