vc: Delete store when new/create container is failed

The container store should be deleted when new/create is failed if the store is newly created. Fixes: #2013 Signed-off-by: Li Yuxuan <[email protected]>
kata-containers · Aug 30, 2019 · a5f1744 · a5f1744
1 parent af57485
commit a5f1744
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 3 deletions.
diff --git a/virtcontainers/container.go b/virtcontainers/container.go
@@ -718,10 +718,18 @@ func newContainer(sandbox *Sandbox, contConfig ContainerConfig) (*Container, err
 		ctx:           sandbox.ctx,
 	}
 
+	storeAlreadyExists := store.VCContainerStoreExists(sandbox.ctx, c.sandboxID, c.id)
 	ctrStore, err := store.NewVCContainerStore(sandbox.ctx, c.sandboxID, c.id)
 	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if err != nil && !storeAlreadyExists {
+			if delerr := c.store.Delete(); delerr != nil {
+				c.Logger().WithError(delerr).WithField("cid", c.id).Error("delete store failed")
+			}
+		}
+	}()
 
 	c.store = ctrStore
 

diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go
@@ -1107,6 +1107,7 @@ func (s *Sandbox) fetchContainers() error {
 // This should be called only when the sandbox is already created.
 // It will add new container config to sandbox.config.Containers
 func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, error) {
+	storeAlreadyExists := store.VCContainerStoreExists(s.ctx, s.id, contConfig.ID)
 	// Create the container.
 	c, err := newContainer(s, contConfig)
 	if err != nil {
@@ -1117,9 +1118,16 @@ func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, erro
 	s.config.Containers = append(s.config.Containers, contConfig)
 
 	defer func() {
-		if err != nil && len(s.config.Containers) > 0 {
-			// delete container config
-			s.config.Containers = s.config.Containers[:len(s.config.Containers)-1]
+		if err != nil {
+			if len(s.config.Containers) > 0 {
+				// delete container config
+				s.config.Containers = s.config.Containers[:len(s.config.Containers)-1]
+			}
+			if !storeAlreadyExists {
+				if delerr := c.store.Delete(); delerr != nil {
+					c.Logger().WithError(delerr).WithField("cid", c.id).Error("delete store failed")
+				}
+			}
 		}
 	}()
 

diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go
@@ -915,6 +915,8 @@ func TestCreateContainer(t *testing.T) {
 	_, err = s.CreateContainer(contConfig)
 	assert.NotNil(t, err, "Should failed to create a duplicated container")
 	assert.Equal(t, len(s.config.Containers), 1, "Container config list length from sandbox structure should be 1")
+	ret := store.VCContainerStoreExists(s.ctx, testSandboxID, contID)
+	assert.True(t, ret, "Should not delete container store that already existed")
 }
 
 func TestDeleteContainer(t *testing.T) {
@@ -1006,6 +1008,47 @@ func TestEnterContainer(t *testing.T) {
 	assert.Nil(t, err, "Enter container failed: %v", err)
 }
 
+func TestDeleteStoreWhenCreateContainerFail(t *testing.T) {
+	hypervisorConfig := newHypervisorConfig(nil, nil)
+	s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hypervisorConfig, NoopAgentType, NetworkConfig{}, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cleanUp()
+
+	contID := "999"
+	contConfig := newTestContainerConfigNoop(contID)
+	contConfig.RootFs = RootFs{Target: "", Mounted: true}
+	s.state.CgroupPath = filepath.Join(testDir, "bad-cgroup")
+	_, err = s.CreateContainer(contConfig)
+	assert.NotNil(t, err, "Should fail to create container due to wrong cgroup")
+	ret := store.VCContainerStoreExists(s.ctx, testSandboxID, contID)
+	assert.False(t, ret, "Should delete configuration root after failed to create a container")
+}
+
+func TestDeleteStoreWhenNewContainerFail(t *testing.T) {
+	hConfig := newHypervisorConfig(nil, nil)
+	p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NoopAgentType, NetworkConfig{}, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cleanUp()
+
+	contID := "999"
+	contConfig := newTestContainerConfigNoop(contID)
+	contConfig.DeviceInfos = []config.DeviceInfo{
+		{
+			ContainerPath: "",
+			DevType:       "",
+		},
+	}
+	_, err = newContainer(p, contConfig)
+	assert.NotNil(t, err, "New container with invalid device info should fail")
+	storePath := store.ContainerConfigurationRootPath(testSandboxID, contID)
+	_, err = os.Stat(storePath)
+	assert.NotNil(t, err, "Should delete configuration root after failed to create a container")
+}
+
 func TestMonitor(t *testing.T) {
 	s, err := testCreateSandbox(t, testSandboxID, MockHypervisor, newHypervisorConfig(nil, nil), NoopAgentType, NetworkConfig{}, nil, nil)
 	assert.Nil(t, err, "VirtContainers should not allow empty sandboxes")

diff --git a/virtcontainers/store/vc.go b/virtcontainers/store/vc.go
@@ -313,3 +313,9 @@ func VCSandboxStoreExists(ctx context.Context, sandboxID string) bool {
 	s := stores.findStore(SandboxConfigurationRoot(sandboxID))
 	return s != nil
 }
+
+// VCContainerStoreExists returns true if a container store already exists.
+func VCContainerStoreExists(ctx context.Context, sandboxID string, containerID string) bool {
+	s := stores.findStore(ContainerConfigurationRoot(sandboxID, containerID))
+	return s != nil
+}