✨ Populate diskMode, provisioning mode and Scrubing of disks in VM status #1337
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1283,6 +1283,55 @@ func updateStorageUsage(vmCtx pkgctx.VirtualMachineContext) []error { | |
| return errs | ||
| } | ||
|
|
||
| // convertDiskMode converts vSphere disk mode string to vmopv1.VolumeDiskMode. | ||
| func convertDiskMode(diskMode string) vmopv1.VolumeDiskMode { | ||
| switch diskMode { | ||
| case string(vimtypes.VirtualDiskModeIndependent_persistent): | ||
| return vmopv1.VolumeDiskModeIndependentPersistent | ||
| case string(vimtypes.VirtualDiskModeIndependent_nonpersistent): | ||
| return vmopv1.VolumeDiskModeIndependentNonPersistent | ||
| case string(vimtypes.VirtualDiskModeNonpersistent): | ||
| return vmopv1.VolumeDiskModeNonPersistent | ||
| case string(vimtypes.VirtualDiskModePersistent): | ||
| return vmopv1.VolumeDiskModePersistent | ||
| default: | ||
| // Default to persistent if unknown or empty. | ||
| return vmopv1.VolumeDiskModePersistent | ||
| } | ||
| } | ||
|
|
||
| // convertSharingMode converts vSphere sharing mode to vmopv1.VolumeSharingMode. | ||
| func convertSharingMode(sharing vimtypes.VirtualDiskSharing) vmopv1.VolumeSharingMode { | ||
| switch sharing { | ||
| case vimtypes.VirtualDiskSharingSharingMultiWriter: | ||
| return vmopv1.VolumeSharingModeMultiWriter | ||
| case vimtypes.VirtualDiskSharingSharingNone: | ||
| return vmopv1.VolumeSharingModeNone | ||
| default: | ||
| // Default to None if unknown or empty. | ||
| return vmopv1.VolumeSharingModeNone | ||
| } | ||
| } | ||
|
|
||
| // convertProvisioningMode determines the provisioning mode from ThinProvisioned | ||
| // and EagerlyScrub flags. | ||
| func convertProvisioningMode(thinProvisioned, eagerlyScrub *bool) vmopv1.VolumeProvisioningMode { | ||
| // If ThinProvisioned is true, it's thin provisioned. | ||
| if thinProvisioned != nil && *thinProvisioned { | ||
| return vmopv1.VolumeProvisioningModeThin | ||
| } | ||
| // If ThinProvisioned is false or nil, check EagerlyScrub for thick variants. | ||
| if eagerlyScrub != nil && *eagerlyScrub { | ||
| return vmopv1.VolumeProvisioningModeThickEagerZero | ||
| } | ||
| // Default to thick (lazy zeroed) if thinProvisioned is false. | ||
| if thinProvisioned != nil && !*thinProvisioned { | ||
| return vmopv1.VolumeProvisioningModeThick | ||
| } | ||
| // If both are nil, return empty string (unknown/unset). | ||
| return "" | ||
| } | ||
|
|
||
| func updateVolumeStatus(vmCtx pkgctx.VirtualMachineContext) { | ||
| var ( | ||
| moVM = vmCtx.MoVM | ||
|
|
@@ -1356,6 +1405,8 @@ func updateVolumeStatus(vmCtx pkgctx.VirtualMachineContext) { | |
| KeyID: ddi.CryptoKey.KeyID, | ||
| } | ||
| } | ||
| // Note: DiskMode, SharingMode, and ProvisioningMode are set later | ||
| // in a single pass for all volumes (both classic and managed). | ||
| // This is for a rare case when VM is upgraded from v1alpha3 to | ||
| // v1alpha4+. Since vm.status.volume.requested was introduced in | ||
| // v1alpha4. So we need to patch it if it's missing from status for | ||
|
|
@@ -1420,6 +1471,8 @@ func updateVolumeStatus(vmCtx pkgctx.VirtualMachineContext) { | |
| KeyID: ddi.CryptoKey.KeyID, | ||
| } | ||
| } | ||
| // Note: DiskMode, SharingMode, and ProvisioningMode are set later | ||
| // in a single pass for all volumes (both classic and managed). | ||
| vm.Status.Volumes = append(vm.Status.Volumes, volStatus) | ||
| } | ||
| } | ||
|
|
@@ -1437,6 +1490,45 @@ func updateVolumeStatus(vmCtx pkgctx.VirtualMachineContext) { | |
| } | ||
| }) | ||
|
|
||
| // Update disk attachment properties for ALL volumes (classic and managed). | ||
| // This is done in a single pass at the end to ensure consistency. | ||
| // For managed volumes, the volume controllers populate status from CnsNodeVmAttachment, | ||
|
Contributor
There was a problem hiding this comment. Better to copy over the ProvisioningMode in volumeBatch controller as well https://github.com/vmware-tanzu/vm-operator/blob/main/controllers/virtualmachine/volumebatch/volumebatch_controller.go#L650 Otherwise it will keep getting removing and adding
Contributor
There was a problem hiding this comment. This got me thinking, we are also patching status.volume with |
||
| // which doesn't include DiskMode, SharingMode, or ProvisioningMode. | ||
| // We populate these properties here by matching DiskUUID with vSphere disk info. | ||
| diskInfoByUUID := make(map[string]pkgvol.VirtualDiskInfo) | ||
| for _, di := range info.Disks { | ||
| if di.UUID != "" { | ||
| diskInfoByUUID[di.UUID] = di | ||
| } | ||
| } | ||
|
|
||
| for i := range vm.Status.Volumes { | ||
| vol := &vm.Status.Volumes[i] | ||
| if vol.DiskUUID == "" { | ||
| continue | ||
| } | ||
|
|
||
| di, ok := diskInfoByUUID[vol.DiskUUID] | ||
| if !ok { | ||
| vmCtx.Logger.V(4).Info("No disk info found for volume", | ||
| "volumeName", vol.Name, | ||
| "volumeType", vol.Type, | ||
| "diskUUID", vol.DiskUUID) | ||
| continue | ||
| } | ||
|
|
||
| // Populate disk attachment properties from vSphere disk info. | ||
| if di.DiskMode != "" { | ||
|
Contributor
There was a problem hiding this comment. If these are "", should the status field be cleared?
Collaborator
Author
There was a problem hiding this comment. Didn't bother about that because a diskMode going from not-empty to empty is not a valid scenario unless the disk's backing is changed to the two specific backing that doesn't have disk mode:
https://github.com/vmware-tanzu/vm-operator/blob/main/pkg/util/devices.go#L288-L335. Also, for regular disks, diskMode is immutable property, so there's no case of a disk's mode changing once it is non-empty. LMK if this makes sense. |
||
| vol.DiskMode = convertDiskMode(di.DiskMode) | ||
| } | ||
| if di.Sharing != "" { | ||
| vol.SharingMode = convertSharingMode(di.Sharing) | ||
| } | ||
| if provMode := convertProvisioningMode(di.ThinProvisioned, di.EagerlyScrub); provMode != "" { | ||
| vol.ProvisioningMode = provMode | ||
| } | ||
| } | ||
|
|
||
| // This sort order is consistent with the logic from the volumes controller. | ||
| vmopv1.SortVirtualMachineVolumeStatuses(vm.Status.Volumes) | ||
| } | ||
|
|
||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by consistency here? And why not in the earlier loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier loop has separate handling for classic / unmanaged disks and PVCs. So, if I handle it there, I would need to duplicate it in both, if and else blocks.
Consistency may have been wrong choice of a word.