Merge pull request #136 from sboeuf/fix_rollback

virtcontainers: Properly rollback mounts and hotplugs when container creation failed

virtcontainers/container.go

@@ -416,63 +416,78 @@ func newContainer(pod *Pod, contConfig ContainerConfig) (*Container, error) {
 	return c, nil
 }
 
+// rollbackFailingContainerCreation rolls back important steps that might have
+// been performed before the container creation failed.
+// - Unplug CPU and memory resources from the VM.
+// - Unplug devices from the VM.
+func rollbackFailingContainerCreation(c *Container, err error) {
+	if err != nil && c != nil {
+		if err2 := c.removeResources(); err2 != nil {
+			c.Logger().WithError(err2).Error("rollback failed removeResources()")
+		}
+		if err2 := c.detachDevices(); err2 != nil {
+			c.Logger().WithError(err2).Error("rollback failed detachDevices()")
+		}
+		if err2 := c.removeDrive(); err2 != nil {
+			c.Logger().WithError(err2).Error("rollback failed removeDrive()")
+		}
+	}
+}
+
 // createContainer creates and start a container inside a Pod. It has to be
 // called only when a new container, not known by the pod, has to be created.
-func createContainer(pod *Pod, contConfig ContainerConfig) (*Container, error) {
+func createContainer(pod *Pod, contConfig ContainerConfig) (c *Container, err error) {
 	if pod == nil {
 		return nil, errNeedPod
 	}
 
-	c, err := newContainer(pod, contConfig)
+	c, err = newContainer(pod, contConfig)
 	if err != nil {
-		return nil, err
+		return
 	}
 
-	if err := c.createContainersDirs(); err != nil {
+	if err = c.createContainersDirs(); err != nil {
-		return nil, err
+		return
 	}
 
-	if !c.pod.config.HypervisorConfig.DisableBlockDeviceUse {
+	// In case the container creation fails, the following takes care
-		agentCaps := c.pod.agent.capabilities()
+	// of rolling back all the actions previously performed.
-		hypervisorCaps := c.pod.hypervisor.capabilities()
+	defer rollbackFailingContainerCreation(c, err)
 
-		if agentCaps.isBlockDeviceSupported() && hypervisorCaps.isBlockDeviceHotplugSupported() {
+	if err = c.hotplugDrive(); err != nil {
-			if err := c.hotplugDrive(); err != nil {
+		return
-				return nil, err
-			}
-		}
 	}
 
 	// Attach devices
-	if err := c.attachDevices(); err != nil {
+	if err = c.attachDevices(); err != nil {
-		return nil, err
+		return
 	}
 
-	if err := c.addResources(); err != nil {
+	if err = c.addResources(); err != nil {
-		return nil, err
+		return
 	}
 
 	// Deduce additional system mount info that should be handled by the agent
 	// inside the VM
 	c.getSystemMountInfo()
 
-	if err := c.storeDevices(); err != nil {
+	if err = c.storeDevices(); err != nil {
-		return nil, err
+		return
 	}
 
 	process, err := pod.agent.createContainer(c.pod, c)
 	if err != nil {
-		return nil, err
+		return c, err
 	}
 	c.process = *process
 
 	// Store the container process returned by the agent.
-	if err := c.storeProcess(); err != nil {
+	if err = c.storeProcess(); err != nil {
-		return nil, err
+		return
 	}
 
-	if err := c.setContainerState(StateReady); err != nil {
+	if err = c.setContainerState(StateReady); err != nil {
-		return nil, err
+		return
 	}
 
 	return c, nil
@@ -666,6 +681,15 @@ func (c *Container) processList(options ProcessListOptions) (ProcessList, error)
 }
 
 func (c *Container) hotplugDrive() error {
+	agentCaps := c.pod.agent.capabilities()
+	hypervisorCaps := c.pod.hypervisor.capabilities()
+
+	if c.pod.config.HypervisorConfig.DisableBlockDeviceUse ||
+		!agentCaps.isBlockDeviceSupported() ||
+		!hypervisorCaps.isBlockDeviceHotplugSupported() {
+		return nil
+	}
+
 	dev, err := getDeviceForPath(c.rootFs)
 
 	if err == errMountPointNotFound {

virtcontainers/kata_agent.go

@@ -599,7 +599,25 @@ func (k *kataAgent) appendDevices(deviceList []*grpc.Device, devices []Device) [
 	return deviceList
 }
 
-func (k *kataAgent) createContainer(pod *Pod, c *Container) (*Process, error) {
+// rollbackFailingContainerCreation rolls back important steps that might have
+// been performed before the container creation failed.
+// - Unmount container volumes.
+// - Unmount container rootfs.
+func (k *kataAgent) rollbackFailingContainerCreation(c *Container, err error) {
+	if err != nil {
+		if c != nil {
+			if err2 := c.unmountHostMounts(); err2 != nil {
+				k.Logger().WithError(err2).Error("rollback failed unmountHostMounts()")
+			}
+
+			if err2 := bindUnmountContainerRootfs(kataHostSharedDir, c.pod.id, c.id); err2 != nil {
+				k.Logger().WithError(err2).Error("rollback failed bindUnmountContainerRootfs()")
+			}
+		}
+	}
+}
+
+func (k *kataAgent) createContainer(pod *Pod, c *Container) (p *Process, err error) {
 	ociSpecJSON, ok := c.config.Annotations[vcAnnotations.ConfigJSONKey]
 	if !ok {
 		return nil, errorMissingOCISpec
@@ -619,6 +637,10 @@ func (k *kataAgent) createContainer(pod *Pod, c *Container) (*Process, error) {
 	rootPathParent := filepath.Join(kataGuestSharedDir, c.id)
 	rootPath := filepath.Join(rootPathParent, rootfsDir)
 
+	// In case the container creation fails, the following defer statement
+	// takes care of rolling back actions previously performed.
+	defer k.rollbackFailingContainerCreation(c, err)
+
 	if c.state.Fstype != "" {
 		// This is a block based device rootfs.
 
@@ -667,27 +689,25 @@ func (k *kataAgent) createContainer(pod *Pod, c *Container) (*Process, error) {
 		// (kataGuestSharedDir) is already mounted in the
 		// guest. We only need to mount the rootfs from
 		// the host and it will show up in the guest.
-		if err := bindMountContainerRootfs(kataHostSharedDir, pod.id, c.id, c.rootFs, false); err != nil {
+		if err = bindMountContainerRootfs(kataHostSharedDir, pod.id, c.id, c.rootFs, false); err != nil {
-			bindUnmountAllRootfs(kataHostSharedDir, *pod)
 			return nil, err
 		}
 	}
 
 	ociSpec := &specs.Spec{}
-	if err := json.Unmarshal([]byte(ociSpecJSON), ociSpec); err != nil {
+	if err = json.Unmarshal([]byte(ociSpecJSON), ociSpec); err != nil {
 		return nil, err
 	}
 
 	// Handle container mounts
 	newMounts, err := c.mountSharedDirMounts(kataHostSharedDir, kataGuestSharedDir)
 	if err != nil {
-		bindUnmountAllRootfs(kataHostSharedDir, *pod)
 		return nil, err
 	}
 
 	// We replace all OCI mount sources that match our container mount
 	// with the right source path (The guest one).
-	if err := k.replaceOCIMountSource(ociSpec, newMounts); err != nil {
+	if err = k.replaceOCIMountSource(ociSpec, newMounts); err != nil {
 		return nil, err
 	}
 
@@ -714,7 +734,7 @@ func (k *kataAgent) createContainer(pod *Pod, c *Container) (*Process, error) {
 		OCI:         grpcSpec,
 	}
 
-	if _, err := k.sendReq(req); err != nil {
+	if _, err = k.sendReq(req); err != nil {
 		return nil, err
 	}