Skip to content

Commit 8ff0b71

Browse files
committed
Ensure we can always terminate the parent process on error
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>
1 parent 4cb480d commit 8ff0b71

File tree

3 files changed

+45
-24
lines changed

3 files changed

+45
-24
lines changed

libcontainer/container_linux.go

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -201,20 +201,31 @@ func (c *Container) Set(config configs.Config) error {
201201
func (c *Container) Start(process *Process) error {
202202
c.m.Lock()
203203
defer c.m.Unlock()
204-
return c.start(process)
204+
205+
if err := c.start(process); err != nil {
206+
c.terminate(process)
207+
return err
208+
}
209+
return nil
205210
}
206211

207212
// Run immediately starts the process inside the container. Returns an error if
208213
// the process fails to start. It does not block waiting for the exec fifo
209214
// after start returns but opens the fifo after start returns.
210-
func (c *Container) Run(process *Process) error {
215+
func (c *Container) Run(process *Process) (retErr error) {
211216
c.m.Lock()
212217
defer c.m.Unlock()
218+
213219
if err := c.start(process); err != nil {
220+
c.terminate(process)
214221
return err
215222
}
216-
if process.Init {
217-
return c.exec()
223+
if !process.Init {
224+
return nil
225+
}
226+
if err := c.exec(); err != nil {
227+
c.terminate(process)
228+
return err
218229
}
219230
return nil
220231
}
@@ -226,6 +237,34 @@ func (c *Container) Exec() error {
226237
return c.exec()
227238
}
228239

240+
// terminate is to kill the container's init/exec process when got failure.
241+
func (c *Container) terminate(process *Process) {
242+
if process.ops == nil {
243+
return
244+
}
245+
if process.Init {
246+
if err := ignoreTerminateErrors(process.ops.terminate()); err != nil {
247+
logrus.WithError(err).Warn("unable to terminate initProcess")
248+
}
249+
// If we haven't saved container's state yet, we need to destroy the
250+
// cgroup & intelRdt manager manually.
251+
if _, err := os.Stat(filepath.Join(c.stateDir, stateFilename)); os.IsNotExist(err) {
252+
if err := c.cgroupManager.Destroy(); err != nil {
253+
logrus.WithError(err).Warn("unable to destroy cgroupManager")
254+
}
255+
if c.intelRdtManager != nil {
256+
if err := c.intelRdtManager.Destroy(); err != nil {
257+
logrus.WithError(err).Warn("unable to destroy intelRdtManager")
258+
}
259+
}
260+
}
261+
return
262+
}
263+
if err := ignoreTerminateErrors(process.ops.terminate()); err != nil {
264+
logrus.WithError(err).Warn("unable to terminate setnsProcess")
265+
}
266+
}
267+
229268
func (c *Container) exec() error {
230269
path := filepath.Join(c.stateDir, execFifoFilename)
231270
pid := c.initProcess.pid()
@@ -356,12 +395,7 @@ func (c *Container) start(process *Process) (retErr error) {
356395
return err
357396
}
358397

359-
if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
360-
if err := ignoreTerminateErrors(parent.terminate()); err != nil {
361-
logrus.Warn(fmt.Errorf("error running poststart hook: %w", err))
362-
}
363-
return err
364-
}
398+
return c.config.Hooks.Run(configs.Poststart, s)
365399
}
366400
}
367401
return nil

libcontainer/process.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
var errInvalidProcess = errors.New("invalid process")
1313

1414
type processOperations interface {
15+
terminate() error
1516
wait() (*os.ProcessState, error)
1617
signal(sig os.Signal) error
1718
pid() int

libcontainer/process_linux.go

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,6 @@ func (p *setnsProcess) start() (retErr error) {
146146
// Someone in this cgroup was killed, this _might_ be us.
147147
retErr = fmt.Errorf("%w (possibly OOM-killed)", retErr)
148148
}
149-
err := ignoreTerminateErrors(p.terminate())
150-
if err != nil {
151-
logrus.WithError(err).Warn("unable to terminate setnsProcess")
152-
}
153149
}
154150
}()
155151

@@ -548,16 +544,6 @@ func (p *initProcess) start() (retErr error) {
548544
retErr = errors.New(oomError)
549545
}
550546
}
551-
552-
// Terminate the process to ensure we can remove cgroups.
553-
if err := ignoreTerminateErrors(p.terminate()); err != nil {
554-
logrus.WithError(err).Warn("unable to terminate initProcess")
555-
}
556-
557-
_ = p.manager.Destroy()
558-
if p.intelRdtManager != nil {
559-
_ = p.intelRdtManager.Destroy()
560-
}
561547
}
562548
}()
563549

0 commit comments

Comments
 (0)