Merge pull request #466 from xibz/stopvm-err

Fix leak of VMs

Author: xibz

Makefile

@@ -196,6 +196,10 @@ ROOTFS_SLOW_REBOOT_INSTALLPATH=$(FIRECRACKER_CONTAINERD_RUNTIME_DIR)/rootfs-slow
 $(ROOTFS_SLOW_REBOOT_INSTALLPATH): tools/image-builder/rootfs-slow-reboot.img $(FIRECRACKER_CONTAINERD_RUNTIME_DIR)
 	install -D -o root -g root -m400 $< $@
 
+ROOTFS_NO_AGENT_INSTALLPATH=$(FIRECRACKER_CONTAINERD_RUNTIME_DIR)/rootfs-no-agent.img
+$(ROOTFS_NO_AGENT_INSTALLPATH): tools/image-builder/rootfs-no-agent.img $(FIRECRACKER_CONTAINERD_RUNTIME_DIR)
+	install -D -o root -g root -m400 $< $@
+
 .PHONY: default-vmlinux
 default-vmlinux: $(DEFAULT_VMLINUX_NAME)
 
@@ -209,7 +213,7 @@ install-default-rootfs: $(DEFAULT_ROOTFS_INSTALLPATH)
 install-default-runc-jailer-config: $(DEFAULT_RUNC_JAILER_CONFIG_INSTALLPATH)
 
 .PHONY: install-test-rootfs
-install-test-rootfs: $(ROOTFS_SLOW_BOOT_INSTALLPATH) $(ROOTFS_SLOW_REBOOT_INSTALLPATH)
+install-test-rootfs: $(ROOTFS_SLOW_BOOT_INSTALLPATH) $(ROOTFS_SLOW_REBOOT_INSTALLPATH) $(ROOTFS_NO_AGENT_INSTALLPATH)
 
 ##########################
 # CNI Network

runtime/service.go

@@ -465,9 +465,7 @@ func (s *service) CreateVM(requestCtx context.Context, request *proto.CreateVMRe
 	// If we failed to create the VM, we have no point in existing anymore, so shutdown
 	if err != nil {
 		s.shimCancel()
-
 		s.logger.WithError(err).Error("failed to create VM")
-
 		if errors.Cause(err) == context.DeadlineExceeded {
 			return nil, status.Errorf(codes.DeadlineExceeded, "VM %q didn't start within %s: %s", request.VMID, timeout, err)
 		}
@@ -481,7 +479,6 @@ func (s *service) CreateVM(requestCtx context.Context, request *proto.CreateVMRe
 	}
 
 	go s.monitorVMExit()
-
 	// let all the other methods know that the VM is ready for tasks
 	close(s.vmReady)
 
@@ -528,6 +525,15 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
 		return errors.Wrap(err, "failed to create jailer")
 	}
 
+	defer func() {
+		// in the event of an error, we should stop the VM
+		if err != nil {
+			if e := s.jailer.Stop(true); e != nil {
+				s.logger.WithError(e).Debug("failed to stop firecracker")
+			}
+		}
+	}()
+
 	s.machineConfig, err = s.buildVMConfiguration(request)
 	if err != nil {
 		return errors.Wrapf(err, "failed to build VM configuration")

runtime/service_integ_test.go

@@ -1909,7 +1909,6 @@ func requireNonEmptyFifo(t testing.TB, path string) {
 
 func TestCreateVM_Isolated(t *testing.T) {
 	prepareIntegTest(t)
-
 	client, err := containerd.New(containerdSockPath, containerd.WithDefaultRuntime(firecrackerRuntime))
 	require.NoError(t, err, "unable to create client to containerd service at %s, is containerd running?", containerdSockPath)
 	defer client.Close()
@@ -1921,17 +1920,38 @@ func TestCreateVM_Isolated(t *testing.T) {
 
 	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
 
-	subtests := []struct {
+	type subtest struct {
 		name     string
 		request  proto.CreateVMRequest
 		validate func(*testing.T, error)
-	}{
+		stopVM   bool
+	}
+
+	subtests := []subtest{
 		{
 			name:    "Happy Case",
 			request: proto.CreateVMRequest{},
 			validate: func(t *testing.T, err error) {
 				require.NoError(t, err)
 			},
+			stopVM: true,
+		},
+		{
+			name: "Error Case",
+			request: proto.CreateVMRequest{
+				TimeoutSeconds: 5,
+				RootDrive: &proto.FirecrackerRootDrive{
+					HostPath: "/var/lib/firecracker-containerd/runtime/rootfs-no-agent.img",
+				},
+			},
+			validate: func(t *testing.T, err error) {
+				require.NotNil(t, err, "expected an error but did not receive any")
+				time.Sleep(5 * time.Second)
+				firecrackerProcesses, err := findProcess(ctx, findFirecracker)
+				require.NoError(t, err, "failed waiting for expected firecracker process %q to come up", firecrackerProcessName)
+				require.Len(t, firecrackerProcesses, 0, "expected only no firecracker processes to exist")
+			},
+			stopVM: false,
 		},
 		{
 			name: "Slow Root FS",
@@ -1945,6 +1965,7 @@ func TestCreateVM_Isolated(t *testing.T) {
 				assert.Equal(t, codes.DeadlineExceeded, status.Code(err))
 				assert.Contains(t, err.Error(), "didn't start within 20s")
 			},
+			stopVM: true,
 		},
 		{
 			name: "Slow Root FS and Longer Timeout",
@@ -1957,10 +1978,11 @@ func TestCreateVM_Isolated(t *testing.T) {
 			validate: func(t *testing.T, err error) {
 				require.NoError(t, err)
 			},
+			stopVM: true,
 		},
 	}
 
-	runTest := func(t *testing.T, request proto.CreateVMRequest, validate func(t *testing.T, err error)) {
+	runTest := func(t *testing.T, request proto.CreateVMRequest, s subtest) {
 		vmID := testNameToVMID(t.Name())
 
 		tempDir, err := ioutil.TempDir("", vmID)
@@ -1981,34 +2003,31 @@ func TestCreateVM_Isolated(t *testing.T) {
 		requireNonEmptyFifo(t, metricsFile)
 
 		// Some test cases are expected to have an error, some are not.
-		validate(t, createVMErr)
-
-		// No VM to stop.
-		if createVMErr != nil {
-			return
-		}
+		s.validate(t, createVMErr)
 
-		// Ensure the response fields are populated correctly
-		assert.Equal(t, request.VMID, resp.VMID)
+		if createVMErr == nil && s.stopVM {
+			// Ensure the response fields are populated correctly
+			assert.Equal(t, request.VMID, resp.VMID)
 
-		_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: request.VMID})
-		require.Equal(t, status.Code(err), codes.OK)
+			_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: request.VMID})
+			require.Equal(t, status.Code(err), codes.OK)
+		}
 	}
 
-	for _, subtest := range subtests {
-		request := subtest.request
-		validate := subtest.validate
-		t.Run(subtest.name, func(t *testing.T) {
-			runTest(t, request, validate)
+	for _, _s := range subtests {
+		s := _s
+		request := s.request
+		t.Run(s.name, func(t *testing.T) {
+			runTest(t, request, s)
 		})
 
-		requestWithJailer := subtest.request
+		requestWithJailer := s.request
 		requestWithJailer.JailerConfig = &proto.JailerConfig{
 			UID: 30000,
 			GID: 30000,
 		}
-		t.Run(subtest.name+"/Jailer", func(t *testing.T) {
-			runTest(t, requestWithJailer, validate)
+		t.Run(s.name+"/Jailer", func(t *testing.T) {
+			runTest(t, requestWithJailer, s)
 		})
 	}
 }

tools/image-builder/Makefile

@@ -37,7 +37,7 @@ fi
 touch --reference=debootstrap_stamp --no-create "$(WORKDIR)"
 endef
 
-all: rootfs.img rootfs-slow-boot.img rootfs-slow-reboot.img
+all: rootfs.img rootfs-slow-boot.img rootfs-slow-reboot.img rootfs-no-agent.img
 
 $(WORKDIR):
 	mkdir $(WORKDIR)
@@ -81,6 +81,13 @@ rootfs-slow-reboot.img: files_common_stamp files_debootstrap_stamp files_ephemer
 	echo 'ExecStop=/bin/sleep 60' >> tmp/$@/etc/systemd/system/firecracker-agent.service
 	mksquashfs tmp/$@ $@ -noappend
 
+rootfs-no-agent.img: files_common_stamp files_debootstrap_stamp files_ephemeral_stamp
+	rm -fr tmp/$@
+	cp -a "$(WORKDIR)" tmp/$@
+	rm tmp/$@/etc/systemd/system/firecracker-agent.service
+	rm tmp/$@/usr/local/bin/agent
+	mksquashfs tmp/$@ $@ -noappend
+
 builder: builder_stamp
 
 builder_stamp: