jasder
/
runc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

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.

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.

I really think we should have len() guards to avoid computing the
state when .Hooks is non-nil but the particular phase we're looking at
is empty.  Aleksa, however, is adamantly against them [3] citing a
risk of sloppy copy/pastes causing the hook slice being len-guarded to
diverge from the hook slice being iterated over within the guard.  I
think that ort of thing is very lo-risk, because:

* We shouldn't be copy/pasting this, right?  DRY for the win :).
* There's only ever a few lines between the guard and the guarded
  loop.  That makes broken copy/pastes easy to catch in review.
* We should have test coverage for these.  Guarding with the wrong
  slice is certainly not the only thing you can break with a sloppy
  copy/paste.

But I'm not a maintainer ;).

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart
[2]: https://github.com/opencontainers/runc/issues/1710
[3]: https://github.com/opencontainers/runc/pull/1741#discussion_r233331570

Signed-off-by: W. Trevor King <wking@tremily.us>
This commit is contained in:
W. Trevor King 2018-02-25 14:47:41 -08:00
parent 10d38b660a
commit e23868603a
8 changed files with 80 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
libcontainer/configs/config.go
View File

@ -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

14
libcontainer/configs/config_test.go
View File

@ -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",
}

7
libcontainer/container.go
View File

@ -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

54
libcontainer/container_linux.go
View File

@ -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 {
@ -349,13 +356,9 @@ 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,
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) {
@ -1537,13 +1542,9 @@ func (c *linuxContainer) criuNotifications(resp *criurpc.CriuResp, process *Proc
}
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,
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) {

3
libcontainer/factory_linux_test.go
View File

@ -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
}

7
libcontainer/integration/exec_test.go
View File

@ -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)
}

26
libcontainer/process_linux.go
View File

@ -365,14 +365,13 @@ 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,
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)
@ -396,14 +395,13 @@ 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,
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)

10
libcontainer/state_linux.go
View File

@ -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"
@ -63,12 +62,9 @@ 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,
s, err := c.currentOCIState()
if err != nil {
return err
}
for _, hook := range c.config.Hooks.Poststop {
if err := hook.Run(s); err != nil {