diff --git a/virtcontainers/container.go b/virtcontainers/container.go index dc1fe9d37..14a37a420 100644 --- 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 index 2e4d5b862..575cb2483 100644 --- 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 index df2a16a3f..67c02c4a8 100644 --- 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 index eecaca2ea..10e729ef0 100644 --- 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 +}