Add owner refs from guest options to config options - #1789
Draft
faisalabujabal wants to merge 5 commits into
Draft
Add owner refs from guest options to config options#1789faisalabujabal wants to merge 5 commits into
faisalabujabal wants to merge 5 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
hpannem asked on PR vmware-tanzu#1782 why VirtualMachineGuestOptions doesn't use owner references back to the VirtualMachineConfigOptions objects that report it, given controllers/configtarget already uses that pattern for VirtualMachineConfigOptions <- ConfigTarget. The status-based GC added there is correct on its own, but it depends on every reconcile running to completion; native Kubernetes garbage collection gives the same outcome for free if a VirtualMachineConfigOptions is force-deleted or otherwise removed outside a normal reconcile. Each contributing VirtualMachineConfigOptions now adds a (non-controller) owner reference to every VirtualMachineGuestOptions it fans out to, mirroring configtarget's reconcileConfigOptions. Deletion is gated on the owner-reference count rather than the status.hardwareVersions entry count, so a reconcile that fails between the status patch and the owner-reference patch resumes correctly on retry instead of leaking a reference. Tracked under vmop-3932 as a follow-up to the GC work in vmware-tanzu#1782.
Minimum allowed line rate is |
faisalabujabal
marked this pull request as draft
July 31, 2026 16:45
faisalabujabal
added
the
pro-ai-review-requested
Contributor
Author
Claude Code review — head
|
This was referenced Jul 31, 2026
Contributor
Author
|
needs rebase after #1782 |
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). hpannem asked there why
VirtualMachineGuestOptionsdoesn't carry owner references back to theVirtualMachineConfigOptionsobjects that report it, givencontrollers/configtargetalready uses that pattern one layer up(
VirtualMachineConfigOptions<-ConfigTarget). The status-based GClanded in #1782 is correct on its own, but it depends on every
reconcile running to completion. Native Kubernetes garbage collection
gives the same end state for free if a
VirtualMachineConfigOptionsisforce-deleted or otherwise removed outside a normal reconcile.
Each contributing
VirtualMachineConfigOptionsnow adds a(non-controller) owner reference to every
VirtualMachineGuestOptionsit fans out to, mirroring
configtarget'sreconcileConfigOptions.status.hardwareVersions[]is unchanged in shape and still carries allthe substantive per-hardware-version data (CPU/memory limits,
recommended controllers, etc.) -- an owner reference has no payload, so
it can't replace that field, only add a second, GC-visible signal
alongside it.
What did change from #1782: the deletion trigger in
removeHardwareVersionAndDeleteIfOrphanedmoves from "nostatus.hardwareVersionsentries remain" to "no owner referencesremain". In the normal path these two counts always move together (every
write path adds/removes both in the same pass), so this is not an
observable behavior change on its own -- the reason for the switch is
that gating on the owner-reference count makes a reconcile that fails
between the status patch and the owner-reference patch resume correctly
on retry (next pass finds no status entry, a no-op there, but still
finds the dangling owner reference and finishes removing it), instead of
silently leaking a reference forever.
Reviewing this diff before #1782 merges: because GitHub requires a
PR's base branch to exist in the base repository, and
vmop-3932-gc-unused-guestoptionsonly exists on my fork, this PR isopened against
mainand its diff currently includes #1782's fullchangeset. In particular, in
.sdd/specs/001-class-policy-resize/tasks.md,T093a and T093b were completed in #1782, not here -- only T093c
and T093d are new to this PR. Once #1782 merges this branch will be
rebased onto
mainand the diff will shrink to just theowner-reference work (commit
36a972d5).Which issue(s) is/are addressed by this PR?:
Tracked under vmop-3932 (same ticket as #1782).
Are there any special notes for your reviewer:
Hold review until #1782 merges. To review only the net-new work now,
look at commit
36a972d5directly rather than the full PR diff.Please add a release note if necessary: