🌱 Add SCSI controller auto-creation and slot validation for volumes #1244
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lubronzhan
reviewed
Oct 15, 2025
zhangheliu
reviewed
Oct 16, 2025
zhangheliu
reviewed
Oct 16, 2025
zhangheliu
reviewed
Oct 16, 2025
zhangheliu
reviewed
Oct 16, 2025
zhangheliu
reviewed
Oct 16, 2025
lubronzhan
reviewed
Oct 21, 2025
Implement mutation webhook to automatically add SCSI controllers when volumes need them, checking status.hardware.controllers for capacity. We create new ParaVirtual SCSI controller (max 4) only when all existing controllers with matching requirements are full. Update the validation webhooks to enforce controller slot limits based on controller type (ParaVirtual: 63 slots, others: 15 slots). Handles OracleRAC (volume sharingMode=MultiWriter -> controller sharingMode=None) and MicrosoftWSFC (applicationType -> controller sharingMode=Physical). This change also sets controllerBusNumber on volume when creating new controller. Otherwise, there's a chance that after the mutation webhooks determines that we need a new controller and adds it to the spec, another volume has been detached and a slot frees up on an existing controller. So, now we have a controller that does not have any devices attached. To handle that, we are explicitly setting the controllerBusnumber for a volume for which we are creating the controller. That way, we guarantee that the device is attached to the controller that we created.
faisalabujabal
force-pushed
the
features/rac-and-wsfc/add-scsi-controller-in-mutation-webhook
branch
2 times, most recently
from
712960c to
ca088f2
Compare
faisalabujabal
force-pushed
the
features/rac-and-wsfc/add-scsi-controller-in-mutation-webhook
branch
from
ca088f2 to
46e95b0
Compare
Minimum allowed line rate is |
akutz
requested changes
Oct 28, 2025
There was a problem hiding this comment.
Great start @faisalabujabal , thank you! In addition to the comments on the review:
- The linter will ping any comments that do not end with periods, so please take care to do that.
- Go is more efficient at memory management when shared by copy than by address. Unless functions need to accept an argument by address or need to return a value by address, then prefer by-copy instead.
- There is extensive use of the
continuedirective in these changes. The use of continue is fine, but it will mean that you have to provide coverage for that logic, versus inverting and indenting and only covering the case about which you care.
| var ( | ||
| // MaxSCSISlotsByType is the maximum number of devices per SCSI controller | ||
| // type. | ||
| // Note: The controller itself occupies one slot. |
There was a problem hiding this comment.
It may be worth noting here that it is always slot 7.
| // MaxSCSISlotsByType is the maximum number of devices per SCSI controller | ||
| // type. | ||
| // Note: The controller itself occupies one slot. | ||
| MaxSCSISlotsByType = map[vmopv1.SCSIControllerType]int32{ |
There was a problem hiding this comment.
Make this a function off of the type itself in api/v1alpha5, ex.:
diff --git a/api/v1alpha5/virtualmachine_hardware_types.go b/api/v1alpha5/virtualmachine_hardware_types.go
index 6bd5c3cf..0de95aab 100644
--- a/api/v1alpha5/virtualmachine_hardware_types.go
+++ b/api/v1alpha5/virtualmachine_hardware_types.go
@@ -17,6 +17,21 @@ const (
VirtualControllerTypeSATA VirtualControllerType = "SATA"
)
+// MaxSlots returns the the maximum number of slots per controller
+// type.
+func (t VirtualControllerType) MaxSlots() int32 {
+ switch t {
+ case VirtualControllerTypeIDE:
+ return 2
+ case VirtualControllerTypeNVME:
+ return 4
+ case VirtualControllerTypeSATA:
+ return 4
+ default:
+ return 0
+ }
+}
+
type VirtualControllerSharingMode string
const (
@@ -36,6 +51,21 @@ const (
SCSIControllerTypeLsiLogicSAS SCSIControllerType = "LsiLogicSAS"
)
+// MaxSlots returns the the maximum number of slots per SCSI controller
+// type. Please note, the controller itself occupies one slot.
+func (t SCSIControllerType) MaxSlots() int32 {
+ switch t {
+ case SCSIControllerTypeParaVirtualSCSI:
+ return 63 // 64 - 1 slot for the SCSI controller
+ case SCSIControllerTypeBusLogic,
+ SCSIControllerTypeLsiLogic,
+ SCSIControllerTypeLsiLogicSAS:
+ return 15 // 16 - 1 slot for the SCSI controller
+ default:
+ return 0
+ }
+}
+
// +kubebuilder:validation:Enum=CDROM;Disk
type VirtualDeviceType string
@@ -54,6 +84,10 @@ type IDEControllerSpec struct {
BusNumber int32 `json:"busNumber"`
}
+func (t IDEControllerSpec) MaxSlots() int32 {
+ return VirtualControllerTypeIDE.MaxSlots()
+}
+
type NVMEControllerSpec struct {
// +required
// +kubebuilder:validation:Minimum=0
@@ -81,6 +115,10 @@ type NVMEControllerSpec struct {
SharingMode VirtualControllerSharingMode `json:"sharingMode,omitempty"`
}
+func (t NVMEControllerSpec) MaxSlots() int32 {
+ return VirtualControllerTypeNVME.MaxSlots()
+}
+
type SATAControllerSpec struct {
// +required
// +kubebuilder:validation:Minimum=0
@@ -99,6 +137,10 @@ type SATAControllerSpec struct {
PCISlotNumber *int32 `json:"pciSlotNumber,omitempty"`
}
+func (t SATAControllerSpec) MaxSlots() int32 {
+ return VirtualControllerTypeSATA.MaxSlots()
+}
+
type SCSIControllerSpec struct {
// +required
// +kubebuilder:validation:Minimum=0
@@ -134,6 +176,10 @@ type SCSIControllerSpec struct {
Type SCSIControllerType `json:"type,omitempty"`
}
+func (t SCSIControllerSpec) MaxSlots() int32 {
+ return t.Type.MaxSlots()
+}
+
type VirtualDeviceStatus struct {
// +required
|
|
||
| // MaxSCSIControllersPerVM is the maximum number of SCSI controllers | ||
| // allowed per VM. (4 controllers, indexed 0-3). | ||
| MaxSCSIControllersPerVM = int32(4) |
There was a problem hiding this comment.
Make this and the next variable a constant, ex.:
const (
// MaxSCSIControllersPerVM is the maximum number of SCSI controllers
// allowed per VM. (4 controllers, indexed 0-3).
MaxSCSIControllersPerVM int32 = 4
// SCSIControllerUnitNumber is the unit number reserved for SCSI controller
// on its own bus.
SCSIControllerUnitNumber int32 = 7
)| func MaxSlotsForControllerType(ctrlType vmopv1.SCSIControllerType) int32 { | ||
| if _, ok := MaxSCSISlotsByType[ctrlType]; !ok { | ||
| // Unknown controller type, default to ParaVirtual | ||
| return MaxSCSISlotsByType[vmopv1.SCSIControllerTypeParaVirtualSCSI] |
There was a problem hiding this comment.
This is wrong. If it is unknown then declaring it to be 63 slots, that is an issue. How will an unknown controller type even work?
Plus, you can get rid of this with the change I requested up above anyway.
| return MaxSCSISlotsByType[ctrlType] | ||
| } | ||
|
|
||
| // ControllerHasAvailableSlots checks if a controller has available slots for |
There was a problem hiding this comment.
A few things:
- Move this to
pkg/util/vmopv1as it is the utility package for functions that act on just the Kube APIs. - Create a new file called
hardware.go. - In the new files:
- Define an interface called
HasMaxSlots// HasMaxSlots is an interface describing a type with a function that // returns a maximum number of device slots. type HasMaxSlots interface { MaxSlots() int32 }
- Rename this function to
NextAvailableUnitNumber// NextAvailableUnitNumber returns the next available unit number for a given // controller. A value of -1 is returned if there are no available slots remaining. func NextAvailableUnitNumber( controller HasMaxSlots, busNumber int32, statusDeviceCounts, volumeAssignments map[int32]int32) int32 { // Get max slots for this controller. maxSlots := controller.MaxSlots() if maxSlots == 0 { return -1 } // Get current device count from status. currentDevices := statusDeviceCounts[busNumber] // Get number of volumes already assigned to this controller in this mutation. assignedVolumes := volumeAssignments[busNumber] // Check if adding one more volume would exceed capacity. return (currentDevices + assignedVolumes + 1) <= maxSlots }
- Define an interface called
There was a problem hiding this comment.
why change the implementation to return the next available unit number if the caller doesn't care about the unit number but only cares about if a slot is available?
| func createSCSIController( | ||
| busNumber int32, | ||
| sharingMode vmopv1.VirtualControllerSharingMode, | ||
| ) *vmopv1.SCSIControllerSpec { |
There was a problem hiding this comment.
Does this need to be returned by address?
|
|
||
| const ( | ||
| invalidControllerBusNumberRangeFmt = "must be between 0 and %d" | ||
| invalidControllerBusNumberDoesNotExist = "SCSI controller with bus number %d does not exist" |
There was a problem hiding this comment.
Simplify and make generic, ex.: controller %s:%d does not exist. That may be:
controller SCSI:0 does not existcontroller IDE:1 does not existcontroller NVME:2 does not existcontroller SATA:3 does not exist
| invalidControllerBusNumberDoesNotExist = "SCSI controller with bus number %d does not exist" | ||
| invalidUnitNumberReserved = "unit number 7 is reserved for the SCSI controller itself" | ||
| invalidUnitNumberRangeFmt = "unit number must be less than %d for %s controller" | ||
| invalidControllerCapacityFmt = "SCSI controller %d (type=%s) has %d devices and cannot accommodate %d more volumes (%v). Maximum slots: %d" |
There was a problem hiding this comment.
Simplify and make generic, ex.: controller %s:%d full, maxDevices=%d. That may be:
controller SCSI:0 full, maxDevices=63controller IDE:1 full, maxDevices=4controller NVME:2 full, maxDevices=4controller SATA:3 full, maxDevices=4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do, and why is it needed?
Implement mutation webhook to automatically add SCSI controllers when volumes need them, checking status.hardware.controllers for capacity. We create new ParaVirtual SCSI controller (max 4) only when all existing controllers with matching requirements are full.
Update the validation webhooks to enforce controller slot limits based on controller type (ParaVirtual: 63 slots, others: 15 slots).
Handles OracleRAC (volume sharingMode=MultiWriter -> controller sharingMode=None) and MicrosoftWSFC (applicationType -> controller sharingMode=Physical).
This change also sets controllerBusNumber on volume when creating new controller. Otherwise, there's a chance that after the mutation webhooks determines that we need a new controller and adds it to the spec, another volume has been detached and a slot frees up on an existing controller. So, now we have a controller that does not have any devices attached. To handle that, we are explicitly setting the controllerBusnumber for a volume for which we are creating the controller. That way, we guarantee that the device is attached to the controller that we created.
Please add a release note if necessary: