Migrate deprecated wait.Poll function #3906

Affan-7 · 2023-08-07T10:15:49Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR replaces the deprecated wait.Poll() function with the wait.PollUntilContextTimeout() function.

Which issue(s) this PR fixes:
Related to #3835
1 out of 4 tasks

Does this PR introduce a user-facing change?:

NONE

karmada-bot · 2023-08-07T10:15:59Z

Welcome @Affan-7! It looks like this is your first PR to karmada-io/karmada 🎉

carlory · 2023-08-07T10:30:47Z

/lgtm

/assign @RainbowMango

for workflow and approval.

RainbowMango

/approve
Thanks~

codecov-commenter · 2023-08-07T11:33:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.33%. Comparing base (d4c2793) to head (197d335).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3906   +/-   ##
=======================================
  Coverage   53.33%   53.33%           
=======================================
  Files         252      252           
  Lines       20482    20482           
=======================================
  Hits        10924    10924           
  Misses       8836     8836           
  Partials      722      722

Flag	Coverage Δ
unittests	`53.33% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Affan-7 · 2023-08-07T16:34:38Z

The tests were failing, here are the logs: https://github.com/karmada-io/karmada/actions/runs/5783869909/job/15676511801#step:5:2635

Should we increase the default value of j.Wait? Or there is some other problem?

I can't run e2e tests locally, because they are too slow on my computer.

RainbowMango · 2023-08-08T01:43:31Z

It might be flak, let's give it another try.

RainbowMango · 2023-08-08T07:24:08Z

Should we increase the default value of j.Wait? Or there is some other problem?

I'm not sure yet. let's give it another try and try to find the root cause.

I'm curious that the test failed in 42 seconds, but the default waiting time(j.Wait) is 60 seconds.

Just echo the log here:
https://github.com/karmada-io/karmada/actions/runs/5787607401/job/15697196471?pr=3906#step:5:2723

[FAILED] [42.440 seconds]
[cluster joined] reschedule testing Deployment propagation testing [AfterEach] testing clusterAffinity of the policy when the ReplicaScheduling of the policy is nil, reschedule testing [needCreateCluster]
  [AfterEach] /home/runner/work/karmada/karmada/test/e2e/rescheduling_test.go:176
  [It] /home/runner/work/karmada/karmada/test/e2e/rescheduling_test.go:271

  Captured StdOut/StdErr Output >>
  I0808 02:23:49.855080   46074 deployment.go:199] The ResourceBinding(karmadatest-2dj87/deploy-tjnp2-deployment) schedule result is: member1,member2
  cluster(member-e2e-lhr) is joined successfully
  I0808 02:23:59.098817   46074 deployment.go:199] The ResourceBinding(karmadatest-2dj87/deploy-tjnp2-deployment) schedule result is: member-e2e-lhr,member1,member2
  I0808 02:23:59.109167   46074 deployment.go:199] The ResourceBinding(karmadatest-2dj87/deploy-tjnp2-deployment) schedule result is: member-e2e-lhr,member1,member2
  E0808 02:23:59.135302   46074 unjoin.go:239] Failed to delete cluster object. cluster name: member-e2e-lhr, error: context deadline exceeded
  E0808 02:23:59.136891   46074 unjoin.go:174] Failed to delete cluster object. cluster name: member-e2e-lhr, error: context deadline exceeded

Jorropo

~~If you change all of the PollUntilContextTimeout like I showed I belive tests would work.~~ 🙂

Edit: PollWithContext is deprecated too.

pkg/controllers/certificate/cert_rotation_controller.go

RainbowMango · 2023-08-10T01:27:08Z

/remove-approve
We need to figure out the root cause, probably affected by kubernetes/kubernetes#119533

Affan-7 · 2023-08-12T20:58:41Z

Here is what I did:

Summary:

Value of j.Wait was zero in the both cases wait.Poll and wait.PollUntilContextTimeout(). Since the wait.Poll() is deprecated it no longer returns any errors. That's why wait.Poll() was passing the test and wait.PollUntilContextTimeout() was failing. I specified the value of j.Wait explicitly under the test file and everything works fine as expected.

Longer explanation:

I added fmt.Printf("+++++ value of j.Wait: %v\n", j.Wait) before wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, j.Wait, .....). And I ran the test on the my fork of Karmada, using github actions. I was surprised that the value of j.Wait was zero.

Reference: https://github.com/Affan-7/karmada/actions/runs/5841035785/job/15840850551#step:5:2798

So I run the test again with the deprecated wait.Poll() method. But the test passed! Then I ran it again with fmt.Printf("+++++ value of j.Wait: %v\n", j.Wait) and the value of j.Wait was also zero.

Reference: https://github.com/Affan-7/karmada/actions/runs/5841648595/job/15842010633#step:5:2999

The value of j.Wait was still zero but the test case weren't failing. Because the wait.Poll() method is deprecated and doesn't return errors anymore. I found this in the documentation of wait.Poll.

Deprecated: This method does not return errors from context, use PollWithContextTimeout. Note that the new method will no longer return ErrWaitTimeout and instead return errors defined by the context package. Will be removed in a future release.

Reference: https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait#Poll

I was assuming that the default value of j.Wait should be 60 because of flags.DurationVar(&j.Wait, "wait", 60*time.Second, ....) under the unjoin.go. But this block of code doesn't get executed during the test (because it's for CLI). Therefore the value of 'j.Wait' remains zero.

So I added the value of j.Wait explicitly under rescheduling_test.go by using Wait: 60 * time.Second,. It's very similar to the Wait: 5 * options.DefaultKarmadactlCommandDuration, on the line 441 of the karmadactl_test.go.

After specifying the value of j.Wait under the rescheduling_test.go all the tests were passing successfully.
Reference: https://github.com/Affan-7/karmada/actions/runs/5841642875/job/15841998395#step:5:3461

Affan-7 · 2023-08-16T04:36:30Z

Can you please merge this @RainbowMango 😊. Or is there something that should I improve?

RainbowMango · 2023-08-16T08:28:32Z

Thanks @Affan-7 for the excellent analysis, I think it is acceptable to set a timeout here.

I wonder if this is affected by kubernetes/kubernetes#119533?

I'm hesitant to do it before kubernetes/kubernetes#119533.

RainbowMango · 2024-05-06T12:55:15Z

Hi @Affan-7 We can get back on this now as we already updated the Kubernetes dependencies to v1.29+ which includes the fix we want. Please feel free to let me know if you can work on this.

zhzhuang-zju · 2024-05-16T03:53:12Z

pkg/karmadactl/register/register.go

@@ -509,7 +509,7 @@ func (o *CommandRegisterOption) constructKarmadaAgentConfig(bootstrapClient *kub
 	}

 	klog.V(1).Infof("Waiting for the client certificate to be issued")
-	err = wait.Poll(1*time.Second, o.Timeout, func() (done bool, err error) {
+	err = wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, o.Timeout, false, func(ctx context.Context) (done bool, err error) {


Why immediate here is set to false? condition can be invoked immediately.

That is because this behavior is equipment with previous.
wait.Poll always waits the interval before the run of condition.

get it, thanks~
In addition this, if try to get a issued certificate right after it is created, the likelihood of failure is higher, so choose to wait a interval before the run of the condition

That's the idea not run the condition so rush.

zhzhuang-zju · 2024-05-16T03:53:18Z

pkg/karmadactl/unjoin/unjoin.go

@@ -223,7 +223,7 @@ func (j *CommandUnjoinOption) deleteClusterObject(controlPlaneKarmadaClient *kar
 	}

 	// make sure the given cluster object has been deleted
-	err = wait.Poll(1*time.Second, j.Wait, func() (done bool, err error) {
+	err = wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, j.Wait, false, func(ctx context.Context) (done bool, err error) {


ditto~
answered at the last comment.

Signed-off-by: Mohammed Affan <mohammed.affan.727@gmail.com>

Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>

RainbowMango · 2024-05-22T03:43:03Z

Just rebased this and fixed the golint issues.
/assign @XiShanYongYe-Chang

XiShanYongYe-Chang

Thanks~
/lgtm

RainbowMango · 2024-05-22T07:51:28Z

/approve

karmada-bot · 2024-05-22T07:51:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~pkg/controllers/OWNERS~~ [RainbowMango]
~~pkg/karmadactl/OWNERS~~ [RainbowMango]
~~pkg/util/OWNERS~~ [RainbowMango]
~~test/OWNERS~~ [RainbowMango]

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

karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 7, 2023

karmada-bot requested review from carlory, chaunceyjiang and Garrybest August 7, 2023 10:15

karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 7, 2023

Affan-7 force-pushed the cleanup-wait.poll branch from 0422064 to 0fecfba Compare August 7, 2023 10:16

Affan-7 changed the title ~~Replace deprecated wait.poll~~ Replace deprecated wait.Poll function Aug 7, 2023

karmada-bot assigned RainbowMango and carlory Aug 7, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2023

RainbowMango mentioned this pull request Aug 7, 2023

Cleanup deprecated methods in wait package #3835

Closed

4 tasks

RainbowMango approved these changes Aug 7, 2023

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2023

karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 7, 2023

Affan-7 force-pushed the cleanup-wait.poll branch from 747247b to 0fecfba Compare August 7, 2023 16:31

karmada-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 7, 2023

Jorropo suggested changes Aug 8, 2023

View reviewed changes

pkg/controllers/certificate/cert_rotation_controller.go Outdated Show resolved Hide resolved

pkg/controllers/certificate/cert_rotation_controller.go Show resolved Hide resolved

karmada-bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2023

Affan-7 force-pushed the cleanup-wait.poll branch from 0fecfba to 2954bcb Compare August 12, 2023 20:22

karmada-bot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 12, 2023

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 12, 2023

Affan-7 changed the title ~~Replace deprecated wait.Poll function~~ Migrate deprecated wait.Poll function Aug 13, 2023

Affan-7 requested a review from RainbowMango August 14, 2023 04:53

zhzhuang-zju reviewed May 16, 2024

View reviewed changes

Migrate deprecated wait.poll

6a07cca

Signed-off-by: Mohammed Affan <mohammed.affan.727@gmail.com>

RainbowMango force-pushed the cleanup-wait.poll branch from 2954bcb to 6a07cca Compare May 22, 2024 03:25

Fixed golint issues and new piecies of wait.Poll

197d335

Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>

karmada-bot assigned XiShanYongYe-Chang May 22, 2024

XiShanYongYe-Chang reviewed May 22, 2024

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2024

karmada-bot merged commit e4a4a47 into karmada-io:master May 22, 2024
12 checks passed

Affan-7 deleted the cleanup-wait.poll branch June 9, 2024 09:40

Affan-7 mentioned this pull request Sep 20, 2024

REQUEST: New membership for affan-7 karmada-io/community#80

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate deprecated wait.Poll function #3906

Migrate deprecated wait.Poll function #3906

Affan-7 commented Aug 7, 2023

karmada-bot commented Aug 7, 2023

carlory commented Aug 7, 2023

RainbowMango left a comment

codecov-commenter commented Aug 7, 2023 •

edited

Loading

Affan-7 commented Aug 7, 2023 •

edited

Loading

RainbowMango commented Aug 8, 2023

RainbowMango commented Aug 8, 2023

Jorropo left a comment •

edited

Loading

RainbowMango commented Aug 10, 2023

Affan-7 commented Aug 12, 2023 •

edited

Loading

Affan-7 commented Aug 16, 2023

RainbowMango commented Aug 16, 2023

RainbowMango commented May 6, 2024

zhzhuang-zju May 16, 2024

RainbowMango May 22, 2024

zhzhuang-zju May 22, 2024

RainbowMango May 22, 2024

zhzhuang-zju May 16, 2024

RainbowMango May 22, 2024

RainbowMango commented May 22, 2024

XiShanYongYe-Chang left a comment

RainbowMango commented May 22, 2024

karmada-bot commented May 22, 2024

Migrate deprecated wait.Poll function #3906

Migrate deprecated wait.Poll function #3906

Conversation

Affan-7 commented Aug 7, 2023

karmada-bot commented Aug 7, 2023

carlory commented Aug 7, 2023

RainbowMango left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 7, 2023 • edited Loading

Codecov Report

Affan-7 commented Aug 7, 2023 • edited Loading

RainbowMango commented Aug 8, 2023

RainbowMango commented Aug 8, 2023

Jorropo left a comment • edited Loading

Choose a reason for hiding this comment

RainbowMango commented Aug 10, 2023

Affan-7 commented Aug 12, 2023 • edited Loading

Here is what I did:

Affan-7 commented Aug 16, 2023

RainbowMango commented Aug 16, 2023

RainbowMango commented May 6, 2024

zhzhuang-zju May 16, 2024

Choose a reason for hiding this comment

RainbowMango May 22, 2024

Choose a reason for hiding this comment

zhzhuang-zju May 22, 2024

Choose a reason for hiding this comment

RainbowMango May 22, 2024

Choose a reason for hiding this comment

zhzhuang-zju May 16, 2024

Choose a reason for hiding this comment

RainbowMango May 22, 2024

Choose a reason for hiding this comment

RainbowMango commented May 22, 2024

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango commented May 22, 2024

karmada-bot commented May 22, 2024

codecov-commenter commented Aug 7, 2023 •

edited

Loading

Affan-7 commented Aug 7, 2023 •

edited

Loading

Jorropo left a comment •

edited

Loading

Affan-7 commented Aug 12, 2023 •

edited

Loading