From 788664809faeb4504d357ae06aa803929c373ac5 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 30 Mar 2018 12:23:58 -0700 Subject: [PATCH 1/2] virtcontainers: container: Rollback when createContainer fails In case the container creation fails, we need a proper rollback regarding the mounts and hotplugs previously performed. This patch also rework the hotplugDrive() function in order to prevent createContainer() function complexity to exceed 15. Fixes #135 Signed-off-by: Sebastien Boeuf --- virtcontainers/container.go | 72 ++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 91164c81c9..3a23359025 100644 --- a/virtcontainers/container.go +++ b/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 { - return nil, err + if err = c.createContainersDirs(); err != nil { + return } - if !c.pod.config.HypervisorConfig.DisableBlockDeviceUse { - agentCaps := c.pod.agent.capabilities() - hypervisorCaps := c.pod.hypervisor.capabilities() + // In case the container creation fails, the following takes care + // of rolling back all the actions previously performed. + defer rollbackFailingContainerCreation(c, err) - if agentCaps.isBlockDeviceSupported() && hypervisorCaps.isBlockDeviceHotplugSupported() { - if err := c.hotplugDrive(); err != nil { - return nil, err - } - } + if err = c.hotplugDrive(); err != nil { + return } // Attach devices - if err := c.attachDevices(); err != nil { - return nil, err + if err = c.attachDevices(); err != nil { + return } - if err := c.addResources(); err != nil { - return nil, err + if err = c.addResources(); err != nil { + return } // Deduce additional system mount info that should be handled by the agent // inside the VM c.getSystemMountInfo() - if err := c.storeDevices(); err != nil { - return nil, err + if err = c.storeDevices(); err != nil { + 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 { - return nil, err + if err = c.storeProcess(); err != nil { + return } - if err := c.setContainerState(StateReady); err != nil { - return nil, err + if err = c.setContainerState(StateReady); err != nil { + 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 { From e98f9305ad85f2c7a31bbd7b48174539dd09f388 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 30 Mar 2018 14:16:04 -0700 Subject: [PATCH 2/2] virtcontainers: kata_agent: Rollback when createContainer fails In case the container creation fails, we need a proper rollback regarding the mounts previously performed. Fixes #135 Signed-off-by: Sebastien Boeuf --- virtcontainers/kata_agent.go | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index f5d950aeee..0a207a90b0 100644 --- a/virtcontainers/kata_agent.go +++ b/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 { - bindUnmountAllRootfs(kataHostSharedDir, *pod) + if err = bindMountContainerRootfs(kataHostSharedDir, pod.id, c.id, c.rootFs, false); err != nil { 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 }