Skip to content
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.

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

chore(dra): improve assert event for unit test for controller #127726

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

googs1025
Copy link
Member

@googs1025 googs1025 commented Sep 29, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

  • remove TODO, assert event for unit test for controller
    Although we don’t have any expected events to assert yet, we will need to verify this part in subsequent iterations.

Which issue(s) this PR fixes:

Fixes # None

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 29, 2024
@googs1025
Copy link
Member Author

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 29, 2024
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 29, 2024
@googs1025
Copy link
Member Author

@pohly /PTAL thanks

cc := ctrl.(*controller)
// We need to mock the event recorder to test the controller's event.
fakeRecorder := record.NewFakeRecorder(100)
cc.eventRecorder = fakeRecorder
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so that explains the client-go/tools/record/event.go failure, right?

Good find, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

// expectedEvent is a slice of strings representing expected events.
// Each string in the slice should follow the format: "EventType Reason Message".
// - "Warning Failed processing failed"
expectedEvent []string
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there no unit tests which emit events? Can you add one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, there is no place in the current code logic where we need to test events. This is because we only test the syncKey here method, but there is no logic for sending events inside. There is only logic in sync here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If no events are generated, then why did I see the event.go error when writing this code initially? I need to look into this a bit more.

Copy link
Contributor

@pohly pohly Oct 1, 2024

Choose a reason for hiding this comment

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

Perhaps I was trying to test events, noticed that problem, and then added that TODO instead. Not sure anymore, it was two years ago 😅

The reason why many controllers, also elsewhere, test their syncKey or sync<Something> methods is that the sync method needs a workqueue and might block. It's hard to make it process exactly one item. I think it's time to write a mock implementation of TypedRateLimitingInterface, if there isn't one already... I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was a little confused when I saw this TODO, because it seems that there is currently no code that needs to cover the test of the event. 😄
Do we need this part of the code? Because we are not sure whether the event will be sent to the controller in the future, but it is lossless for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Below is what that could look like.

@googs1025 if this makes sense to you, can you add this mock_queue_test.go with me as author in a separate commit and then adapt the controller_test.go and controller.go accordingly?

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..44f6f2f1e9f 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)
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..4747a6ff348 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
@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -340,16 +341,13 @@ 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 m Mock[string]
+			m.SyncOne(test.key, ctrl.(*controller).sync)
+			if diff := cmp.Diff(Mock[string]{}, m); diff != "" {
+				t.Fatalf("unexpected state of mock work queue after sync (-want, +got):\n%s", diff)
 			}
+			// TODO: replace test.expectError with expectations for the state of the work queue.
 			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..08474b0652c
--- /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++
+}
+
+// Len 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]
+}

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I will take look tonight

Copy link
Member Author

Choose a reason for hiding this comment

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

we have merged #127789, so unit testing can cover the event generation part, and we will have some case for event generation

@haircommander
Copy link
Contributor

/triage accepted
/priority important-longterm
/cc @bart0sh

@k8s-ci-robot k8s-ci-robot requested a review from bart0sh October 2, 2024 17:43
@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 2, 2024
@googs1025 googs1025 force-pushed the dra/ut_events_assert branch from ca5775b to 25ae942 Compare October 3, 2024 00:41
@googs1025 googs1025 force-pushed the dra/ut_events_assert branch from 25ae942 to 6219445 Compare October 3, 2024 00:44
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2024
@googs1025
Copy link
Member Author

/test pull-kubernetes-integration

@googs1025
Copy link
Member Author

/test pull-kubernetes-integration

@googs1025
Copy link
Member Author

/test pull-kubernetes-integration
retest for no relevant flake

Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ffc5eac67223eea11b28f171e5852189c7cb653b

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: googs1025, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2024
@pohly
Copy link
Contributor

pohly commented Oct 4, 2024

/retest

@k8s-ci-robot k8s-ci-robot merged commit c9a61af into kubernetes:master Oct 4, 2024
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants