From 60716e5ef4a92bb2f9fe575b5f665beec5281c7c Mon Sep 17 00:00:00 2001 From: Avnish Jaltare Date: Mon, 1 Jun 2026 17:03:38 +0000 Subject: [PATCH 1/2] test(workloadmanager): add unit tests for SandboxReconciler and fix watcher leaks Signed-off-by: Avnish Jaltare --- .../sandbox_controller_test.go | 220 ++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 pkg/workloadmanager/sandbox_controller_test.go diff --git a/pkg/workloadmanager/sandbox_controller_test.go b/pkg/workloadmanager/sandbox_controller_test.go new file mode 100644 index 00000000..86afedb6 --- /dev/null +++ b/pkg/workloadmanager/sandbox_controller_test.go @@ -0,0 +1,220 @@ +/* +Copyright The Volcano Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package workloadmanager + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + sandboxv1alpha1 "sigs.k8s.io/agent-sandbox/api/v1alpha1" +) + +func setupSandboxReconciler() (*SandboxReconciler, *runtime.Scheme) { + scheme := runtime.NewScheme() + _ = sandboxv1alpha1.AddToScheme(scheme) + + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + return &SandboxReconciler{ + Client: c, + Scheme: scheme, + }, scheme +} + +func TestSandboxReconciler_Reconcile_NonExistentSandbox(t *testing.T) { + reconciler, _ := setupSandboxReconciler() + ctx := context.TODO() + + req := ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "default", + Name: "non-existent", + }, + } + + res, err := reconciler.Reconcile(ctx, req) + assert.NoError(t, err) + assert.Equal(t, ctrl.Result{}, res) +} + +func TestSandboxReconciler_Reconcile_PendingSandbox(t *testing.T) { + reconciler, scheme := setupSandboxReconciler() + ctx := context.TODO() + + sandbox := &sandboxv1alpha1.Sandbox{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pending-sandbox", + Namespace: "default", + }, + Status: sandboxv1alpha1.SandboxStatus{ + Conditions: []metav1.Condition{ + { + Type: string(sandboxv1alpha1.SandboxConditionReady), + Status: metav1.ConditionFalse, + }, + }, + }, + } + + // Create sandbox in fake client + c := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(sandbox).Build() + reconciler.Client = c + + // Register watcher + resultChan := reconciler.WatchSandboxOnce(ctx, "default", "pending-sandbox") + + req := ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "default", + Name: "pending-sandbox", + }, + } + + res, err := reconciler.Reconcile(ctx, req) + assert.NoError(t, err) + assert.Equal(t, ctrl.Result{}, res) + + // Watcher should not be notified since the sandbox is not ready + select { + case <-resultChan: + t.Fatal("watcher was unexpectedly notified for non-ready sandbox") + case <-time.After(100 * time.Millisecond): + // Success: timeout means no notification was sent + } +} + +func TestSandboxReconciler_Reconcile_ReadySandboxNotification(t *testing.T) { + reconciler, scheme := setupSandboxReconciler() + ctx := context.TODO() + + sandbox := &sandboxv1alpha1.Sandbox{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ready-sandbox", + Namespace: "default", + }, + Status: sandboxv1alpha1.SandboxStatus{ + Conditions: []metav1.Condition{ + { + Type: string(sandboxv1alpha1.SandboxConditionReady), + Status: metav1.ConditionTrue, + }, + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(sandbox).Build() + reconciler.Client = c + + // Register watcher + resultChan := reconciler.WatchSandboxOnce(ctx, "default", "ready-sandbox") + + req := ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "default", + Name: "ready-sandbox", + }, + } + + res, err := reconciler.Reconcile(ctx, req) + assert.NoError(t, err) + assert.Equal(t, ctrl.Result{}, res) + + // Watcher should be notified since the sandbox is ready + select { + case update, ok := <-resultChan: + require.True(t, ok) + assert.NotNil(t, update.Sandbox) + assert.Equal(t, "ready-sandbox", update.Sandbox.Name) + case <-time.After(1 * time.Second): + t.Fatal("watcher was not notified for ready sandbox within timeout") + } + + // Verify the watcher has been removed from the map + reconciler.mu.RLock() + _, exists := reconciler.watchers[req.NamespacedName] + reconciler.mu.RUnlock() + assert.False(t, exists, "watcher should be removed from the map after notification") +} + +func TestSandboxReconciler_WatchSandboxOnce_Duplicate(t *testing.T) { + reconciler, _ := setupSandboxReconciler() + ctx := context.TODO() + + namespace := "default" + name := "test-sandbox" + key := types.NamespacedName{Namespace: namespace, Name: name} + + // Register first watcher + chan1 := reconciler.WatchSandboxOnce(ctx, namespace, name) + require.NotNil(t, chan1) + + // Register second watcher (duplicate) + chan2 := reconciler.WatchSandboxOnce(ctx, namespace, name) + require.NotNil(t, chan2) + assert.NotEqual(t, chan1, chan2) + + // The first channel should be closed immediately when replaced + select { + case _, ok := <-chan1: + assert.False(t, ok, "first channel should be closed") + default: + t.Fatal("first channel was not closed after duplicate registration") + } + + // The second channel should still be registered in the map + reconciler.mu.RLock() + registeredChan, exists := reconciler.watchers[key] + reconciler.mu.RUnlock() + assert.True(t, exists) + assert.Equal(t, chan2, (<-chan SandboxStatusUpdate)(registeredChan)) +} + +func TestSandboxReconciler_UnWatchSandbox(t *testing.T) { + reconciler, _ := setupSandboxReconciler() + ctx := context.TODO() + + namespace := "default" + name := "test-sandbox" + key := types.NamespacedName{Namespace: namespace, Name: name} + + // Register watcher + reconciler.WatchSandboxOnce(ctx, namespace, name) + + // Verify it exists in map + reconciler.mu.RLock() + _, existsBefore := reconciler.watchers[key] + reconciler.mu.RUnlock() + assert.True(t, existsBefore) + + // Unwatch + reconciler.UnWatchSandbox(namespace, name) + + // Verify it is removed from map + reconciler.mu.RLock() + _, existsAfter := reconciler.watchers[key] + reconciler.mu.RUnlock() + assert.False(t, existsAfter) +} From ea1beb90fb000cfa94a146ff47bb07fc924a4f9d Mon Sep 17 00:00:00 2001 From: Avnish Jaltare Date: Mon, 1 Jun 2026 18:05:45 +0000 Subject: [PATCH 2/2] test(workloadmanager): address PR review comments for sandbox reconciler and tests Signed-off-by: Avnish Jaltare --- pkg/workloadmanager/sandbox_controller.go | 3 ++ .../sandbox_controller_test.go | 29 ++++++++++--------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/workloadmanager/sandbox_controller.go b/pkg/workloadmanager/sandbox_controller.go index ccc4632c..1fe0f457 100644 --- a/pkg/workloadmanager/sandbox_controller.go +++ b/pkg/workloadmanager/sandbox_controller.go @@ -88,6 +88,9 @@ func (r *SandboxReconciler) WatchSandboxOnce(_ context.Context, namespace, name if r.watchers == nil { r.watchers = make(map[types.NamespacedName]chan SandboxStatusUpdate) } + if oldChan, exists := r.watchers[key]; exists { + close(oldChan) + } r.watchers[key] = resultChan klog.V(2).Infof("Registered for future notification for sandbox %s/%s", key.Namespace, key.Name) diff --git a/pkg/workloadmanager/sandbox_controller_test.go b/pkg/workloadmanager/sandbox_controller_test.go index 86afedb6..10c375ff 100644 --- a/pkg/workloadmanager/sandbox_controller_test.go +++ b/pkg/workloadmanager/sandbox_controller_test.go @@ -32,9 +32,10 @@ import ( sandboxv1alpha1 "sigs.k8s.io/agent-sandbox/api/v1alpha1" ) -func setupSandboxReconciler() (*SandboxReconciler, *runtime.Scheme) { +func setupSandboxReconciler(t *testing.T) (*SandboxReconciler, *runtime.Scheme) { scheme := runtime.NewScheme() - _ = sandboxv1alpha1.AddToScheme(scheme) + err := sandboxv1alpha1.AddToScheme(scheme) + require.NoError(t, err) c := fake.NewClientBuilder().WithScheme(scheme).Build() @@ -45,7 +46,7 @@ func setupSandboxReconciler() (*SandboxReconciler, *runtime.Scheme) { } func TestSandboxReconciler_Reconcile_NonExistentSandbox(t *testing.T) { - reconciler, _ := setupSandboxReconciler() + reconciler, _ := setupSandboxReconciler(t) ctx := context.TODO() req := ctrl.Request{ @@ -61,7 +62,7 @@ func TestSandboxReconciler_Reconcile_NonExistentSandbox(t *testing.T) { } func TestSandboxReconciler_Reconcile_PendingSandbox(t *testing.T) { - reconciler, scheme := setupSandboxReconciler() + reconciler, scheme := setupSandboxReconciler(t) ctx := context.TODO() sandbox := &sandboxv1alpha1.Sandbox{ @@ -101,13 +102,13 @@ func TestSandboxReconciler_Reconcile_PendingSandbox(t *testing.T) { select { case <-resultChan: t.Fatal("watcher was unexpectedly notified for non-ready sandbox") - case <-time.After(100 * time.Millisecond): - // Success: timeout means no notification was sent + default: + // Success: no notification was sent } } func TestSandboxReconciler_Reconcile_ReadySandboxNotification(t *testing.T) { - reconciler, scheme := setupSandboxReconciler() + reconciler, scheme := setupSandboxReconciler(t) ctx := context.TODO() sandbox := &sandboxv1alpha1.Sandbox{ @@ -148,8 +149,8 @@ func TestSandboxReconciler_Reconcile_ReadySandboxNotification(t *testing.T) { require.True(t, ok) assert.NotNil(t, update.Sandbox) assert.Equal(t, "ready-sandbox", update.Sandbox.Name) - case <-time.After(1 * time.Second): - t.Fatal("watcher was not notified for ready sandbox within timeout") + default: + t.Fatal("watcher was not notified for ready sandbox") } // Verify the watcher has been removed from the map @@ -160,7 +161,7 @@ func TestSandboxReconciler_Reconcile_ReadySandboxNotification(t *testing.T) { } func TestSandboxReconciler_WatchSandboxOnce_Duplicate(t *testing.T) { - reconciler, _ := setupSandboxReconciler() + reconciler, _ := setupSandboxReconciler(t) ctx := context.TODO() namespace := "default" @@ -180,8 +181,8 @@ func TestSandboxReconciler_WatchSandboxOnce_Duplicate(t *testing.T) { select { case _, ok := <-chan1: assert.False(t, ok, "first channel should be closed") - default: - t.Fatal("first channel was not closed after duplicate registration") + case <-time.After(1 * time.Second): + t.Fatal("first channel was not closed after duplicate registration within timeout") } // The second channel should still be registered in the map @@ -189,11 +190,11 @@ func TestSandboxReconciler_WatchSandboxOnce_Duplicate(t *testing.T) { registeredChan, exists := reconciler.watchers[key] reconciler.mu.RUnlock() assert.True(t, exists) - assert.Equal(t, chan2, (<-chan SandboxStatusUpdate)(registeredChan)) + assert.True(t, chan2 == registeredChan, "registered channel should match the new channel") } func TestSandboxReconciler_UnWatchSandbox(t *testing.T) { - reconciler, _ := setupSandboxReconciler() + reconciler, _ := setupSandboxReconciler(t) ctx := context.TODO() namespace := "default"