Skip to content

execution: add exec for additional processes#696

Merged
estesp merged 2 commits intocontainerd:masterfrom
crosbymichael:exec
Apr 6, 2017
Merged

execution: add exec for additional processes#696
estesp merged 2 commits intocontainerd:masterfrom
crosbymichael:exec

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Apr 5, 2017 •
edited
Loading

This adds support the the execution service to exec additional processes inside and existing container.

It also adds the ctr exec command that can be used to add processes.

michael > sudo ctr exec --id test ps 
PID   USER     TIME   COMMAND
    1 redis      0:00 redis-server
   29 root       0:00 ps
michael > sudo ctr exec --id test -t sh
/ # ls
bin    dev    home   media  proc   run    srv    tmp    var
data   etc    lib    mnt    root   sbin   sys    usr
/ # exit
michael > sudo ctr exec --id test -t sh
/ # exit 2
michael > echo $?
2

@crosbymichael
Add exec APIs
7715ddc 
Signed-off-by: Michael Crosby <[email protected]>
@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 5, 2017 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #696   +/-   ##
=======================================
  Coverage   62.73%   62.73%           
=======================================
  Files           5        5           
  Lines         687      687           
=======================================
  Hits          431      431           
  Misses        160      160           
  Partials       96       96

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 0a484cc...47225c1. Read the comment docs.

@hqhq
Copy link
Copy Markdown
Contributor

hqhq commented Apr 6, 2017

LGTM

Do we prefer to use Signal than Kill? If so I can open a PR to change the old Kill APIs to Signal to keep consistency.

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Apr 6, 2017

@hqhq I just added Signal to the lower level Go API. Kill can still be in our external API. I needed two different ones because runc does not support signaling an exec process, only the main container process.

estesp
estesp approved these changes Apr 6, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM with one minor typo nit

Comment thread linux/shim/process.go Outdated
Copy link
Copy Markdown
Member

@estesp estesp Apr 6, 2017

Choose a reason for hiding this comment

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

minor typo: "Delete deletes the process.."

@crosbymichael
Add ctr exec
47225c1 
Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Apr 6, 2017

@estesp updated

@estesp
Copy link
Copy Markdown
Member

estesp commented Apr 6, 2017

Thanks. LGTM.

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Apr 6, 2017

@estesp you can merge if you want ;) :)

@estesp
Copy link
Copy Markdown
Member

estesp commented Apr 6, 2017

knee-jerk reaction from years in docker/docker looking for 2 LGTMs :) :)

@estesp estesp merged commit e5c8c56 into containerd:master Apr 6, 2017
@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Apr 6, 2017

there is a second one above yours

@crosbymichael crosbymichael deleted the exec branch April 6, 2017 17:54
@estesp
Copy link
Copy Markdown
Member

estesp commented Apr 6, 2017

ha, of course.. saw it earlier, but totally forgot

@mlaventure mlaventure mentioned this pull request Apr 6, 2017
Merged
@hqhq
Copy link
Copy Markdown
Contributor

hqhq commented Apr 7, 2017

@crosbymichael Maybe we should just merge opencontainers/runc#808 , I'll rebase that PR later today.

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

Reviewers

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@crosbymichael @codecov-io @hqhq @estesp