From c98cb8c2020c11ce334f2659bb346ffef318fffe Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sun, 25 Feb 2018 14:47:41 -0800 Subject: [PATCH] libcontainer: Set 'status' in hook stdin Finish off the work started in a344b2d6 (sync up `HookState` with OCI spec `State`, 2016-12-19, #1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f276498 (Move pre-start hooks after container mounts, 2016-02-17, #568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: https://github.com/opencontainers/runc/issues/1710 Signed-off-by: W. Trevor King Signed-off-by: Dongsu Park --- libcontainer/configs/config.go | 13 +++--- libcontainer/configs/config_test.go | 14 ++++--- libcontainer/container.go | 7 ++++ libcontainer/container_linux.go | 58 ++++++++++++++++++--------- libcontainer/factory_linux_test.go | 3 +- libcontainer/integration/exec_test.go | 7 ++-- libcontainer/process_linux.go | 30 +++++++------- libcontainer/state_linux.go | 12 ++---- 8 files changed, 85 insertions(+), 59 deletions(-) diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index b065f7f28ec..7728522fef6 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -272,26 +272,23 @@ func (hooks Hooks) MarshalJSON() ([]byte, error) { }) } -// HookState is the payload provided to a hook on execution. -type HookState specs.State - type Hook interface { // Run executes the hook with the provided state. - Run(HookState) error + Run(*specs.State) error } // NewFunctionHook will call the provided function when the hook is run. -func NewFunctionHook(f func(HookState) error) FuncHook { +func NewFunctionHook(f func(*specs.State) error) FuncHook { return FuncHook{ run: f, } } type FuncHook struct { - run func(HookState) error + run func(*specs.State) error } -func (f FuncHook) Run(s HookState) error { +func (f FuncHook) Run(s *specs.State) error { return f.run(s) } @@ -314,7 +311,7 @@ type CommandHook struct { Command } -func (c Command) Run(s HookState) error { +func (c Command) Run(s *specs.State) error { b, err := json.Marshal(s) if err != nil { return err diff --git a/libcontainer/configs/config_test.go b/libcontainer/configs/config_test.go index 74683eaff78..c89a764de1a 100644 --- a/libcontainer/configs/config_test.go +++ b/libcontainer/configs/config_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runtime-spec/specs-go" ) func TestUnmarshalHooks(t *testing.T) { @@ -101,7 +102,7 @@ func TestMarshalUnmarshalHooks(t *testing.T) { } func TestMarshalHooksWithUnexpectedType(t *testing.T) { - fHook := configs.NewFunctionHook(func(configs.HookState) error { + fHook := configs.NewFunctionHook(func(*specs.State) error { return nil }) hook := configs.Hooks{ @@ -119,14 +120,15 @@ func TestMarshalHooksWithUnexpectedType(t *testing.T) { } func TestFuncHookRun(t *testing.T) { - state := configs.HookState{ + state := &specs.State{ Version: "1", ID: "1", + Status: "created", Pid: 1, Bundle: "/bundle", } - fHook := configs.NewFunctionHook(func(s configs.HookState) error { + fHook := configs.NewFunctionHook(func(s *specs.State) error { if !reflect.DeepEqual(state, s) { t.Errorf("Expected state %+v to equal %+v", state, s) } @@ -137,9 +139,10 @@ func TestFuncHookRun(t *testing.T) { } func TestCommandHookRun(t *testing.T) { - state := configs.HookState{ + state := &specs.State{ Version: "1", ID: "1", + Status: "created", Pid: 1, Bundle: "/bundle", } @@ -160,9 +163,10 @@ func TestCommandHookRun(t *testing.T) { } func TestCommandHookRunTimeout(t *testing.T) { - state := configs.HookState{ + state := &specs.State{ Version: "1", ID: "1", + Status: "created", Pid: 1, Bundle: "/bundle", } diff --git a/libcontainer/container.go b/libcontainer/container.go index 2e31b4d4fce..ba7541c5fd6 100644 --- a/libcontainer/container.go +++ b/libcontainer/container.go @@ -9,6 +9,7 @@ import ( "time" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runtime-spec/specs-go" ) // Status is the status of a container. @@ -85,6 +86,12 @@ type BaseContainer interface { // SystemError - System error. State() (*State, error) + // OCIState returns the current container's state information. + // + // errors: + // SystemError - System error. + OCIState() (*specs.State, error) + // Returns the current config of the container. Config() configs.Config diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 7d2c684484b..018aad13ec9 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -25,6 +25,7 @@ import ( "github.com/opencontainers/runc/libcontainer/intelrdt" "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/runc/libcontainer/utils" + "github.com/opencontainers/runtime-spec/specs-go" "github.com/golang/protobuf/proto" "github.com/sirupsen/logrus" @@ -156,6 +157,12 @@ func (c *linuxContainer) State() (*State, error) { return c.currentState() } +func (c *linuxContainer) OCIState() (*specs.State, error) { + c.m.Lock() + defer c.m.Unlock() + return c.currentOCIState() +} + func (c *linuxContainer) Processes() ([]int, error) { pids, err := c.cgroupManager.GetAllPids() if err != nil { @@ -348,14 +355,10 @@ func (c *linuxContainer) start(process *Process) error { } c.initProcessStartTime = state.InitProcessStartTime - if c.config.Hooks != nil { - bundle, annotations := utils.Annotations(c.config.Labels) - s := configs.HookState{ - Version: c.config.Version, - ID: c.id, - Pid: parent.pid(), - Bundle: bundle, - Annotations: annotations, + if c.config.Hooks != nil && len(c.config.Hooks.Poststart) > 0 { + s, err := c.currentOCIState() + if err != nil { + return err } for i, hook := range c.config.Hooks.Poststart { if err := hook.Run(s); err != nil { @@ -493,7 +496,7 @@ func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, parentPipe, c if err != nil { return nil, err } - return &initProcess{ + init := &initProcess{ cmd: cmd, childPipe: childPipe, parentPipe: parentPipe, @@ -504,7 +507,9 @@ func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, parentPipe, c process: p, bootstrapData: data, sharePidns: sharePidns, - }, nil + } + c.initProcess = init + return init, nil } func (c *linuxContainer) newSetnsProcess(p *Process, cmd *exec.Cmd, parentPipe, childPipe *os.File) (*setnsProcess, error) { @@ -1536,14 +1541,10 @@ func (c *linuxContainer) criuNotifications(resp *criurpc.CriuResp, process *Proc return err } case notify.GetScript() == "setup-namespaces": - if c.config.Hooks != nil { - bundle, annotations := utils.Annotations(c.config.Labels) - s := configs.HookState{ - Version: c.config.Version, - ID: c.id, - Pid: int(notify.GetPid()), - Bundle: bundle, - Annotations: annotations, + if c.config.Hooks != nil && len(c.config.Hooks.Prestart) > 0 { + s, err := c.currentOCIState() + if err != nil { + return nil } for i, hook := range c.config.Hooks.Prestart { if err := hook.Run(s); err != nil { @@ -1738,6 +1739,27 @@ func (c *linuxContainer) currentState() (*State, error) { return state, nil } +func (c *linuxContainer) currentOCIState() (*specs.State, error) { + bundle, annotations := utils.Annotations(c.config.Labels) + state := &specs.State{ + Version: specs.Version, + ID: c.ID(), + Bundle: bundle, + Annotations: annotations, + } + status, err := c.currentStatus() + if err != nil { + return nil, err + } + state.Status = status.String() + if status != Stopped { + if c.initProcess != nil { + state.Pid = c.initProcess.pid() + } + } + return state, nil +} + // orderNamespacePaths sorts namespace paths into a list of paths that we // can setns in order. func (c *linuxContainer) orderNamespacePaths(namespaces map[configs.NamespaceType]string) ([]string, error) { diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go index 92188001c81..8d0ca8a4743 100644 --- a/libcontainer/factory_linux_test.go +++ b/libcontainer/factory_linux_test.go @@ -12,6 +12,7 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/mount" "github.com/opencontainers/runc/libcontainer/utils" + "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/sys/unix" ) @@ -229,6 +230,6 @@ func marshal(path string, v interface{}) error { type unserializableHook struct{} -func (unserializableHook) Run(configs.HookState) error { +func (unserializableHook) Run(*specs.State) error { return nil } diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index d457e87d8a7..eb0f987fc8e 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -16,6 +16,7 @@ import ( "github.com/opencontainers/runc/libcontainer" "github.com/opencontainers/runc/libcontainer/cgroups/systemd" "github.com/opencontainers/runc/libcontainer/configs" + "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/sys/unix" ) @@ -1148,7 +1149,7 @@ func TestHook(t *testing.T) { config.Hooks = &configs.Hooks{ Prestart: []configs.Hook{ - configs.NewFunctionHook(func(s configs.HookState) error { + configs.NewFunctionHook(func(s *specs.State) error { if s.Bundle != expectedBundle { t.Fatalf("Expected prestart hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) } @@ -1165,7 +1166,7 @@ func TestHook(t *testing.T) { }), }, Poststart: []configs.Hook{ - configs.NewFunctionHook(func(s configs.HookState) error { + configs.NewFunctionHook(func(s *specs.State) error { if s.Bundle != expectedBundle { t.Fatalf("Expected poststart hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) } @@ -1178,7 +1179,7 @@ func TestHook(t *testing.T) { }), }, Poststop: []configs.Hook{ - configs.NewFunctionHook(func(s configs.HookState) error { + configs.NewFunctionHook(func(s *specs.State) error { if s.Bundle != expectedBundle { t.Fatalf("Expected poststop hook bundlePath '%s'; got '%s'", expectedBundle, s.Bundle) } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index f4c54410187..a97bdb5b4c8 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -364,15 +364,14 @@ func (p *initProcess) start() error { } } - if p.config.Config.Hooks != nil { - bundle, annotations := utils.Annotations(p.container.config.Labels) - s := configs.HookState{ - Version: p.container.config.Version, - ID: p.container.id, - Pid: p.pid(), - Bundle: bundle, - Annotations: annotations, + if p.config.Config.Hooks != nil && len(p.config.Config.Hooks.Prestart) > 0 { + s, err := p.container.currentOCIState() + if err != nil { + return err } + // initProcessStartTime hasn't been set yet. + s.Pid = p.cmd.Process.Pid + s.Status = "creating" for i, hook := range p.config.Config.Hooks.Prestart { if err := hook.Run(s); err != nil { return newSystemErrorWithCausef(err, "running prestart hook %d", i) @@ -395,15 +394,14 @@ func (p *initProcess) start() error { return newSystemErrorWithCause(err, "setting Intel RDT config for procHooks process") } } - if p.config.Config.Hooks != nil { - bundle, annotations := utils.Annotations(p.container.config.Labels) - s := configs.HookState{ - Version: p.container.config.Version, - ID: p.container.id, - Pid: p.pid(), - Bundle: bundle, - Annotations: annotations, + if p.config.Config.Hooks != nil && len(p.config.Config.Hooks.Prestart) > 0 { + s, err := p.container.currentOCIState() + if err != nil { + return err } + // initProcessStartTime hasn't been set yet. + s.Pid = p.cmd.Process.Pid + s.Status = "creating" for i, hook := range p.config.Config.Hooks.Prestart { if err := hook.Run(s); err != nil { return newSystemErrorWithCausef(err, "running prestart hook %d", i) diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go index b45ce23e4a5..07c9349c3a0 100644 --- a/libcontainer/state_linux.go +++ b/libcontainer/state_linux.go @@ -8,7 +8,6 @@ import ( "path/filepath" "github.com/opencontainers/runc/libcontainer/configs" - "github.com/opencontainers/runc/libcontainer/utils" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -62,13 +61,10 @@ func destroy(c *linuxContainer) error { } func runPoststopHooks(c *linuxContainer) error { - if c.config.Hooks != nil { - bundle, annotations := utils.Annotations(c.config.Labels) - s := configs.HookState{ - Version: c.config.Version, - ID: c.id, - Bundle: bundle, - Annotations: annotations, + if c.config.Hooks != nil && len(c.config.Hooks.Poststop) > 0 { + s, err := c.currentOCIState() + if err != nil { + return err } for _, hook := range c.config.Hooks.Poststop { if err := hook.Run(s); err != nil {