Merge pull request #244 from kzys/patch-drive-2

Make sure the error message regarding stub drives is clear

Author: kzys

runtime/drive_handler.go

@@ -51,16 +51,41 @@ type stubDriveHandler struct {
 	mutex          sync.Mutex
 }
 
-func newStubDriveHandler(path string, logger *logrus.Entry) stubDriveHandler {
-	return stubDriveHandler{
+func newStubDriveHandler(path string, logger *logrus.Entry, count int) (*stubDriveHandler, error) {
+	h := stubDriveHandler{
 		RootPath: path,
 		logger:   logger,
 	}
+	drives, err := h.createStubDrives(count)
+	if err != nil {
+		return nil, err
+	}
+	h.drives = drives
+	return &h, nil
 }
 
-// StubDrivePaths will create stub drives and return the paths associated with
+func (h *stubDriveHandler) createStubDrives(stubDriveCount int) ([]models.Drive, error) {
+	paths, err := h.stubDrivePaths(stubDriveCount)
+	if err != nil {
+		return nil, err
+	}
+
+	stubDrives := make([]models.Drive, 0, stubDriveCount)
+	for i, path := range paths {
+		stubDrives = append(stubDrives, models.Drive{
+			DriveID:      firecracker.String(fmt.Sprintf("stub%d", i)),
+			IsReadOnly:   firecracker.Bool(false),
+			PathOnHost:   firecracker.String(path),
+			IsRootDevice: firecracker.Bool(false),
+		})
+	}
+
+	return stubDrives, nil
+}
+
+// stubDrivePaths will create stub drives and return the paths associated with
 // the stub drives.
-func (h *stubDriveHandler) StubDrivePaths(count int) ([]string, error) {
+func (h *stubDriveHandler) stubDrivePaths(count int) ([]string, error) {
 	paths := []string{}
 	for i := 0; i < count; i++ {
 		driveID := fmt.Sprintf("stub%d", i)
@@ -125,13 +150,6 @@ func (h *stubDriveHandler) createStubDrive(driveID, path string) error {
 	return nil
 }
 
-// SetDrives will set the given drives and the offset to which the stub drives
-// start.
-func (h *stubDriveHandler) SetDrives(offset int64, d []models.Drive) {
-	h.drives = d
-	h.stubDriveIndex = offset
-}
-
 // GetDrives returns the associated stub drives
 func (h *stubDriveHandler) GetDrives() []models.Drive {
 	return h.drives

runtime/drive_handler_test.go

@@ -40,10 +40,9 @@ func TestStubDriveHandler(t *testing.T) {
 	}()
 
 	logger := log.G(context.Background())
-	handler := newStubDriveHandler(tempPath, logger)
-	paths, err := handler.StubDrivePaths(5)
+	handler, err := newStubDriveHandler(tempPath, logger, 5)
 	assert.NoError(t, err)
-	assert.Equal(t, 5, len(paths))
+	assert.Equal(t, 5, len(handler.GetDrives()))
 
 	infos, err := ioutil.ReadDir(tempPath)
 	assert.NoError(t, err)
@@ -236,9 +235,11 @@ func TestCreateStubDrive(t *testing.T) {
 		c := c // see https://github.com/kyoh86/scopelint/issues/4
 		t.Run(c.Name, func(t *testing.T) {
 			logger := log.G(context.Background())
-			handler := newStubDriveHandler(path, logger)
+			handler, err := newStubDriveHandler(path, logger, 0)
+			assert.NoError(t, err)
+
 			stubDrivePath := filepath.Join(path, c.Name)
-			err := handler.createStubDrive(c.DriveID, stubDrivePath)
+			err = handler.createStubDrive(c.DriveID, stubDrivePath)
 			assert.Equal(t, c.ExpectedError, err != nil, "invalid error: %v", err)
 
 			info, err := os.Stat(stubDrivePath)

runtime/service.go

@@ -15,7 +15,6 @@ package main
 
 import (
 	"context"
-	"fmt"
 	"math"
 	"net"
 	"os"
@@ -43,7 +42,6 @@ import (
 	"github.com/containerd/fifo"
 	"github.com/containerd/ttrpc"
 	"github.com/firecracker-microvm/firecracker-go-sdk"
-	models "github.com/firecracker-microvm/firecracker-go-sdk/client/models"
 	"github.com/gofrs/uuid"
 	ptypes "github.com/gogo/protobuf/types"
 	"github.com/golang/protobuf/ptypes/empty"
@@ -124,7 +122,7 @@ type service struct {
 	vmStartOnce              sync.Once
 	agentClient              taskAPI.TaskService
 	eventBridgeClient        eventbridge.Getter
-	stubDriveHandler         stubDriveHandler
+	stubDriveHandler         *stubDriveHandler
 	exitAfterAllTasksDeleted bool // exit the VM and shim when all tasks are deleted
 
 	machine          *firecracker.Machine
@@ -198,7 +196,6 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish
 		vmReady: make(chan struct{}),
 	}
 
-	s.stubDriveHandler = newStubDriveHandler(s.shimDir.RootPath(), logger)
 	s.startEventForwarders(remotePublisher)
 
 	err = s.serveFCControl()
@@ -588,25 +585,6 @@ func (s *service) SetVMMetadata(requestCtx context.Context, request *proto.SetVM
 	return &empty.Empty{}, nil
 }
 
-func (s *service) createStubDrives(stubDriveCount int) ([]models.Drive, error) {
-	paths, err := s.stubDriveHandler.StubDrivePaths(stubDriveCount)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to retrieve stub drive paths")
-	}
-
-	stubDrives := make([]models.Drive, 0, stubDriveCount)
-	for i, path := range paths {
-		stubDrives = append(stubDrives, models.Drive{
-			DriveID:      firecracker.String(fmt.Sprintf("stub%d", i)),
-			IsReadOnly:   firecracker.Bool(false),
-			PathOnHost:   firecracker.String(path),
-			IsRootDevice: firecracker.Bool(false),
-		})
-	}
-
-	return stubDrives, nil
-}
-
 func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker.Config, error) {
 	logger := s.logger.WithField("cid", s.machineCID)
 
@@ -646,11 +624,11 @@ func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker
 	}
 
 	// Create stub drives first and let stub driver handler manage the drives
-	stubDrives, err := s.createStubDrives(containerCount)
+	handler, err := newStubDriveHandler(s.shimDir.RootPath(), logger, containerCount)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create stub drives")
 	}
-	s.stubDriveHandler.SetDrives(0, stubDrives)
+	s.stubDriveHandler = handler
 
 	var driveBuilder firecracker.DrivesBuilder
 	// Create non-stub drives
@@ -665,8 +643,7 @@ func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker
 	}
 
 	// a micro VM must know all drives
-	// nolint: gocritic
-	cfg.Drives = append(stubDrives, driveBuilder.Build()...)
+	cfg.Drives = append(handler.GetDrives(), driveBuilder.Build()...)
 
 	// Setup network interfaces
 
@@ -715,9 +692,11 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
 	for _, mnt := range request.Rootfs {
 		driveID, err = s.stubDriveHandler.PatchStubDrive(requestCtx, s.machine, mnt.Source)
 		if err != nil {
+			if err == ErrDrivesExhausted {
+				return nil, errors.Wrapf(errdefs.ErrUnavailable, "no remaining stub drives to be used")
+			}
 			return nil, errors.Wrapf(err, "failed to patch stub drive")
 		}
-
 	}
 
 	ociConfigBytes, err := bundleDir.OCIConfig().Bytes()

runtime/service_integ_test.go

@@ -40,6 +40,7 @@ import (
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/shirou/gopsutil/process"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	_ "github.com/firecracker-microvm/firecracker-containerd/firecracker-control"
@@ -676,3 +677,54 @@ func TestCreateContainerWithSameName_Isolated(t *testing.T) {
 	vmID := fmt.Sprintf("same-vm-%d", time.Now().UnixNano())
 	testCreateContainerWithSameName(t, vmID)
 }
+
+func TestCreateTooManyContainers_Isolated(t *testing.T) {
+	internal.RequiresIsolation(t)
+	assert := assert.New(t)
+
+	ctx := namespaces.WithNamespace(context.Background(), "default")
+
+	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()
+
+	image, err := client.Pull(ctx, guestDockerImage, containerd.WithPullUnpack, containerd.WithPullSnapshotter(naiveSnapshotterName))
+	require.NoError(t, err, "failed to pull image %s, is the the %s snapshotter running?", guestDockerImage, naiveSnapshotterName)
+
+	runEchoHello := containerd.WithNewSpec(oci.WithProcessArgs("echo", "-n", "hello"), firecrackeroci.WithVMID("reuse-same-vm"))
+
+	c1, err := client.NewContainer(ctx,
+		"c1",
+		containerd.WithSnapshotter(naiveSnapshotterName),
+		containerd.WithNewSnapshot("c1", image),
+		runEchoHello,
+	)
+	assert.Equal("hello", startAndWaitTask(ctx, t, c1))
+	require.NoError(t, err, "failed to create a container")
+
+	defer func() {
+		err = c1.Delete(ctx, containerd.WithSnapshotCleanup)
+		require.NoError(t, err, "failed to delete a container")
+	}()
+
+	c2, err := client.NewContainer(ctx,
+		"c2",
+		containerd.WithSnapshotter(naiveSnapshotterName),
+		containerd.WithNewSnapshot("c2", image),
+		runEchoHello,
+	)
+	require.NoError(t, err, "failed to create a container")
+
+	defer func() {
+		err := c2.Delete(ctx, containerd.WithSnapshotCleanup)
+		require.NoError(t, err, "failed to delete a container")
+	}()
+
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+
+	// When we reuse a VM explicitly, we cannot start multiple containers unless we pre-allocate stub drives.
+	_, err = c2.NewTask(ctx, cio.NewCreator(cio.WithStreams(nil, &stdout, &stderr)))
+	assert.Equal("no remaining stub drives to be used: unavailable: unknown", err.Error())
+	require.Error(t, err)
+}

runtime/service_test.go

@@ -23,6 +23,7 @@ import (
 	"testing"
 
 	"github.com/firecracker-microvm/firecracker-containerd/internal"
+	"github.com/firecracker-microvm/firecracker-containerd/internal/vm"
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
 	"github.com/firecracker-microvm/firecracker-go-sdk"
 	models "github.com/firecracker-microvm/firecracker-go-sdk/client/models"
@@ -216,7 +217,7 @@ func TestBuildVMConfiguration(t *testing.T) {
 			assert.NoError(t, err)
 			defer os.RemoveAll(tempDir)
 
-			svc.stubDriveHandler = newStubDriveHandler(tempDir, svc.logger)
+			svc.shimDir = vm.Dir(tempDir)
 			// For values that remain constant between tests, they are written here
 			tc.expectedCfg.SocketPath = svc.shimDir.FirecrackerSockPath()
 			tc.expectedCfg.VsockDevices = []firecracker.VsockDevice{{Path: "root", CID: svc.machineCID}}
@@ -293,7 +294,7 @@ func TestDebugConfig(t *testing.T) {
 		stubDrivePath := filepath.Join(path, fmt.Sprintf("%d", i))
 		err := os.MkdirAll(stubDrivePath, os.ModePerm)
 		assert.NoError(t, err, "failed to create stub drive path")
-		c.service.stubDriveHandler = newStubDriveHandler(stubDrivePath, c.service.logger)
+		c.service.shimDir = vm.Dir(stubDrivePath)
 		req := proto.CreateVMRequest{}
 
 		cfg, err := c.service.buildVMConfiguration(&req)