extensions: propagate claim identity labels to Sandboxes and backing Pods#455
extensions: propagate claim identity labels to Sandboxes and backing Pods#455noeljackson 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: noeljackson 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 @noeljackson. 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 Regular contributors should join the org to skip this step. 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. |
|
|
||
| // ClaimNameLabel is propagated to Sandboxes and backing Pods so consumers can | ||
| // locate the workload associated with a SandboxClaim. | ||
| ClaimNameLabel = "agents.x-k8s.io/claim-name" |
There was a problem hiding this comment.
How about we just keep the claim-uid?
- What if the user deletes and recreates the claim with the same name? What happens to the labels of the underlying Pod?
uidis a better strategy for labelling as it has fixed character of 36 conforming to label limits.
There was a problem hiding this comment.
Agreed. We use the claim UID (not name) for exactly those reasons. The label key is agents.x-k8s.io/claim-uid and the value is the claim's .metadata.uid, which changes on delete/recreate. No duplicate constant -- we use SandboxIDLabel directly.
8015eb6 to
9ed6899
Compare
codebot-robot
left a comment
There was a problem hiding this comment.
Overall, the implementation correctly propagates the claim UID labels to both Sandboxes and their backing Pods, which is beneficial for tracking and discovery. The test coverage effectively validates this behavior across both cold-start and warm-pool adoption paths.
However, there is a notable issue: the newly introduced ClaimUIDLabel constant has the exact same string value (agents.x-k8s.io/claim-uid) as the already existing SandboxIDLabel constant. This duplication results in redundant map assignments in the controllers and overlapping assertions in the tests.
Please see the inline comments for specific suggestions on consolidating these constants and cleaning up the redundant code.
(This review was generated by Overseer)
|
|
||
| // ClaimUIDLabel is propagated to Sandboxes and backing Pods as a stable, | ||
| // immutable identity that survives delete/recreate of identically-named claims. | ||
| ClaimUIDLabel = "agents.x-k8s.io/claim-uid" |
There was a problem hiding this comment.
This constant has the exact same string value (agents.x-k8s.io/claim-uid) as SandboxIDLabel defined in sandboxtemplate_types.go. Defining two distinct constants for the exact same label key can be confusing and lead to redundant assignments. I recommend either reusing SandboxIDLabel directly or replacing SandboxIDLabel globally with ClaimUIDLabel if you feel it's a more accurate name.
There was a problem hiding this comment.
The current version uses SandboxIDLabel directly. No duplicate constant.
| adopted.Spec.PodTemplate.ObjectMeta.Labels = make(map[string]string) | ||
| } | ||
| adopted.Labels = ensureClaimIdentityLabels(adopted.Labels, claim) | ||
| adopted.Spec.PodTemplate.ObjectMeta.Labels = ensureClaimIdentityLabels(adopted.Spec.PodTemplate.ObjectMeta.Labels, claim) |
There was a problem hiding this comment.
Because ClaimUIDLabel (set inside ensureClaimIdentityLabels) and SandboxIDLabel share the exact same string value, this code ends up writing the exact same map key twice to adopted.Spec.PodTemplate.ObjectMeta.Labels. Consolidating the constants will resolve this duplication.
There was a problem hiding this comment.
Resolved. Using SandboxIDLabel only, no duplicate writes.
| sandbox.Spec.PodTemplate.ObjectMeta.Labels = make(map[string]string) | ||
| } | ||
| sandbox.Labels = ensureClaimIdentityLabels(sandbox.Labels, claim) | ||
| sandbox.Spec.PodTemplate.ObjectMeta.Labels = ensureClaimIdentityLabels(sandbox.Spec.PodTemplate.ObjectMeta.Labels, claim) |
There was a problem hiding this comment.
The manual nil check for sandbox.Spec.PodTemplate.ObjectMeta.Labels on the preceding lines (508-510) is now redundant since ensureClaimIdentityLabels already securely handles nil map initialization.
There was a problem hiding this comment.
Removed the redundant nil check. ensureClaimIdentityLabels handles nil initialization.
| } | ||
|
|
||
| // 2. Verify pod template claim-uid label was added | ||
| if val := adoptedSandbox.Spec.PodTemplate.ObjectMeta.Labels[extensionsv1alpha1.ClaimUIDLabel]; val != string(claim.UID) { |
There was a problem hiding this comment.
These two tests (step 2 and step 3) are asserting the exact same label key (agents.x-k8s.io/claim-uid) in PodTemplate.ObjectMeta.Labels, just using two different constants. If you consolidate the constants as suggested, one of these assertions can be removed.
There was a problem hiding this comment.
Already consolidated. Tests use SandboxIDLabel only.
| PodTemplate: sandboxv1alpha1.PodTemplate{ | ||
| ObjectMeta: sandboxv1alpha1.PodMetadata{ | ||
| Labels: map[string]string{ | ||
| extensionsv1alpha1.ClaimUIDLabel: "claim-uid-1", |
There was a problem hiding this comment.
If ClaimUIDLabel is removed in favor of SandboxIDLabel to prevent duplicate constants, ensure this test uses the correct constant to verify label propagation.
There was a problem hiding this comment.
Already consolidated. Tests use SandboxIDLabel only.
| if err != nil { | ||
| t.Fatalf("expected sandbox to be created but got error: %v", err) | ||
| } | ||
| if val := sandbox.Labels[extensionsv1alpha1.ClaimUIDLabel]; val != string(claim.UID) { |
There was a problem hiding this comment.
Like the adoption tests above, if the duplicate constants are consolidated, these assertions will need to be updated to use the single unified constant.
There was a problem hiding this comment.
Already consolidated. Tests use SandboxIDLabel only.
9ed6899 to
cc62407
Compare
cc62407 to
9546055
Compare
|
Friendly bump. All review comments addressed, tests pass. Could a maintainer run |
9546055 to
6bb656a
Compare
|
/ok-to-test |
When a SandboxClaim creates or adopts a Sandbox, the controller now propagates agents.x-k8s.io/claim-uid (via the existing SandboxIDLabel constant) to both the Sandbox CR and its Pod template labels. This consolidates with SandboxIDLabel rather than introducing a duplicate constant — both represent the claim UID for discovery and NetworkPolicy targeting.
6bb656a to
0bbf49b
Compare
|
/retest |
Summary
This adds two generic labels:
agents.x-k8s.io/claim-nameagents.x-k8s.io/claim-uidThey provide a consumer-agnostic way to locate the Sandbox and Pod associated with a SandboxClaim, including after warm-pool adoption.
Testing
GOTMPDIR=/home/noel/src/codewire/isol8/.tmp/go-build GOCACHE=/home/noel/src/codewire/isol8/.tmp/go-cache go test ./extensions/controllers -count=1GOTMPDIR=/home/noel/src/codewire/isol8/.tmp/go-build GOCACHE=/home/noel/src/codewire/isol8/.tmp/go-cache go test ./controllers -count=1