✨ add CD-ROM controller support for VMSharedDisks #1261
+2,501
−65
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
zhangheliu
requested review from
akutz,
aruneshpa,
bryanv,
dilyar85 and
lubronzhan
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
from
c9c1149 to
209e396
Compare
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
from
209e396 to
fc2d256
Compare
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
from
fc2d256 to
17ddd0b
Compare
lubronzhan
reviewed
Oct 25, 2025
pkg/providers/vsphere/upgrade/virtualmachine/vm_schema_upgrade.go
Outdated
Show resolved
Hide resolved
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
3 times, most recently
from
3618af2 to
75bec62
Compare
zhangheliu
commented
Oct 28, 2025
| ) | ||
|
|
||
| const ( | ||
| // IDEControllerMaxSlotCount is the maximum number of slots |
There was a problem hiding this comment.
There is a review from https://github.com/vmware-tanzu/vm-operator/pull/1244/files#r2469798582. I will update this as well once the mentioned PR is merged to align with the change.
lubronzhan
reviewed
Oct 28, 2025
|
|
||
| ctrlMap, ctrlUsedUnitNumMap := buildControllerMaps(curDevices) | ||
|
|
||
| for bFileName, cdrom := range expectedCdromDevices { |
There was a problem hiding this comment.
The api says
// CD-ROM devices can be added, updated, or removed when the VM is powered
// off.
Now here we only handle add I think? Should we at least have a TODO for this?
There was a problem hiding this comment.
Previous thread mentioned edit https://github.com/vmware-tanzu/vm-operator/pull/1261/files#r2462256036. I think we are also missing delete
There was a problem hiding this comment.
They all handled it with the existing logic. We are not changing anything about how devices (CD-ROMs) are attached, but instead of adding a new device change for a new controller, we just update the vm.spec.hardware.controller.
And let the virtualmachine controller reconciler add the device change for adding a new controller.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sorry, too dumb that I missed the && at the end and though that's an assignment.
Then just the edit is left over? Since we are not checking whether the current controller has corresponding busNumber/Type/slotNumber
There was a problem hiding this comment.
Thank you for the great catch! Just added support for "edit" the CD-ROM controller placement spec
|
Have you verified in a real env? |
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
from
75bec62 to
838ecf9
Compare
Not yet since changes are behind FSS. I am thinking about doing end-to-end testing once the majority of the PRs are in. |
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
2 times, most recently
from
e04bf60 to
95b0729
Compare
Add comprehensive CD-ROM controller management for the VMSharedDisks feature including: - CD-ROM controller auto-assignment and validation - Dynamic IDE/SATA controller creation when needed - Webhook mutation for controller spec assignment - Webhook validation for controller configuration - VM schema upgrade reconciliation for existing VMs - Support for CD-ROM placement changes when VM is powered off This enables proper CD-ROM device management when the VMSharedDisks capability is enabled, ensuring CD-ROMs are correctly assigned to available controller slots and new controllers are created as needed. When VMSharedDisks is enabled, the operator now supports changing CD-ROM placement (controller type, bus number, or unit number) when the VM is powered off. Placement changes are handled via a remove-and-add pattern, removing the CD-ROM with outdated placement and recreating it with the new controller assignment. Webhook validation prevents such changes when the VM is powered on.
zhangheliu
force-pushed
the
zhangheliu/feature/shared-disk-cdrom
branch
from
95b0729 to
62901cd
Compare
Minimum allowed line rate is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Add comprehensive CD-ROM controller management for the VMSharedDisks
feature including:
This enables proper CD-ROM device management when the VMSharedDisks
capability is enabled, ensuring CD-ROMs are correctly assigned to
available controller slots and new controllers are created as needed.
When VMSharedDisks is enabled, the operator now supports changing CD-ROM
placement (controller type, bus number, or unit number) when the VM is
powered off. Placement changes are handled via a remove-and-add pattern,
removing the CD-ROM with outdated placement and recreating it with the
new controller assignment. Webhook validation prevents such changes when
the VM is powered on.
Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # N/A
Are there any special notes for your reviewer:
#1241 needs to be merged first.
Please add a release note if necessary: