Skip to content

Commit 4e8ad74

Browse files
kzyskzys
authored
Merge pull request #240 from kzys/fix-patch-drive
Before this change, a stub drive handler knew non-stub drives, including the root drive. However patching non-stub drives is not supported and must be prevented.
2 parents 0b05161 + 0f7744f commit 4e8ad74

File tree

2 files changed

+81
-34
lines changed

2 files changed

+81
-34
lines changed

‎runtime/service.go

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -588,6 +588,25 @@ func (s *service) SetVMMetadata(requestCtx context.Context, request *proto.SetVM
588588
return &empty.Empty{}, nil
589589
}
590590

591+
func (s *service) createStubDrives(stubDriveCount int) ([]models.Drive, error) {
592+
paths, err := s.stubDriveHandler.StubDrivePaths(stubDriveCount)
593+
if err != nil {
594+
return nil, errors.Wrap(err, "failed to retrieve stub drive paths")
595+
}
596+
597+
stubDrives := make([]models.Drive, 0, stubDriveCount)
598+
for i, path := range paths {
599+
stubDrives = append(stubDrives, models.Drive{
600+
DriveID: firecracker.String(fmt.Sprintf("stub%d", i)),
601+
IsReadOnly: firecracker.Bool(false),
602+
PathOnHost: firecracker.String(path),
603+
IsRootDevice: firecracker.Bool(false),
604+
})
605+
}
606+
607+
return stubDrives, nil
608+
}
609+
591610
func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker.Config, error) {
592611
logger := s.logger.WithField("cid", s.machineCID)
593612

@@ -618,15 +637,6 @@ func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker
618637
}
619638

620639
// Drives configuration
621-
622-
var driveBuilder firecracker.DrivesBuilder
623-
if root := req.RootDrive; root != nil {
624-
driveBuilder = firecracker.NewDrivesBuilder(root.PathOnHost)
625-
} else {
626-
driveBuilder = firecracker.NewDrivesBuilder(s.config.RootDrive)
627-
}
628-
629-
stubDriveIndex := int64(len(driveBuilder.Build()) - 1)
630640
containerCount := int(req.ContainerCount)
631641
if containerCount < 1 {
632642
// containerCount should always be positive so that at least one container
@@ -635,26 +645,28 @@ func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker
635645
containerCount = 1
636646
}
637647

638-
paths, err := s.stubDriveHandler.StubDrivePaths(containerCount)
648+
// Create stub drives first and let stub driver handler manage the drives
649+
stubDrives, err := s.createStubDrives(containerCount)
639650
if err != nil {
640-
return nil, errors.Wrap(err, "failed to retrieve stub drive paths")
651+
return nil, errors.Wrap(err, "failed to create stub drives")
641652
}
653+
s.stubDriveHandler.SetDrives(0, stubDrives)
642654

643-
for i, path := range paths {
644-
driveID := fmt.Sprintf("stub%d", i)
645-
driveBuilder = driveBuilder.AddDrive(path, false, func(drive *models.Drive) {
646-
drive.DriveID = firecracker.String(driveID)
647-
})
655+
var driveBuilder firecracker.DrivesBuilder
656+
// Create non-stub drives
657+
if root := req.RootDrive; root != nil {
658+
driveBuilder = firecracker.NewDrivesBuilder(root.PathOnHost)
659+
} else {
660+
driveBuilder = firecracker.NewDrivesBuilder(s.config.RootDrive)
648661
}
649662

650-
cfg.Drives = driveBuilder.Build()
651-
s.stubDriveHandler.SetDrives(stubDriveIndex, cfg.Drives)
652-
653663
for _, drive := range req.AdditionalDrives {
654664
driveBuilder = addDriveFromProto(driveBuilder, drive)
655665
}
656666

657-
cfg.Drives = driveBuilder.Build()
667+
// a micro VM must know all drives
668+
// nolint: gocritic
669+
cfg.Drives = append(stubDrives, driveBuilder.Build()...)
658670

659671
// Setup network interfaces
660672

‎runtime/service_test.go

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,12 @@ func TestFindNextAvailableVsockCID_Isolated(t *testing.T) {
6262

6363
func TestBuildVMConfiguration(t *testing.T) {
6464
namespace := "TestBuildVMConfiguration"
65-
rootfsDrive := models.Drive{
66-
DriveID: firecracker.String("stub0"),
67-
PathOnHost: nil, // will be populated in the for loop
68-
IsReadOnly: firecracker.Bool(false),
69-
IsRootDevice: firecracker.Bool(false),
70-
}
7165
testcases := []struct {
72-
name string
73-
request *proto.CreateVMRequest
74-
config *Config
75-
expectedCfg *firecracker.Config
66+
name string
67+
request *proto.CreateVMRequest
68+
config *Config
69+
expectedCfg *firecracker.Config
70+
expectedStubDriveCount int
7671
}{
7772
{
7873
name: "ConfigFile",
@@ -88,7 +83,6 @@ func TestBuildVMConfiguration(t *testing.T) {
8883
KernelArgs: "KERNEL ARGS",
8984
KernelImagePath: "KERNEL IMAGE",
9085
Drives: []models.Drive{
91-
rootfsDrive,
9286
{
9387
DriveID: firecracker.String("root_drive"),
9488
PathOnHost: firecracker.String("ROOT DRIVE"),
@@ -103,6 +97,7 @@ func TestBuildVMConfiguration(t *testing.T) {
10397
HtEnabled: firecracker.Bool(false),
10498
},
10599
},
100+
expectedStubDriveCount: 1,
106101
},
107102
{
108103
name: "Input",
@@ -122,7 +117,6 @@ func TestBuildVMConfiguration(t *testing.T) {
122117
KernelArgs: "REQUEST KERNEL ARGS",
123118
KernelImagePath: "REQUEST KERNEL IMAGE",
124119
Drives: []models.Drive{
125-
rootfsDrive,
126120
{
127121
DriveID: firecracker.String("root_drive"),
128122
PathOnHost: firecracker.String("REQUEST ROOT DRIVE"),
@@ -137,6 +131,7 @@ func TestBuildVMConfiguration(t *testing.T) {
137131
HtEnabled: firecracker.Bool(false),
138132
},
139133
},
134+
expectedStubDriveCount: 1,
140135
},
141136
{
142137
name: "Priority",
@@ -160,7 +155,6 @@ func TestBuildVMConfiguration(t *testing.T) {
160155
KernelArgs: "REQUEST KERNEL ARGS",
161156
KernelImagePath: "KERNEL IMAGE",
162157
Drives: []models.Drive{
163-
rootfsDrive,
164158
{
165159
DriveID: firecracker.String("root_drive"),
166160
PathOnHost: firecracker.String("REQUEST ROOT DRIVE"),
@@ -175,6 +169,37 @@ func TestBuildVMConfiguration(t *testing.T) {
175169
HtEnabled: firecracker.Bool(false),
176170
},
177171
},
172+
expectedStubDriveCount: 1,
173+
},
174+
{
175+
name: "Container Count",
176+
request: &proto.CreateVMRequest{ContainerCount: 2},
177+
config: &Config{
178+
KernelArgs: "KERNEL ARGS",
179+
KernelImagePath: "KERNEL IMAGE",
180+
RootDrive: "ROOT DRIVE",
181+
CPUTemplate: "C3",
182+
CPUCount: 2,
183+
},
184+
expectedCfg: &firecracker.Config{
185+
KernelArgs: "KERNEL ARGS",
186+
KernelImagePath: "KERNEL IMAGE",
187+
Drives: []models.Drive{
188+
{
189+
DriveID: firecracker.String("root_drive"),
190+
PathOnHost: firecracker.String("ROOT DRIVE"),
191+
IsReadOnly: firecracker.Bool(false),
192+
IsRootDevice: firecracker.Bool(true),
193+
},
194+
},
195+
MachineCfg: models.MachineConfiguration{
196+
CPUTemplate: models.CPUTemplateC3,
197+
VcpuCount: firecracker.Int64(2),
198+
MemSizeMib: firecracker.Int64(defaultMemSizeMb),
199+
HtEnabled: firecracker.Bool(false),
200+
},
201+
},
202+
expectedStubDriveCount: 2,
178203
},
179204
}
180205

@@ -197,11 +222,21 @@ func TestBuildVMConfiguration(t *testing.T) {
197222
tc.expectedCfg.VsockDevices = []firecracker.VsockDevice{{Path: "root", CID: svc.machineCID}}
198223
tc.expectedCfg.LogFifo = svc.shimDir.FirecrackerLogFifoPath()
199224
tc.expectedCfg.MetricsFifo = svc.shimDir.FirecrackerMetricsFifoPath()
200-
tc.expectedCfg.Drives[0].PathOnHost = firecracker.String(filepath.Join(tempDir, "stub0"))
225+
226+
drives := make([]models.Drive, tc.expectedStubDriveCount)
227+
for i := 0; i < tc.expectedStubDriveCount; i++ {
228+
drives[i].PathOnHost = firecracker.String(filepath.Join(tempDir, fmt.Sprintf("stub%d", i)))
229+
drives[i].DriveID = firecracker.String(fmt.Sprintf("stub%d", i))
230+
drives[i].IsReadOnly = firecracker.Bool(false)
231+
drives[i].IsRootDevice = firecracker.Bool(false)
232+
}
233+
tc.expectedCfg.Drives = append(drives, tc.expectedCfg.Drives...)
201234

202235
actualCfg, err := svc.buildVMConfiguration(tc.request)
203236
assert.NoError(t, err)
204237
require.Equal(t, tc.expectedCfg, actualCfg)
238+
239+
require.Equal(t, tc.expectedStubDriveCount, len(svc.stubDriveHandler.drives), "The stub driver only knows stub drives")
205240
})
206241
}
207242
}

0 commit comments

Comments
 (0)