✨ Mount vm snapshot disk - #1857
Draft
cc005511 wants to merge 3 commits into
Draft
Conversation
cc005511
force-pushed
the
mount_vm_snapshot_disk
branch
3 times, most recently
from
September 7, 2026 06:45
fc01553 to
c74b40e
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
cc005511
force-pushed
the
mount_vm_snapshot_disk
branch
2 times, most recently
from
September 7, 2026 10:10
22a2e4d to
bf97481
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
cc005511
force-pushed
the
mount_vm_snapshot_disk
branch
from
September 8, 2026 08:44
bf97481 to
661ea04
Compare
stevenhuang-vcf
suggested changes
Sep 10, 2026
stevenhuang-vcf
left a comment
stevenhuang-vcf
left a comment
There was a problem hiding this comment.
You need to add more comments in the code.
Comment on lines
+881
to
+886
| } else if volSpec.VirtualMachineSnapshot != nil { | ||
| // For snapshot volumes, we don't block power on for them being unattached here, as they are attached | ||
| // during the reconcile loop itself (in reconcileSnapshotDisks), unlike PVCs which | ||
| // are attached by an external controller. | ||
| // We also don't require them to be in status yet, as they might be added in the same reconcile loop. | ||
| continue |
There was a problem hiding this comment.
This else if is meaningless here as your description. So why not just add comments without the check of the spec type of VM snapshot disk?
| case *vimtypes.VirtualDiskSparseVer2BackingInfo: | ||
| return &vimtypes.VirtualDiskSparseVer2BackingInfo{VirtualDeviceFileBackingInfo: vimtypes.VirtualDeviceFileBackingInfo{FileName: backing.FileName}, DiskMode: string(vimtypes.VirtualDiskModeIndependent_nonpersistent), Parent: backing, Uuid: backing.Uuid} | ||
| default: | ||
| return targetDisk.Backing |
There was a problem hiding this comment.
should return the target backing info for a unsupported type?
Comparing the logic in isSnapshotDisk only VirtualDiskFlatVer2BackingInfo, VirtualDiskSeSparseBackingInfo and VirtualDiskSparseVer2BackingInfo are supported.
Comment on lines
+1899
to
+1908
| if targetDisk == nil && len(moSnap.Config.Hardware.Device) == 0 && moVM.Config != nil { | ||
| for _, dev := range moVM.Config.Hardware.Device { | ||
| if disk, ok := dev.(*vimtypes.VirtualDisk); ok { | ||
| if strings.EqualFold(getVirtualDiskUUID(disk), diskID) { | ||
| targetDisk = disk | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why need to look for a target snapshot disk in a VM's device list?
I think it should be in a VM snapshot's device list. Is there any specific case?
stevenhuang-vcf
suggested changes
Sep 10, 2026
| } | ||
| } | ||
|
|
||
| if err := reconcileSnapshotDisks( |
| pkglog.FromContextOrDefault(ctx).Info("Appended new volume to status", "volName", volName, "newVolumesLen", len(vm.Status.Volumes)) | ||
| } | ||
|
|
||
| func isSnapshotDiskAttached(moVM mo.VirtualMachine, configSpec *vimtypes.VirtualMachineConfigSpec, diskID string) bool { |
There was a problem hiding this comment.
why not as setSnapshotVolumeAttached does, the check runs on the status list of volumes?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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?
VirtualMachineSnapshotDiskSpec is used to define a specific disk from a VirtualMachineSnapshot. VirtualMachineSnapshotDiskSpec field added in VirtualMachineVolumeSource is to support mounting a VirtualMachine snapshot disk as a volume of the VM service VM.
PASS
go test -v ./pkg/providers/vsphere/session/...
go test -v ./webhooks/virtualmachine/validation/...
go test -v ./controllers/virtualmachine/volume/...
Are there any special notes for your reviewer:
Expose the disk list in VirtualMachineSnapshot: #1744
CRD change of supporting VM snapshot disk as the volume source: #1751
Please add a release note if necessary:
Support mounting a VM snapshot disk as a VM volume. When using the VM Snapshot disk as the volume source, diskMode must be set to "IndependentNonPersistent" and removable must be set to true.