fix: reconcile selector and sandbox label drift for sandbox-owned Services#561
fix: reconcile selector and sandbox label drift for sandbox-owned Services#561rayowang wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rayowang The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @rayowang. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
codebot-robot
left a comment
There was a problem hiding this comment.
Overall, this is a clean and solid fix that addresses the service drift issue nicely. The PR successfully implements the drift reconciliation for metadata.labels and spec.selector on already-owned Services, preserving unrelated user labels as intended.
I've left a few minor, non-blocking suggestions detailing some improvements that may be helpful for long-term maintainability:
- Go Standards: Consider swapping
reflect.DeepEqualwith the newer standard library featuremaps.Equalfor better performance. - Observability: Include namespace context in the new logging statement.
- Controller best practices: A suggestion to evaluate
.Patchinstead of.Updateto mitigate potentialOptimisticLockErrors on highly-contested resources. - Testing: Recommendations to include
ClusterIP: "None"and a purelyniltest fixture to bulletproof the test suite and improve robustness against fake client behavior. - Behavior clarification: A quick confirmation regarding strict selector overwriting vs merging.
Great job explicitly handling map initializations to avoid panics. None of these block the PR.
(This review was generated by Overseer)
Fixes #560
Summary
This change makes
reconcileServicerepair critical drift on Services that are already owned by the Sandbox.Previously, the controller repaired unowned Services during adoption, but skipped reconciliation for already-owned Services. As a result, an owned Service could keep a stale selector or lose the sandbox label indefinitely.
With this change, owned Services now reconcile:
metadata.labels[agents.x-k8s.io/sandbox-name-hash]spec.selectorUnrelated labels are preserved, and the Service is only updated when drift is detected.