Merge pull request #3773 from mikemccracken/2018-01-18/destroy-contai…

…ner-on-err

lxc: cleanup partially configured containers after errors in Start

client/driver/lxc.go

@@ -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 occurred while cleaning up from error in Start: %v", cleanupErr)
+		}
+	}
+	return sresp, err
+}
+
+func (d *LxcDriver) startWithCleanup(ctx *ExecContext, task *structs.Task) (*StartResponse, error, func() error) {
+	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,28 @@ 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
+		}
+		return c.Destroy()
 	}
 
 	// 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 +353,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 }

client/driver/lxc_test.go

@@ -310,15 +310,27 @@ func TestLxcDriver_Start_NoVolumes(t *testing.T) {
 	ctx := testDriverContexts(t, task)
 	defer ctx.AllocDir.Destroy()
 
+	// set lxcVolumesConfigOption to false to disallow absolute paths as the source for the bind mount
 	ctx.DriverCtx.config.Options = map[string]string{lxcVolumesConfigOption: "false"}
 
 	d := NewLxcDriver(ctx.DriverCtx)
 
 	if _, err := d.Prestart(ctx.ExecCtx, task); err != nil {
 		t.Fatalf("prestart err: %v", err)
 	}
+
+	// expect the "absolute bind-mount volume in config.. " error
 	_, err := d.Start(ctx.ExecCtx, task)
 	if err == nil {
 		t.Fatalf("expected error in start, got nil.")
 	}
+
+	// Because the container was created but not started before
+	// the expected error, we can test that the destroy-only
+	// cleanup is done here.
+	containerName := fmt.Sprintf("%s-%s", task.Name, ctx.DriverCtx.allocID)
+	if err := exec.Command("bash", "-c", fmt.Sprintf("lxc-ls -1 | grep -q %s", containerName)).Run(); err == nil {
+		t.Fatalf("error, container '%s' is still around", containerName)
+	}
+
 }