virtcontainers: Fix Store related vm factory leak

We are creating Store directories but never removing them. Calling into a VM factory created vm Stop() will now clean the VM Store artifacts up. Signed-off-by: Samuel Ortiz <[email protected]>
kata-containers · Feb 6, 2019 · bb99e41 · bb99e41
1 parent 7b0376f
commit bb99e41
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 23 deletions.
diff --git a/virtcontainers/factory/cache/cache_test.go b/virtcontainers/factory/cache/cache_test.go
@@ -40,7 +40,10 @@ func TestTemplateFactory(t *testing.T) {
 	assert.Equal(f.Config(), vmConfig)
 
 	// GetBaseVM
-	_, err := f.GetBaseVM(ctx, vmConfig)
+	vm, err := f.GetBaseVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// CloseFactory

diff --git a/virtcontainers/factory/direct/direct_test.go b/virtcontainers/factory/direct/direct_test.go
@@ -39,7 +39,10 @@ func TestTemplateFactory(t *testing.T) {
 	assert.Equal(f.Config(), vmConfig)
 
 	// GetBaseVM
-	_, err := f.GetBaseVM(ctx, vmConfig)
+	vm, err := f.GetBaseVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// CloseFactory

diff --git a/virtcontainers/factory/factory_test.go b/virtcontainers/factory/factory_test.go
@@ -190,7 +190,10 @@ func TestFactoryGetVM(t *testing.T) {
 	f, err := NewFactory(ctx, Config{VMConfig: vmConfig}, false)
 	assert.Nil(err)
 
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err := f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	f.CloseFactory(ctx)
@@ -199,7 +202,10 @@ func TestFactoryGetVM(t *testing.T) {
 	f, err = NewFactory(ctx, Config{Template: true, VMConfig: vmConfig}, false)
 	assert.Nil(err)
 
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	f.CloseFactory(ctx)
@@ -211,7 +217,10 @@ func TestFactoryGetVM(t *testing.T) {
 	_, err = NewFactory(ctx, Config{Template: true, VMConfig: vmConfig}, true)
 	assert.Error(err)
 
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	f.CloseFactory(ctx)
@@ -220,7 +229,10 @@ func TestFactoryGetVM(t *testing.T) {
 	f, err = NewFactory(ctx, Config{Cache: 2, VMConfig: vmConfig}, false)
 	assert.Nil(err)
 
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	f.CloseFactory(ctx)
@@ -229,22 +241,34 @@ func TestFactoryGetVM(t *testing.T) {
 	f, err = NewFactory(ctx, Config{Template: true, Cache: 2, VMConfig: vmConfig}, false)
 	assert.Nil(err)
 
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// CPU hotplug
 	vmConfig.HypervisorConfig.NumVCPUs++
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// Memory hotplug
 	vmConfig.HypervisorConfig.MemorySize += 128
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// checkConfig fall back
 	vmConfig.HypervisorConfig.Mlock = true
-	_, err = f.GetVM(ctx, vmConfig)
+	vm, err = f.GetVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	f.CloseFactory(ctx)

diff --git a/virtcontainers/factory/template/template_test.go b/virtcontainers/factory/template/template_test.go
@@ -46,7 +46,10 @@ func TestTemplateFactory(t *testing.T) {
 	assert.Equal(f.Config(), vmConfig)
 
 	// GetBaseVM
-	_, err = f.GetBaseVM(ctx, vmConfig)
+	vm, err := f.GetBaseVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// Fetch
@@ -74,19 +77,31 @@ func TestTemplateFactory(t *testing.T) {
 	assert.Error(err)
 
 	templateProxyType = vc.NoopProxyType
-	_, err = tt.GetBaseVM(ctx, vmConfig)
+	vm, err = tt.GetBaseVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
+	assert.Nil(err)
+
+	vm, err = f.GetBaseVM(ctx, vmConfig)
 	assert.Nil(err)
 
-	_, err = f.GetBaseVM(ctx, vmConfig)
+	err = vm.Stop()
 	assert.Nil(err)
 
 	err = tt.createTemplateVM(ctx)
 	assert.Nil(err)
 
-	_, err = tt.GetBaseVM(ctx, vmConfig)
+	vm, err = tt.GetBaseVM(ctx, vmConfig)
+	assert.Nil(err)
+
+	err = vm.Stop()
+	assert.Nil(err)
+
+	vm, err = f.GetBaseVM(ctx, vmConfig)
 	assert.Nil(err)
 
-	_, err = f.GetBaseVM(ctx, vmConfig)
+	err = vm.Stop()
 	assert.Nil(err)
 
 	// CloseFactory

diff --git a/virtcontainers/vm.go b/virtcontainers/vm.go
@@ -31,6 +31,8 @@ type VM struct {
 	memory uint32
 
 	cpuDelta uint32
+
+	store *store.VCStore
 }
 
 // VMConfig is a collection of all info that a new blackbox VM needs.
@@ -113,19 +115,21 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) {
 
 	virtLog.WithField("vm", id).WithField("config", config).Info("create new vm")
 
-	defer func() {
-		if err != nil {
-			virtLog.WithField("vm", id).WithError(err).Error("failed to create new vm")
-		}
-	}()
-
 	vcStore, err := store.NewVCStore(ctx,
 		store.SandboxConfigurationRoot(id),
 		store.SandboxRuntimeRoot(id))
 	if err != nil {
 		return nil, err
 	}
 
+	defer func() {
+		if err != nil {
+			virtLog.WithField("vm", id).WithError(err).Error("failed to create new vm")
+			virtLog.WithField("vm", id).Errorf("Deleting store for %s", id)
+			vcStore.Delete()
+		}
+	}()
+
 	if err = hypervisor.createSandbox(ctx, id, &config.HypervisorConfig, vcStore); err != nil {
 		return nil, err
 	}
@@ -184,6 +188,7 @@ func NewVM(ctx context.Context, config VMConfig) (*VM, error) {
 		proxyURL:   url,
 		cpu:        config.HypervisorConfig.NumVCPUs,
 		memory:     config.HypervisorConfig.MemorySize,
+		store:      vcStore,
 	}, nil
 }
 
@@ -235,9 +240,13 @@ func (v *VM) Disconnect() error {
 
 // Stop stops a VM process.
 func (v *VM) Stop() error {
-	v.logger().Info("kill vm")
+	v.logger().Info("stop vm")
+
+	if err := v.hypervisor.stopSandbox(); err != nil {
+		return err
+	}
 
-	return v.hypervisor.stopSandbox()
+	return v.store.Delete()
 }
 
 // AddCPUs adds num of CPUs to the VM.