Add prestart/poststop hooks to runc #160

mrunalp · 2015-07-28T22:34:57Z

The spec.go commit will be taken out once merged upstream.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

mrunalp · 2015-07-28T22:38:08Z

ashahab-altiscale · 2015-08-03T17:26:20Z

@mrunalp Not sure if this is a related question, but can I get the pid of the container when I'm running a pre-start hook? Also, can I be running in the host's namespace?

LK4D4 · 2015-08-03T17:27:07Z

@ashahab-altiscale Yes for both I think. Cause it's like whole idea :D

marcosnils · 2015-08-03T17:56:17Z

@LK4D4 @mrunalp sorry for the ignorance, but isn't this the same as doing some command ; runc config.json; some other command in a *nix system?.

I know that if it's included in runc it will be agnostic to the platform and becomes part of the spec but what are the advantages/use cases of doing it natively rather than from the OS?

OT: Is there a place where runc features / roadmap is available to the community?. I'd be great of some users can actually take a look into it and maybe leave some comments which will make the product better.

Thanks!.

marcosnils · 2015-08-03T18:02:39Z

BTW: This doesn't have any tests?

mrunalp · 2015-08-03T18:02:55Z

@marcosnils Actually, there is a difference and a reason why we introduce the prestart hook :)
The prestart hook is run after the container process is up (cloned into new namespaces) but before the user command (for e.g. bash) is executed. That allows someone to do something useful in the namespaces of the container. See opencontainers/runtime-spec#34 for more details.

marcosnils · 2015-08-03T18:07:59Z

@mrunalp got it. My impression was correct, I though that the hooks were executed inside the container namespace. I still have to look into runc code a bit more to understand it's components.

Thanks!.

mrunalp · 2015-08-04T17:15:09Z

Rebased.

crosbymichael · 2015-08-04T18:19:18Z

libcontainer/process_linux.go

+	// Call the poststop hooks
+	statePath := p.container.stateFilePath()
+	for _, poststopcmd := range p.poststop {
+		// TODO: Log the errors


return errors?

mrunalp · 2015-08-05T22:42:20Z

Added test.

mrunalp · 2015-08-06T19:28:36Z

@crosbymichael @LK4D4 PTAL

dqminh · 2015-08-07T00:10:28Z

LGTM

estesp · 2015-08-12T18:01:53Z

+1 Would love to see this get in soon so we can continue re-instatement of user namespace support merging into Docker.

mrunalp · 2015-08-18T22:01:17Z

Rebased.

ashahab-altiscale · 2015-08-20T15:54:37Z

+1 Lets get this merged!

On Tue, Aug 18, 2015 at 3:01 PM, Mrunal Patel notifications@github.com
wrote:

Rebased.

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

dqminh · 2015-08-20T15:57:15Z

Still LGTM

ping @LK4D4 @crosbymichael

wking · 2015-08-20T17:25:52Z

libcontainer/process_linux.go

@@ -271,6 +294,27 @@ func (p *initProcess) createNetworkInterfaces() error {
 	return nil
 }

+func runCmd(command configs.Command, statePath string) error {
+	cmd := exec.Command(command.Path, command.Args[:]...)


exec.Command doesn't let you set argv[0] independently of command.Path, which seemed like something desired by the spec (but the spec isn't clear on how it should be handled).

@mrunalp is there a reason to use exec.Command instead of populating exec.Cmd directly with the args ?
As @wking indicated exec.Command forces the argv[0] to be the path : https://golang.org/src/os/exec/exec.go?s=3999:4044#L112 , and hence command.Args can only occupy argv[1]...

IMO, the following is better

- cmd := exec.Command(command.Path, command.Args[:]...) + cmd := exec.Cmd{ + Path: command.Path, + Args: command.Args[:], + }

wking · 2015-08-20T17:42:12Z

On Mon, Aug 03, 2015 at 11:02:57AM -0700, Mrunal Patel wrote:

The prestart hook is run after the container process is up (cloned
into new namespaces) but before the user command (for e.g. bash) is
executed. That allows someone to do something useful in the
namespaces of the container.

If so, I think it's probably worth clarifing that in the spec. It
seems like you could handle that by adjusting process.args in your
config to call a script that handled in-container setup, execution,
and teardown. I though the pre-start hooks would be run from the host
environment after the container was setup but before the container's
process.args was launched, and then the post-stop hooks would be run
from the host's environment after the process.args process was
stopped, but before any explicit environment teardown commenced.

For example, the host-environment semantics would let you manipulate
the container itself, which you may not have permission to do from
within the container.

I'm not even sure how in-container hooks would work, since the hook's
path is spec'ed to refer to the host filesystem 1, but inside the
container we'd be using the container's filesystem, no?

LK4D4 · 2015-08-25T21:24:19Z

ping @Madhu @mrjana for comments

mrunalp · 2015-08-28T18:16:45Z

@LK4D4 @crosbymichael ping
@mavenugo did confirm on IRC that this works for him with the changes he suggested.

crosbymichael · 2015-08-28T18:19:47Z

@mrunalp do we want to wait on the state PR in specs so that we have something to provide the hook?

mrunalp · 2015-08-28T18:22:33Z

@crosbymichael works for me. (We do pass state in this PR but it isn't standardized).

estesp · 2015-09-07T21:26:33Z

Now that the state PR is completed, do you have time to update this @mrunalp?

wking · 2015-09-07T21:54:19Z

On Mon, Sep 07, 2015 at 02:26:36PM -0700, Phil Estes wrote:

Now that the state PR is completed, do you have time to update this
@mrunalp?

Things that still have to happen:

a. rebase this to pull in the state.json specified by
opencontainers/runtime-spec#87
b. decide on args[0] handling in the spec (opencontainers/runtime-spec#118)
c. possibly rebase this to pull in an alternative args[0] approach
(e.g. Python's optional-path approach from 1)
d. final review and merge of this PR

(b) is external to this PR, so I don't see any need to rush forward
with (a). But (a) and (b/c) are very orthogonal issues, so if we want
to handle (a) before (b/c) that's probably an efficient use of
development time too.

wking · 2015-09-07T22:01:28Z

On Thu, Aug 20, 2015 at 10:40:11AM -0700, W. Trevor King wrote:

Mon, Aug 03, 2015 at 11:02:57AM -0700, Mrunal Patel:

The prestart hook is run after the container process is up (cloned
into new namespaces) but before the user command (for e.g. bash)
is executed. That allows someone to do something useful in the
namespaces of the container.

If so, I think it's probably worth clarifing that in the spec… I
though the pre-start hooks would be run from the host environment
after the container was setup but before the container's
process.args was launched, and then the post-stop hooks would be run
from the host's environment after the process.args process was
stopped, but before any explicit environment teardown commenced.

I still haven't looked over the backing code here, but while testing
hook-based oom_score_adj handling for opencontainers/runtime-spec#115 1, it
seems pretty clear that at least the pre-start hooks are executing in
the host environment (I could write to the host's
/proc/${PID}/oom_score_adj, which is /proc/1/oom_score_adj inside my
PID-namespaced container). Is that enough consensus (me agreeing with
what runC does) to open an opencontainers/specs PR, or do we want
further discussion about the hook execution environment (here or on
the mailing list) before I open a spec PR?

mrunalp · 2015-09-09T21:34:32Z

@estesp I am waiting on #242 to go in.

cyphar · 2015-09-15T17:49:07Z

#242 is in. Let's get this merged. 👍

mrunalp · 2015-09-15T22:25:56Z

Updated the PR to integrate with the libcontainer support.

mrunalp · 2015-09-16T21:12:07Z

@LK4D4 @crosbymichael @dqminh PTAL.
Also, do we want to replace HookState by State from spec repository or make it json compatible?

dqminh · 2015-09-16T21:29:31Z

LGTM

Also, do we want to replace HookState by State from spec repository or make it json compatible?

Also +1 :-)

mrunalp · 2015-09-23T18:06:15Z

Updated to use State from the spec. @crosbymichael @LK4D4 @dqminh PTAL.

runcom · 2015-09-23T18:13:07Z

can libcontainer continue to be specs free? even if go get'ing it still requires to pull specs for runc itself I like the configs layer in between and libcontainer users may benefit from having something before specs gets updated, no big deal tho
maybe plans are to integrate specs objs in libcontainer

crosbymichael · 2015-09-23T23:14:56Z

Why not keep it json compatible then we don't have to mix the two?

cyphar · 2015-09-23T23:18:59Z

IMO libcontainer should not use the spec, since it's technically an implementation detail of runc and thus independent of the opencontainers spec.

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp · 2015-09-24T00:14:39Z

@crosbymichael Updated.

crosbymichael · 2015-09-24T21:15:32Z

LGTM

LK4D4 · 2015-09-24T21:52:27Z

LGTM

Add prestart/poststop hooks to runc

Modify the capabilities constants to match header files like other constants

GordonTheTurtle added the status/0-triage label Jul 28, 2015

mrunalp force-pushed the feature/hooks branch from d5ca4e1 to c533970 Compare August 4, 2015 16:55

crosbymichael reviewed Aug 4, 2015
View reviewed changes

mrunalp force-pushed the feature/hooks branch from c533970 to d1e3e5e Compare August 4, 2015 18:53

mrunalp mentioned this pull request Aug 5, 2015

User namespaces - Phase 1 moby/moby#15187

Closed

4 tasks

mrunalp force-pushed the feature/hooks branch 2 times, most recently from f32e01e to e3553a5 Compare August 5, 2015 22:35

mrunalp mentioned this pull request Aug 15, 2015

Creating a network bridge? #201

Closed

mrunalp force-pushed the feature/hooks branch from e3553a5 to fea8032 Compare August 18, 2015 21:53

mrunalp mentioned this pull request Aug 19, 2015

Simple Cleanups #212

Merged

wking reviewed Aug 20, 2015
View reviewed changes

mrunalp force-pushed the feature/hooks branch from fea8032 to 590c7ad Compare August 24, 2015 21:16

wking mentioned this pull request Aug 29, 2015

Adding oom_score_adj as a container config param #232

Merged

crosbymichael mentioned this pull request Sep 11, 2015

Implement hooks in libcontainer code base #261

Merged

mrunalp force-pushed the feature/hooks branch from 4f7ff04 to f29794b Compare September 15, 2015 22:25

wking mentioned this pull request Sep 18, 2015

Branching scheme for versioned releases #277

Closed

mrunalp force-pushed the feature/hooks branch from f29794b to f7750e5 Compare September 23, 2015 18:05

mrunalp added 2 commits September 23, 2015 16:29

hooks: Integrate spec hooks with libcontainer

9964fcd

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

Add version to HookState to make it json-compatible with spec State

dcafe48

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp force-pushed the feature/hooks branch from f7750e5 to dcafe48 Compare September 24, 2015 00:14

LK4D4 added a commit that referenced this pull request Sep 24, 2015

Merge pull request #160 from mrunalp/feature/hooks

aac9179

Add prestart/poststop hooks to runc

LK4D4 merged commit aac9179 into opencontainers:master Sep 24, 2015

hqhq mentioned this pull request Nov 26, 2015

Clarify the meaning of hook elements opencontainers/runtime-spec#255

Merged

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#160 from mrunalp/cap_fix

9a8748c

Modify the capabilities constants to match header files like other constants

Add prestart/poststop hooks to runc #160

Add prestart/poststop hooks to runc #160

Conversation

mrunalp commented Jul 28, 2015

mrunalp commented Jul 28, 2015

ashahab-altiscale commented Aug 3, 2015

LK4D4 commented Aug 3, 2015

marcosnils commented Aug 3, 2015

marcosnils commented Aug 3, 2015

mrunalp commented Aug 3, 2015

marcosnils commented Aug 3, 2015

mrunalp commented Aug 4, 2015

crosbymichael Aug 4, 2015

Choose a reason for hiding this comment

mrunalp Aug 4, 2015

Choose a reason for hiding this comment

mrunalp commented Aug 5, 2015

mrunalp commented Aug 6, 2015

dqminh commented Aug 7, 2015

estesp commented Aug 12, 2015

mrunalp commented Aug 18, 2015

ashahab-altiscale commented Aug 20, 2015

dqminh commented Aug 20, 2015

wking Aug 20, 2015

Choose a reason for hiding this comment

mavenugo Aug 26, 2015

Choose a reason for hiding this comment

mrunalp Aug 26, 2015

Choose a reason for hiding this comment

wking commented Aug 20, 2015

LK4D4 commented Aug 25, 2015

mrunalp commented Aug 28, 2015

crosbymichael commented Aug 28, 2015

mrunalp commented Aug 28, 2015

estesp commented Sep 7, 2015

wking commented Sep 7, 2015

wking commented Sep 7, 2015

mrunalp commented Sep 9, 2015

cyphar commented Sep 15, 2015

mrunalp commented Sep 15, 2015

mrunalp commented Sep 16, 2015

dqminh commented Sep 16, 2015

mrunalp commented Sep 23, 2015

runcom commented Sep 23, 2015

crosbymichael commented Sep 23, 2015

cyphar commented Sep 23, 2015

mrunalp commented Sep 24, 2015

crosbymichael commented Sep 24, 2015

LK4D4 commented Sep 24, 2015