From 7b5edc7e79e94966748356bf75d1a2c01a779824 Mon Sep 17 00:00:00 2001 From: Kazuyoshi Kato Date: Tue, 6 Aug 2019 17:48:16 -0700 Subject: [PATCH 1/3] Make sure the error message regarding stub drives is clear 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. Signed-off-by: Kazuyoshi Kato --- runtime/drive_handler.go | 4 +-- runtime/service.go | 2 +- runtime/service_integ_test.go | 52 +++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/runtime/drive_handler.go b/runtime/drive_handler.go index 6f9b563dc..c58c94aac 100644 --- a/runtime/drive_handler.go +++ b/runtime/drive_handler.go @@ -127,9 +127,9 @@ func (h *stubDriveHandler) createStubDrive(driveID, path string) error { // SetDrives will set the given drives and the offset to which the stub drives // start. -func (h *stubDriveHandler) SetDrives(offset int64, d []models.Drive) { +func (h *stubDriveHandler) SetDrives(d []models.Drive) { h.drives = d - h.stubDriveIndex = offset + h.stubDriveIndex = 0 } // GetDrives returns the associated stub drives diff --git a/runtime/service.go b/runtime/service.go index 7acd3ac7d..ffbea8f84 100644 --- a/runtime/service.go +++ b/runtime/service.go @@ -650,7 +650,7 @@ func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker if err != nil { return nil, errors.Wrap(err, "failed to create stub drives") } - s.stubDriveHandler.SetDrives(0, stubDrives) + s.stubDriveHandler.SetDrives(stubDrives) var driveBuilder firecracker.DrivesBuilder // Create non-stub drives diff --git a/runtime/service_integ_test.go b/runtime/service_integ_test.go index ed0d03364..c46e3d11b 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("failed to patch stub drive: There are no remaining drives to be used: unknown", err.Error()) + require.Error(t, err) +} From db8ba7c587bf52ee18740d34256ad454a255df2f Mon Sep 17 00:00:00 2001 From: Kazuyoshi Kato Date: Fri, 9 Aug 2019 10:24:01 -0700 Subject: [PATCH 2/3] Use containerd's ErrUnavailable instead of our own error type Signed-off-by: Kazuyoshi Kato --- runtime/service.go | 4 +++- runtime/service_integ_test.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/runtime/service.go b/runtime/service.go index ffbea8f84..c1beb3800 100644 --- a/runtime/service.go +++ b/runtime/service.go @@ -715,9 +715,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() diff --git a/runtime/service_integ_test.go b/runtime/service_integ_test.go index c46e3d11b..687dc8c15 100644 --- a/runtime/service_integ_test.go +++ b/runtime/service_integ_test.go @@ -725,6 +725,6 @@ func TestCreateTooManyContainers_Isolated(t *testing.T) { // 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("failed to patch stub drive: There are no remaining drives to be used: unknown", err.Error()) + assert.Equal("no remaining stub drives to be used: unavailable: unknown", err.Error()) require.Error(t, err) } From e897807190ef08b28a7021cf72b67436f51c36e7 Mon Sep 17 00:00:00 2001 From: Kazuyoshi Kato Date: Fri, 9 Aug 2019 13:14:01 -0700 Subject: [PATCH 3/3] Remove stubDriveHandler#SetDrives() The number of stub drives cannot be changed once a microVM is started. Modeling the limitation on the stub driver interface may prevent programmer errors. Signed-off-by: Kazuyoshi Kato --- runtime/drive_handler.go | 40 +++++++++++++++++++++++++---------- runtime/drive_handler_test.go | 11 +++++----- runtime/service.go | 31 ++++----------------------- runtime/service_test.go | 5 +++-- 4 files changed, 42 insertions(+), 45 deletions(-) diff --git a/runtime/drive_handler.go b/runtime/drive_handler.go index c58c94aac..552eac9a4 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(d []models.Drive) { - h.drives = d - h.stubDriveIndex = 0 -} - // GetDrives returns the associated stub drives func (h *stubDriveHandler) GetDrives() []models.Drive { return h.drives diff --git a/runtime/drive_handler_test.go b/runtime/drive_handler_test.go index a0bce807c..e1e401fc7 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.go b/runtime/service.go index c1beb3800..c538cf73f 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(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 diff --git a/runtime/service_test.go b/runtime/service_test.go index 07789aa53..7a71ad2b5 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)