From 6b6df2c7c9d84acfe24df4642c520e3e9a2dbb60 Mon Sep 17 00:00:00 2001 From: Zhou Zihang Date: Fri, 23 Oct 2026 16:24:45 +0800 Subject: [PATCH 1/2] Reuse sandbox deletion helpers in workload manager Signed-off-by: Zhou Zihang --- pkg/workloadmanager/garbage_collection.go | 29 +------------- .../garbage_collection_test.go | 20 ++++++++++ pkg/workloadmanager/handlers.go | 25 +++--------- pkg/workloadmanager/handlers_test.go | 40 +++++++++++++++++++ 4 files changed, 67 insertions(+), 47 deletions(-) diff --git a/pkg/workloadmanager/garbage_collection.go b/pkg/workloadmanager/garbage_collection.go index 0de41d186..f31059baf 100644 --- a/pkg/workloadmanager/garbage_collection.go +++ b/pkg/workloadmanager/garbage_collection.go @@ -18,11 +18,8 @@ package workloadmanager import ( "context" - "fmt" "time" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" @@ -121,9 +118,9 @@ func (gc *garbageCollector) once() { // delete sandboxes for _, gcSandbox := range gcSandboxes { if gcSandbox.Kind == types.SandboxClaimsKind { - err = gc.deleteSandboxClaim(ctx, gcSandbox.SandboxNamespace, gcSandbox.Name) + err = deleteSandboxClaim(ctx, gc.k8sClient.dynamicClient, gcSandbox.SandboxNamespace, gcSandbox.Name) } else { - err = gc.deleteSandbox(ctx, gcSandbox.SandboxNamespace, gcSandbox.Name) + err = deleteSandbox(ctx, gc.k8sClient.dynamicClient, gcSandbox.SandboxNamespace, gcSandbox.Name) } if err != nil { errs = append(errs, err) @@ -141,28 +138,6 @@ func (gc *garbageCollector) once() { } } -func (gc *garbageCollector) deleteSandbox(ctx context.Context, namespace, name string) error { - err := gc.k8sClient.dynamicClient.Resource(SandboxGVR).Namespace(namespace).Delete(ctx, name, metav1.DeleteOptions{}) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - return fmt.Errorf("error deleting sandbox %s/%s: %w", namespace, name, err) - } - return nil -} - -func (gc *garbageCollector) deleteSandboxClaim(ctx context.Context, namespace, name string) error { - err := gc.k8sClient.dynamicClient.Resource(SandboxClaimGVR).Namespace(namespace).Delete(ctx, name, metav1.DeleteOptions{}) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - return fmt.Errorf("error deleting sandboxClaim %s/%s: %w", namespace, name, err) - } - return nil -} - // deduplicateSandboxes merges multiple sandbox lists and removes duplicates // by SessionID, preserving the order of first occurrence. func deduplicateSandboxes(lists ...[]*types.SandboxInfo) []*types.SandboxInfo { diff --git a/pkg/workloadmanager/garbage_collection_test.go b/pkg/workloadmanager/garbage_collection_test.go index 531e99086..ede61f81a 100644 --- a/pkg/workloadmanager/garbage_collection_test.go +++ b/pkg/workloadmanager/garbage_collection_test.go @@ -26,6 +26,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" dynamicfake "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" "github.com/volcano-sh/agentcube/pkg/common/types" "github.com/volcano-sh/agentcube/pkg/store" @@ -272,3 +273,22 @@ func TestGarbageCollector_once_noSandboxes(t *testing.T) { gc.once() assert.Empty(t, fs.deleted) } + +func TestGarbageCollector_once_k8sDeleteFailurePreservesSession(t *testing.T) { + for _, kind := range []string{types.SandboxKind, types.SandboxClaimsKind} { + t.Run(kind, func(t *testing.T) { + fs := &gcFakeStore{expired: []*types.SandboxInfo{{ + Kind: kind, SessionID: "session-1", Name: "sandbox-1", SandboxNamespace: "default", + }}} + gc := newTestGC(fs) + gc.k8sClient.dynamicClient.(*dynamicfake.FakeDynamicClient).PrependReactor("delete", "*", + func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("Kubernetes unavailable") + }) + + gc.once() + + assert.Empty(t, fs.deleted, "failed Kubernetes deletion must preserve the session for retry") + }) + } +} diff --git a/pkg/workloadmanager/handlers.go b/pkg/workloadmanager/handlers.go index a4ecb2be2..bc87e2664 100644 --- a/pkg/workloadmanager/handlers.go +++ b/pkg/workloadmanager/handlers.go @@ -520,28 +520,13 @@ func (s *Server) handleDeleteSandbox(c *gin.Context) { if sandbox.Kind == types.SandboxClaimsKind { err = deleteSandboxClaim(c.Request.Context(), dynamicClient, sandbox.SandboxNamespace, sandbox.Name) - if err != nil { - if apierrors.IsNotFound(err) { - // Already deleted, consider as success - klog.Infof("sandbox claim %s/%s already deleted", sandbox.SandboxNamespace, sandbox.Name) - } else { - klog.Errorf("failed to delete sandbox claim %s/%s: %v", sandbox.SandboxNamespace, sandbox.Name, err) - respondError(c, http.StatusInternalServerError, "internal server error") - return - } - } } else { err = deleteSandbox(c.Request.Context(), dynamicClient, sandbox.SandboxNamespace, sandbox.Name) - if err != nil { - if apierrors.IsNotFound(err) { - // Already deleted, consider as success - klog.Infof("sandbox %s/%s already deleted", sandbox.SandboxNamespace, sandbox.Name) - } else { - klog.Errorf("failed to delete sandbox %s/%s: %v", sandbox.SandboxNamespace, sandbox.Name, err) - respondError(c, http.StatusInternalServerError, "internal server error") - return - } - } + } + if err != nil { + klog.Errorf("failed to delete %s %s/%s: %v", sandbox.Kind, sandbox.SandboxNamespace, sandbox.Name, err) + respondError(c, http.StatusInternalServerError, "internal server error") + return } // Use a detached context for the store delete so a client disconnect diff --git a/pkg/workloadmanager/handlers_test.go b/pkg/workloadmanager/handlers_test.go index 823ccb341..f9845e03a 100644 --- a/pkg/workloadmanager/handlers_test.go +++ b/pkg/workloadmanager/handlers_test.go @@ -1015,3 +1015,43 @@ func TestHandleSandboxCreate_IdentityErrors(t *testing.T) { }) } } + +func TestHandleDeleteSandbox_KubernetesResult(t *testing.T) { + gin.SetMode(gin.TestMode) + for _, kind := range []string{types.SandboxKind, types.SandboxClaimsKind} { + for _, failDelete := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/fail=%t", kind, failDelete), func(t *testing.T) { + s := newFakeServer() + fs := &fakeStore{} + s.storeClient = fs + patches := gomonkey.NewPatches() + defer patches.Reset() + patches.ApplyMethod(reflect.TypeOf(fs), "GetSandboxBySessionID", func(_ *fakeStore, _ context.Context, _ string) (*types.SandboxInfo, error) { + return &types.SandboxInfo{Kind: kind, Name: "sandbox-1", SandboxNamespace: "default"}, nil + }) + // An empty tracker returns NotFound; the shared helpers treat it as success. + dc := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) + if failDelete { + dc.PrependReactor("delete", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("Kubernetes unavailable") + }) + } + s.k8sClient.dynamicClient = dc + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodDelete, "/sandboxes/session-1", nil) + c.Params = gin.Params{{Key: "sessionId", Value: "session-1"}} + + s.handleDeleteSandbox(c) + + if failDelete { + require.Equal(t, http.StatusInternalServerError, w.Code) + require.Zero(t, fs.deleteSessionCalls) + } else { + require.Equal(t, http.StatusOK, w.Code) + require.Equal(t, 1, fs.deleteSessionCalls) + } + }) + } + } +} From 654af35b09c0c2876cf8b31ee3968c2e0d19fd4b Mon Sep 17 00:00:00 2001 From: Zhou Zihang Date: Fri, 23 Oct 2026 17:20:39 +0800 Subject: [PATCH 2/2] Avoid unchecked type assertion in garbage collector test Signed-off-by: Zhou Zihang --- pkg/workloadmanager/garbage_collection_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/workloadmanager/garbage_collection_test.go b/pkg/workloadmanager/garbage_collection_test.go index ede61f81a..c823eb880 100644 --- a/pkg/workloadmanager/garbage_collection_test.go +++ b/pkg/workloadmanager/garbage_collection_test.go @@ -281,7 +281,9 @@ func TestGarbageCollector_once_k8sDeleteFailurePreservesSession(t *testing.T) { Kind: kind, SessionID: "session-1", Name: "sandbox-1", SandboxNamespace: "default", }}} gc := newTestGC(fs) - gc.k8sClient.dynamicClient.(*dynamicfake.FakeDynamicClient).PrependReactor("delete", "*", + dc := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) + gc.k8sClient.dynamicClient = dc + dc.PrependReactor("delete", "*", func(k8stesting.Action) (bool, runtime.Object, error) { return true, nil, fmt.Errorf("Kubernetes unavailable") })