Fix for TestWithMaxWaitRateLimiter test case failure #105312

Karthik-K-N · 2021-09-28T07:48:27Z

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

This PR fixes the rate limiter test case failure

Which issue(s) this PR fixes:

Fixes #105311

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2021-09-28T07:48:35Z

Hi @Karthik-K-N. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Karthik-K-N · 2021-09-28T07:48:55Z

/cc @mkumatag

mkumatag · 2021-09-28T08:04:33Z

/ok-to-test

fedebongio · 2021-09-28T20:12:45Z

/assign @caesarxuchao
/triage accepted

MadhavJivrajani · 2021-09-29T05:25:40Z

/priority important-soon

caesarxuchao · 2021-09-29T23:31:13Z

staging/src/k8s.io/client-go/util/workqueue/default_rate_limiters_test.go

@@ -191,7 +191,7 @@ func TestWithMaxWaitRateLimiter(t *testing.T) {
 		}
 	}

-	for i := 100; i < 5100; i++ {
+	for i := 100; i <= 5100; i++ {


Hi, @Karthik-K-N, thanks for looking into the flake! I don't think this is enough to fix the flake though. Because the DefaultControllerRateLimiter uses a real-time clock underneath, if this test gets suspended by the golang scheduler when executing line 195, we can still get a <500s return value at line 201.

I think instead of using DefaultControllerRateLimiter, we should make a fake limiter, which returns steadily increasing delay every time When() is called, and we verify that MaxWaitRateLimiter respects the 500s maximum even if the fake limiter returns >500s delay.

Thanks @caesarxuchao for your inputs, I have added a new commit with a simple fake rate limiter which will either return base delay duration or max delay based on the limit set, This will help to test the MaxWaitRateLimiter. Could you please take a look

caesarxuchao

A few nits. Otherwise lgtm. Can you squash the commits?

caesarxuchao · 2021-09-30T17:41:10Z

staging/src/k8s.io/client-go/util/workqueue/default_rate_limiters.go

+var _ RateLimiter = &FakeRateLimiter{}
+
+// FakeControllerRateLimiter is a constructor for a rate limiter for a workqueue.
+func FakeControllerRateLimiter(baseDelay, maxDelay time.Duration, limit int) RateLimiter {


This wrapper function seems redundant. Can we just use NewFakeRateLimiter?

Removed the wrapper function

caesarxuchao · 2021-09-30T17:41:31Z

staging/src/k8s.io/client-go/util/workqueue/default_rate_limiters.go

@@ -236,3 +236,42 @@ func (w WithMaxWaitRateLimiter) Forget(item interface{}) {
 func (w WithMaxWaitRateLimiter) NumRequeues(item interface{}) int {
 	return w.limiter.NumRequeues(item)
 }
+
+var _ RateLimiter = &FakeRateLimiter{}


Let's move these fakes to default_rate_limiters_test.go

Move to default_rate_limiters_test.go file

caesarxuchao · 2021-09-30T17:45:21Z

staging/src/k8s.io/client-go/util/workqueue/default_rate_limiters.go

+	return &FakeRateLimiter{
+		baseDelay: baseDelay,
+		maxDelay:  maxDelay,
+		limit:     limit,


threshold sounds more accurate than limit here.

renamed to threshold

caesarxuchao · 2021-09-30T17:51:05Z

staging/src/k8s.io/client-go/util/workqueue/default_rate_limiters.go

+	}
+}
+
+type FakeRateLimiter struct {


How about StepRateLimiter since the output of this rate limiter is a step function? A more descriptive name gives us space to add other types of fake rate limiters in the future.

Renamed to StepRateLimiter

caesarxuchao · 2021-10-04T18:45:53Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2021-10-04T18:46:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, Karthik-K-N

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 28, 2021

k8s-ci-robot requested review from mkumatag, caesarxuchao and jpbetz September 28, 2021 07:48

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 28, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 28, 2021

k8s-ci-robot assigned caesarxuchao Sep 28, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 28, 2021

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 29, 2021

caesarxuchao reviewed Sep 29, 2021

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 30, 2021

caesarxuchao reviewed Sep 30, 2021

View reviewed changes

Changes to fix the flake test case in ratelimiter

7134657

Karthik-K-N force-pushed the fix-reatelimite-testfail branch from 21f2493 to 7134657 Compare October 1, 2021 07:12

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2021

k8s-ci-robot merged commit 9fdcc4b into kubernetes:master Oct 4, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 4, 2021

Karthik-K-N mentioned this pull request Nov 22, 2021

REQUEST: New membership for Karthik-K-N kubernetes/org#3111

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for TestWithMaxWaitRateLimiter test case failure #105312

Fix for TestWithMaxWaitRateLimiter test case failure #105312

Karthik-K-N commented Sep 28, 2021

k8s-ci-robot commented Sep 28, 2021

Karthik-K-N commented Sep 28, 2021

mkumatag commented Sep 28, 2021

fedebongio commented Sep 28, 2021

MadhavJivrajani commented Sep 29, 2021

caesarxuchao Sep 29, 2021

Karthik-K-N Sep 30, 2021

caesarxuchao left a comment

caesarxuchao Sep 30, 2021

Karthik-K-N Oct 1, 2021

caesarxuchao Sep 30, 2021

Karthik-K-N Oct 1, 2021

caesarxuchao Sep 30, 2021

Karthik-K-N Oct 1, 2021

caesarxuchao Sep 30, 2021

Karthik-K-N Oct 1, 2021

caesarxuchao commented Oct 4, 2021

k8s-ci-robot commented Oct 4, 2021

Fix for TestWithMaxWaitRateLimiter test case failure #105312

Fix for TestWithMaxWaitRateLimiter test case failure #105312

Conversation

Karthik-K-N commented Sep 28, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Sep 28, 2021

Karthik-K-N commented Sep 28, 2021

mkumatag commented Sep 28, 2021

fedebongio commented Sep 28, 2021

MadhavJivrajani commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Oct 4, 2021

k8s-ci-robot commented Oct 4, 2021