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 <[email protected]>

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(d []models.Drive) {
-	h.drives = d
-	h.stubDriveIndex = 0
-}
-
 // 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(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
 

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)