fix: pre-NodeUpdate cleanup — metrics, deletion comment, drift-gated plans#93
Merged
fix: pre-NodeUpdate cleanup — metrics, deletion comment, drift-gated plans#93
Conversation
…plans Three cleanup items identified during the comprehensive architecture review: 1. Fix controllerName() in planner metrics — GVK is stripped by controller-runtime on fetched objects, so all metric labels were "unknown". Now uses a type switch on the known resource types. 2. Document the deletion path's separate Status().Patch() call — it's an intentional exception to the single-patch model, not an oversight. 3. Running-phase plans are now drift-gated — buildRunningPlan returns nil when spec.image == status.currentImage (no drift). This eliminates unnecessary plan creation/flush/execution every 30s for idle Running nodes, and establishes the extension point for NodeUpdate plans. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
Three items from the comprehensive architecture review, cleaning up before NodeUpdate implementation:
Fix
controllerName()metrics — GVK is stripped by controller-runtime on fetched objects, so all plan metric labels were"unknown". Now uses a type switch on known resource types.Document deletion path exception —
handleNodeDeletionhas its ownStatus().Patch()which is intentional (runs before the main reconcile flow). Added a comment so it's not mistaken for drift from the single-patch model.Drift-gated Running plans —
buildRunningPlannow returnsnilwhenspec.image == status.currentImage(no drift). This eliminates unnecessary plan creation every 30s for idle nodes and establishes the hook where NodeUpdate drift detection will live.Test plan
make build && make test && make lint— all green🤖 Generated with Claude Code