fix(controller): rebalance-pending annotation strip never reached the CRD#139
Conversation
The k8s store's mergeUserAnnotationsInto treats a nil wire-side annotation map as "leave stored annotations untouched" and a non-nil (including empty) map as "replace the user-annotation set". The InMemory store replaced rows wholesale on Update/Patch, so the two backends silently diverged and production-only behaviour was invisible to unit tests. Carry stored annotations across nil wire maps in the RG, RD, Resource and Snapshot Update/Patch paths and document the contract on the store interfaces. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
… CRD stripRebalanceAnnotation and stripShortfallAnnotation nil-ed the annotation map once it became empty, but the store Update contract reads a nil wire map as "annotations untouched", so deleting the last marker never propagated to the CRD and the rebalance pass re-fired on every event. Keep the emptied map non-nil so the strip persists; the wire JSON shape is unchanged because the annotations field is omitempty either way. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Cover the three transitions (nil = untouched, non-nil = replace, empty = clear) for RG, RD, Resource and Snapshot stores so the InMemory and CRD-backed implementations cannot diverge again. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Drive the real strip paths through the CRD-backed store against envtest and assert the markers are gone from the CRD: a lone rebalance-pending annotation stripped via Reconcile, a multi-key strip that must keep sibling annotations, and the spawn-shortfall strip on an RD. The lone-marker specs fail without the fix. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Warning Review limit reached
More reviews will be available in 18 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses Bug-021, where deleting the last remaining annotation on a resource would silently fail because setting the annotation map to nil was interpreted by the Kubernetes backend as 'annotations untouched'. The changes ensure that empty annotation maps are kept non-nil during stripping operations, and the in-memory store implementation is updated to match this contract. Comprehensive integration and unit tests have been added to verify this behavior. The reviewer's feedback is highly constructive, pointing out that since Go maps are reference types, we should clone the annotations using maps.Clone when carrying them over or before passing them to mutator functions to prevent accidental in-place mutations from leaking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func carryAnnotationsOnNil(next, prev map[string]string) map[string]string { | ||
| if next == nil { | ||
| return prev | ||
| } | ||
|
|
||
| return next | ||
| } |
There was a problem hiding this comment.
To prevent accidental mutations of the stored annotations from leaking or being shared across different updates/retries, we should clone the prev map using maps.Clone when carrying it over. Additionally, we can define a package-level helper cloneAnnotations to safely clone annotations in other store files without needing to import maps everywhere.
| func carryAnnotationsOnNil(next, prev map[string]string) map[string]string { | |
| if next == nil { | |
| return prev | |
| } | |
| return next | |
| } | |
| func carryAnnotationsOnNil(next, prev map[string]string) map[string]string { | |
| if next == nil { | |
| return maps.Clone(prev) | |
| } | |
| return next | |
| } | |
| func cloneAnnotations(m map[string]string) map[string]string { | |
| return maps.Clone(m) | |
| } |
| return errors.Wrapf(ErrNotFound, "resource %q on node %q", rdName, node) | ||
| } | ||
|
|
||
| prevAnnotations := r.Annotations |
There was a problem hiding this comment.
Since r.Annotations is a map (reference type), passing &r to mutate allows the mutator to modify the map in-place. If the mutator mutates the map and then sets r.Annotations = nil, prevAnnotations (which points to the same map) will also reflect those mutations, violating the 'untouched' contract. Cloning the annotations before calling mutate ensures prevAnnotations remains a true, untouched snapshot of the original annotations.
| prevAnnotations := r.Annotations | |
| prevAnnotations := cloneAnnotations(r.Annotations) |
| return errors.Wrapf(ErrNotFound, "resource definition %q", name) | ||
| } | ||
|
|
||
| prevAnnotations := rd.Annotations |
There was a problem hiding this comment.
Since rd.Annotations is a map (reference type), passing &rd to mutate allows the mutator to modify the map in-place. If the mutator mutates the map and then sets rd.Annotations = nil, prevAnnotations (which points to the same map) will also reflect those mutations, violating the 'untouched' contract. Cloning the annotations before calling mutate ensures prevAnnotations remains a true, untouched snapshot of the original annotations.
| prevAnnotations := rd.Annotations | |
| prevAnnotations := cloneAnnotations(rd.Annotations) |
| return errors.Wrapf(ErrNotFound, "resource group %q", name) | ||
| } | ||
|
|
||
| prevAnnotations := rg.Annotations |
There was a problem hiding this comment.
Since rg.Annotations is a map (reference type), passing &rg to mutate allows the mutator to modify the map in-place. If the mutator mutates the map and then sets rg.Annotations = nil, prevAnnotations (which points to the same map) will also reflect those mutations, violating the 'untouched' contract. Cloning the annotations before calling mutate ensures prevAnnotations remains a true, untouched snapshot of the original annotations.
| prevAnnotations := rg.Annotations | |
| prevAnnotations := cloneAnnotations(rg.Annotations) |
1 participant
Problem
The
blockstor.io/rebalance-pendingRG annotation could never be removed in production: once the rebalance pass completed, the strip helper nil-ed the wire annotation map when it became empty, but the k8s store's annotation merge treats a nil wire map as "annotations untouched" (deliberately, to protect reconciler-stamped keys). The deletion therefore never reached the CRD and the rebalance pass re-fired on every RG event — observed as the annotation persisting indefinitely with constant placer churn. The InMemory store replaced rows wholesale, which is why unit tests never caught it. Theblockstor.io/spawn-shortfallstrip on RDs had the identical defect.Fix
Annotationsmap means "leave stored annotations untouched"; a non-nil map — including an empty one — means "replace the user-annotation set with exactly this set". The k8s backend already behaved this way; the InMemory backend now carries stored annotations across nil wire maps in the RG, RD, Resource and Snapshot Update/Patch paths, so both backends are behaviourally identical. The contract is documented on the store interfaces.stripRebalanceAnnotationandstripShortfallAnnotationkeep the emptied map non-nil so the deletion propagates. The wire JSON shape is unchanged (annotations,omitemptyelides empty and nil alike).Tests
pkg/store/storetest):UpdateAnnotationContractsubtests cover all three transitions (nil = untouched, non-nil = replace, empty = clear) for RG, RD, Resource and Snapshot, and run against both backends. Red against the pre-fix InMemory store.internal/controller): drives the real strip paths through the CRD-backed store against envtest and asserts the markers are gone from the CRD — a lone rebalance-pending annotation stripped viaReconcile, a multi-key strip that must keep sibling annotations, and the spawn-shortfall strip. The lone-marker specs fail without the fix.Verified locally:
go test ./...,go test -race ./internal/controller/...andgolangci-lint runare clean; the operator-level integration pinTestGroupKWFSpawnAndDependentReAutoplace(PR #137 branch) goes red on main and green with this fix.