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

Proposal: user should be able to stop/kill an exec command #9167

Closed
wants to merge 1 commit into from

Conversation

dqminh
Copy link
Contributor

@dqminh dqminh commented Nov 14, 2014

Fix #9098

This proposes additional extenstions to the remote API for managing exec commands.

  • POST /exec/id/stop similar to POST /containers/id/stop
    this will stop or kill the exec command after timeout
  • POST /exec/id/kill similar to POST /containers/id/kill
    this will kill the exec command (optionally with a custom signal)

cc @vishh
also @vieux @jfrazelle for the API change

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh dqminh89@gmail.com (github: dqminh)

this proposes additional extenstions to the remote API for managing exec
commands.
- POST /exec/id/stop similar to POST /containers/id/stop
  this will stop or kill the exec command after timeout
- POST /exec/id/kill similar to POST /containers/id/kill
  this will kill the exec command (optionally with a custom signal)

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)
@vishh
Copy link
Contributor

vishh commented Nov 14, 2014

👍 for these APIs. @dqminh do you plan to work on implementing them as well?

@dqminh
Copy link
Contributor Author

dqminh commented Nov 17, 2014

@vishh yes, i have some patches that implementing them. It still need more work though. Can I ping you for comments sometime soon 😄 ?

@vishh
Copy link
Contributor

vishh commented Nov 17, 2014

Yeah sure.

On Sun, Nov 16, 2014 at 9:20 PM, Daniel, Dao Quang Minh <
notifications@github.com> wrote:

@vishh https://github.com/vishh yes, i have some patches that
implementing them. It still need more work though. Can I ping you for
comments sometime soon [image: 😄] ?


Reply to this email directly or view it on GitHub
#9167 (comment).

dqminh added a commit to dqminh/moby that referenced this pull request Nov 19, 2014
This adds support for `POST /exec/(name)/stop` and `POST /exec/(name)kill`
following the proposal at moby#9167

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)
dqminh added a commit to dqminh/moby that referenced this pull request Nov 24, 2014
This adds support for `POST /exec/(name)/stop` and `POST /exec/(name)kill`
following the proposal at moby#9167

Docker-DCO-1.1-Signed-off-by: Daniel, Dao Quang Minh <dqminh89@gmail.com> (github: dqminh)
@icecrime
Copy link
Contributor

I think @ewindisch's comment on a related issue is very relevant here.

@SvenDowideit
Copy link
Contributor

assuming this will go into Docker 1.5, the documentation change needs to go into the new v1.17 API doc.

@ghost
Copy link

ghost commented Mar 5, 2015

Need this badly .. +1

@icecrime
Copy link
Contributor

This PR has been stalled for quite some time. Personally I'm -1 on design... Anyone else?

@dqminh
Copy link
Contributor Author

dqminh commented Mar 17, 2015

@icecrime Sorry about this. I will need to rebase some of the old code to new libcontainer API.

What do you not like about the design ?

@icecrime
Copy link
Contributor

It's the very general thing of making exec first class citizen as containers are, but that's just personal taste I guess.

@icecrime
Copy link
Contributor

See #9402 for an example of the endless debates that this kind of things bring :-)

@dqminh
Copy link
Contributor Author

dqminh commented Mar 17, 2015

It's the very general thing of making exec first class citizen as containers are

Ah, i remember it now ( #9402 (comment) ). Yes i agree that having exec first class citizen solves many problems ( this included ), but that itself is also a very hard problem :-)

That being said, to be able to stop/kill a (rogue/dangling) execin processes in the current API is very helpful. I tried to not introduce the CLI dependencies here, only the API (because we only consume the API), hopefully that it will be easier to be merged/deprecated after if necessary.

@duglin
Copy link
Contributor

duglin commented Mar 17, 2015

@icecrime don't be so hard on exec, it just needs some love too :-)

@icecrime
Copy link
Contributor

Collective review with @LK4D4 @calavera @jfrazelle @crosbymichael @tiborvass

Sorry, we're not currently comfortable with expanding the capabilities of exec, and would like as much to keep it as a debugging utility. Regarding the interactions with signals and the top issue, there might be some bugs there with signal forwarding, but we don't feel this is the proper solution.

@icecrime icecrime closed this Apr 15, 2015
@robert-blankenship
Copy link

:( This should at least be better documented. Burnt a lot of cycles with the idea in my head that this would work

@c1tru55
Copy link

c1tru55 commented Nov 23, 2017

👍 definitely a bug, strange that it's still not fixed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kill docker exec command will not terminate the spawned process
10 participants