Rename VirtualMachineGuestOptions hardwareVersion field to version - #1793
Draft
faisalabujabal wants to merge 7 commits into
Draft
Rename VirtualMachineGuestOptions hardwareVersion field to version#1793faisalabujabal wants to merge 7 commits into
faisalabujabal wants to merge 7 commits into
Conversation
The VirtualMachineConfigOptions controller fans out a VirtualMachineGuestOptions per guest OS descriptor and upserts a status.hardwareVersions entry per hardware version, but never removed that entry once a hardware version stopped reporting the guest OS -- unlike the analogous ConfigTarget -> VirtualMachineConfigOptions garbage collection. Add garbageCollectGuestOptions, invoked at the end of every VirtualMachineConfigOptions reconcile: it removes the current hardware version's status.hardwareVersions entry from any VirtualMachineGuestOptions no longer reporting that guest OS, and deletes the object outright once no hardware-version entries remain. The live set is keyed by the sanitized object name (not the raw guest OS ID) so two IDs that collide onto the same truncated, DNS-safe name cannot cause a spurious deletion. Update spec.md, research.md, and tasks.md for 001-class-policy-resize to reflect that this follow-up (previously called out as deferred) is now implemented. vmop-3932
QueryConfigOptionEx returning nil with no error is vSphere's positive answer that a hardware version has no config option in this environment -- not a transient signal, which surfaces as a non-nil error instead. ReconcileNormal treated that nil result as a dead end and returned before fanOutGuestOptions ran, so a hardware version that stopped being supported left its previously fanned-out VirtualMachineGuestOptions entries orphaned forever, even though the VirtualMachineConfigOptions object itself kept reconciling. Route the nil-config-option path through fanOutGuestOptions with an empty descriptor list instead, so it is pruned exactly like the already-covered non-nil-but-empty case. vmop-3932
removeHardwareVersionAndDeleteIfOrphaned fetches obj via the
caller's List, then deletes it unconditionally once its last
hardwareVersions entry is being removed. Between that List and the
Delete, another hardware version's reconcile could concurrently
upsert a new entry into this shared object; the unconditional delete
would silently discard that entry along with the object.
Pass client.Preconditions{ResourceVersion: &obj.ResourceVersion} to
Delete so a racing writer causes a conflict here instead, which
surfaces as a normal reconcile error and gets retried with fresh
data on the next attempt -- the same optimistic-locking approach
already used for this file's status patches.
Per automated review on vmop-3932.
vmop-3932
ReconcileDelete only removed the finalizer, so when a VirtualMachineConfigOptions was deleted -- e.g. by ConfigTarget's own GC dropping a hardware version -- any VirtualMachineGuestOptions it had fanned out kept their status.hardwareVersions entry forever, even though the owning object was gone. Run the same garbageCollectGuestOptions used by ReconcileNormal before removing the finalizer, so deletion prunes exactly like a live reconcile that observes zero descriptors. The finalizer is removed only after GC succeeds -- it exists to hold the object until cleanup completes, so removing it unconditionally would let a GC error vanish along with the object, and the leak would never retry. Per review feedback on vmop-3932 (why doesn't deleting the CR clean up its guest options?). vmop-3932
aruneshpa suggested slices.DeleteFunc instead of the manual find-index-then-splice in removeHardwareVersionAndDeleteIfOrphaned. Deleting is now decided by comparing the slice length before and after, rather than a separate found-index check, and the delete-vs- patch branch reads directly off the resulting length. base is captured before DeleteFunc runs, since DeleteFunc mutates the slice's backing array in place -- copying after would capture the already-pruned state and make the status patch below compute an empty diff.
Claude Code review on vmware-tanzu#1782 flagged that the fan-out write path has no defense against two guest OS descriptors sanitizing to the same object name, unlike the GC path which is deliberately keyed on the sanitized name for that exact reason -- a real collision would make reconcileGuestOptions patch spec.ID back and forth every reconcile, a self-sustaining hot loop. This is latent, not live: vSphere guest IDs are camelCase-alphanumeric, so a collision would need two IDs differing only by case, which vSphere doesn't produce. Document the assumption next to the helper that encodes it, so it's explicit rather than implied.
aruneshpa flagged on PR vmware-tanzu#1782 that VirtualMachineGuestOptionsHardwareVersionStatus.HardwareVersion stutters with its parent field, HardwareVersions -- giving status.hardwareVersions[].hardwareVersion instead of status.hardwareVersions[].version. vim.vmware.com/v1alpha1 has never been tagged or released (no shipped consumers), so this is a clean rename rather than a breaking API change requiring a conversion webhook. Updated the +listMapKey marker alongside the field, and regenerated the deepcopy code and external CRD manifest via controller-gen.
Minimum allowed line rate is |
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
1 participant
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.
Draft — stacked on #1782, do not review until that merges.
What does this PR do, and why is it needed?
Follow-up to #1782 (vmop-3932). aruneshpa flagged that
VirtualMachineGuestOptionsHardwareVersionStatus.HardwareVersionstutters with its parent field,
HardwareVersions-- givingstatus.hardwareVersions[].hardwareVersioninstead ofstatus.hardwareVersions[].version.Confirmed
vim.vmware.com/v1alpha1has never been tagged or released(no shipped consumers), so this is a clean rename rather than a
breaking API change requiring a conversion webhook. Updated the
+listMapKeymarker alongside the field, and regenerated the deepcopycode and external CRD manifest via
controller-gen.This PR stacks on #1782, same as #1789: opened against
mainonly because GitHub requires a PR's base branch to exist in the base
repository, and
vmop-3932-gc-unused-guestoptionsonly exists on myfork. The diff currently includes #1782's full changeset. Once #1782
merges this branch will be rebased onto
mainand the diff willshrink to just this rename (commit
c35ca36e).Which issue(s) is/are addressed by this PR?:
Tracked under vmop-3932 (same ticket as #1782 and #1789).
Are there any special notes for your reviewer:
Hold review until #1782 merges. To review only the net-new work now,
look at commit
c35ca36edirectly rather than the full PR diff.Please add a release note if necessary:
— Faisal + Claude