-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lxc: cleanup partially configured containers after errors in Start #3773
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,9 +210,20 @@ func (d *LxcDriver) Prestart(*ExecContext, *structs.Task) (*PrestartResponse, er | |
|
||
// Start starts the LXC Driver | ||
func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, error) { | ||
sresp, err, errCleanup := d.StartWithCleanup(ctx, task) | ||
if err != nil { | ||
if cleanupErr := errCleanup(); cleanupErr != nil { | ||
d.logger.Printf("[ERR] error occured:\n%v\nwhile cleaning up from error in Start: %v", cleanupErr, err) | ||
} | ||
} | ||
return sresp, err | ||
} | ||
|
||
func (d *LxcDriver) StartWithCleanup(ctx *ExecContext, task *structs.Task) (*StartResponse, error, func() error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nitpick, but let's lowercase |
||
noCleanup := func() error { return nil } | ||
var driverConfig LxcDriverConfig | ||
if err := mapstructure.WeakDecode(task.Config, &driverConfig); err != nil { | ||
return nil, err | ||
return nil, err, noCleanup | ||
} | ||
lxcPath := lxc.DefaultConfigPath() | ||
if path := d.config.Read("driver.lxc.path"); path != "" { | ||
|
@@ -222,7 +233,7 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
containerName := fmt.Sprintf("%s-%s", task.Name, d.DriverContext.allocID) | ||
c, err := lxc.NewContainer(containerName, lxcPath) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to initialize container: %v", err) | ||
return nil, fmt.Errorf("unable to initialize container: %v", err), noCleanup | ||
} | ||
|
||
var verbosity lxc.Verbosity | ||
|
@@ -232,7 +243,7 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
case "", "quiet": | ||
verbosity = lxc.Quiet | ||
default: | ||
return nil, fmt.Errorf("lxc driver config 'verbosity' can only be either quiet or verbose") | ||
return nil, fmt.Errorf("lxc driver config 'verbosity' can only be either quiet or verbose"), noCleanup | ||
} | ||
c.SetVerbosity(verbosity) | ||
|
||
|
@@ -249,7 +260,7 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
case "", "error": | ||
logLevel = lxc.ERROR | ||
default: | ||
return nil, fmt.Errorf("lxc driver config 'log_level' can only be trace, debug, info, warn or error") | ||
return nil, fmt.Errorf("lxc driver config 'log_level' can only be trace, debug, info, warn or error"), noCleanup | ||
} | ||
c.SetLogLevel(logLevel) | ||
|
||
|
@@ -267,12 +278,12 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
} | ||
|
||
if err := c.Create(options); err != nil { | ||
return nil, fmt.Errorf("unable to create container: %v", err) | ||
return nil, fmt.Errorf("unable to create container: %v", err), noCleanup | ||
} | ||
|
||
// Set the network type to none | ||
if err := c.SetConfigItem("lxc.network.type", "none"); err != nil { | ||
return nil, fmt.Errorf("error setting network type configuration: %v", err) | ||
return nil, fmt.Errorf("error setting network type configuration: %v", err), c.Destroy | ||
} | ||
|
||
// Bind mount the shared alloc dir and task local dir in the container | ||
|
@@ -290,7 +301,7 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
|
||
if filepath.IsAbs(paths[0]) { | ||
if !volumesEnabled { | ||
return nil, fmt.Errorf("absolute bind-mount volume in config but '%v' is false", lxcVolumesConfigOption) | ||
return nil, fmt.Errorf("absolute bind-mount volume in config but '%v' is false", lxcVolumesConfigOption), c.Destroy | ||
} | ||
} else { | ||
// Relative source paths are treated as relative to alloc dir | ||
|
@@ -302,21 +313,31 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
|
||
for _, mnt := range mounts { | ||
if err := c.SetConfigItem("lxc.mount.entry", mnt); err != nil { | ||
return nil, fmt.Errorf("error setting bind mount %q error: %v", mnt, err) | ||
return nil, fmt.Errorf("error setting bind mount %q error: %v", mnt, err), c.Destroy | ||
} | ||
} | ||
|
||
// Start the container | ||
if err := c.Start(); err != nil { | ||
return nil, fmt.Errorf("unable to start container: %v", err) | ||
return nil, fmt.Errorf("unable to start container: %v", err), c.Destroy | ||
} | ||
|
||
stopAndDestroyCleanup := func() error { | ||
if err := c.Stop(); err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we attempt to Destroy the container even if Stop fails, or will Destroy never succeed if Stop errors? I'm unfamiliar with lxc's behavior in this circumstance and the Go library's docs aren't very useful. If it's safe to call Destroy if Stop errors, I'd suggest just ignoring Destroy's return value and returning the original Stop effort (so Destroy is just a best-effort at cleaning up). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the only reasons that Stop might fail would also cause Destroy to fail. I think returning without trying Destroy is the right thing here. |
||
} | ||
if err := c.Destroy(); err != nil { | ||
return err | ||
} | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This last few lines can be simplified to |
||
} | ||
|
||
// Set the resource limits | ||
if err := c.SetMemoryLimit(lxc.ByteSize(task.Resources.MemoryMB) * lxc.MB); err != nil { | ||
return nil, fmt.Errorf("unable to set memory limits: %v", err) | ||
return nil, fmt.Errorf("unable to set memory limits: %v", err), stopAndDestroyCleanup | ||
} | ||
if err := c.SetCgroupItem("cpu.shares", strconv.Itoa(task.Resources.CPU)); err != nil { | ||
return nil, fmt.Errorf("unable to set cpu shares: %v", err) | ||
return nil, fmt.Errorf("unable to set cpu shares: %v", err), stopAndDestroyCleanup | ||
} | ||
|
||
h := lxcDriverHandle{ | ||
|
@@ -335,7 +356,7 @@ func (d *LxcDriver) Start(ctx *ExecContext, task *structs.Task) (*StartResponse, | |
|
||
go h.run() | ||
|
||
return &StartResponse{Handle: &h}, nil | ||
return &StartResponse{Handle: &h}, nil, noCleanup | ||
} | ||
|
||
func (d *LxcDriver) Cleanup(*ExecContext, *CreatedResources) error { return nil } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the newline and
err
from this message sinceerr
will be logged later anyway.