Skip to content

Commit

Permalink
Ensure we can always terminate the parent process on error
Browse files Browse the repository at this point in the history
As we all know, we should terminate the parent process if there is an
error when starting the container process, but these terminate function
are called in many places, for example: `initProcess`, `setnsProcess`,
and `Container`, if we forget this terminate call for some errors, it
will let the container in unknown state, so we should change to call it
in some final places.

Signed-off-by: lifubang <lifubang@acmcoder.com>
  • Loading branch information
lifubang committed Sep 23, 2024
1 parent 1590491 commit 8dcd42b
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 23 deletions.
53 changes: 44 additions & 9 deletions libcontainer/container_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -201,18 +201,30 @@ func (c *Container) Set(config configs.Config) error {

// Start starts a process inside the container. Returns error if process fails
// to start. You can track process lifecycle with passed Process structure.
func (c *Container) Start(process *Process) error {
func (c *Container) Start(process *Process) (retErr error) {
c.m.Lock()
defer c.m.Unlock()
defer func() {
if retErr != nil {
c.terminate(process)
}
c.m.Unlock()
}()
return c.start(process)
}

// Run immediately starts the process inside the container. Returns an error if
// the process fails to start. It does not block waiting for the exec fifo
// after start returns but opens the fifo after start returns.
func (c *Container) Run(process *Process) error {
func (c *Container) Run(process *Process) (retErr error) {
c.m.Lock()
defer c.m.Unlock()

defer func() {
if retErr != nil {
c.terminate(process)
}
}()

if err := c.start(process); err != nil {
return err
}
Expand All @@ -229,6 +241,34 @@ func (c *Container) Exec() error {
return c.exec()
}

// terminate is to kill the container's init/exec process when got failure.
func (c *Container) terminate(process *Process) {
if process.ops == nil {
return
}
if process.Init {
if err := ignoreTerminateErrors(process.ops.terminate()); err != nil {
logrus.WithError(err).Warn("unable to terminate initProcess")
}
// If we haven't saved container's state yet, we need to destroy the
// cgroup & intelRdt manager manually.
if _, err := os.Stat(filepath.Join(c.stateDir, stateFilename)); os.IsNotExist(err) {
if err := c.cgroupManager.Destroy(); err != nil {
logrus.WithError(err).Warn("unable to destroy cgroupManager")
}
if c.intelRdtManager != nil {
if err := c.intelRdtManager.Destroy(); err != nil {
logrus.WithError(err).Warn("unable to destroy intelRdtManager")
}
}
}
return
}
if err := ignoreTerminateErrors(process.ops.terminate()); err != nil {
logrus.WithError(err).Warn("unable to terminate setnsProcess")
}
}

func (c *Container) exec() error {
path := filepath.Join(c.stateDir, execFifoFilename)
pid := c.initProcess.pid()
Expand Down Expand Up @@ -359,12 +399,7 @@ func (c *Container) start(process *Process) (retErr error) {
return err
}

if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
if err := ignoreTerminateErrors(parent.terminate()); err != nil {
logrus.Warn(fmt.Errorf("error running poststart hook: %w", err))
}
return err
}
return c.config.Hooks.Run(configs.Poststart, s)
}
}
return nil
Expand Down
1 change: 1 addition & 0 deletions libcontainer/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
var errInvalidProcess = errors.New("invalid process")

type processOperations interface {
terminate() error
wait() (*os.ProcessState, error)
signal(sig os.Signal) error
pid() int
Expand Down
14 changes: 0 additions & 14 deletions libcontainer/process_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,6 @@ func (p *setnsProcess) start() (retErr error) {
if werr != nil {
logrus.WithError(werr).Warn()
}
err := ignoreTerminateErrors(p.terminate())
if err != nil {
logrus.WithError(err).Warn("unable to terminate setnsProcess")
}
}
}()

Expand Down Expand Up @@ -563,16 +559,6 @@ func (p *initProcess) start() (retErr error) {
if werr != nil {
logrus.WithError(werr).Warn()
}

// Terminate the process to ensure we can remove cgroups.
if err := ignoreTerminateErrors(p.terminate()); err != nil {
logrus.WithError(err).Warn("unable to terminate initProcess")
}

_ = p.manager.Destroy()
if p.intelRdtManager != nil {
_ = p.intelRdtManager.Destroy()
}
}
}()

Expand Down

0 comments on commit 8dcd42b

Please sign in to comment.