Skip to content

feat: added shutdownTime to extensions CRs #124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vicentefb wants to merge 1 commit into
base: main
Choose a base branch
from
+182 −16
Open

feat: added shutdownTime to extensions CRs #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions extensions/api/v1alpha1/sandboxclaim_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ type SandboxClaimSpec struct {
// SandboxTemplateRefName - name of the SandboxTemplate to be used for creating a Sandbox
// +kubebuilder:validation:Required
TemplateRef SandboxTemplateRef `json:"sandboxTemplateRef,omitempty" protobuf:"bytes,3,name=sandboxTemplateRef"`

// ShutdownTime is the absolute time when this specific sandbox
// should be deleted. If set, this overrides the shutdownTime
// specified in the SandboxTemplate.
// +kubebuilder:validation:Format="date-time"
// +optional
ShutdownTime *metav1.Time `json:"shutdownTime,omitempty"`
}

// SandboxClaimStatus defines the observed state of Sandbox.
Expand Down
6 changes: 5 additions & 1 deletion extensions/api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 26 additions & 1 deletion extensions/controllers/sandboxclaim_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ func (r *SandboxClaimReconciler) createSandbox(ctx context.Context, claim *exten
sandbox.Spec.PodTemplate.Spec.AutomountServiceAccountToken = &automount
}

sandbox.Spec.ShutdownTime = claim.Spec.ShutdownTime
Copy link
Member

@janetkuo janetkuo Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is different from what's documented in the API spec

Copy link
Contributor Author

@vicentefb vicentefb Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh true, i forgot to update the API spec comments.

if err := controllerutil.SetControllerReference(claim, sandbox, r.Scheme); err != nil {
err = fmt.Errorf("failed to set controller reference for sandbox: %w", err)
logger.Error(err, "Error creating sandbox for claim: %q", claim.Name)
Expand Down Expand Up @@ -315,12 +316,36 @@ func (r *SandboxClaimReconciler) getOrCreateSandbox(ctx context.Context, claim *
}

if sandbox != nil {
logger.Info("sandbox already exists, skipping update", "name", sandbox.Name)
if !r.isControlledByClaim(sandbox, claim) {
err := fmt.Errorf("sandbox %q is not controlled by claim %q. Please use a different claim name or delete the sandbox manually", sandbox.Name, claim.Name)
logger.Error(err, "Sandbox controller mismatch")
return nil, err
}

needsUpdate := false

claimTime := claim.Spec.ShutdownTime
sandboxTime := sandbox.Spec.ShutdownTime

if claimTime == nil && sandboxTime != nil {
Copy link
Member

@janetkuo janetkuo Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we are reconciling the claim time against the live sandbox here. What's the intention here? Claim doesn't support updating sandbox currently.

Copy link
Contributor Author

@vicentefb vicentefb Dec 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i saw that the sandbox_controller.go allows and correctly handles updates to the shutdownTime field so in this case i was also allowing updates to the field from the SandboxClaim. Since the SandboxClaim is the user-facing interface, i thought users shouldn't need to find and edit the underlying Sandbox resource directly to change the field. I may be wrong by thinking about it like this but lmk if this makes sense or if i should revert this change.

// Claim wants infinity, Sandbox has a limit -> UPDATE
needsUpdate = true
} else if claimTime != nil && sandboxTime == nil {
// Claim has a limit, Sandbox is infinite -> UPDATE
needsUpdate = true
} else if claimTime != nil && sandboxTime != nil && !claimTime.Equal(sandboxTime) {
// Both have limits, but they are different -> UPDATE
needsUpdate = true
}

if needsUpdate {
logger.Info("Updating Sandbox ShutdownTime", "old", sandboxTime, "new", claimTime)
sandbox.Spec.ShutdownTime = claimTime
if err := r.Update(ctx, sandbox); err != nil {
return nil, fmt.Errorf("failed to update sandbox shutdownTime: %w", err)
}
}

return sandbox, nil
}

Expand Down
155 changes: 141 additions & 14 deletions extensions/controllers/sandboxclaim_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controllers
import (
"context"
"testing"
"time"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
Expand All @@ -31,7 +32,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"sigs.k8s.io/agent-sandbox/api/v1alpha1"
sandboxv1alpha1 "sigs.k8s.io/agent-sandbox/api/v1alpha1"
sandboxcontrollers "sigs.k8s.io/agent-sandbox/controllers"
extensionsv1alpha1 "sigs.k8s.io/agent-sandbox/extensions/api/v1alpha1"
Expand All @@ -44,7 +44,7 @@ func TestSandboxClaimReconcile(t *testing.T) {
Namespace: "default",
},
Spec: extensionsv1alpha1.SandboxTemplateSpec{
PodTemplate: v1alpha1.PodTemplate{
PodTemplate: sandboxv1alpha1.PodTemplate{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Expand All @@ -69,19 +69,19 @@ func TestSandboxClaimReconcile(t *testing.T) {
},
}

uncontrolledSandbox := &v1alpha1.Sandbox{
uncontrolledSandbox := &sandboxv1alpha1.Sandbox{
ObjectMeta: metav1.ObjectMeta{
Name: "test-claim",
Namespace: "default",
},
Spec: v1alpha1.SandboxSpec{
PodTemplate: v1alpha1.PodTemplate{
Spec: sandboxv1alpha1.SandboxSpec{
PodTemplate: sandboxv1alpha1.PodTemplate{
Spec: template.Spec.PodTemplate.Spec,
},
},
}

controlledSandbox := &v1alpha1.Sandbox{
controlledSandbox := &sandboxv1alpha1.Sandbox{
ObjectMeta: metav1.ObjectMeta{
Name: "test-claim",
Namespace: "default",
Expand All @@ -95,8 +95,8 @@ func TestSandboxClaimReconcile(t *testing.T) {
},
},
},
Spec: v1alpha1.SandboxSpec{
PodTemplate: v1alpha1.PodTemplate{
Spec: sandboxv1alpha1.SandboxSpec{
PodTemplate: sandboxv1alpha1.PodTemplate{
Spec: template.Spec.PodTemplate.Spec,
},
},
Expand Down Expand Up @@ -151,7 +151,7 @@ func TestSandboxClaimReconcile(t *testing.T) {
}

// It checks that the PodSpec was copied correctly AND that the automount token field was injected as false (default).
validateSandboxHasDefaultAutomountToken := func(t *testing.T, sandbox *v1alpha1.Sandbox, template *extensionsv1alpha1.SandboxTemplate) {
validateSandboxHasDefaultAutomountToken := func(t *testing.T, sandbox *sandboxv1alpha1.Sandbox, template *extensionsv1alpha1.SandboxTemplate) {
expectedSpec := template.Spec.PodTemplate.Spec.DeepCopy()
// Expect false by default
expectedSpec.AutomountServiceAccountToken = ptr.To(false)
Expand All @@ -161,7 +161,7 @@ func TestSandboxClaimReconcile(t *testing.T) {
}

// New validation function to check for AutomountServiceAccountToken: true
validateSandboxAutomountTrue := func(t *testing.T, sandbox *v1alpha1.Sandbox, _ *extensionsv1alpha1.SandboxTemplate) {
validateSandboxAutomountTrue := func(t *testing.T, sandbox *sandboxv1alpha1.Sandbox, _ *extensionsv1alpha1.SandboxTemplate) {
if sandbox.Spec.PodTemplate.Spec.AutomountServiceAccountToken == nil {
t.Error("expected AutomountServiceAccountToken to be set, but it was nil")
return
Expand All @@ -177,7 +177,7 @@ func TestSandboxClaimReconcile(t *testing.T) {
expectSandbox bool
expectError bool
expectedCondition metav1.Condition
validateSandbox func(t *testing.T, sandbox *v1alpha1.Sandbox, template *extensionsv1alpha1.SandboxTemplate)
validateSandbox func(t *testing.T, sandbox *sandboxv1alpha1.Sandbox, template *extensionsv1alpha1.SandboxTemplate)
}{
{
name: "sandbox is created when a claim is made",
Expand Down Expand Up @@ -301,7 +301,7 @@ func TestSandboxClaimReconcile(t *testing.T) {
t.Fatalf("reconcile: (%v)", err)
}

var sandbox v1alpha1.Sandbox
var sandbox sandboxv1alpha1.Sandbox
err = client.Get(context.Background(), req.NamespacedName, &sandbox)
if tc.expectSandbox && err != nil {
t.Fatalf("get sandbox: (%v)", err)
Expand Down Expand Up @@ -535,7 +535,7 @@ func TestSandboxClaimPodAdoption(t *testing.T) {
}

// Verify sandbox was created
var sandbox v1alpha1.Sandbox
var sandbox sandboxv1alpha1.Sandbox
err = client.Get(ctx, req.NamespacedName, &sandbox)
if tc.expectSandboxCreate && err != nil {
t.Fatalf("expected sandbox to be created but got error: %v", err)
Expand Down Expand Up @@ -574,9 +574,136 @@ func TestSandboxClaimPodAdoption(t *testing.T) {
}
}

func TestSandboxClaimShutdownTime(t *testing.T) {
// 1. Setup static "fake" time for deterministic tests
fakeClaimTime := &metav1.Time{Time: time.Date(2025, 1, 1, 11, 0, 0, 0, time.UTC)}

// 2. Define the base template
template := &extensionsv1alpha1.SandboxTemplate{
ObjectMeta: metav1.ObjectMeta{
Name: "test-template",
Namespace: "default",
},
Spec: extensionsv1alpha1.SandboxTemplateSpec{
PodTemplate: sandboxv1alpha1.PodTemplate{}, // Not relevant for this test
},
}

// 3. Define test cases
testCases := []struct {
name string
claimToReconcile *extensionsv1alpha1.SandboxClaim
existingObjects []client.Object
expectedShutdownTime *metav1.Time
}{
{
name: "should use claim shutdownTime",
claimToReconcile: &extensionsv1alpha1.SandboxClaim{
ObjectMeta: metav1.ObjectMeta{Name: "claim-time", Namespace: "default", UID: "uid-2"},
Spec: extensionsv1alpha1.SandboxClaimSpec{
TemplateRef: extensionsv1alpha1.SandboxTemplateRef{Name: "test-template"},
ShutdownTime: fakeClaimTime,
},
},
existingObjects: []client.Object{template},
expectedShutdownTime: fakeClaimTime, // Expects the claim's time
},
{
name: "should handle nil shutdownTime (infinity)",
claimToReconcile: &extensionsv1alpha1.SandboxClaim{
ObjectMeta: metav1.ObjectMeta{Name: "claim-forever", Namespace: "default", UID: "uid-3"},
Spec: extensionsv1alpha1.SandboxClaimSpec{
TemplateRef: extensionsv1alpha1.SandboxTemplateRef{Name: "test-template"},
ShutdownTime: nil, // User did not specify a time
},
},
existingObjects: []client.Object{template},
expectedShutdownTime: nil, // Result should also be nil
},
{
name: "should update existing sandbox shutdownTime",
claimToReconcile: &extensionsv1alpha1.SandboxClaim{
ObjectMeta: metav1.ObjectMeta{Name: "claim-update", Namespace: "default", UID: "uid-4"},
Spec: extensionsv1alpha1.SandboxClaimSpec{
TemplateRef: extensionsv1alpha1.SandboxTemplateRef{Name: "test-template"},
ShutdownTime: fakeClaimTime, // New desired time
},
},
existingObjects: []client.Object{
template,
// The Sandbox already exists, but with a different (nil) time
&sandboxv1alpha1.Sandbox{
ObjectMeta: metav1.ObjectMeta{
Name: "claim-update",
Namespace: "default",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "extensions.agents.x-k8s.io/v1alpha1",
Kind: "SandboxClaim",
Name: "claim-update",
UID: "uid-4", // Must match the claim UID
Controller: func(b bool) *bool { return &b }(true),
},
}},
Spec: sandboxv1alpha1.SandboxSpec{ShutdownTime: nil},
},
},
expectedShutdownTime: fakeClaimTime, // It should be patched to the new time
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
scheme := newScheme(t)
allObjects := append(tc.existingObjects, tc.claimToReconcile)
client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(allObjects...).WithStatusSubresource(tc.claimToReconcile).Build()

reconciler := &SandboxClaimReconciler{
Client: client,
Scheme: scheme,
}
req := reconcile.Request{
NamespacedName: types.NamespacedName{
Name: tc.claimToReconcile.Name,
Namespace: tc.claimToReconcile.Namespace,
},
}

// ACT
_, err := reconciler.Reconcile(context.Background(), req)
if err != nil {
t.Fatalf("reconcile: (%v)", err)
}

// ASSERT
// Check that the created Sandbox has the correct shutdownTime
var sandbox sandboxv1alpha1.Sandbox
err = client.Get(context.Background(), req.NamespacedName, &sandbox)
if err != nil {
t.Fatalf("get sandbox: (%v)", err)
}

// Use a comparer that treats the time values as equal
timeComparer := cmp.Comparer(func(x, y *metav1.Time) bool {
if x == nil && y == nil {
return true
}
if x != nil && y != nil {
return x.Time.Equal(y.Time)
}
return false
})

if diff := cmp.Diff(tc.expectedShutdownTime, sandbox.Spec.ShutdownTime, timeComparer); diff != "" {
t.Errorf("unexpected sandbox ShutdownTime (-want +got):\n%s", diff)
}
})
}
}

func newScheme(t *testing.T) *runtime.Scheme {
scheme := runtime.NewScheme()
if err := v1alpha1.AddToScheme(scheme); err != nil {
if err := sandboxv1alpha1.AddToScheme(scheme); err != nil {
t.Fatalf("reconcile: (%v)", err)
}
if err := extensionsv1alpha1.AddToScheme(scheme); err != nil {
Expand Down
3 changes: 3 additions & 0 deletions k8s/crds/extensions.agents.x-k8s.io_sandboxclaims.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ spec:
required:
- name
type: object
shutdownTime:
format: date-time
type: string
required:
- sandboxTemplateRef
type: object
Expand Down