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

Implement exec kill API #38704

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@dtaniwaki
Copy link

commented Feb 10, 2019

Closes #35703 #9098

- What I did

I implemented a feature to kill exec instance.

- How I did it

I added an API endpoint for the action and a method to call it in the client, then implemented an exec action in the daemon.

- How to verify it

  • Run sleep container by docker run --name exec-kill -d ubuntu:16.04 sleep infinity
  • Then, attach a process by docker exec -d exec-kill sleep 99999. (use different sleep time from the container command to easily identify it)
  • Now, you can see the executed process by executing ps aux.
  • You can get the exec ID by docker inspect exec-kill | grep ExecIDs -A 3.
  • Then kill the exec instance by curl --unix-socket /var/run/docker.sock -X DELETE "http:/v1.40/exec/<EXEC_ID>".
  • Check if the executed process is gone by ps aux.

- Description for the changelog

Add exec kill feature with API endpoint and a client method

- A picture of a cute animal (not mandatory but encouraged)

268878_2231235108554_8050336_n

@GordonTheTurtle

This comment has been minimized.

Copy link

commented Feb 10, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "35703-exec-kill" git@github.com:dtaniwaki/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch from 5c280de to fdb822f Feb 10, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Feb 10, 2019

Show resolved Hide resolved api/swagger.yaml Outdated
@AkihiroSuda
Copy link
Member

left a comment

Design looks good. Thank you

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch 2 times, most recently from 69439f2 to 880c309 Feb 11, 2019

@dtaniwaki

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

@AkihiroSuda Thank you for reviewing my pull request. I updated the code based on your review result. Would you check it again?

Show resolved Hide resolved integration/container/exec_test.go Outdated
Show resolved Hide resolved daemon/exec.go Outdated
err = client.ContainerExecKill(ctx, id.ID, types.ExecKillOptions{})
assert.NilError(t, err)

ticker := time.NewTicker(1 * time.Second)

This comment has been minimized.

Copy link
@AkihiroSuda

This comment has been minimized.

Copy link
@dtaniwaki

dtaniwaki Feb 11, 2019

Author

I'd like to have your opinion about the behavior of the exec kill feature.

Currently, it just sends a signal to the process and doesn't remove the instance from execCommands. That's why I assumed it might not finish just after calling the method. If we want to make the kill action with a synchronized manner, we have to wait for the process termination in ContainerExecKill to remove the exec instance, but it requires timeout in its option. On the other hand, if we remove the exec instance just after sending a signal to the process, we can make it synchronized easily without timeout, but have the possibility of resource leakage.

Which do you think is better?

This comment has been minimized.

Copy link
@dtaniwaki

dtaniwaki Feb 11, 2019

Author

To consider non-force cases (TERM signal), I think ContainerExecKill should just send a signal to the running process and the caller should take care of the result just like the kill command of Linux.

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch 2 times, most recently from 8830e2b to 0e55615 Feb 11, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 11, 2019

Codecov Report

Merging #38704 into master will increase coverage by 0.03%.
The diff coverage is 64.7%.

@@            Coverage Diff             @@
##           master   #38704      +/-   ##
==========================================
+ Coverage   36.53%   36.57%   +0.03%     
==========================================
  Files         610      610              
  Lines       45395    45410      +15     
==========================================
+ Hits        16584    16607      +23     
+ Misses      26517    26506      -11     
- Partials     2294     2297       +3

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch from 0e55615 to 22bc8fe Feb 11, 2019

@dtaniwaki

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

I think the error of janky and experimental is not related to my change.

17:20:11 OOPS: 1472 passed, 51 skipped, 1 FAILED
17:20:11 --- FAIL: Test (5541.25s)
17:20:11 FAIL
17:20:11 ---> Making bundle: .integration-daemon-stop (in bundles/test-integration)
@dtaniwaki

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

I added a test for daemon so it doesn't decrease the coverage by codecov.

@cpuguy83
Copy link
Contributor

left a comment

This endpoint should really match the container kill endpoint (POST) and accept a kill signal.

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch from 824a642 to d48ff49 Feb 13, 2019

@dtaniwaki

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

Hmm... It's weird. The test passes in my local and the error in CI

09:44:53 # github.com/docker/docker/daemon [github.com/docker/docker/daemon.test]
09:44:53 daemon\exec_test.go:15:2: undefined: MockContainerdClient
09:44:54 FAIL	github.com/docker/docker/daemon [build failed]

should not happen.

@dtaniwaki

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

@cpuguy83 The error windows CI failed with is weird. The same composition exists in resize test of daemon. Could you tell me possible reason of this failure?

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@dtaniwaki MockContainerdClient is only compiled for Linux

// +build linux

defer ticker.Stop()

select {
case <-time.After(5 * time.Second):

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Feb 13, 2019

Member

how can this happen

This comment has been minimized.

Copy link
@dtaniwaki

dtaniwaki Feb 13, 2019

Author

The state of a process is checked periodically just below this line. I'd like to make sure if it's running.

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Feb 13, 2019

Member

IIUC case <-ticker.C: is always executed after 1sec? Then case <-time.After(5 * time.Second): won't be executed?

This comment has been minimized.

Copy link
@dtaniwaki

dtaniwaki Feb 13, 2019

Author

Yes, that's true. However, I put the safety guard just in case if it doesn't finish in a few seconds. I'll remove it and put a sleep of 1 second instead if it's better.

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Feb 13, 2019

Member

Please refer to

var (
waitCh = make(chan struct{})
resCh = make(chan struct {
content string
err error
})
)
go func() {
close(waitCh)
defer close(resCh)
r, err := ioutil.ReadAll(resp.Reader)
resCh <- struct {
content string
err error
}{
content: string(r),
err: err,
}
}()
<-waitCh
select {
case <-time.After(3 * time.Second):
t.Fatal("failed to read the content in time")
case got := <-resCh:
assert.NilError(t, got.err)
// NOTE: using Contains because no-tty's stream contains UX information
// like size, stream type.
assert.Assert(t, is.Contains(got.content, expected))
}

This comment has been minimized.

Copy link
@dtaniwaki

dtaniwaki Feb 14, 2019

Author

I think the code you mentioned is to wait for closed connection which can block until disconnection. The test I wrote uses detach option in ContainerExecCreate, and ContainerExecInspect may return not found error or a response with Running=false without blocking. Could you tell me how to wait for an active exec instance in more detail?

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch 2 times, most recently from ce4cb29 to 342d072 Feb 13, 2019

Implement exec kill API
Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:35703-exec-kill branch from 342d072 to c8e398b Feb 13, 2019

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Windows does not use containerd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.