Allow sandbox claim to specify whether to get pod from sandbox warm pool #212

PersistentJZH · 2025-12-16T08:21:39Z

Summary

Allow sandbox claim to specify whether to get pod from sandbox warm pool

Related Issues
#208

Signed-off-by: PersistentJZH <zhihao.kan17@gmail.com>

k8s-ci-robot · 2025-12-16T08:21:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PersistentJZH
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2025-12-16T08:21:47Z

✅ Deploy Preview for agent-sandbox canceled.

Name	Link
🔨 Latest commit	`acc7355`
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/694116965f3e59000813352c

k8s-ci-robot · 2025-12-16T08:21:48Z

Welcome @PersistentJZH!

It looks like this is your first PR to kubernetes-sigs/agent-sandbox 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/agent-sandbox has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-12-16T08:21:49Z

Hi @PersistentJZH. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

PersistentJZH · 2025-12-16T08:22:42Z

/cc @peterzhongyi

k8s-ci-robot · 2025-12-16T08:22:44Z

@PersistentJZH: GitHub didn't allow me to request PR reviews from the following users: peterzhongyi.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @peterzhongyi

Instructions 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.

janetkuo · 2025-12-16T17:30:34Z

/ok-to-test

justinsb · 2025-12-16T19:09:55Z

(BTW, /cc requests that the person be added to the list of reviewers by prow, and requires "membership". You should just be able to mention their username and then they will probably get a notification from github, subject to their notification settings)

peterzhongyi

/lgtm

k8s-ci-robot · 2025-12-17T03:41:27Z

@peterzhongyi: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions 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.

hzxuzhonghu · 2025-12-18T03:59:44Z

Not seeing much discussion, why is this case needed? If there is a use case, cann't user create sandbox directly?

PersistentJZH · 2025-12-18T09:56:29Z

Not seeing much discussion, why is this case needed? If there is a use case, cann't user create sandbox directly?

This field is used in scenarios where a user wants to use a template but doesn't want to fetch a pod from the warm pool, add this field can make SandboxClaims more flexible.

@peterzhongyi , or do you have more cases about this?

barney-s · 2025-12-18T22:10:43Z

Not seeing much discussion, why is this case needed? If there is a use case, cann't user create sandbox directly?

+1 to this. Why is this a requirement ?

barney-s

Wondering if we have only one warm pool per template ?
If we plan to have multiple warm pools then redoing this change to something like this would be better:

.spec.warmpool=default|none

This is a breaking change but clearly indicates dont use a warmpool OR use the default warmpool and allows adding additional warmpools in future.

I have a few comments regarding the test variable naming, and verifying the protobuf tags.

barney-s · 2025-12-18T22:09:59Z

extensions/controllers/sandboxclaim_controller_test.go

+			createWarmPoolPod("pool-pod-1", metav1.Now()),
+		}
+
+		client := fake.NewClientBuilder().


The variable name client here shadows the package name client (likely from sigs.k8s.io/controller-runtime/pkg/client) which is used in []client.Object just a few lines above.
Consider renaming this variable to fakeClient or k8sClient to avoid confusion and potential compilation issues if the package client is needed later in the scope.

barney-s · 2025-12-18T22:09:59Z

extensions/api/v1alpha1/sandboxclaim_types.go

+	// SkipWarmPool, when set to true, prevents the controller from attempting
+	// to adopt a pre-warmed pod from a SandboxWarmPool. Instead, a new pod will always be created fresh.
+	// +optional
+	SkipWarmPool *bool `json:"skipWarmPool,omitempty" protobuf:"varint,4,opt,name=skipWarmPool"`


Do we need the protobuf ID ?

barney-s · 2025-12-18T22:09:59Z

extensions/controllers/sandboxclaim_controller_test.go

+
+		// Pool labels should still be present since the pod was not adopted
+		if _, exists := poolPod.Labels[poolLabel]; !exists {
+			t.Error("expected pool label to still be present on pod (not adopted)")


Nit: For better debugging if this test fails, consider including the actual labels found in the error message, e.g., t.Errorf("expected pool label to still be present, but got labels: %v", poolPod.Labels).

peterzhongyi · 2025-12-19T04:11:41Z

Not seeing much discussion, why is this case needed? If there is a use case, can't user create sandbox directly?

They can, but it's mainly for the ease-of-use aspect of the Sandbox Claim CRD. It's kind of messy for a user to go back to creating sandboxes when they want to avoid warm pool. Ideally, when a user starts using Sandbox Claim, it would be nice for the user to perform most of the things on this level of CRD.

peterzhongyi · 2025-12-19T04:14:24Z

Not seeing much discussion, why is this case needed? If there is a use case, cann't user create sandbox directly?

This field is used in scenarios where a user wants to use a template but doesn't want to fetch a pod from the warm pool, add this field can make SandboxClaims more flexible.

@peterzhongyi , or do you have more cases about this?

The use case is for when a user wants a sandbox that is not a fresh image, but a pod snapshot that stored some previous state of the user's work. Since a snapshot is restored at the creation of the pod, we can't just give the user a pod from the warm pool anymore because those pods are all using fresh images.

barney-s · 2025-12-19T06:16:49Z

Not seeing much discussion, why is this case needed? If there is a use case, cann't user create sandbox directly?

This field is used in scenarios where a user wants to use a template but doesn't want to fetch a pod from the warm pool, add this field can make SandboxClaims more flexible.
@peterzhongyi , or do you have more cases about this?

The use case is for when a user wants a sandbox that is not a fresh image, but a pod snapshot that stored some previous state of the user's work. Since a snapshot is restored at the creation of the pod, we can't just give the user a pod from the warm pool anymore because those pods are all using fresh images.

Why a claim though ? What is restored from a snapshot ? Would the pod spec be restored from snapshot ?

peterzhongyi · 2026-01-06T21:49:52Z

Not seeing much discussion, why is this case needed? If there is a use case, cann't user create sandbox directly?

This field is used in scenarios where a user wants to use a template but doesn't want to fetch a pod from the warm pool, add this field can make SandboxClaims more flexible.
@peterzhongyi , or do you have more cases about this?

The use case is for when a user wants a sandbox that is not a fresh image, but a pod snapshot that stored some previous state of the user's work. Since a snapshot is restored at the creation of the pod, we can't just give the user a pod from the warm pool anymore because those pods are all using fresh images.

Why a claim though ? What is restored from a snapshot ? Would the pod spec be restored from snapshot ?

This can help simplify the way customer uses agent sandbox. Ideally, a customer should be able to just create claims and get sandboxes created or resumed.

The memory of the workload is restored from a snapshot. The pod spec wouldn't be restored from snapshot, the pod spec is needed for the snapshot restoration, and the pod spec is provided by the sandbox template.

I have some more details about this feature in this issue: #208

k8s-ci-robot · 2026-01-09T02:33:55Z

PR needs rebase.

Details

Instructions 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.

allow sandbox claim to specify whether to get pod from sandbox warmpool

acc7355

Signed-off-by: PersistentJZH <zhihao.kan17@gmail.com>

k8s-ci-robot requested a review from janetkuo December 16, 2025 08:21

k8s-ci-robot requested a review from justinsb December 16, 2025 08:21

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 16, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 16, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 16, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2025

peterzhongyi reviewed Dec 17, 2025

View reviewed changes

barney-s reviewed Dec 18, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2026

Allow sandbox claim to specify whether to get pod from sandbox warm pool #212

Are you sure you want to change the base?

Allow sandbox claim to specify whether to get pod from sandbox warm pool #212

Conversation

PersistentJZH commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 16, 2025

Uh oh!

netlify bot commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for agent-sandbox canceled.

Uh oh!

k8s-ci-robot commented Dec 16, 2025

Uh oh!

k8s-ci-robot commented Dec 16, 2025

Uh oh!

PersistentJZH commented Dec 16, 2025

Uh oh!

k8s-ci-robot commented Dec 16, 2025

Uh oh!

janetkuo commented Dec 16, 2025

Uh oh!

justinsb commented Dec 16, 2025

Uh oh!

peterzhongyi left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 17, 2025

Uh oh!

hzxuzhonghu commented Dec 18, 2025

Uh oh!

PersistentJZH commented Dec 18, 2025

Uh oh!

barney-s commented Dec 18, 2025

Uh oh!

barney-s left a comment

Choose a reason for hiding this comment

Uh oh!

barney-s Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

barney-s Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

barney-s Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

peterzhongyi commented Dec 19, 2025

Uh oh!

peterzhongyi commented Dec 19, 2025

Uh oh!

barney-s commented Dec 19, 2025

Uh oh!

peterzhongyi commented Jan 6, 2026

Uh oh!

k8s-ci-robot commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

PersistentJZH commented Dec 16, 2025 •

edited

Loading

netlify bot commented Dec 16, 2025 •

edited

Loading