Repository: firecracker-microvm/firecracker-containerd (go)

Issue:
Support using a stub drive of a deleted container
As part of #230 @kzys noticed that if a container is deleted, the stub drive for it cannot be re-used. This results in situations like the following:

1. Call CreateVM with ContainerCount set to 2
2. Start container A
3. Start container B, let it exit and then Delete it.
3. Try to start container C, it will fail due to being out of stub drives

In theory, once container B is deleted, the stub drive is no longer needed and should be able to be used by container C. The implementation would need an update to allow this. To be clear, this is just re-use of stub-drives within the context of a single VM (not across different VMs, which creates a bunch of other concurrency related issues).


Pull request description:
Make sure the error message regarding stub drives is clear

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

*Issue #, if available:*

Related, but doesn't fix #243

*Description of changes:*

While we don't support the use-case right now, the error message
regarding the lack of free stub drives should be clear for customers.

This change also cleans up drive_handler.go.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/go.mod b/go.mod
index 2fbeb6f..0abf85c 100644
--- a/go.mod
+++ b/go.mod
@@ -1,7 +1,7 @@
 module github.com/firecracker-microvm/firecracker-containerd
 
 require (
-	github.com/Microsoft/go-winio v0.4.14 // indirect
+	github.com/Microsoft/go-winio v0.4.12 // indirect
 	github.com/Microsoft/hcsshim v0.8.1 // indirect
 	github.com/StackExchange/wmi v0.0.0-20181212234831-e0a55b97c705 // indirect
 	github.com/containerd/cgroups v0.0.0-20181105182409-82cb49fc1779 // indirect
@@ -17,7 +17,7 @@ require (
 	github.com/docker/go-events v0.0.0-20170721190031-9461782956ad // indirect
 	github.com/docker/go-metrics v0.0.0-20181218153428-b84716841b82 // indirect
 	github.com/docker/go-units v0.3.3
-	github.com/firecracker-microvm/firecracker-go-sdk v0.17.0
+	github.com/firecracker-microvm/firecracker-go-sdk v0.15.2-0.20190627223500-b2e8284e890c
 	github.com/go-ole/go-ole v1.2.4 // indirect
 	github.com/godbus/dbus v0.0.0-20181025153459-66d97aec3384 // indirect
 	github.com/gofrs/uuid v3.2.0+incompatible
@@ -31,7 +31,7 @@ require (
 	github.com/opencontainers/image-spec v1.0.1 // indirect
 	github.com/opencontainers/runc v0.1.1 // indirect
 	github.com/opencontainers/runtime-spec v0.1.2-0.20181106065543-31e0d16c1cb7
-	github.com/pkg/errors v0.8.1
+	github.com/pkg/errors v0.8.0
 	github.com/prometheus/client_golang v0.9.2 // indirect
 	github.com/shirou/gopsutil v2.18.12+incompatible
 	github.com/shirou/w32 v0.0.0-20160930032740-bb4de0191aa4 // indirect
@@ -41,7 +41,7 @@ require (
 	github.com/urfave/cli v1.20.0 // indirect
 	go.etcd.io/bbolt v1.3.1-etcd.8
 	golang.org/x/sync v0.0.0-20181108010431-42b317875d0f
-	golang.org/x/sys v0.0.0-20190507160741-ecd444e8653b
+	golang.org/x/sys v0.0.0-20190329044733-9eb1bfa1ce65
 	google.golang.org/genproto v0.0.0-20181109154231-b5d43981345b // indirect
 	google.golang.org/grpc v1.21.0
 	gotest.tools v2.2.0+incompatible // indirect
diff --git a/go.sum b/go.sum
index df149d2..e9c7b3e 100644
--- a/go.sum
+++ b/go.sum
@@ -1,8 +1,8 @@
 cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
-github.com/Microsoft/go-winio v0.4.14 h1:+hMXMk01us9KgxGb7ftKQt2Xpf5hH/yky+TDA+qxleU=
-github.com/Microsoft/go-winio v0.4.14/go.mod h1:qXqCSQ3Xa7+6tgxaGTIe4Kpcdsi+P8jBhyzoq1bpyYA=
+github.com/Microsoft/go-winio v0.4.12 h1:xAfWHN1IrQ0NJ9TBC0KBZoqLjzDTr1ML+4MywiUOryc=
+github.com/Microsoft/go-winio v0.4.12/go.mod h1:VhR8bwka0BXejwEJY73c50VrPtXAaKcyvVC4A4RozmA=
 github.com/Microsoft/hcsshim v0.8.1 h1:0RKPd1pQB/4YRjdw0jFwq3A5nWFN4n1ojNzcm4B+8ZI=
 github.com/Microsoft/hcsshim v0.8.1/go.mod h1:Op3hHsoHPAvb6lceZHDtd9OkTew38wNoXnJs8iY7rUg=
 github.com/PuerkitoBio/purell v1.1.0 h1:rmGxhojJlM0tuKtfdvliR84CFHljx9ag64t2xmVkjK4=
@@ -44,8 +44,8 @@ github.com/docker/go-metrics v0.0.0-20181218153428-b84716841b82 h1:X0fj836zx99zF
 github.com/docker/go-metrics v0.0.0-20181218153428-b84716841b82/go.mod h1:/u0gXw0Gay3ceNrsHubL3BtdOL2fHf93USgMTe0W5dI=
 github.com/docker/go-units v0.3.3 h1:Xk8S3Xj5sLGlG5g67hJmYMmUgXv5N4PhkjJHHqrwnTk=
 github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
-github.com/firecracker-microvm/firecracker-go-sdk v0.17.0 h1:k0Nj15aYbuuJ3nP8yUDWvQKqAoQgxUXtw6go8RLFP4k=
-github.com/firecracker-microvm/firecracker-go-sdk v0.17.0/go.mod h1:QcNsz2gYkcTI/zAQl3dYeLwf7KxtjKnt7aGklhn9yYk=
+github.com/firecracker-microvm/firecracker-go-sdk v0.15.2-0.20190627223500-b2e8284e890c h1:jAAxKV5SD/y2nNKNLHgAUmfgbrvfN4xBiKR+b2qK2pM=
+github.com/firecracker-microvm/firecracker-go-sdk v0.15.2-0.20190627223500-b2e8284e890c/go.mod h1:QcNsz2gYkcTI/zAQl3dYeLwf7KxtjKnt7aGklhn9yYk=
 github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb h1:D4uzjWwKYQ5XnAvUbuvHW93esHg7F8N/OYeBBcJoTr0=
 github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q=
 github.com/go-ole/go-ole v1.2.4 h1:nNBDSCOigTSiarFpYE9J/KtEA1IOW4CNeqT9TQDqCxI=
@@ -124,8 +124,8 @@ github.com/opencontainers/runtime-spec v0.1.2-0.20181106065543-31e0d16c1cb7 h1:v
 github.com/opencontainers/runtime-spec v0.1.2-0.20181106065543-31e0d16c1cb7/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
 github.com/pborman/uuid v1.2.0 h1:J7Q5mO4ysT1dv8hyrUGHb9+ooztCXu1D8MY8DZYsu3g=
 github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k=
-github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
-github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
+github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw=
+github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/prometheus/client_golang v0.9.2 h1:awm861/B8OKDd2I/6o1dy3ra4BamzKhYOiGItCeZ740=
@@ -174,8 +174,6 @@ golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5h
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190329044733-9eb1bfa1ce65 h1:hOY+O8MxdkPV10pNf7/XEHaySCiPKxixMKUshfHsGn0=
 golang.org/x/sys v0.0.0-20190329044733-9eb1bfa1ce65/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
-golang.org/x/sys v0.0.0-20190507160741-ecd444e8653b h1:ag/x1USPSsqHud38I9BAC88qdNLDHHtQ4mlgQIZPPNA=
-golang.org/x/sys v0.0.0-20190507160741-ecd444e8653b/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
 golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
diff --git a/runtime/drive_handler.go b/runtime/drive_handler.go
index 6f9b563..552eac9 100644
--- a/runtime/drive_handler.go
+++ b/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
diff --git a/runtime/service.go b/runtime/service.go
index 7acd3ac..c538cf7 100644
--- a/runtime/service.go
+++ b/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()


Test patch:
diff --git a/runtime/drive_handler_test.go b/runtime/drive_handler_test.go
index a0bce80..e1e401f 100644
--- a/runtime/drive_handler_test.go
+++ b/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)
diff --git a/runtime/service_integ_test.go b/runtime/service_integ_test.go
index ed0d033..687dc8c 100644
--- a/runtime/service_integ_test.go
+++ b/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)
+}
diff --git a/runtime/service_test.go b/runtime/service_test.go
index 07789aa..7a71ad2 100644
--- a/runtime/service_test.go
+++ b/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)