diff --git a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go index 2a0ae0b2a79..04719959fd8 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller.go @@ -328,7 +328,7 @@ func (ctrl *controller) Run(workers int) { } for i := 0; i < workers; i++ { - go wait.Until(ctrl.sync, 0, stopCh) + go wait.Until(func() { ctrl.sync(ctrl.queue) }, 0, stopCh) } <-stopCh @@ -344,12 +344,12 @@ var errRequeue = errors.New("requeue") var errPeriodic = errors.New("periodic") // sync is the main worker. -func (ctrl *controller) sync() { - key, quit := ctrl.queue.Get() +func (ctrl *controller) sync(queue workqueue.TypedRateLimitingInterface[string]) { + key, quit := queue.Get() if quit { return } - defer ctrl.queue.Done(key) + defer queue.Done(key) logger := klog.LoggerWithValues(ctrl.logger, "key", key) ctx := klog.NewContext(ctrl.ctx, logger) @@ -358,20 +358,20 @@ func (ctrl *controller) sync() { switch err { case nil: logger.V(5).Info("completed") - ctrl.queue.Forget(key) + queue.Forget(key) case errRequeue: logger.V(5).Info("requeue") - ctrl.queue.AddRateLimited(key) + queue.AddRateLimited(key) case errPeriodic: logger.V(5).Info("recheck periodically") - ctrl.queue.AddAfter(key, recheckDelay) + queue.AddAfter(key, recheckDelay) default: logger.Error(err, "processing failed") if obj != nil { // TODO: We don't know here *what* failed. Determine based on error? ctrl.eventRecorder.Event(obj, v1.EventTypeWarning, "Failed", err.Error()) } - ctrl.queue.AddRateLimited(key) + queue.AddRateLimited(key) } } diff --git a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go index a8d2f24748d..dfd203f1881 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/controller/controller_test.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -49,7 +50,7 @@ func TestController(t *testing.T) { claim := createClaim(claimName, claimNamespace, driverName) otherClaim := createClaim(claimName, claimNamespace, otherDriverName) podName := "pod" - podKey := "schedulingCtx:default/pod" + podSchedulingCtxKey := "schedulingCtx:default/pod" pod := createPod(podName, claimNamespace, nil) podClaimName := "my-pod-claim" podSchedulingCtx := createPodSchedulingContexts(pod) @@ -125,11 +126,15 @@ func TestController(t *testing.T) { pod *corev1.Pod schedulingCtx, expectedSchedulingCtx *resourceapi.PodSchedulingContext claim, expectedClaim *resourceapi.ResourceClaim - expectedError string + expectedWorkQueueState Mock[string] }{ "invalid-key": { - key: "claim:x/y/z", - expectedError: `unexpected key format: "x/y/z"`, + key: "claim:x/y/z", + expectedWorkQueueState: Mock[string]{ + Failures: map[string]int{ + "claim:x/y/z": 1, + }, + }, }, "not-found": { key: "claim:default/claim", @@ -154,7 +159,11 @@ func TestController(t *testing.T) { claim: withDeallocate(withAllocate(claim)), driver: m.expectDeallocate(map[string]error{claimName: errors.New("fake error")}), expectedClaim: withDeallocate(withAllocate(claim)), - expectedError: "deallocate: fake error", + expectedWorkQueueState: Mock[string]{ + Failures: map[string]int{ + claimKey: 1, + }, + }, }, // deletion time stamp set, our finalizer set, not allocated -> remove finalizer @@ -170,7 +179,11 @@ func TestController(t *testing.T) { claim: withFinalizer(withDeletionTimestamp(claim), ourFinalizer), driver: m.expectDeallocate(map[string]error{claimName: errors.New("fake error")}), expectedClaim: withFinalizer(withDeletionTimestamp(claim), ourFinalizer), - expectedError: "stop allocation: fake error", + expectedWorkQueueState: Mock[string]{ + Failures: map[string]int{ + claimKey: 1, + }, + }, }, // deletion time stamp set, other finalizer set, not allocated -> do nothing "deleted-finalizer-no-removal": { @@ -191,7 +204,11 @@ func TestController(t *testing.T) { claim: withAllocate(withDeletionTimestamp(claim)), driver: m.expectDeallocate(map[string]error{claimName: errors.New("fake error")}), expectedClaim: withAllocate(withDeletionTimestamp(claim)), - expectedError: "deallocate: fake error", + expectedWorkQueueState: Mock[string]{ + Failures: map[string]int{ + claimKey: 1, + }, + }, }, // deletion time stamp set, finalizer not set -> do nothing "deleted-no-finalizer": { @@ -208,16 +225,23 @@ func TestController(t *testing.T) { // pod with no claims -> shouldn't occur, check again anyway "pod-nop": { - key: podKey, + key: podSchedulingCtxKey, pod: pod, schedulingCtx: withSelectedNode(podSchedulingCtx), expectedSchedulingCtx: withSelectedNode(podSchedulingCtx), - expectedError: errPeriodic.Error(), + expectedWorkQueueState: Mock[string]{ + Later: []MockDelayedItem[string]{ + { + Item: podSchedulingCtxKey, + Duration: time.Second * 30, + }, + }, + }, }, // no potential nodes -> shouldn't occur "no-nodes": { - key: podKey, + key: podSchedulingCtxKey, claim: claim, expectedClaim: claim, pod: podWithClaim, @@ -227,7 +251,7 @@ func TestController(t *testing.T) { // potential nodes -> provide unsuitable nodes "info": { - key: podKey, + key: podSchedulingCtxKey, claim: claim, expectedClaim: claim, pod: podWithClaim, @@ -236,12 +260,19 @@ func TestController(t *testing.T) { expectClaimParameters(map[string]interface{}{claimName: 2}). expectUnsuitableNodes(map[string][]string{podClaimName: unsuitableNodes}, nil), expectedSchedulingCtx: withUnsuitableNodes(withPotentialNodes(podSchedulingCtx)), - expectedError: errPeriodic.Error(), + expectedWorkQueueState: Mock[string]{ + Later: []MockDelayedItem[string]{ + { + Item: podSchedulingCtxKey, + Duration: time.Second * 30, + }, + }, + }, }, // potential nodes, selected node -> allocate "allocate": { - key: podKey, + key: podSchedulingCtxKey, claim: claim, expectedClaim: withReservedFor(withAllocate(claim), pod), pod: podWithClaim, @@ -251,11 +282,18 @@ func TestController(t *testing.T) { expectUnsuitableNodes(map[string][]string{podClaimName: unsuitableNodes}, nil). expectAllocate(map[string]allocate{claimName: {allocResult: &allocation, selectedNode: nodeName, allocErr: nil}}), expectedSchedulingCtx: withUnsuitableNodes(withSelectedNode(withPotentialNodes(podSchedulingCtx))), - expectedError: errPeriodic.Error(), + expectedWorkQueueState: Mock[string]{ + Later: []MockDelayedItem[string]{ + { + Item: "schedulingCtx:default/pod", + Duration: time.Second * 30, + }, + }, + }, }, // potential nodes, selected node, all unsuitable -> update unsuitable nodes "is-potential-node": { - key: podKey, + key: podSchedulingCtxKey, claim: claim, expectedClaim: claim, pod: podWithClaim, @@ -264,11 +302,18 @@ func TestController(t *testing.T) { expectClaimParameters(map[string]interface{}{claimName: 2}). expectUnsuitableNodes(map[string][]string{podClaimName: potentialNodes}, nil), expectedSchedulingCtx: withSpecificUnsuitableNodes(withSelectedNode(withPotentialNodes(podSchedulingCtx)), potentialNodes), - expectedError: errPeriodic.Error(), + expectedWorkQueueState: Mock[string]{ + Later: []MockDelayedItem[string]{ + { + Item: podSchedulingCtxKey, + Duration: time.Second * 30, + }, + }, + }, }, // max potential nodes, other selected node, all unsuitable -> update unsuitable nodes with truncation at start "is-potential-node-truncate-first": { - key: podKey, + key: podSchedulingCtxKey, claim: claim, expectedClaim: claim, pod: podWithClaim, @@ -277,11 +322,18 @@ func TestController(t *testing.T) { expectClaimParameters(map[string]interface{}{claimName: 2}). expectUnsuitableNodes(map[string][]string{podClaimName: append(maxNodes, nodeName)}, nil), expectedSchedulingCtx: withSpecificUnsuitableNodes(withSelectedNode(withSpecificPotentialNodes(podSchedulingCtx, maxNodes)), append(maxNodes[1:], nodeName)), - expectedError: errPeriodic.Error(), + expectedWorkQueueState: Mock[string]{ + Later: []MockDelayedItem[string]{ + { + Item: podSchedulingCtxKey, + Duration: time.Second * 30, + }, + }, + }, }, // max potential nodes, other selected node, all unsuitable (but in reverse order) -> update unsuitable nodes with truncation at end "pod-selected-is-potential-node-truncate-last": { - key: podKey, + key: podSchedulingCtxKey, claim: claim, expectedClaim: claim, pod: podWithClaim, @@ -290,7 +342,14 @@ func TestController(t *testing.T) { expectClaimParameters(map[string]interface{}{claimName: 2}). expectUnsuitableNodes(map[string][]string{podClaimName: append([]string{nodeName}, maxNodes...)}, nil), expectedSchedulingCtx: withSpecificUnsuitableNodes(withSelectedNode(withSpecificPotentialNodes(podSchedulingCtx, maxNodes)), append([]string{nodeName}, maxNodes[:len(maxNodes)-1]...)), - expectedError: errPeriodic.Error(), + expectedWorkQueueState: Mock[string]{ + Later: []MockDelayedItem[string]{ + { + Item: podSchedulingCtxKey, + Duration: time.Second * 30, + }, + }, + }, }, } { t.Run(name, func(t *testing.T) { @@ -340,16 +399,11 @@ func TestController(t *testing.T) { ) { t.Fatal("could not sync caches") } - _, err := ctrl.(*controller).syncKey(ctx, test.key) - if err != nil && test.expectedError == "" { - t.Fatalf("unexpected error: %v", err) - } - if err == nil && test.expectedError != "" { - t.Fatalf("did not get expected error %q", test.expectedError) - } - if err != nil && err.Error() != test.expectedError { - t.Fatalf("expected error %q, got %q", test.expectedError, err.Error()) - } + var workQueueState Mock[string] + c := ctrl.(*controller) + workQueueState.SyncOne(test.key, c.sync) + assert.Equal(t, test.expectedWorkQueueState, workQueueState) + claims, err := kubeClient.ResourceV1alpha3().ResourceClaims("").List(ctx, metav1.ListOptions{}) require.NoError(t, err, "list claims") var expectedClaims []resourceapi.ResourceClaim diff --git a/staging/src/k8s.io/dynamic-resource-allocation/controller/mock_queue_test.go b/staging/src/k8s.io/dynamic-resource-allocation/controller/mock_queue_test.go new file mode 100644 index 00000000000..aa9e013ba18 --- /dev/null +++ b/staging/src/k8s.io/dynamic-resource-allocation/controller/mock_queue_test.go @@ -0,0 +1,165 @@ +/* +Copyright 2024 The Kubernetes 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 controller + +import ( + "slices" + "time" + + "k8s.io/client-go/util/workqueue" +) + +// TODO (pohly): move this to k8s.io/client-go/util/workqueue/workqueue.go +// if it turns out to be generally useful. Doc comments are already written +// as if the code was there. + +// MockQueue is an implementation of [TypedRateLimitingInterface] which +// can be used to test a function which pulls work items out of a queue +// and processes them. +// +// A null instance is directly usable. The usual usage is: +// +// var m workqueue.Mock[string] +// m.SyncOne("some-item", func(queue workqueue.TypedRateLimitingInterface[string]) { ... } ) +// if diff := cmp.Diff(workqueue.Mock[string]{}, m); diff != "" { +// t.Errorf("unexpected state of mock work queue after sync (-want, +got):\n%s", diff) +// } +// +// All slices get reset to nil when they become empty, so there are no spurious +// differences because of the nil vs. empty slice. +type Mock[T comparable] struct { + // Ready contains the items which are ready for processing. + Ready []T + + // InFlight contains the items which are currently being processed (= Get + // was called, Done not yet). + InFlight []T + + // MismatchedDone contains the items for which Done was called without + // a matching Get. + MismatchedDone []T + + // Later contains the items which are meant to be added to the queue after + // a certain delay (= AddAfter was called for them). + Later []MockDelayedItem[T] + + // Failures contains the items and their retry count which failed to be + // processed (AddRateLimited called at least once, Forget not yet). + // The retry count is always larger than zero. + Failures map[T]int + + // ShutDownCalled tracks how often ShutDown got called. + ShutDownCalled int + + // ShutDownWithDrainCalled tracks how often ShutDownWithDrain got called. + ShutDownWithDrainCalled int +} + +// MockDelayedItem is an item which was queue for later processing. +type MockDelayedItem[T comparable] struct { + Item T + Duration time.Duration +} + +// SyncOne adds the item to the work queue and calls sync. +// That sync function can pull one or more items from the work +// queue until the queue is empty. Then it is told that the queue +// is shutting down, which must cause it to return. +// +// The test can then retrieve the state of the queue to check the result. +func (m *Mock[T]) SyncOne(item T, sync func(workqueue.TypedRateLimitingInterface[T])) { + m.Ready = append(m.Ready, item) + sync(m) +} + +// Add implements [TypedInterface]. +func (m *Mock[T]) Add(item T) { + m.Ready = append(m.Ready, item) +} + +// Len implements [TypedInterface]. +func (m *Mock[T]) Len() int { + return len(m.Ready) +} + +// Get implements [TypedInterface]. +func (m *Mock[T]) Get() (item T, shutdown bool) { + if len(m.Ready) == 0 { + shutdown = true + return + } + item = m.Ready[0] + m.Ready = m.Ready[1:] + if len(m.Ready) == 0 { + m.Ready = nil + } + m.InFlight = append(m.InFlight, item) + return item, false +} + +// Done implements [TypedInterface]. +func (m *Mock[T]) Done(item T) { + index := slices.Index(m.InFlight, item) + if index < 0 { + m.MismatchedDone = append(m.MismatchedDone, item) + } + m.InFlight = slices.Delete(m.InFlight, index, index+1) + if len(m.InFlight) == 0 { + m.InFlight = nil + } +} + +// ShutDown implements [TypedInterface]. +func (m *Mock[T]) ShutDown() { + m.ShutDownCalled++ +} + +// ShutDownWithDrain implements [TypedInterface]. +func (m *Mock[T]) ShutDownWithDrain() { + m.ShutDownWithDrainCalled++ +} + +// ShuttingDown implements [TypedInterface]. +func (m *Mock[T]) ShuttingDown() bool { + return m.ShutDownCalled > 0 || m.ShutDownWithDrainCalled > 0 +} + +// AddAfter implements [TypedDelayingInterface.AddAfter] +func (m *Mock[T]) AddAfter(item T, duration time.Duration) { + m.Later = append(m.Later, MockDelayedItem[T]{Item: item, Duration: duration}) +} + +// AddRateLimited implements [TypedRateLimitingInterface.AddRateLimited]. +func (m *Mock[T]) AddRateLimited(item T) { + if m.Failures == nil { + m.Failures = make(map[T]int) + } + m.Failures[item]++ +} + +// Forget implements [TypedRateLimitingInterface.Forget]. +func (m *Mock[T]) Forget(item T) { + if m.Failures == nil { + return + } + delete(m.Failures, item) +} + +// NumRequeues implements [TypedRateLimitingInterface.NumRequeues]. +func (m *Mock[T]) NumRequeues(item T) int { + return m.Failures[item] +}