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

Cleanup deprecated methods in wait package #3835

Closed
4 tasks done
RainbowMango opened this issue Jul 25, 2023 · 17 comments · Fixed by #4992
Closed
4 tasks done

Cleanup deprecated methods in wait package #3835

RainbowMango opened this issue Jul 25, 2023 · 17 comments · Fixed by #4992
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Milestone

Comments

@RainbowMango
Copy link
Member

RainbowMango commented Jul 25, 2023

What would you like to be added:

--- a/pkg/controllers/certificate/cert_rotation_controller.go
+++ b/pkg/controllers/certificate/cert_rotation_controller.go
@@ -165,7 +165,7 @@ func (c *CertRotationController) syncCertRotation(secret *corev1.Secret) error {

        var newCertData []byte
        klog.V(1).Infof("Waiting for the client certificate to be issued")
-       err = wait.Poll(1*time.Second, 5*time.Minute, func() (done bool, err error) {
+       err = wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, 5*time.Minute, false, func(ctx context.Context) (done bool, err error) {
                csr, err := c.KubeClient.CertificatesV1().CertificateSigningRequests().Get(context.TODO(), csr, metav1.GetOptions{})
                if err != nil {
                        return false, fmt.Errorf("failed to get the cluster csr %s. err: %v", clusterName, err)
diff --git a/pkg/controllers/cluster/cluster_controller.go b/pkg/controllers/cluster/cluster_controller.go
index ae123a86e..678bf7188 100644
--- a/pkg/controllers/cluster/cluster_controller.go
+++ b/pkg/controllers/cluster/cluster_controller.go
@@ -394,7 +394,7 @@ func (c *Controller) monitorClusterHealth(ctx context.Context) (err error) {
        for i := range clusters {
                cluster := &clusters[i]
                var observedReadyCondition, currentReadyCondition *metav1.Condition
-               if err = wait.PollImmediate(MonitorRetrySleepTime, MonitorRetrySleepTime*HealthUpdateRetry, func() (bool, error) {
+               if err = wait.PollUntilContextTimeout(ctx, MonitorRetrySleepTime, MonitorRetrySleepTime*HealthUpdateRetry, true, func(ctx context.Context) (bool, error) {
                        // Cluster object may be changed in this function.
                        observedReadyCondition, currentReadyCondition, err = c.tryUpdateClusterHealth(ctx, cluster)
                        if err == nil {

This might be a little bit challenging, we need to transition channel to context. Probably could leverage ContextForChannel.

Why is this needed:
The wait.PollImmediate, wait.Poll, etc methods were deprecated in Kubernetes v1.27 (#107826), lint check is falling during update the dependencies of Kubernetes at #3730.
This kind of check has been disabled by #3730 to avoid huge modifications.

test/e2e/framework/cluster.go:320:9: SA1019: wait.PollImmediate is 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. (staticcheck)
	return wait.PollImmediate(2*time.Second, 10*time.Second, func() (done bool, err error) {
@RainbowMango RainbowMango added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jul 25, 2023
@liangyuanpeng
Copy link
Contributor

So it's pending for #3730 if i'm understand correctly.

/assign

@RainbowMango
Copy link
Member Author

Yes, exactly.

@RainbowMango
Copy link
Member Author

Hi @liangyuanpeng I added some examples to the issue description, considering that the amount of changes may be relatively large, I have split these tasks into some interactive tasks. Hope this may make the review process more easier.

@Affan-7
Copy link
Member

Affan-7 commented Aug 5, 2023

Can I work on this?

@RainbowMango
Copy link
Member Author

RainbowMango commented Aug 7, 2023

@Affan-7 Sure. How about taking the first one as a start? Update wait.Poll to wait.PollUntilContextTimeout (with immediately = false)

Just reserved this for you.

@Affan-7
Copy link
Member

Affan-7 commented Aug 7, 2023

Thanks @RainbowMango.

/assign @Affan-7

@liangyuanpeng
Copy link
Contributor

liangyuanpeng commented Aug 9, 2023

@RainbowMango Thanks for tasks list and i'm taking the wait. PollImmediate.

@liangyuanpeng
Copy link
Contributor

Please check the kubernetes/kubernetes#119762 before working for Update wait.PollUntil to PollUntilContextCancel

Seems like the PollUntilContextCancel immediately have some problem.

@RainbowMango
Copy link
Member Author

Thanks @liangyuanpeng for the update, While migrating the wait.Poll to wait.PollUntilContextTimeout, we are currently blocked by weird failing tests. I'm not sure if it is the same issue yet.

@wlq1212
Copy link

wlq1212 commented Sep 4, 2023

Can I work on this?

@RainbowMango

@Rei1010
Copy link

Rei1010 commented Sep 4, 2023

@RainbowMango anything I can do?

@RainbowMango
Copy link
Member Author

@wlq1212 @Rei1010 Sure, thank you both in advance. There are two tasks left, please feel free to help with that.
But it seems kubernetes/kubernetes#119762 blocks this issue, I guess we need to wait for a while until there is an official Kubernetes release comes out.

I'll let you know once we are able to do it.

@liangyuanpeng
Copy link
Contributor

Have create PR kubernetes/kubernetes#122119 to cherrypick kubernetes#119762 for kubernetes v1.27, once it's merge, we can keeping going.

@RainbowMango RainbowMango added this to the v1.9 milestone Dec 1, 2023
@RainbowMango RainbowMango moved this to In Progress in Karmada Release 1.9 Dec 1, 2023
@RainbowMango RainbowMango moved this to Planned In Release 1.9 in Karmada Overall Backlog Dec 1, 2023
@RainbowMango RainbowMango modified the milestones: v1.9, v1.10 Feb 29, 2024
@XiShanYongYe-Chang XiShanYongYe-Chang moved this from Planned In Release 1.9 to Planned In Release 1.10 in Karmada Overall Backlog Mar 1, 2024
@RainbowMango
Copy link
Member Author

@Affan-7 @liangyuanpeng 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.

@wlq1212 @Rei1010 Please let me know(just leave your comments here) if you want help, there are two items without assignments, you can pick anyone you like.

@RainbowMango
Copy link
Member Author

@zhzhuang-zju Would you like to help with this?
You can get started by Update wait.PollUntil to PollUntilContextCancel.

@zhzhuang-zju
Copy link
Contributor

@zhzhuang-zju Would you like to help with this?

Sure, I would.

@liangyuanpeng
Copy link
Contributor

seems like i missed the ping, let's keep going 🔨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
Status: Planned In Release 1.10
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants