Skip to content

Commit

Permalink
scsi: Add tests and fix refcount bug
Browse files Browse the repository at this point in the history
Adds various tests to the SCSI manager code. As part of this testing,
a bug tracking attachment refcounts was also found. The attachment
refcount is increased every time a new top-level request comes in, even
if an existing mount is re-used. This means that the attachment refcount
should also be decremented every time one is released, even if the mount
refcount is not at 0.

Signed-off-by: Kevin Parsons <[email protected]>
  • Loading branch information
kevpar committed Sep 27, 2024
1 parent c1d6492 commit 3b7c087
Show file tree
Hide file tree
Showing 5 changed files with 248 additions and 14 deletions.
7 changes: 1 addition & 6 deletions internal/uvm/scsi/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,14 +292,9 @@ func (m *Manager) add(ctx context.Context, attachConfig *attachConfig, guestPath

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 {
Expand Down
239 changes: 239 additions & 0 deletions internal/uvm/scsi/manager_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
8 changes: 4 additions & 4 deletions internal/uvm/scsi/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func (mm *mountManager) mount(ctx context.Context, controller, lun uint, path st
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()

Expand All @@ -100,15 +100,15 @@ 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, path string, c *mountConfig) (*mount, bool, error) {
Expand Down
2 changes: 1 addition & 1 deletion test/functional/uvm_scratch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
6 changes: 3 additions & 3 deletions test/functional/uvm_scsi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit 3b7c087

Please sign in to comment.