karmada icon indicating copy to clipboard operation
karmada copied to clipboard

Cleanup deprecated methods in wait package

Open RainbowMango opened this issue 1 year ago • 17 comments

What would you like to be added:

  • [x] Update wait.Poll to wait.PollUntilContextTimeout (with immediately = false) (@Affan-7, #3906 ) For example:
--- 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)
  • [x] Update wait.PollImmediate to wait.PollUntilContextTimeout (with immediately = true) (@liangyuanpeng, #3921) For example:
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 {

  • [x] Update wait.PollUntil to PollUntilContextCancel (@zhzhuang-zju, #4947)

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 avatar Jul 25 '23 10:07 RainbowMango

So it's pending for https://github.com/karmada-io/karmada/pull/3730 if i'm understand correctly.

/assign

liangyuanpeng avatar Jul 25 '23 10:07 liangyuanpeng

Yes, exactly.

RainbowMango avatar Jul 25 '23 11:07 RainbowMango

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.

RainbowMango avatar Aug 04 '23 03:08 RainbowMango

Can I work on this?

Affan-7 avatar Aug 05 '23 09:08 Affan-7

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

RainbowMango avatar Aug 07 '23 02:08 RainbowMango

Thanks @RainbowMango.

/assign @Affan-7

Affan-7 avatar Aug 07 '23 03:08 Affan-7

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

liangyuanpeng avatar Aug 09 '23 06:08 liangyuanpeng

Please check the https://github.com/kubernetes/kubernetes/pull/119762 before working for Update wait.PollUntil to PollUntilContextCancel

Seems like the PollUntilContextCancel immediately have some problem.

liangyuanpeng avatar Aug 09 '23 07:08 liangyuanpeng

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.

RainbowMango avatar Aug 10 '23 01:08 RainbowMango

Can I work on this?

@RainbowMango

wlq1212 avatar Sep 04 '23 13:09 wlq1212

@RainbowMango anything I can do?

Rei1010 avatar Sep 04 '23 13:09 Rei1010

@wlq1212 @Rei1010 Sure, thank you both in advance. There are two tasks left, please feel free to help with that. But it seems https://github.com/kubernetes/kubernetes/pull/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.

RainbowMango avatar Sep 05 '23 02:09 RainbowMango

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

liangyuanpeng avatar Dec 01 '23 05:12 liangyuanpeng

@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 avatar May 06 '24 12:05 RainbowMango

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

RainbowMango avatar May 15 '24 09:05 RainbowMango

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

Sure, I would.

zhzhuang-zju avatar May 15 '24 09:05 zhzhuang-zju

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

liangyuanpeng avatar May 22 '24 04:05 liangyuanpeng