Merge pull request #348 from kzys/stopvm-timeout

Returns codes.DeadlineExceeded when we forcebily kill a VM

Author: kzys

Makefile

@@ -183,6 +183,10 @@ DEFAULT_RUNC_JAILER_CONFIG_INSTALLPATH?=/etc/containerd/firecracker-runc-config.
 $(DEFAULT_RUNC_JAILER_CONFIG_INSTALLPATH): $(ETC_CONTAINERD) runtime/firecracker-runc-config.json.example
 	install -D -o root -g root -m400 runtime/firecracker-runc-config.json.example $@
 
+ROOTFS_SLOW_REBOOT_INSTALLPATH=$(FIRECRACKER_CONTAINERD_RUNTIME_DIR)/rootfs-slow-reboot.img
+$(ROOTFS_SLOW_REBOOT_INSTALLPATH): tools/image-builder/rootfs-slow-reboot.img $(FIRECRACKER_CONTAINERD_RUNTIME_DIR)
+	install -D -o root -g root -m400 $< $@
+
 .PHONY: default-vmlinux
 default-vmlinux: $(DEFAULT_VMLINUX_NAME)
 
@@ -195,6 +199,8 @@ install-default-rootfs: $(DEFAULT_ROOTFS_INSTALLPATH)
 .PHONY: install-default-runc-jailer-config
 install-default-runc-jailer-config: $(DEFAULT_RUNC_JAILER_CONFIG_INSTALLPATH)
 
+.PHONY: install-test-rootfs
+install-test-rootfs: $(ROOTFS_SLOW_REBOOT_INSTALLPATH)
 
 ##########################
 # CNI Network

agent/main.go

@@ -108,7 +108,12 @@ func main() {
 	}
 
 	group.Go(func() error {
-		return server.Serve(shimCtx, listener)
+		err := server.Serve(shimCtx, listener)
+		if err == ttrpc.ErrServerClosed {
+			// Calling server.Shutdown() from another goroutine will cause ErrServerClosed, which is fine.
+			return nil
+		}
+		return err
 	})
 
 	group.Go(func() error {

firecracker-control/local.go

@@ -218,7 +218,6 @@ func (s *local) StopVM(requestCtx context.Context, req *proto.StopVMRequest) (*e
 
 	resp, err := client.StopVM(requestCtx, req)
 	if err != nil {
-		err = errors.Wrap(err, "shim client failed to stop VM")
 		s.logger.WithError(err).Error()
 		return nil, err
 	}

runtime/service.go

@@ -1186,6 +1186,7 @@ func (s *service) shutdown(requestCtx, shutdownCtx context.Context, req *taskAPI
 		}
 		s.logger.WithError(shutdownErr).Error("the VM returns unknown error")
 	case <-shutdownCtx.Done():
+		shutdownErr = status.Errorf(codes.DeadlineExceeded, "the VM %q will be killed forcebily", req.ID)
 		s.logger.Error("the VM hasn't been stopped before the context's deadline")
 	}
 

runtime/service_integ_test.go

@@ -50,6 +50,9 @@ import (
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
 	fccontrol "github.com/firecracker-microvm/firecracker-containerd/proto/service/fccontrol/ttrpc"
 	"github.com/firecracker-microvm/firecracker-containerd/runtime/firecrackeroci"
+
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 const (
@@ -1303,32 +1306,63 @@ func TestStopVM_Isolated(t *testing.T) {
 	pluginClient, err := ttrpcutil.NewClient(containerdSockPath + ".ttrpc")
 	require.NoError(err, "failed to create ttrpc client")
 
-	vmID := testNameToVMID(t.Name())
+	tests := []struct {
+		name            string
+		createVMRequest proto.CreateVMRequest
+		errorCode       codes.Code
+	}{
+		{
+			name:            "Successful",
+			createVMRequest: proto.CreateVMRequest{},
+			errorCode:       codes.OK,
+		},
 
-	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
-	_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{VMID: vmID})
-	require.NoError(err)
+		// Firecracker is too fast to test a case where we hit the timeout on a StopVMRequest.
+		// The rootfs below explicitly sleeps 60 seconds after shutting down the agent to simulate the case.
+		{
+			name: "Timeout",
+			createVMRequest: proto.CreateVMRequest{
+				RootDrive: &proto.FirecrackerRootDrive{
+					HostPath: "/var/lib/firecracker-containerd/runtime/rootfs-slow-reboot.img",
+				},
+			},
+			errorCode: codes.DeadlineExceeded,
+		},
+	}
 
-	c, err := client.NewContainer(ctx,
-		"container",
-		containerd.WithSnapshotter(defaultSnapshotterName()),
-		containerd.WithNewSnapshot("snapshot", image),
-		containerd.WithNewSpec(oci.WithProcessArgs("/bin/echo", "-n", "hello"), firecrackeroci.WithVMID(vmID)),
-	)
-	require.NoError(err)
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			vmID := testNameToVMID(t.Name())
+			createVMRequest := test.createVMRequest
+			createVMRequest.VMID = vmID
 
-	stdout := startAndWaitTask(ctx, t, c)
-	require.Equal("hello", stdout)
+			fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
+			_, err = fcClient.CreateVM(ctx, &createVMRequest)
+			require.NoError(err)
 
-	shimProcesses, err := internal.WaitForProcessToExist(ctx, time.Second, findShim)
-	require.NoError(err, "failed waiting for expected shim process %q to come up", shimProcessName)
-	require.Len(shimProcesses, 1, "expected only one shim process to exist")
+			c, err := client.NewContainer(ctx,
+				"container-"+vmID,
+				containerd.WithSnapshotter(defaultSnapshotterName()),
+				containerd.WithNewSnapshot("snapshot-"+vmID, image),
+				containerd.WithNewSpec(oci.WithProcessArgs("/bin/echo", "-n", "hello"), firecrackeroci.WithVMID(vmID)),
+			)
+			require.NoError(err)
 
-	_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})
-	require.NoError(err)
+			stdout := startAndWaitTask(ctx, t, c)
+			require.Equal("hello", stdout)
 
-	err = internal.WaitForPidToExit(ctx, time.Second, shimProcesses[0].Pid)
-	require.NoError(err, "shim hasn't been terminated")
+			shimProcesses, err := internal.WaitForProcessToExist(ctx, time.Second, findShim)
+			require.NoError(err, "failed waiting for expected shim process %q to come up", shimProcessName)
+			require.Len(shimProcesses, 1, "expected only one shim process to exist")
+
+			_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})
+			require.Equal(status.Code(err), test.errorCode)
+
+			err = internal.WaitForPidToExit(ctx, time.Second, shimProcesses[0].Pid)
+			require.NoError(err, "shim hasn't been terminated")
+		})
+	}
 }
 
 func findShim(ctx context.Context, p *process.Process) (bool, error) {

tools/docker/Dockerfile.integ-test

@@ -51,6 +51,7 @@ RUN --mount=type=bind,target=/src make -C /src \
   install-default-rootfs \
   install-default-runc-jailer-config \
   install-test-cni-bins \
+  install-test-rootfs \
   demo-network
 
 COPY tools/docker/entrypoint.sh /entrypoint

tools/image-builder/Makefile

@@ -12,7 +12,7 @@
 # permissions and limitations under the License.
 
 UID        := $(shell id -u)
-WORKDIR    := rootfs
+WORKDIR    := tmp/rootfs
 WORKDIRLOC := $(shell readlink -f $(WORKDIR))
 IMAGE_DIRS := /dev /bin /etc /etc/init.d /tmp /var /run /proc /sys /container/rootfs /agent /rom /overlay
 DIRS       := $(foreach dir,$(IMAGE_DIRS),"$(WORKDIR)$(dir)")
@@ -37,7 +37,7 @@ fi
 touch --reference=debootstrap_stamp --no-create "$(WORKDIR)"
 endef
 
-all: rootfs.img
+all: rootfs.img rootfs-slow-reboot.img
 
 $(WORKDIR):
 	mkdir $(WORKDIR)
@@ -67,6 +67,13 @@ endif
 rootfs.img: files_common_stamp files_debootstrap_stamp files_ephemeral_stamp
 	mksquashfs "$(WORKDIR)" rootfs.img -noappend
 
+# Intentionally break the rootfs to simulate the case where StopVM is taking longer than the minimal timeout
+rootfs-slow-reboot.img: files_common_stamp files_debootstrap_stamp files_ephemeral_stamp
+	rm -fr tmp/$@
+	cp -a "$(WORKDIR)" tmp/$@
+	echo 'ExecStop=/bin/sleep 60' >> tmp/$@/etc/systemd/system/firecracker-agent.service
+	mksquashfs tmp/$@ $@ -noappend
+
 builder: builder_stamp
 
 builder_stamp:
@@ -81,21 +88,20 @@ builder_stamp:
 %-in-docker: builder_stamp
 	docker run --rm \
 		--security-opt=apparmor=unconfined \
-		-it --volume $(CURDIR):/src \
+		-it \
+		--volume $(CURDIR):/src \
+		--volume /src/tmp \
 		--cap-add=sys_admin \
 		--cap-add=sys_chroot \
 		--env=DEBMIRROR \
 		fc-image-builder:$(DOCKER_IMAGE_TAG) $(subst -in-docker,,$@)
 
 clean:
 	-rm -f *stamp
-	if [ -d $(WORKDIR) ] || [ -e rootfs.img ]; then \
-	  if [ $(UID) -eq 0 ]; then \
-	    rm -rf $(WORKDIR) \
-	    rm -f rootfs.img ;\
-	   else \
-	     $(MAKE) clean-in-docker ;\
-	   fi ; \
+	if [ $(UID) -eq 0 ]; then \
+	  rm -f rootfs.img rootfs-slow-reboot.img ;\
+	else \
+	  $(MAKE) clean-in-docker ;\
 	fi
 
 distclean: clean