diff --git a/internal/devices/drivers.go b/internal/devices/drivers.go index f5dc588e02..eb908583b1 100644 --- a/internal/devices/drivers.go +++ b/internal/devices/drivers.go @@ -60,6 +60,7 @@ func InstallDrivers(ctx context.Context, vm *uvm.UtilityVM, share string) (close share, true, vm.ID(), + "", &scsi.MountConfig{}, ) if err != nil { diff --git a/internal/hcsoci/resources_lcow.go b/internal/hcsoci/resources_lcow.go index b98493ce4c..633bde02eb 100644 --- a/internal/hcsoci/resources_lcow.go +++ b/internal/hcsoci/resources_lcow.go @@ -92,6 +92,7 @@ func allocateLinuxResources(ctx context.Context, coi *createOptionsInternal, r * hostPath, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{Options: mount.Options, BlockDev: isBlockDev}, ) if err != nil { @@ -114,6 +115,7 @@ func allocateLinuxResources(ctx context.Context, coi *createOptionsInternal, r * hostPath, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{Options: mount.Options, BlockDev: isBlockDev}, ) if err != nil { diff --git a/internal/hcsoci/resources_wcow.go b/internal/hcsoci/resources_wcow.go index 0503b371f6..604ba8b550 100644 --- a/internal/hcsoci/resources_wcow.go +++ b/internal/hcsoci/resources_wcow.go @@ -144,6 +144,7 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R mount.Source, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{}, ) case MountTypeVirtualDisk: @@ -153,6 +154,7 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R mount.Source, readOnly, coi.HostingSystem.ID(), + "", &scsi.MountConfig{}, ) case MountTypeExtensibleVirtualDisk: @@ -161,6 +163,7 @@ func setupMounts(ctx context.Context, coi *createOptionsInternal, r *resources.R ctx, mount.Source, readOnly, + "", &scsi.MountConfig{}, ) } diff --git a/internal/layers/lcow.go b/internal/layers/lcow.go index cc934121b4..2a6da2a622 100644 --- a/internal/layers/lcow.go +++ b/internal/layers/lcow.go @@ -136,6 +136,7 @@ func MountLCOWLayers(ctx context.Context, containerID string, layers *LCOWLayers hostPath, false, vm.ID(), + guestRoot, mConfig, ) if err != nil { @@ -194,6 +195,7 @@ func addLCOWLayer(ctx context.Context, vm *uvm.UtilityVM, layer *LCOWLayer) (uvm layer.VHDPath, true, "", + "", &scsi.MountConfig{ Partition: layer.Partition, Options: []string{"ro"}, diff --git a/internal/layers/wcow_mount.go b/internal/layers/wcow_mount.go index d12593d179..a5f706f940 100644 --- a/internal/layers/wcow_mount.go +++ b/internal/layers/wcow_mount.go @@ -331,7 +331,7 @@ func mountHypervIsolatedWCIFSLayers(ctx context.Context, l *wcowWCIFSLayers, vm hostPath := filepath.Join(l.scratchLayerPath, "sandbox.vhdx") log.G(ctx).WithField("hostPath", hostPath).Debug("mounting scratch VHD") - scsiMount, err := vm.SCSIManager.AddVirtualDisk(ctx, hostPath, false, vm.ID(), &scsi.MountConfig{}) + scsiMount, err := vm.SCSIManager.AddVirtualDisk(ctx, hostPath, false, vm.ID(), "", &scsi.MountConfig{}) if err != nil { return nil, nil, fmt.Errorf("failed to add SCSI scratch VHD: %w", err) } diff --git a/internal/lcow/disk.go b/internal/lcow/disk.go index 4e68f26258..08fad8d2ea 100644 --- a/internal/lcow/disk.go +++ b/internal/lcow/disk.go @@ -31,7 +31,7 @@ func FormatDisk(ctx context.Context, lcowUVM *uvm.UtilityVM, destPath string) er }).Debug("lcow::FormatDisk opts") // Attach without mounting. - scsi, err := lcowUVM.SCSIManager.AddPhysicalDisk(ctx, destPath, false, lcowUVM.ID(), nil) + scsi, err := lcowUVM.SCSIManager.AddPhysicalDisk(ctx, destPath, false, lcowUVM.ID(), "", nil) if err != nil { return err } diff --git a/internal/lcow/scratch.go b/internal/lcow/scratch.go index 3c424dcbad..e44a6b388a 100644 --- a/internal/lcow/scratch.go +++ b/internal/lcow/scratch.go @@ -75,6 +75,7 @@ func CreateScratch(ctx context.Context, lcowUVM *uvm.UtilityVM, destFile string, destFile, false, lcowUVM.ID(), + "", &scsi.MountConfig{ BlockDev: true, }, diff --git a/internal/tools/uvmboot/mounts.go b/internal/tools/uvmboot/mounts.go index 2f160c01ca..0f546b9f86 100644 --- a/internal/tools/uvmboot/mounts.go +++ b/internal/tools/uvmboot/mounts.go @@ -25,6 +25,7 @@ func mountSCSI(ctx context.Context, c *cli.Context, vm *uvm.UtilityVM) error { m.host, !m.writable, vm.ID(), + "", &scsi.MountConfig{}, ) if err != nil { diff --git a/internal/uvm/scsi/manager.go b/internal/uvm/scsi/manager.go index fd7173b5b1..ff8374038e 100644 --- a/internal/uvm/scsi/manager.go +++ b/internal/uvm/scsi/manager.go @@ -141,6 +141,7 @@ func (m *Manager) AddVirtualDisk( hostPath string, readOnly bool, vmID string, + guestPath string, mc *MountConfig, ) (*Mount, error) { if m == nil { @@ -169,6 +170,7 @@ func (m *Manager) AddVirtualDisk( readOnly: readOnly, typ: "VirtualDisk", }, + guestPath, mcInternal) } @@ -187,6 +189,7 @@ func (m *Manager) AddPhysicalDisk( hostPath string, readOnly bool, vmID string, + guestPath string, mc *MountConfig, ) (*Mount, error) { if m == nil { @@ -215,6 +218,7 @@ func (m *Manager) AddPhysicalDisk( readOnly: readOnly, typ: "PassThru", }, + guestPath, mcInternal) } @@ -233,6 +237,7 @@ func (m *Manager) AddExtensibleVirtualDisk( ctx context.Context, hostPath string, readOnly bool, + guestPath string, mc *MountConfig, ) (*Mount, error) { if m == nil { @@ -260,10 +265,11 @@ func (m *Manager) AddExtensibleVirtualDisk( typ: "ExtensibleVirtualDisk", evdType: evdType, }, + guestPath, mcInternal) } -func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, mountConfig *mountConfig) (_ *Mount, err error) { +func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, guestPath string, mountConfig *mountConfig) (_ *Mount, err error) { controller, lun, err := m.attachManager.attach(ctx, attachConfig) if err != nil { return nil, err @@ -274,9 +280,8 @@ func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, mountConf } }() - var guestPath string if mountConfig != nil { - guestPath, err = m.mountManager.mount(ctx, controller, lun, mountConfig) + guestPath, err = m.mountManager.mount(ctx, controller, lun, guestPath, mountConfig) if err != nil { return nil, err } @@ -287,14 +292,9 @@ func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, mountConf func (m *Manager) remove(ctx context.Context, controller, lun uint, guestPath string) error { if guestPath != "" { - removed, err := m.mountManager.unmount(ctx, guestPath) - if err != nil { + if err := m.mountManager.unmount(ctx, guestPath); err != nil { return err } - - if !removed { - return nil - } } if _, err := m.attachManager.detach(ctx, controller, lun); err != nil { diff --git a/internal/uvm/scsi/manager_test.go b/internal/uvm/scsi/manager_test.go new file mode 100644 index 0000000000..8c52d51b24 --- /dev/null +++ b/internal/uvm/scsi/manager_test.go @@ -0,0 +1,239 @@ +//go:build windows + +package scsi + +import ( + "context" + "reflect" + "testing" +) + +func removeIndex[T any](a []T, i int) []T { + return append(a[:i], a[i+1:]...) +} + +func TestRemoveIndex(t *testing.T) { + a := []int{1, 2, 3, 4, 5} + a = removeIndex(a, 2) + if !reflect.DeepEqual(a, []int{1, 2, 4, 5}) { + t.Errorf("wrong values: %v", a) + } + a = removeIndex(a, 1) + if !reflect.DeepEqual(a, []int{1, 4, 5}) { + t.Errorf("wrong values: %v", a) + } + a = removeIndex(a, 0) + if !reflect.DeepEqual(a, []int{4, 5}) { + t.Errorf("wrong values: %v", a) + } + a = removeIndex(a, 1) + if !reflect.DeepEqual(a, []int{4}) { + t.Errorf("wrong values: %v", a) + } + a = removeIndex(a, 0) + if !reflect.DeepEqual(a, []int{}) { + t.Errorf("wrong values: %v", a) + } +} + +type testAttachment struct { + controller uint + lun uint + config *attachConfig +} + +type testMount struct { + controller uint + lun uint + path string + config *mountConfig +} + +type hostBackend struct { + attachments []*testAttachment +} + +func (hb *hostBackend) attach(ctx context.Context, controller uint, lun uint, config *attachConfig) error { + hb.attachments = append(hb.attachments, &testAttachment{ + controller: controller, + lun: lun, + config: config, + }) + return nil +} + +func (hb *hostBackend) detach(ctx context.Context, controller uint, lun uint) error { + for i, a := range hb.attachments { + if a.controller == controller && a.lun == lun { + hb.attachments = removeIndex(hb.attachments, i) + break + } + } + return nil +} + +func (hb *hostBackend) attachmentPaths() []string { + ret := []string{} + for _, a := range hb.attachments { + ret = append(ret, a.config.path) + } + return ret +} + +type guestBackend struct { + mounts []*testMount +} + +func (gb *guestBackend) mount(ctx context.Context, controller uint, lun uint, path string, config *mountConfig) error { + gb.mounts = append(gb.mounts, &testMount{ + controller: controller, + lun: lun, + path: path, + config: config, + }) + return nil +} + +func (gb *guestBackend) unmount(ctx context.Context, controller uint, lun uint, path string, config *mountConfig) error { + for i, m := range gb.mounts { + if m.path == path { + gb.mounts = removeIndex(gb.mounts, i) + break + } + } + return nil +} + +func (gb *guestBackend) unplug(ctx context.Context, controller uint, lun uint) error { + return nil +} + +func (gb *guestBackend) mountPaths() []string { + ret := []string{} + for _, m := range gb.mounts { + ret = append(ret, m.path) + } + return ret +} + +func TestAddAddRemoveRemove(t *testing.T) { + ctx := context.Background() + + hb := &hostBackend{} + gb := &guestBackend{} + mgr, err := NewManager(hb, gb, 4, 64, "/var/run/scsi/%d", nil) + if err != nil { + t.Fatal(err) + } + m1, err := mgr.AddVirtualDisk(ctx, "path", true, "", "", &MountConfig{}) + if err != nil { + t.Fatal(err) + } + m2, err := mgr.AddVirtualDisk(ctx, "path", true, "", "", &MountConfig{}) + if err != nil { + t.Fatal(err) + } + if m1.GuestPath() == "" { + t.Error("guest path for m1 should not be empty") + } + if m1.GuestPath() != m2.GuestPath() { + t.Errorf("expected same guest paths for both mounts, but got %q and %q", m1.GuestPath(), m2.GuestPath()) + } + if !reflect.DeepEqual(hb.attachmentPaths(), []string{"path"}) { + t.Errorf("wrong attachment paths after add: %v", hb.attachmentPaths()) + } + if err := m1.Release(ctx); err != nil { + t.Fatal(err) + } + if err := m2.Release(ctx); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(hb.attachmentPaths(), []string{}) { + t.Errorf("wrong attachment paths after remove: %v", hb.attachmentPaths()) + } +} + +func TestGuestPath(t *testing.T) { + ctx := context.Background() + + hb := &hostBackend{} + gb := &guestBackend{} + mgr, err := NewManager(hb, gb, 4, 64, "/var/run/scsi/%d", nil) + if err != nil { + t.Fatal(err) + } + + m1, err := mgr.AddVirtualDisk(ctx, "path", true, "", "/mnt1", &MountConfig{}) + if err != nil { + t.Fatal(err) + } + // m1 should get the guest path it asked for. + if m1.GuestPath() != "/mnt1" { + t.Errorf("wrong guest path for m1: %s", m1.GuestPath()) + } + if !reflect.DeepEqual(gb.mountPaths(), []string{"/mnt1"}) { + t.Errorf("wrong mount paths after adding m1: %v", gb.mountPaths()) + } + + m2, err := mgr.AddVirtualDisk(ctx, "path", true, "", "", &MountConfig{}) + if err != nil { + t.Fatal(err) + } + // m2 didn't ask for a guest path, so it should re-use m1. + if m2.GuestPath() != "/mnt1" { + t.Errorf("wrong guest path for m2: %s", m2.GuestPath()) + } + if !reflect.DeepEqual(gb.mountPaths(), []string{"/mnt1"}) { + t.Errorf("wrong mount paths after adding m2: %v", gb.mountPaths()) + } + + m3, err := mgr.AddVirtualDisk(ctx, "path", true, "", "/mnt2", &MountConfig{}) + if err != nil { + t.Fatal(err) + } + // m3 should get the guest path it asked for. + if m3.GuestPath() != "/mnt2" { + t.Errorf("wrong guest path for m3: %s", m2.GuestPath()) + } + if !reflect.DeepEqual(gb.mountPaths(), []string{"/mnt1", "/mnt2"}) { + t.Errorf("wrong mount paths after adding m3: %v", gb.mountPaths()) + } + + if err := m3.Release(ctx); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(gb.mountPaths(), []string{"/mnt1"}) { + t.Errorf("wrong mount paths after removing m3: %v", gb.mountPaths()) + } + if err := m2.Release(ctx); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(gb.mountPaths(), []string{"/mnt1"}) { + t.Errorf("wrong mount paths after removing m2: %v", gb.mountPaths()) + } + if err := m1.Release(ctx); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(gb.mountPaths(), []string{}) { + t.Errorf("wrong mount paths after removing m1: %v", gb.mountPaths()) + } +} + +func TestConflictingGuestPath(t *testing.T) { + ctx := context.Background() + + hb := &hostBackend{} + gb := &guestBackend{} + mgr, err := NewManager(hb, gb, 4, 64, "/var/run/scsi/%d", nil) + if err != nil { + t.Fatal(err) + } + + if _, err := mgr.AddVirtualDisk(ctx, "path", true, "", "/mnt1", &MountConfig{}); err != nil { + t.Fatal(err) + } + // Different host path but same guest path as m1, should conflict. + if _, err := mgr.AddVirtualDisk(ctx, "path2", true, "", "/mnt1", &MountConfig{}); err == nil { + t.Fatalf("expected error but got none") + } +} diff --git a/internal/uvm/scsi/mount.go b/internal/uvm/scsi/mount.go index ed77fa9991..1f2fb831cc 100644 --- a/internal/uvm/scsi/mount.go +++ b/internal/uvm/scsi/mount.go @@ -47,14 +47,17 @@ type mountConfig struct { filesystem string } -func (mm *mountManager) mount(ctx context.Context, controller, lun uint, c *mountConfig) (_ string, err error) { +func (mm *mountManager) mount(ctx context.Context, controller, lun uint, path string, c *mountConfig) (_ string, err error) { // Normalize the mount config for comparison. // Config equality relies on the options slices being compared element-wise. Sort the options // slice first so that two slices with different ordering compare as equal. We assume that // order will never matter for mount options. sort.Strings(c.options) - mount, existed := mm.trackMount(controller, lun, c) + mount, existed, err := mm.trackMount(controller, lun, path, c) + if err != nil { + return "", err + } if existed { select { case <-ctx.Done(): @@ -84,7 +87,7 @@ func (mm *mountManager) mount(ctx context.Context, controller, lun uint, c *moun return mount.path, nil } -func (mm *mountManager) unmount(ctx context.Context, path string) (bool, error) { +func (mm *mountManager) unmount(ctx context.Context, path string) error { mm.m.Lock() defer mm.m.Unlock() @@ -97,18 +100,18 @@ func (mm *mountManager) unmount(ctx context.Context, path string) (bool, error) mount.refCount-- if mount.refCount > 0 { - return false, nil + return nil } if err := mm.mounter.unmount(ctx, mount.controller, mount.lun, mount.path, mount.config); err != nil { - return false, fmt.Errorf("unmount scsi controller %d lun %d at path %s: %w", mount.controller, mount.lun, mount.path, err) + return fmt.Errorf("unmount scsi controller %d lun %d at path %s: %w", mount.controller, mount.lun, mount.path, err) } mm.untrackMount(mount) - return true, nil + return nil } -func (mm *mountManager) trackMount(controller, lun uint, c *mountConfig) (*mount, bool) { +func (mm *mountManager) trackMount(controller, lun uint, path string, c *mountConfig) (*mount, bool, error) { mm.m.Lock() defer mm.m.Unlock() @@ -120,15 +123,19 @@ func (mm *mountManager) trackMount(controller, lun uint, c *mountConfig) (*mount } } else if controller == mount.controller && lun == mount.lun && + (path == "" || path == mount.path) && reflect.DeepEqual(c, mount.config) { mount.refCount++ - return mount, true + return mount, true, nil + } else if path != "" && path == mount.path { + return nil, false, fmt.Errorf("cannot mount over an existing mountpoint: %s", path) } } // New mount. mount := &mount{ + path: path, // If path is empty, this will be replaced with a generated path below. controller: controller, lun: lun, config: c, @@ -142,9 +149,11 @@ func (mm *mountManager) trackMount(controller, lun uint, c *mountConfig) (*mount mount.index = freeIndex mm.mounts[freeIndex] = mount } - // Use the mount index to produce a unique guest path. - mount.path = fmt.Sprintf(mm.mountFmt, mount.index) - return mount, false + if mount.path == "" { + // Use the mount index to produce a unique guest path. + mount.path = fmt.Sprintf(mm.mountFmt, mount.index) + } + return mount, false, nil } // Caller must be holding mm.m. diff --git a/test/functional/uvm_scratch_test.go b/test/functional/uvm_scratch_test.go index ada16b2fea..76213b2475 100644 --- a/test/functional/uvm_scratch_test.go +++ b/test/functional/uvm_scratch_test.go @@ -49,7 +49,7 @@ func TestScratchCreateLCOW(t *testing.T) { } // Make sure it can be added (verifies it has access correctly) - scsiMount, err := targetUVM.SCSIManager.AddVirtualDisk(context.Background(), destTwo, false, targetUVM.ID(), nil) + scsiMount, err := targetUVM.SCSIManager.AddVirtualDisk(context.Background(), destTwo, false, targetUVM.ID(), "", nil) if err != nil { t.Fatal(err) } diff --git a/test/functional/uvm_scsi_test.go b/test/functional/uvm_scsi_test.go index 1bf06a64b2..7ffc5ed550 100644 --- a/test/functional/uvm_scsi_test.go +++ b/test/functional/uvm_scsi_test.go @@ -66,7 +66,7 @@ func testAddSCSI(u *uvm.UtilityVM, disks []string, attachOnly bool) ([]*scsi.Mou if !attachOnly { mc = &scsi.MountConfig{} } - scsiMount, err := u.SCSIManager.AddVirtualDisk(context.Background(), disks[i], false, u.ID(), mc) + scsiMount, err := u.SCSIManager.AddVirtualDisk(context.Background(), disks[i], false, u.ID(), "", mc) if err != nil { return nil, err } @@ -280,7 +280,7 @@ func TestParallelScsiOps(t *testing.T) { continue } - mount, err := u.SCSIManager.AddVirtualDisk(context.Background(), path, false, u.ID(), nil) + mount, err := u.SCSIManager.AddVirtualDisk(context.Background(), path, false, u.ID(), "", nil) if err != nil { os.Remove(path) t.Errorf("failed to add SCSI disk for worker: %d, iteration: %d with err: %v", scsiIndex, iteration, err) @@ -293,7 +293,7 @@ func TestParallelScsiOps(t *testing.T) { break } - mount, err = u.SCSIManager.AddVirtualDisk(context.Background(), path, false, u.ID(), &scsi.MountConfig{}) + mount, err = u.SCSIManager.AddVirtualDisk(context.Background(), path, false, u.ID(), "", &scsi.MountConfig{}) if err != nil { os.Remove(path) t.Errorf("failed to add SCSI disk for worker: %d, iteration: %d with err: %v", scsiIndex, iteration, err)