[Proposal] add sandbox-pool management proposal#431
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Welcome @lichuqiang! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
|
/cc @RainbowMango |
![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 introduces a design proposal for SandboxPool Resource Pool Management, which outlines a two-layer architecture to declaratively manage and elastically scale node-level sandbox resource pools. Feedback on the proposal includes fixing a markdown rendering issue with un-opened code blocks, using resource.Quantity instead of string for CPU and memory fields to align with Kubernetes API standards, and refining the state machine logic to ensure all node conditions are re-evaluated when a node is recovered rather than unconditionally transitioning to a Ready state.
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.
There was a problem hiding this comment.
Pull request overview
Adds a new design proposal documenting the “slow track” SandboxPool resource pool management architecture, intended to address AgentCube’s current Kubernetes control-plane bottlenecks (startup latency and scale) discussed in #430.
Changes:
- Introduces a new proposal document describing a two-layer SandboxPool architecture (controller + node-local placeholder-agent).
- Defines proposed CRD data models (SandboxPoolClass/SandboxPool), status/conditions ownership (SSA FieldOwner), and a phase state machine.
- Documents operational flows (create/update/delete), RBAC, webhook validation rules, and a phased implementation plan.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
==========================================
+ Coverage 58.41% 58.44% +0.02%
==========================================
Files 36 36
Lines 3463 3463
==========================================
+ Hits 2023 2024 +1
+ Misses 1231 1229 -2
- Partials 209 210 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Could this PR use My understanding is that #430 is a broader architecture discussion, while this proposal focuses on the slow resource track / SandboxPool management. Using |
|
@lichuqiang please commit signoff statement too , dco also failling |
abso , this is discussion issue not a bug or need to fix , u should first purpose and discuss then open pr @lichuqiang |
|
I want to better understand the relationship with the existing agent-sandbox WarmPool path. Today |
RainbowMango
left a comment
RainbowMango
left a comment
There was a problem hiding this comment.
Review is still ongoing.
|
|
||
| - Using the **Static Pod model** to lock scheduling resources; mirror pods are automatically rebuilt after accidental deletion. | ||
| - Declarative state synchronization based on **CRD Watch/List**. | ||
| - Implements vertical resource scaling via **containerd runtime shim (v2 Task API)** (VPA analogy), independent of InPlacePodVerticalScaling Feature Gate. |
There was a problem hiding this comment.
Can you help add a link to the containerd runtime shim (v2 Task API) as a reference?
There was a problem hiding this comment.
I see, but that's a dead link: https://github.com/containerd/containerd/blob/main/api/runtime/task/v2/README.md
ranxi2001
left a comment
ranxi2001
left a comment
There was a problem hiding this comment.
Follow-up review on current head c2f2502 after reading the full proposal and existing threads. These inline comments focus on design contracts that are new or remain unresolved after the latest edits. I used AI-assisted tooling to help trace the proposal and verified each concern against the current text and upstream contracts.
Signed-off-by: lichuqiang <[email protected]>
|
Given that quite a few commits were added to address review comments, I squashed them to make it easier to read. |
| // Target node selector; immutable after creation | ||
| Selector metav1.LabelSelector `json:"selector"` |
There was a problem hiding this comment.
Would you mind explaining why the Selector is immutable? What's your concern here?
It would be great to explain it in comments.
| Selector metav1.LabelSelector `json:"selector"` | ||
|
|
||
| // Additional node selection criteria | ||
| NodeSelector map[string]string `json:"nodeSelector,omitempty"` |
There was a problem hiding this comment.
Can you elaborate on why the NodeSelector is needed in addition to the Selector above? What's the relationship between them?
There was a problem hiding this comment.
Actually, this isn't necessary — it's a redundant design. Let me revise these API design suggestions
| // Per-node resource quota | ||
| ResourcePolicy ResourcePolicy `json:"resourcePolicy"` |
There was a problem hiding this comment.
Listing each resource as a field doesn't scale. With hardcoded CPU and Memory, we need to update the API when we try to support a new resource type, like GPU, hugepages, or vendor-specific resources like example.com/cpu. We should use the same structure as Pod, i.e. corev1.ResourceList:
Resources corev1.ResourceList `json:"resources"`By the way, the name ResourcePolicy is not accurate; there is no policy here, just a declaration of resource amounts.
There was a problem hiding this comment.
I prefer the current naming convention to preserve room for future extension — for example, someone has already mentioned the possibility of specifying resource specs by percentage later on.
| // Placeholder Pod template | ||
| PlaceholderPodTemplate *PlaceholderPodTemplateSpec `json:"placeholderPodTemplate,omitempty"` |
There was a problem hiding this comment.
| // Placeholder Pod template | |
| PlaceholderPodTemplate *PlaceholderPodTemplateSpec `json:"placeholderPodTemplate,omitempty"` | |
| // Placeholder Pod template | |
| PlaceholderPod *PlaceholderPodTemplate `json:"placeholderPod,omitempty"` |
| // node-ctl endpoint (informational only; placeholder-agent obtains the actual address | ||
| // via --node-ctl-socket startup parameter, not from this field) | ||
| // Reserved for future declarative reconciliation; currently only unix:// supported | ||
| NodeCtlEndpoint string `json:"nodeCtlEndpoint,omitempty"` |
There was a problem hiding this comment.
Do we really expect the user to provide the information? I bet not, and suggest removing it from the API.
| **Spec Definition**: | ||
|
|
||
| ```go | ||
| type SandboxPoolSpec struct { |
There was a problem hiding this comment.
The comments in SandboxPoolSpec (and its sub-types like OverrideSpec , ClassRef , NodeCtlConfig ) are too brief to understand the design intent.
Could you please add complete comments for each field? Following the Kubernetes API convention: what the field means, who sets it, whether it is optional/immutable, and the default behavior when unset.
| 1. Watch detects new SandboxPool CRD (filtered by node label) | ||
| 2. Generate and write manifest file → /etc/kubernetes/manifests/sandbox-pool-{pool}.yaml | ||
| 3. kubelet detects new manifest → creates Static Pod | ||
| 4. kubelet → containerd (CRI) → starts placeholder-agent shim by runtimeClassName |
There was a problem hiding this comment.
Do we need to change kubelet's configuration to let it handle the customized runtime class?
If yes, we need to document it, as it is a serious constraint.
There was a problem hiding this comment.
No, as described here, it actually registers as a runtime shim implementation under containerd, so kubelet doesn't need to be directly aware of it.
|
|
||
| > **Security Boundary — Per-Node Status Write Scoping**: The `sandbox-pool-placeholder-agent` ClusterRole grants `sandboxpools/status (patch)` cluster-wide, which technically allows any node's agent to patch any Pool's status. To prevent a compromised node from forging another node's pool health, the ValidatingWebhook enforces a server-side write boundary: on a SandboxPool STATUS UPDATE, the webhook inspects the authenticated request identity (`userInfo.username` / `userInfo.groups` from the admission review) and rejects the request with `Forbidden` unless the caller is bound to `spec.nodeName`. Concretely, each placeholder-agent authenticates as a per-node identity — either a node-scoped client cert yielding `system:node:<nodeName>` in group `system:nodes`, or a dedicated per-node ServiceAccount whose name encodes the node name. The webhook extracts `<nodeName>` from the authenticated identity and compares it against `spec.nodeName`; only the agent running on that node may patch that Pool's status. This makes the client-side `spec.nodeName` watch filter a server-enforced invariant rather than a convention. | ||
|
|
||
| ### Validating Webhook |
There was a problem hiding this comment.
We don't need to specify the validation items separately. They should be included in each field's comments.
Signed-off-by: lichuqiang <[email protected]>
|
|
||
| This "data source and aggregator separated" design ensures Phase can still be updated in extreme scenarios — when a node is deleted, the Controller sets Phase=Unready based on the NodeNotFound Condition; when the agent crashes but the Node still exists, the Controller detects the expired Lease (TTL > 40s with no renewal) and sets `PlaceholderAgentHealthy=False`, downgrading Phase to Degraded/Unready. | ||
|
|
||
| > **Lease-Based Heartbeat**: Each placeholder-agent maintains a `coordination.k8s.io/Lease` object (name = Pool name, cluster-scoped) and renews it every 10s. The Controller Watches Leases; when a Lease's `renewTime` exceeds its TTL (default 40s = 4× renewal interval), the Controller sets `PlaceholderAgentHealthy=False` and downgrades Phase. This mirrors the kubelet heartbeat pattern: the Lease is a high-frequency small object (just renewTime + holderIdentity), while the status SSA Apply is event-driven — only triggered when a status field actually changes. In a 10,000-node cluster, steady-state load is ~1,000 Lease renewals/s (lightweight) with near-zero status Apply requests, versus ~333 status Apply/s if status were used for both heartbeat and state. The Lease renewal interval and TTL are configurable via `--lease-renew-interval` (default 10s) and `--lease-ttl` (default 40s). |
| type ResourcePolicy struct { | ||
| Resources corev1.ResourceList `json:"resources,omitempty"` | ||
| } |
What type of PR is this?
/kind feature
What this PR does / why we need it:
As discussed in #430, the current agentcube architecture has limitations in certain scenarios. To break through the
bottlenecks of the Kubernetes architectural paradigm in workload startup speed and managed resource scale, we envision an architectural evolution of agentcube that establishes a dual fast/slow resource track. The slow track builds sandbox resource pool
management capabilities on top of the Kubernetes CRD framework, supporting the partitioning of resources on K8s nodes as resource pools for sandbox execution. In parallel, a lightweight, fast, Kubernetes-independent sandbox lifecycle management platform is
established to achieve more extreme performance and a larger management scale.
This proposal focuses on the slow track. The system implements declarative management of sandbox resources through a two-layer architecture:
synchronization, and Phase aggregation.
checking, and status reporting.
Core capabilities include:
deletion
Integrated with the fast-track data plane, this ultimately enables co-location of regular Pods and sandboxes on the same node.
Which issue(s) this PR fixes:
Refs #430
Does this PR introduce a user-facing change?:
Architecture evolution