From 556f798a19ecf23b20c518db5fa5fcec4c5034b6 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 21 Jan 2016 16:43:33 -0800 Subject: [PATCH 1/2] Fix various state bugs for pause and destroy There were issues where a process could die before pausing completed leaving the container in an inconsistent state and unable to be destoryed. This makes sure that if the container is paused and the process is dead it will unfreeze the cgroup before removing them. Signed-off-by: Michael Crosby --- libcontainer/container_linux.go | 90 ++++++++++++++++++-------------- libcontainer/error.go | 3 ++ libcontainer/state_linux.go | 17 ++++-- libcontainer/state_linux_test.go | 8 +-- 4 files changed, 68 insertions(+), 50 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index a08bcae2f..19b8f3a75 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -189,29 +189,27 @@ func (c *linuxContainer) Start(process *Process) error { } return newSystemError(err) } + c.state = &runningState{ + c: c, + } if doInit { if err := c.updateState(parent); err != nil { return err } - } else { - c.state.transition(&nullState{ - c: c, - s: Running, - }) - } - if c.config.Hooks != nil { - s := configs.HookState{ - Version: c.config.Version, - ID: c.id, - Pid: parent.pid(), - Root: c.config.Rootfs, - } - for _, hook := range c.config.Hooks.Poststart { - if err := hook.Run(s); err != nil { - if err := parent.terminate(); err != nil { - logrus.Warn(err) + if c.config.Hooks != nil { + s := configs.HookState{ + Version: c.config.Version, + ID: c.id, + Pid: parent.pid(), + Root: c.config.Rootfs, + } + for _, hook := range c.config.Hooks.Poststart { + if err := hook.Run(s); err != nil { + if err := parent.terminate(); err != nil { + logrus.Warn(err) + } + return newSystemError(err) } - return newSystemError(err) } } } @@ -340,6 +338,13 @@ func (c *linuxContainer) Destroy() error { func (c *linuxContainer) Pause() error { c.m.Lock() defer c.m.Unlock() + status, err := c.currentStatus() + if err != nil { + return err + } + if status != Running { + return newGenericError(fmt.Errorf("container not running"), ContainerNotRunning) + } if err := c.cgroupManager.Freeze(configs.Frozen); err != nil { return err } @@ -351,6 +356,13 @@ func (c *linuxContainer) Pause() error { func (c *linuxContainer) Resume() error { c.m.Lock() defer c.m.Unlock() + status, err := c.currentStatus() + if err != nil { + return err + } + if status != Paused { + return newGenericError(fmt.Errorf("container not paused"), ContainerNotPaused) + } if err := c.cgroupManager.Freeze(configs.Thawed); err != nil { return err } @@ -939,9 +951,6 @@ func (c *linuxContainer) criuNotifications(resp *criurpc.CriuResp, process *Proc func (c *linuxContainer) updateState(process parentProcess) error { c.initProcess = process - if err := c.refreshState(); err != nil { - return err - } state, err := c.currentState() if err != nil { return err @@ -1017,35 +1026,36 @@ func (c *linuxContainer) isPaused() (bool, error) { } func (c *linuxContainer) currentState() (*State, error) { - status, err := c.currentStatus() - if err != nil { - return nil, err - } - if status == Destroyed { - return nil, newGenericError(fmt.Errorf("container destroyed"), ContainerNotExists) - } - startTime, err := c.initProcess.startTime() - if err != nil { - return nil, newSystemError(err) + var ( + startTime string + externalDescriptors []string + pid = -1 + ) + if c.initProcess != nil { + pid = c.initProcess.pid() + startTime, _ = c.initProcess.startTime() + externalDescriptors = c.initProcess.externalDescriptors() } state := &State{ BaseState: BaseState{ ID: c.ID(), Config: *c.config, - InitProcessPid: c.initProcess.pid(), + InitProcessPid: pid, InitProcessStartTime: startTime, }, CgroupPaths: c.cgroupManager.GetPaths(), NamespacePaths: make(map[configs.NamespaceType]string), - ExternalDescriptors: c.initProcess.externalDescriptors(), + ExternalDescriptors: externalDescriptors, } - for _, ns := range c.config.Namespaces { - state.NamespacePaths[ns.Type] = ns.GetPath(c.initProcess.pid()) - } - for _, nsType := range configs.NamespaceTypes() { - if _, ok := state.NamespacePaths[nsType]; !ok { - ns := configs.Namespace{Type: nsType} - state.NamespacePaths[ns.Type] = ns.GetPath(c.initProcess.pid()) + if pid > 0 { + for _, ns := range c.config.Namespaces { + state.NamespacePaths[ns.Type] = ns.GetPath(pid) + } + for _, nsType := range configs.NamespaceTypes() { + if _, ok := state.NamespacePaths[nsType]; !ok { + ns := configs.Namespace{Type: nsType} + state.NamespacePaths[ns.Type] = ns.GetPath(pid) + } } } return state, nil diff --git a/libcontainer/error.go b/libcontainer/error.go index aa59d2aec..378ef4694 100644 --- a/libcontainer/error.go +++ b/libcontainer/error.go @@ -16,6 +16,7 @@ const ( ContainerPaused ContainerNotStopped ContainerNotRunning + ContainerNotPaused // Process errors ProcessNotExecuted @@ -46,6 +47,8 @@ func (c ErrorCode) String() string { return "Container is not running" case ConsoleExists: return "Console exists for process" + case ContainerNotPaused: + return "Container is not paused" default: return "Unknown error" } diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go index 5e1bb731f..d352c4826 100644 --- a/libcontainer/state_linux.go +++ b/libcontainer/state_linux.go @@ -49,6 +49,7 @@ func destroy(c *linuxContainer) error { if herr := runPoststopHooks(c); err == nil { err = herr } + c.state = &stoppedState{c: c} return err } @@ -116,10 +117,10 @@ func (r *runningState) transition(s containerState) error { } r.c.state = s return nil - case *pausedState: + case *pausedState, *nullState: r.c.state = s return nil - case *runningState, *nullState: + case *runningState: return nil } return newStateTransitionError(r, s) @@ -148,7 +149,7 @@ func (p *pausedState) status() Status { func (p *pausedState) transition(s containerState) error { switch s.(type) { - case *runningState: + case *runningState, *stoppedState: p.c.state = s return nil case *pausedState: @@ -158,6 +159,16 @@ func (p *pausedState) transition(s containerState) error { } func (p *pausedState) destroy() error { + isRunning, err := p.c.isRunning() + if err != nil { + return err + } + if !isRunning { + if err := p.c.cgroupManager.Freeze(configs.Thawed); err != nil { + return err + } + return destroy(p.c) + } return newGenericError(fmt.Errorf("container is paused"), ContainerPaused) } diff --git a/libcontainer/state_linux_test.go b/libcontainer/state_linux_test.go index e2740ca63..6281d6f45 100644 --- a/libcontainer/state_linux_test.go +++ b/libcontainer/state_linux_test.go @@ -49,19 +49,13 @@ func TestPausedStateTransition(t *testing.T) { valid := []containerState{ &pausedState{}, &runningState{}, + &stoppedState{}, } for _, v := range valid { if err := s.transition(v); err != nil { t.Fatal(err) } } - err := s.transition(&stoppedState{}) - if err == nil { - t.Fatal("transition to stopped state should fail") - } - if !isStateTransitionError(err) { - t.Fatal("expected stateTransitionError") - } } func TestRestoredStateTransition(t *testing.T) { From 9c3fa7928e21ce86ad8b79f13b392beb41b39bb7 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 21 Jan 2016 16:48:05 -0800 Subject: [PATCH 2/2] Allow switch to anything from nullState Signed-off-by: Michael Crosby --- libcontainer/factory_linux.go | 3 +++ libcontainer/state_linux.go | 7 +------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index ae5db9cdb..eb2bb5fbe 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -203,6 +203,9 @@ func (l *LinuxFactory) Load(id string) (Container, error) { root: containerRoot, } c.state = &nullState{c: c} + if err := c.refreshState(); err != nil { + return nil, err + } return c, nil } diff --git a/libcontainer/state_linux.go b/libcontainer/state_linux.go index d352c4826..fcd4e17dc 100644 --- a/libcontainer/state_linux.go +++ b/libcontainer/state_linux.go @@ -214,12 +214,7 @@ func (n *nullState) status() Status { } func (n *nullState) transition(s containerState) error { - switch s.(type) { - case *restoredState: - n.c.state = s - default: - // do nothing for null states - } + n.c.state = s return nil }