Skip to content

[shimV2] refactor scsi device controller to V2 template#2660

Open
rawahars wants to merge 1 commit intomicrosoft:mainfrom
rawahars:scsi_controller_touch_up
Open

[shimV2] refactor scsi device controller to V2 template#2660
rawahars wants to merge 1 commit intomicrosoft:mainfrom
rawahars:scsi_controller_touch_up

Conversation

@rawahars
Copy link
Copy Markdown
Contributor

@rawahars rawahars commented Apr 4, 2026

This PR is mostly a structural refactor + type renames + logging.

Summary of what was done (from code changes)

A) Reorganized the SCSI code into “template-style” modules

Moved definitions out of the main implementation files into dedicated files:

  • internal/controller/device/scsi/

    • Added doc.go (package docs)
    • Added types.go (interfaces + numLUNsPerController + reservation struct)
    • Deleted scsi.go (old minimal package comment file)
  • internal/controller/device/scsi/disk/

    • Added types.go (disk Type, disk Config, VM/guest interfaces)
    • Added state.go (disk lifecycle state enum + String())
    • Added doc.go (disk lifecycle documentation)
    • Added disk_lcow.go and disk_wcow.go (currently only //go:build windows + package disk; effectively placeholders)
  • internal/controller/device/scsi/mount/

    • Added types.go (mount Config + guest interfaces + Equals)
    • Added state.go (mount lifecycle state enum + String())
    • Added doc.go (mount lifecycle documentation)

Net effect: same concepts, but split into consistent “types/state/doc” layout.

B) Renamed public types

  • disk.DiskConfigdisk.Config
  • disk.DiskType*disk.Type*
  • disk.DiskState*disk.State*
  • mount.MountConfigmount.Config
  • mount.MountState*mount.State*

Also Controller’s reservation ID type changed:

  • Controller.Reserve(...) returns guid.GUID instead of uuid.UUID
  • Controller.MapToGuest/UnmapFromGuest(...) now take guid.GUID instead of uuid.UUID
  • Internally: reservations map[guid.GUID]*reservation

C) Added structured logging + new logfields

Changes are sprinkled through Controller, disk.Disk, and mount.Mount:

  • internal/logfields/fields.go adds:

    • Controller, LUN, DiskType, Partition
  • Controller.Reserve, MapToGuest, UnmapFromGuest now:

    • add log context fields (host path, partition, reservation)
    • emit debug logs on reserve/map/unmap and slot freed
  • disk.Disk.AttachToVM/DetachFromVM/ReservePartition and mount.Mount.MountToGuest/UnmountFromGuest now emit debug/trace logs and enrich context.

Are there functional changes?

1) Yes: panic → error in Mount.MountToGuest when both guests are nil

Old behavior:

  • MountToGuest(ctx, nil, nil) panicked.

New behavior:

  • it returns an error: both linuxGuest and windowsGuest cannot be nil
  • the unit test was changed accordingly (TestMountToGuest_PanicsWhenBothGuestsNilTestMountToGuest_ErrorWhenBothGuestsNil).

2) Slight semantic tightening in Disk.MountPartitionToGuest when partition wasn’t reserved

Old behavior:

  • if partition not in d.mounts, it returned: partition %d not found on disk

New behavior:

  • returns a more specific error including controller/lun and “not reserved”.

@rawahars rawahars requested a review from a team as a code owner April 4, 2026 23:53
In this commit, we are just ensuring that all the files and format for this device/scsi package conforms with the other controllers for V2 shims. This includes-
- `states` encapsulated in states file
- `types` encapsulated in types file
- package documentation in `doc.go`
- Similar comment and error message style

Note that in this commit, we have not made changes to the unit test files and hence we can be sure that the refactor has not broken any functional behavior.

Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
@rawahars rawahars force-pushed the scsi_controller_touch_up branch from 15c8ba7 to 5228c78 Compare April 6, 2026 00:02
@@ -0,0 +1,3 @@
//go:build windows

package disk
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have this and lcow when they have the same build tags?

Copy link
Copy Markdown
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we follow the same logic in #2633 and have [Linux|Windows]GuestSCSI(Un)Mounter and co be gated by lcow/wcow build tags so the Controller doesn't need fields and logic for both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants