Implement exec kill API by dtaniwaki · Pull Request #38704 · moby/moby

dtaniwaki · 2019-02-10T16:44:13Z

- 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)

GordonTheTurtle · 2019-02-10T16:44:15Z

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.

integration-cli/docker_cli_exec_test.go

api/swagger.yaml

AkihiroSuda

Design looks good. Thank you

dtaniwaki · 2019-02-11T04:02:21Z

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

integration/container/exec_test.go

daemon/exec.go

AkihiroSuda · 2019-02-11T04:20:27Z

integration/container/exec_test.go

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?

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.

codecov · 2019-02-11T06:03:13Z

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 · 2019-02-11T08:46:50Z

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 · 2019-02-13T00:31:08Z

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

cpuguy83

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

dtaniwaki · 2019-02-13T11:02:12Z

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 · 2019-02-13T12:39:18Z

@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 · 2019-02-13T12:52:52Z

@dtaniwaki MockContainerdClient is only compiled for Linux

moby/daemon/util_test.go

Line 1 in 0111ee7

// +build linux

docs/api/version-history.md

AkihiroSuda · 2019-02-13T12:55:01Z

integration/container/exec_test.go

how can this happen

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

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

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.

Please refer to

moby/integration/container/exec_test.go

Lines 51 to 83 in c8e398b

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))

}

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?

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

cpuguy83 · 2019-02-13T13:46:15Z

Windows does not use containerd.

awesomebytes · 2020-10-06T06:34:14Z

Will this PR be re-reviewed at some point? It would be great to have it. It would allow to make docker exec more of a first class citizen like docker run and either avoid or ease the workaround I needed to create to ensure that processes spawned by docker exec die when docker exec dies: #9098 (comment)

AkihiroSuda · 2020-10-06T07:02:19Z

needs rebase

awesomebytes · 2020-10-14T10:57:34Z

@AkihiroSuda I've given a shot to try to rebase it here: #41548

I pre-apologize if it just explodes in the CI, I've never used Go before, neither I knew how to build the project to test it.

AkihiroSuda · 2020-10-14T11:15:27Z

Thanks @awesomebytes

I've never used Go before, neither I knew how to build the project to test it.

Testing Moby is quite different from other Go programs; the test scripts can be locally executed by running make test-integration
https://github.com/moby/moby/blob/master/docs/contributing/test.md

awesomebytes · 2020-10-14T11:23:40Z

Thanks for the link @AkihiroSuda I'll try to fix things locally before further spamming your CI system.

dtaniwaki · 2021-07-20T05:10:20Z

I'm closing this PR as the implementation is taken over to #41548 thanks to @awesomebytes and @adamjseitz.

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Feb 10, 2019

dtaniwaki force-pushed the 35703-exec-kill branch from 5c280de to fdb822f Compare February 10, 2019 16:46

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 10, 2019

AkihiroSuda reviewed Feb 10, 2019

View reviewed changes

integration-cli/docker_cli_exec_test.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 10, 2019

View reviewed changes

api/swagger.yaml Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 10, 2019

View reviewed changes

AkihiroSuda added status/1-design-review and removed status/0-triage labels Feb 10, 2019

dtaniwaki force-pushed the 35703-exec-kill branch 2 times, most recently from 69439f2 to 880c309 Compare February 11, 2019 03:57

AkihiroSuda reviewed Feb 11, 2019

View reviewed changes

dtaniwaki force-pushed the 35703-exec-kill branch 2 times, most recently from 8830e2b to 0e55615 Compare February 11, 2019 06:03

dtaniwaki force-pushed the 35703-exec-kill branch from 0e55615 to 22bc8fe Compare February 11, 2019 06:13

yongtang added rebuild/janky labels Feb 11, 2019

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed rebuild/experimental labels Feb 11, 2019

dtaniwaki force-pushed the 35703-exec-kill branch from 3d0da4e to 824a642 Compare February 12, 2019 21:47

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 12, 2019

cpuguy83 requested changes Feb 13, 2019

View reviewed changes

dtaniwaki force-pushed the 35703-exec-kill branch from 824a642 to d48ff49 Compare February 13, 2019 09:38

AkihiroSuda reviewed Feb 13, 2019

View reviewed changes

docs/api/version-history.md Outdated Show resolved Hide resolved

AkihiroSuda reviewed Feb 13, 2019

View reviewed changes

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

Implement exec kill API

c8e398b

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

dtaniwaki force-pushed the 35703-exec-kill branch from 342d072 to c8e398b Compare February 13, 2019 13:24

GordonTheTurtle assigned tiborvass Feb 25, 2019

awesomebytes mentioned this pull request Oct 14, 2020

[Carry 38704] Implement exec kill API #41548

Open

dtaniwaki closed this Jul 20, 2021

dtaniwaki deleted the 35703-exec-kill branch July 20, 2021 05:10

	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))
	}

Conversation

dtaniwaki commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Feb 10, 2019

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

dtaniwaki commented Feb 11, 2019

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

dtaniwaki Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtaniwaki Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dtaniwaki commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaniwaki commented Feb 13, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

dtaniwaki commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaniwaki commented Feb 13, 2019

Uh oh!

AkihiroSuda commented Feb 13, 2019

Uh oh!

Uh oh!

AkihiroSuda Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

dtaniwaki Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

dtaniwaki Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

dtaniwaki Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Feb 13, 2019

Uh oh!

awesomebytes commented Oct 6, 2020

Uh oh!

AkihiroSuda commented Oct 6, 2020

Uh oh!

awesomebytes commented Oct 14, 2020

Uh oh!

AkihiroSuda commented Oct 14, 2020

Uh oh!

awesomebytes commented Oct 14, 2020

Uh oh!

dtaniwaki commented Jul 20, 2021

Uh oh!

Reviewers

dtaniwaki commented Feb 10, 2019 •

edited

Loading

dtaniwaki Feb 11, 2019 •

edited

Loading

dtaniwaki Feb 11, 2019 •

edited

Loading

codecov bot commented Feb 11, 2019 •

edited

Loading

dtaniwaki commented Feb 11, 2019 •

edited

Loading

dtaniwaki commented Feb 13, 2019 •

edited

Loading

dtaniwaki Feb 13, 2019 •

edited

Loading