Fix : Remove default maxSessionDuration for AgentRuntime#410
Fix : Remove default maxSessionDuration for AgentRuntime#410safiya2610 wants to merge 2 commits into
Conversation
Signed-off-by: Safiya <147792763+safiya2610@users.noreply.github.com>
|
[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 |
![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 makes maxSessionDuration optional for AgentRuntime by removing its default value and required validation in both the CRD and Go API definitions. It also updates the workload builder logic to only set a sandbox shutdown time if a TTL is specified, defaulting to DefaultSandboxTTL specifically for code interpreters, and adds a corresponding unit test assertion. Feedback highlights that maxSessionDuration is still listed in the required list at the bottom of the CRD file, which will cause Kubernetes to reject resources that omit it.
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
This PR aims to resolve Issue #303 by making AgentRuntime.spec.maxSessionDuration optional (removing the implicit 8h hard lifetime), while preserving the existing default max session duration behavior for CodeInterpreter. It updates sandbox construction so a hard ShutdownTime is only set when explicitly configured, updates the AgentRuntime API/CRD, and adjusts unit tests accordingly.
Changes:
- Remove the default hard max lifetime for AgentRuntime sessions by not setting
Sandbox.Spec.Lifecycle.ShutdownTimeunlessmaxSessionDurationis provided. - Preserve CodeInterpreter’s default hard max lifetime (8h) by explicitly applying
DefaultSandboxTTLwhenmaxSessionDurationis omitted. - Update the AgentRuntime API/CRD and unit tests to reflect the new optional semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/workloadmanager/workload_builder.go |
Stop defaulting TTL in the generic sandbox builder; set ShutdownTime only when TTL is explicitly configured; preserve CodeInterpreter TTL defaulting. |
pkg/workloadmanager/workload_builder_test.go |
Update AgentRuntime default timeout test to assert ShutdownTime is nil when maxSessionDuration is omitted. |
pkg/apis/runtime/v1alpha1/agent_type.go |
Remove kubebuilder-required/default markers and document MaxSessionDuration as optional for AgentRuntime. |
manifests/charts/base/crds/runtime.agentcube.volcano.sh_agentruntimes.yaml |
Remove the CRD default value for maxSessionDuration. |
Comments suppressed due to low confidence (1)
manifests/charts/base/crds/runtime.agentcube.volcano.sh_agentruntimes.yaml:50
- The Helm CRD still marks
spec.maxSessionDurationas required even though the default was removed. In this CRD,spec.requiredstill includes- maxSessionDuration(see same file around line 8505), so clients will still be forced to provide the field, which contradicts making it optional inAgentRuntimeSpec.
Update the CRD schema to remove maxSessionDuration from the spec.required list (ideally by regenerating the CRD from controller-gen after removing the +kubebuilder:validation:Required tag in pkg/apis/runtime/v1alpha1/agent_type.go).
maxSessionDuration:
description: |-
MaxSessionDuration describes the maximum duration for a session.
After this duration, the session will be terminated no matter active or inactive.
type: string
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
===========================================
+ Coverage 47.57% 58.62% +11.05%
===========================================
Files 30 37 +7
Lines 2819 3500 +681
===========================================
+ Hits 1341 2052 +711
+ Misses 1338 1238 -100
- Partials 140 210 +70
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:
|
Signed-off-by: Safiya <147792763+safiya2610@users.noreply.github.com>
|
|
||
| shutdownTime := metav1.NewTime(time.Now().Add(params.ttl)) | ||
| var shutdownTime *metav1.Time | ||
| if params.ttl > 0 { |
| // NoExpiryDeadline represents a sandbox with no hard expiry deadline (used for AgentRuntime without maxSessionDuration). | ||
| // Setting ExpiresAt to this value ensures the sandbox won't be selected by ListExpiredSandboxes queries. | ||
| var NoExpiryDeadline = time.Date(9999, 12, 31, 23, 59, 59, 0, time.UTC) |
| // Filter out sandboxes with no hard expiry deadline (ExpiresAt >= NoExpiryDeadline) | ||
| // These are AgentRuntime sandboxes without maxSessionDuration that should not be GC'd by TTL. | ||
| filteredExpiredSandboxes := make([]*types.SandboxInfo, 0, len(expiredSandboxes)) | ||
| for _, s := range expiredSandboxes { | ||
| if s.ExpiresAt.Before(NoExpiryDeadline) { |
What type of PR is this?
This PR resolves Issue #303 by making AgentRuntime.spec.maxSessionDuration optional instead of defaulting to 8 hours.
Changes
Removed the default maxSessionDuration value for AgentRuntime.
Updated the workload builder to set Sandbox.Spec.Lifecycle.ShutdownTime only when maxSessionDuration is explicitly configured.
Preserved the existing default maximum session duration behavior for CodeInterpreter.
Updated the AgentRuntime CRD to reflect the optional field.
Added and updated unit tests to cover the new behavior.
Why
Previously, every AgentRuntime was assigned a default maximum session duration of 8 hours, causing long-running agents to be terminated even when they were still active. With this change, long-running AgentRuntime instances are no longer force-terminated unless a maximum session duration is explicitly specified, while CodeInterpreter continues to retain its existing behavior.
Testing
go test ./pkg/workloadmanager/...
go test ./pkg/apis/runtime/v1alpha1/...
go build ./...
Which issue(s) this PR fixes:
Fixes #303