🐛 Atomic status patching and controller cleanups for VM publish operations - #1888
Draft
bryanv wants to merge 8 commits into
Draft
🐛 Atomic status patching and controller cleanups for VM publish operations#1888bryanv wants to merge 8 commits into
bryanv wants to merge 8 commits into
Conversation
Since the current patch helper splits the conditions and the rest of status into two separate patches, with conditions happening first, if the later fails, the short circuit checks in these controllers that were only checking the conditions means the rest of the status will never be updated. There is a deeper issue here in that the reasons that the patch helper pattern we adopted from CAPI long ago don't really apply to use - at least for these controllers - and I think in general so we need to revisit it. Until then, it is possible to observed an half-baked status since we cannot do status as an all or nothing.
We should use what is in the context. Different loggers were being passed into register and remove the publish metrics.
Just get the ConditionUploaded once so it is clearer what is being checked
If the group publish spec was invalid, the controller would update the condition with a reason that does not meet the field pattern for the field. This invalid spec is not expected due to the webhooks but fix the condition.
For objects that implement Getter/Setter - basically all of our types since we use the CAPI condition helpers as well - the patch helper would split out the conditions into a separate patch from the rest of the status. The reason CAPI has this is because they have multiple controllers updating status, which usually is not true for us. Splitting the conditions patch can allow a user to see a half-baked status, like for example a completed condition is true, but the expected resulting fields are not set in the status.
These objects have both conditions and other fields in the status that are expected to be set when the publish is complete. But by doing the conditions separately first, just the condition update can be observed by a user without the expected results. For example, for a group publish the completed condition is set but the image names are not.
The CompleteTime was being set separately from the Completed condition. In practice, it would be quite close to completed condition since it is set right afterwards. But the condition timestamp is truncated to the second, while the status StartTime was not, so I don't think that CompletionTime should be either.
The completion workflow spec built spec.ttlSecondsAfterFinished from a time.Duration, so the value was in nanoseconds rather than seconds. The intended "expires ten seconds from now" TTL was really ~332 years, and the spec only passed because multiplying that back out by time.Second overflowed int64 into a negative duration, which made the controller treat the TTL as already expired and delete the request immediately. Convert the duration with Seconds() and name the grace period so it stays well under the suite's ten second default Eventually timeout, which keeps the spec from racing that budget. Fixing the units alone is not enough. The spec only asserted eventual deletion, so it passed whether the TTL was correct or already expired. Add a Consistently guard that the request is retained until the TTL elapses, so the requeue-then-delete path in reconcileSpecTTL is actually exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bryanv
force-pushed
the
bryanv/vmpubcontroller-imagenamelsss-complete
branch
from
September 3, 2026 03:46
caf646a to
b83315d
Compare
Contributor
Minimum allowed line rate is |
bryanv
added
the
pro-ai-review-requested
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.
What does this PR do, and why is it needed?
Refactors the status patching logic across VirtualMachinePublishRequest and group publish controllers to prevent race conditions and incomplete status updates caused by split patch operations. It also includes minor cleanup for controller loggers, condition validation, and requeue logic.
Key Changes
• Atomic Status Updates: Added an option to the patch helper to issue a single status patch rather than splitting conditions and other status fields. Updated publish and group publish controllers to patch status in one atomic operation, ensuring fields like image names are set simultaneously with completion conditions.
• Controller Cleanups:
• Switched publish controllers to use contextual loggers instead of dedicated logger instances.
• Corrected invalid condition reasons on invalid group publish specs to comply with field patterns.
• Simplified condition checks in the publish request requeue logic.
• Cleaned up inaccurate documentation regarding CompleteTime behavior.
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 # vmop-4152
Are there any special notes for your reviewer:
Please add a release note if necessary: