Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where deprecated managed annotations could linger on services upgraded from older MetalLB versions. It updates convergeBalancer to delete the deprecated annotation and adds a corresponding unit test to verify this behavior. The feedback suggests also removing this deprecated annotation in clearServiceState to ensure it is properly cleaned up when a service is unassigned or cleared.
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.
| // Drop the deprecated managed annotation that may linger on services | ||
| // allocated by an older MetalLB version, so it does not coexist with the | ||
| // current one after an upgrade. See https://github.com/metallb/metallb/issues/2642. | ||
| delete(svc.Annotations, DeprecatedAnnotationIPAllocateFromPool) |
There was a problem hiding this comment.
While deleting the deprecated managed annotation DeprecatedAnnotationIPAllocateFromPool in convergeBalancer is correct, we should also ensure it is removed when the service state is cleared in clearServiceState (around line 251). If a service carrying the deprecated annotation is unassigned or cleared, clearServiceState currently only deletes the active AnnotationIPAllocateFromPool annotation, leaving the deprecated one behind. This would falsely indicate to users or external tools that the service is still allocated from a pool.
There was a problem hiding this comment.
Thanks — this is already covered in the same commit. clearServiceState drops the deprecated annotation right after the active one:
func (c *controller) clearServiceState(key string, svc *v1.Service) {
c.ips.Unassign(key)
delete(svc.Annotations, AnnotationIPAllocateFromPool)
// Also drop the deprecated managed annotation so an unassigned or cleared
// service does not keep falsely advertising a pool allocation after an
// upgrade.
delete(svc.Annotations, DeprecatedAnnotationIPAllocateFromPool)
svc.Status.LoadBalancer = v1.LoadBalancerStatus{}
}There is a dedicated regression test for exactly this path too — TestControllerClearsDeprecatedManagedAnnotation in controller/controller_test.go, alongside TestControllerRemovesDeprecatedManagedAnnotation for the convergeBalancer path. Both pass.
I've also just rebased onto main to clear the stale static-security-analysis failure.
19c566e to
e48b493
Compare
|
The This is a known incompatibility between |
e48b493 to
91e4dd8
Compare
|
Could I ask for a re-run of the The failing step is Deploy MetalLB, so zero specs ran (the follow-on Also, is For context on review state: the only review this PR ever received was from |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Signed-off-by: somaz <genius5711@gmail.com>
91e4dd8 to
4673254
Compare
|
This PR has been automatically marked as stale because it has been open 30 days
|
|
Still relevant — this fixes #2642 and the diff hasn't changed. On the red Could someone re-run that job or apply a hold label? Attempt 2 is already used up. |
|
Updating my earlier note on the red checks, which is now out of date.
The same job fails identically on
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe controller now removes deprecated managed annotations when allocating or clearing service state. Tests record updated services and verify cleanup for LoadBalancer and non-LoadBalancer services. ChangesManaged annotation cleanup
Priority: ⬇️ Low — Impact reflects low issue severity. Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Services upgraded from older MetalLB versions will have the deprecated managed IP-pool annotation removed while current allocation state is retained or cleared as appropriate. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
@oribon this is the follow-up to #3079: it removes the deprecated managed annotation after an upgrade, so the state left by the old controller does not linger. When you have a moment, could you take a look? The CI matrix on this head is red, but it is red the same way on every PR this week, including the dependabot bumps, so I do not think it is this change. I will rebase to re-run it whenever you prefer. |
The controller writes the current metallb.io/ip-allocated-from-pool managed
annotation but never removes the deprecated metallb.universe.tf/ip-allocated-from-pool
one, so a service allocated by an older MetalLB version keeps both annotations
after an upgrade (#2642). This deletes the deprecated managed annotation in
convergeBalancer right after the current one is set.
Scope: only the controller-managed annotation is removed. User-set deprecated
annotations (address-pool, loadBalancerIPs, allow-shared-ip) are left untouched
since the controller does not own them.
Validation (local):
change (service keeps both annotations) and passes after
Fixes #2642
Summary by CodeRabbit