Add support for 'docker exec' - phase 1 by vishh · Pull Request #7409 · moby/moby

vishh · 2014-08-05T00:09:28Z

This patch brings in support for running a new command in an existing and active container.
Fixes #6777 and #1228.

Some use cases:

Perform administrative tasks - add/remove devices, update files, etc
Debugging a running container - gdb, ssh
Run periodic commands
Run commands that are not part of the static container specification.

API
Client: 'docker exec redis touch /tmp/file'
Remote: '/containers/redis/exec'

vishh · 2014-08-05T00:09:56Z

Note: This patch depends on docker-archive/libcontainer#141.
The changes to libcontainer in this patch will be reverted once 141 has been merged.

vishh · 2014-08-05T00:10:10Z

Ping @crosbymichael @tianon

cyphar · 2014-08-05T04:43:13Z

+1. An invaluable feature which means that you no longer need to manually use nsinit exec.

mrunalp · 2014-08-05T18:20:35Z

strncmp? (Just to avoid all known vulnerable functions)

Well, strcmp isn't vulnerable unless you're doing cryptographic comparisons (where you need to use strictly O(n) comparisons for any inputs). There's no danger of buffer overflow (since there's no writes occuring) or any real out-of-bounds errors (since IIRC strcmp compares lengths to give you a positive or negative return value).

SvenDowideit · 2014-08-06T00:03:11Z

This UX change desperatly needs documentation, both in cli.md and in the man pages. plus an example of when and how you'd use it.

vishh · 2014-08-08T17:05:16Z

@SvenDowideit I will add/update the docs once I get a go ahead from the maintainers for this patch.

cyphar · 2014-09-07T05:31:01Z

Bumping this. Is there any work happening on this PR?

vishh · 2014-09-08T16:18:02Z

I am working on rebasing this to current head. I will update this PR once
thats done.

On Sat, Sep 6, 2014 at 10:31 PM, Aleksa Sarai [email protected]
wrote:

Bumping this. Is there any work happening on this PR?

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

vishh · 2014-09-10T07:24:03Z

I have rebased this PR against the current docker head and libcontainer head.

PTAL @crosbymichael @vieux

LK4D4 · 2014-09-10T07:31:08Z

@vishh This patch need some integration-cli tests.

vishh · 2014-09-10T07:41:24Z

@LK4D4: I will add a few tests soon. Meanwhile, it will be helpful to know if the maintainers are committed to merging this feature anytime soon.

LK4D4 · 2014-09-10T07:54:22Z

can body be data? I personally always think about body as about []byte.

Done. But it is the body of the request.

LK4D4 · 2014-09-10T15:32:48Z

Phew, I've read all code. This will be a "killer feature".

vishh · 2014-09-11T16:23:22Z

@crosbymichael: there should already be a TODO to add resizing support. I
will take a look at the closed connection errors now.

On Wed, Sep 10, 2014, 11:03 PM Sven Dowideit [email protected]
wrote:

Docs LGTM - @jamtur01 https://github.com/jamtur01 @fredlf
https://github.com/fredlf @ostezer https://github.com/ostezer

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

LK4D4 · 2014-09-11T18:04:10Z

@vishh Seems like closed connection thing is present in master as well: #3631

vishh · 2014-09-11T21:00:27Z

Thanks for the pointer @LK4D4. @crosbymichael: If the two issues you raised are not showstoppers for this feature, can we proceed with merging this feature, while I work on resolving those issues? The daemon connection error is not user visible.

crosbymichael · 2014-09-11T21:04:55Z

I think the resize issue maybe a big one

fredlf · 2014-09-12T17:43:41Z

instantiate

Also, pardon my ignorance, but the "select" what?

@fredlf line 2391 in this file...

fredlf · 2014-09-12T17:50:03Z

Docs LGTM once my nit-picky comments are addressed. Thanks for your contribution!

LK4D4 · 2014-09-12T18:54:07Z

Need rebase :/ So big patch...

vishh · 2014-09-12T19:50:41Z

As per discussions on 'docker-dev' IRC, I will be stashing the CLI and docs
until the daemon can support resizing of tty for 'exec'.
Thanks for the review everyone.

On Fri, Sep 12, 2014 at 11:54 AM, Alexandr Morozov <[email protected]

wrote:

Need rebase :/ So big patch...

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

Modified Attach() method to support docker exec. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

…' feature. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

vishh · 2014-09-15T17:09:03Z

I have removed the 'exec' feature from the CLI.
Ping @crosbymichael , @tianon

ghost · 2014-09-16T09:46:06Z

@vishh I strongly believe the docs here need the following amendments:

docker should be Docker in most cases

stdin, stdout, etc., should be all capitalized (e.g., .. exit on stdin close >> .. on STDIN close)

Please clarify or point out what "exec" is (used at various places)

Please clarify what "command" refers to (e.g., Error running command.. Does this mean any command? The command passed? A command Docker was just asked to execute?)

lxc > LXC as well.

@ostezer:
The docs have been moved to #8062 for technical reasons.

It is already 'Docker'. By 'docs' are you referring to the source code or the changes to files inside docs/ directory?

Done.

I followed the docs that exist for 'docker run'. Can you point me at specific places in the docs where the usage of 'exec' is not clear? I will be happy to address them.

Can you point me at the line in the source where the usage of 'command' is not clear?

Again point me at the source please.

… resizing of tty sessions for exec'ed commands. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

vishh · 2014-09-16T19:58:00Z

Closed in favor of #8062

YungSang mentioned this pull request Aug 5, 2014

docker exec YungSang/docker-attach#2

Open

mrunalp reviewed Aug 5, 2014
View reviewed changes

vmarmol mentioned this pull request Aug 7, 2014

Install nsinit that is vendored into docker. kubernetes/kubernetes#820

Merged

w8ai mentioned this pull request Aug 22, 2014

update Godep commit hashes for docker 1.2.0; removed nsinit from service... control-center/serviced#908

Merged

vishh mentioned this pull request Aug 26, 2014

Refactoring of daemon/* code to support exec and related features #7750

Closed

w8ai mentioned this pull request Aug 29, 2014

fallback to docker exec for attach when nsenter is not available control-center/serviced#956

Merged

vishh force-pushed the run_in_phase1 branch from 740e6ed to 57cd0b5 Compare September 10, 2014 07:22

LK4D4 reviewed Sep 10, 2014
View reviewed changes

kolypto mentioned this pull request Sep 11, 2014

running a different command on an existing container #1228

Closed

fredlf reviewed Sep 12, 2014
View reviewed changes

vishh added 8 commits September 15, 2014 16:57

Adding Exec method to native execdriver.

f3c767d

Modified Attach() method to support docker exec. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Adding support for docker exec in daemon.

5130fe5

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Adding 'exec' command to remote API and CLI.

985d579

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Import nsenter in docker.

e1cf95b

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Fix bug in attach handling for docker exec. Add docs for 'docker exec…

d130c10

…' feature. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Update cli.md to include 'docker exec' feature.

e1818d2

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Adding integration tests for docker exec feature.

d980589

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

Address review comments.

669561c

Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

vishh force-pushed the run_in_phase1 branch from dcdf093 to 0029180 Compare September 15, 2014 17:07

duglin mentioned this pull request Sep 15, 2014

Add Run in support in docker #6777

Closed

vishh changed the title ~~Add support for 'docker exec'~~ Add support for 'docker exec' - phase 1 Sep 16, 2014

vishh mentioned this pull request Sep 16, 2014

Add support for 'docker exec' - phase 2 #8062

Merged

ghost reviewed Sep 16, 2014
View reviewed changes

Removing 'exec' feature from the CLI until the docker daemon supports…

57fa003

… resizing of tty sessions for exec'ed commands. Docker-DCO-1.1-Signed-off-by: Vishnu Kannan <[email protected]> (github: vishh)

vishh force-pushed the run_in_phase1 branch from 0029180 to 57fa003 Compare September 16, 2014 17:32

LK4D4 closed this in #8062 Sep 16, 2014

Conversation

vishh commented Aug 5, 2014

Uh oh!

vishh commented Aug 5, 2014

Uh oh!

vishh commented Aug 5, 2014

Uh oh!

cyphar commented Aug 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Aug 6, 2014

Uh oh!

vishh commented Aug 8, 2014

Uh oh!

cyphar commented Sep 7, 2014

Uh oh!

vishh commented Sep 8, 2014

Uh oh!

vishh commented Sep 10, 2014

Uh oh!

LK4D4 commented Sep 10, 2014

Uh oh!

vishh commented Sep 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Sep 10, 2014

Uh oh!

vishh commented Sep 11, 2014

Uh oh!

LK4D4 commented Sep 11, 2014

Uh oh!

vishh commented Sep 11, 2014

Uh oh!

crosbymichael commented Sep 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredlf commented Sep 12, 2014

Uh oh!

LK4D4 commented Sep 12, 2014

Uh oh!

vishh commented Sep 12, 2014

Uh oh!

vishh commented Sep 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishh commented Sep 16, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants