fix(kubernetes): label allocated pool pods with sandbox identity - #1610
fix(kubernetes): label allocated pool pods with sandbox identity#1610Kavinjsir wants to merge 1 commit into
Conversation
github-actions
Bot
added
component/k8s
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dfcdfd5a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := r.syncPodSandboxLabels(ctx, newlyAllocatedPods, newPodAllocation); err != nil { | ||
| return err |
There was a problem hiding this comment.
When several newly allocated Pods are patched and one patch fails after another succeeds, this return skips every SyncSandboxAllocation call but leaves the successful Pod labels in place. Until a later reconciliation completes, those idle Pods advertise an unpublished sandbox identity to telemetry and label-based snapshot lookups. Converge the Pods back to the allocator's current view before returning, as is already done for annotation-publication failures.
AGENTS.md reference: kubernetes/AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
| if updated.Labels == nil { | ||
| updated.Labels = make(map[string]string) | ||
| } | ||
| updated.Labels[LabelBatchSandboxNameKey] = sandboxName |
There was a problem hiding this comment.
For a valid BatchSandbox whose metadata.name exceeds 63 characters, assigning the raw name here produces an invalid Kubernetes label value. The Pod patch is therefore rejected before the allocation annotation is published, leaving pooled sandboxes with such names permanently unschedulable even though Kubernetes resource names may be longer. Encode a label-safe identity or otherwise preserve lookup without using the full name as a label value.
AGENTS.md reference: kubernetes/AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return toSyncMap, orphanPods | ||
| } | ||
|
|
||
| func (r *PoolReconciler) scheduleSandbox(ctx context.Context, pool *sandboxv1alpha1.Pool, batchSandboxes []*sandboxv1alpha1.BatchSandbox, pods []*corev1.Pod) (*ScheduleResult, error) { |
There was a problem hiding this comment.
我这里有一些建议，可以参考下：
（1）在pod上打标这个事情会对性能有影响，因此应该作为pool级别的可选项（可以参考Recycle的设计）
（2）可以参考Recycle的设计，将这个功能设计成一个通用的Sandbox初始化功能，以在将Pod交付给Sandbox使用前对Pod做一些操作，这些操作可以拓展。
doAllocate: Allocate -> Init(本次新增，用于对pod做一些操作，命名可以再斟酌下) -> SyncToSandbox
doRelease: Recycle(你可以在这里拓展recycle的实现用于去掉pod标签) -> Release -> SyncToSandbox
Here are some suggestions for your reference:
- Since labeling Pods incurs a performance overhead, this should be implemented as an optional feature at the pool level (similar to the design of the Recycle mechanism).
- Drawing on the Recycle pattern, consider designing this functionality as a generic Sandbox initialization hook. This would allow for extensible pre-processing operations on Pods before they are handed over to the Sandbox.
Suggested workflow:
● doAllocate: Allocate → Init (newly added step for Pod pre-processing; naming subject to refinement) → SyncToSandbox
● doRelease: Recycle (the implementation can be extended here to handle Pod label removal) → Release → SyncToSandbox
5 participants
Adds the existing BatchSandbox name label to warm Pool Pods before the allocation annotation is published, and removes it when Pods return idle. This gives pooled and direct-created Pods one stable identity for node-local telemetry without changing scheduling choices. Publication failures compensate labels to the allocator final state, disappearing Pods block publication, and label Patch concurrency is memory-bounded.\n\nValidation: Go 1.25 make test; Go 1.25 golangci-lint; focused race tests for ordering, deletion, partial failure, release, reassignment, no-op, and bounded concurrency.