From fe46e47bd134549a7b64b75d2d83007075d511f4 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Mon, 11 Nov 2024 22:09:53 +0200 Subject: [PATCH 1/6] chore: update deprecated polling methods in apiextensions-apiserver Signed-off-by: Omer Aplatony --- .../pkg/apiserver/apiserver.go | 7 +++--- .../customresource_discovery_controller.go | 12 +++++----- .../pkg/controller/finalizer/crd_finalizer.go | 2 +- .../test/integration/apiapproval_test.go | 4 ++-- .../integration/conversion/conversion_test.go | 4 ++-- .../test/integration/conversion/webhook.go | 5 +++-- .../test/integration/defaulting_test.go | 14 ++++++------ .../test/integration/fixtures/resources.go | 12 +++++----- .../test/integration/listtype_test.go | 4 ++-- .../test/integration/validation_test.go | 22 +++++++++---------- .../test/integration/versioning_test.go | 8 +++---- 11 files changed, 49 insertions(+), 45 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go index d791dd228c9..24514973fff 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go @@ -17,6 +17,7 @@ limitations under the License. package apiserver import ( + "context" "fmt" "net/http" "time" @@ -258,14 +259,14 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget) // we don't want to report healthy until we can handle all CRDs that have already been registered. Waiting for the informer // to sync makes sure that the lister will be valid before we begin. There may still be races for CRDs added after startup, // but we won't go healthy until we can handle the ones already present. - s.GenericAPIServer.AddPostStartHookOrDie("crd-informer-synced", func(context genericapiserver.PostStartHookContext) error { - return wait.PollImmediateUntil(100*time.Millisecond, func() (bool, error) { + s.GenericAPIServer.AddPostStartHookOrDie("crd-informer-synced", func(ctx genericapiserver.PostStartHookContext) error { + return wait.PollUntilContextCancel(ctx.Context, 100*time.Millisecond, true, func(ctx context.Context) (bool, error) { if s.Informers.Apiextensions().V1().CustomResourceDefinitions().Informer().HasSynced() { close(hasCRDInformerSyncedSignal) return true, nil } return false, nil - }, context.Done()) + }) }) return s, nil diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go index 0c3b2f59fc8..4d0ae4835d2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go @@ -17,6 +17,7 @@ limitations under the License. package apiserver import ( + "context" "fmt" "sort" "time" @@ -297,7 +298,7 @@ func (c *DiscoveryController) Run(stopCh <-chan struct{}, synchedCh chan<- struc } // initially sync all group versions to make sure we serve complete discovery - if err := wait.PollImmediateUntil(time.Second, func() (bool, error) { + if err := wait.PollUntilContextCancel(context.Background(), time.Second, true, func(ctx context.Context) (bool, error) { crds, err := c.crdLister.List(labels.Everything()) if err != nil { utilruntime.HandleError(fmt.Errorf("failed to initially list CRDs: %v", err)) @@ -313,10 +314,11 @@ func (c *DiscoveryController) Run(stopCh <-chan struct{}, synchedCh chan<- struc } } return true, nil - }, stopCh); err == wait.ErrWaitTimeout { - utilruntime.HandleError(fmt.Errorf("timed out waiting for discovery endpoint to initialize")) - return - } else if err != nil { + }); err != nil { + if err == context.DeadlineExceeded { + utilruntime.HandleError(fmt.Errorf("timed out waiting for initial discovery sync")) + return + } panic(fmt.Errorf("unexpected error: %v", err)) } close(synchedCh) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index b5d55953f3f..2f1cd28fd4f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -235,7 +235,7 @@ func (c *CRDFinalizer) deleteInstances(crd *apiextensionsv1.CustomResourceDefini // now we need to wait until all the resources are deleted. Start with a simple poll before we do anything fancy. // TODO not all servers are synchronized on caches. It is possible for a stale one to still be creating things. // Once we have a mechanism for servers to indicate their states, we should check that for concurrence. - err = wait.PollImmediate(5*time.Second, 1*time.Minute, func() (bool, error) { + err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 1*time.Minute, true, func(ctx context.Context) (bool, error) { listObj, err := crClient.List(ctx, nil) if err != nil { return false, err diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go index a1f8132a618..558b0c66410 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go @@ -72,8 +72,8 @@ func TestAPIApproval(t *testing.T) { if err != nil { t.Fatal(err) } - err = wait.PollImmediate(100*time.Millisecond, 30*time.Second, func() (bool, error) { - approvedKubeAPI, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), approvedKubeAPI.Name, metav1.GetOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { + approvedKubeAPI, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, approvedKubeAPI.Name, metav1.GetOptions{}) if err != nil { return false, err } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index a420fef9992..cf970fff1ba 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -219,8 +219,8 @@ func testWebhookConverter(t *testing.T, watchCache bool) { defer ctc.removeConversionWebhook(t) // wait until new webhook is called the first time - if err := wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { - _, err := ctc.versionedClient(marker.GetNamespace(), "v1alpha1").Get(context.TODO(), marker.GetName(), metav1.GetOptions{}) + if err := wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { + _, err = ctc.versionedClient(marker.GetNamespace(), "v1alpha1").Get(ctx, marker.GetName(), metav1.GetOptions{}) select { case <-upCh: return true, nil diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go index 71ecdc80cfb..6da7c438810 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go @@ -17,6 +17,7 @@ limitations under the License. package conversion import ( + "context" "crypto/tls" "crypto/x509" "encoding/json" @@ -63,8 +64,8 @@ func StartConversionWebhookServer(handler http.Handler) (func(), *apiextensionsv } // StartTLS returns immediately, there is a small chance of a race to avoid. - if err := wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { - _, err := webhookServer.Client().Get(webhookServer.URL) // even a 404 is fine + if err := wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { + _, err = webhookServer.Client().Get(webhookServer.URL) // even a 404 is fine return err == nil, nil }); err != nil { webhookServer.Close() diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go index 7162811148e..dfb39571366 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go @@ -316,8 +316,8 @@ func testDefaulting(t *testing.T, watchCache bool) { addDefault("v1beta2", "c", "C") t.Logf("wait until GET sees 'c' in both status and spec") - if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - obj, err := fooClient.Get(context.TODO(), foo.GetName(), metav1.GetOptions{}) + if err := wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { + obj, err := fooClient.Get(ctx, foo.GetName(), metav1.GetOptions{}) if err != nil { return false, err } @@ -333,7 +333,7 @@ func testDefaulting(t *testing.T, watchCache bool) { mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) t.Logf("wait until GET sees 'c' in both status and spec of cached get") - if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + if err := wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { obj, err := fooClient.Get(context.TODO(), foo.GetName(), metav1.GetOptions{ResourceVersion: "0"}) if err != nil { return false, err @@ -409,8 +409,8 @@ func testDefaulting(t *testing.T, watchCache bool) { t.Logf("Add 'c' default to the REST version, remove it from the storage version, and wait until GET no longer sees it in both status and spec") addDefault("v1beta1", "c", "C") removeDefault("v1beta2", "c") - if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - obj, err := fooClient.Get(context.TODO(), foo.GetName(), metav1.GetOptions{}) + if err := wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { + obj, err := fooClient.Get(ctx, foo.GetName(), metav1.GetOptions{}) if err != nil { return false, err } @@ -434,8 +434,8 @@ func testDefaulting(t *testing.T, watchCache bool) { removeDefault("v1beta1", "a") removeDefault("v1beta1", "b") removeDefault("v1beta1", "c") - if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - obj, err := fooClient.Get(context.TODO(), foo.GetName(), metav1.GetOptions{}) + if err := wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { + obj, err := fooClient.Get(ctx, foo.GetName(), metav1.GetOptions{}) if err != nil { return false, err } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go index 3647e56ae35..85d7361f52a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go @@ -347,7 +347,7 @@ func existsInDiscoveryV1(crd *apiextensionsv1.CustomResourceDefinition, apiExten func waitForCRDReadyWatchUnsafe(crd *apiextensionsv1.CustomResourceDefinition, apiExtensionsClient clientset.Interface) (*apiextensionsv1.CustomResourceDefinition, error) { // wait until all resources appears in discovery for _, version := range servedV1Versions(crd) { - err := wait.PollImmediate(500*time.Millisecond, 30*time.Second, func() (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { return existsInDiscoveryV1(crd, apiExtensionsClient, version) }) if err != nil { @@ -375,7 +375,7 @@ func waitForCRDReady(crd *apiextensionsv1.CustomResourceDefinition, apiExtension // For this test, we'll actually cycle, "list/watch/create/delete" until we get an RV from list that observes the create and not an error. // This way all the tests that are checking for watches don't have to worry about RV too old problems because crazy things *could* happen // before like the created RV could be too old to watch. - err = wait.PollImmediate(500*time.Millisecond, 30*time.Second, func() (bool, error) { + err = wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { return isWatchCachePrimed(v1CRD, dynamicClientSet) }) if err != nil { @@ -396,7 +396,7 @@ func CreateNewV1CustomResourceDefinitionWatchUnsafe(v1CRD *apiextensionsv1.Custo // wait until all resources appears in discovery for _, version := range servedV1Versions(v1CRD) { - err := wait.PollImmediate(500*time.Millisecond, 30*time.Second, func() (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { return existsInDiscoveryV1(v1CRD, apiExtensionsClient, version) }) if err != nil { @@ -424,7 +424,7 @@ func CreateNewV1CustomResourceDefinition(v1CRD *apiextensionsv1.CustomResourceDe // For this test, we'll actually cycle, "list/watch/create/delete" until we get an RV from list that observes the create and not an error. // This way all the tests that are checking for watches don't have to worry about RV too old problems because crazy things *could* happen // before like the created RV could be too old to watch. - err = wait.PollImmediate(500*time.Millisecond, 30*time.Second, func() (bool, error) { + err = wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { return isWatchCachePrimed(v1CRD, dynamicClientSet) }) if err != nil { @@ -518,7 +518,7 @@ func DeleteV1CustomResourceDefinition(crd *apiextensionsv1.CustomResourceDefinit return err } for _, version := range servedV1Versions(crd) { - err := wait.PollImmediate(500*time.Millisecond, 30*time.Second, func() (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { exists, err := existsInDiscoveryV1(crd, apiExtensionsClient, version) return !exists, err }) @@ -540,7 +540,7 @@ func DeleteV1CustomResourceDefinitions(deleteListOpts metav1.ListOptions, apiExt } for _, crd := range list.Items { for _, version := range servedV1Versions(&crd) { - err := wait.PollImmediate(500*time.Millisecond, 30*time.Second, func() (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { exists, err := existsInDiscoveryV1(&crd, apiExtensionsClient, version) return !exists, err }) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go index ea9d0cf867a..9e0dc864411 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go @@ -209,8 +209,8 @@ func TestListTypes(t *testing.T) { } t.Logf("Updating again with invalid values, eventually successfully due to ratcheting logic") - err = wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { - _, err = fooClient.Update(context.TODO(), modifiedInstance, metav1.UpdateOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), time.Microsecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { + _, err = fooClient.Update(ctx, modifiedInstance, metav1.UpdateOptions{}) if err == nil { return true, err } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index e9925960f37..11ec1bceb66 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -777,8 +777,8 @@ func TestCRValidationOnCRDUpdate(t *testing.T) { } // CR is now accepted - err = wait.Poll(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - _, err := noxuResourceClient.Create(context.TODO(), instanceToCreate, metav1.CreateOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { + _, err = noxuResourceClient.Create(ctx, instanceToCreate, metav1.CreateOptions{}) if _, isStatus := err.(*apierrors.StatusError); isStatus { if apierrors.IsInvalid(err) { return false, nil @@ -925,8 +925,8 @@ spec: // wait for condition with violations t.Log("Waiting for NonStructuralSchema condition") var cond *apiextensionsv1.CustomResourceDefinitionCondition - err = wait.PollImmediate(100*time.Millisecond, 5*time.Second, func() (bool, error) { - obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), name, metav1.GetOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (done bool, err error) { + obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, name, metav1.GetOptions{}) if err != nil { return false, err } @@ -963,8 +963,8 @@ spec: // wait for condition to go away t.Log("Wait for condition to disappear") - err = wait.PollImmediate(100*time.Millisecond, 5*time.Second, func() (bool, error) { - obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), name, metav1.GetOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (done bool, err error) { + obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, name, metav1.GetOptions{}) if err != nil { return false, err } @@ -1529,8 +1529,8 @@ properties: if len(tst.expectedViolations) == 0 { // wait for condition to not appear var cond *apiextensionsv1.CustomResourceDefinitionCondition - err := wait.PollImmediate(100*time.Millisecond, 5*time.Second, func() (bool, error) { - obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), betaCRD.Name, metav1.GetOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (done bool, err error) { + obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, betaCRD.Name, metav1.GetOptions{}) if err != nil { return false, err } @@ -1540,7 +1540,7 @@ properties: } return true, nil }) - if err != wait.ErrWaitTimeout { + if err != context.DeadlineExceeded { t.Fatalf("expected no NonStructuralSchema condition, but got one: %v", cond) } return @@ -1548,8 +1548,8 @@ properties: // wait for condition to appear with the given violations var cond *apiextensionsv1.CustomResourceDefinitionCondition - err = wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), betaCRD.Name, metav1.GetOptions{}) + err = wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { + obj, err := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, betaCRD.Name, metav1.GetOptions{}) if err != nil { return false, err } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go index 66ff77f7b67..c46e5240d22 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go @@ -88,11 +88,11 @@ func TestInternalVersionIsHandlerVersion(t *testing.T) { { t.Logf("patch of handler version v1beta1 (non-storage version) should succeed") i := 0 - err = wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { + err = wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { patch := []byte(fmt.Sprintf(`{"i": %d}`, i)) i++ - _, err := noxuNamespacedResourceClientV1beta1.Patch(context.TODO(), "foo", types.MergePatchType, patch, metav1.PatchOptions{}) + _, err = noxuNamespacedResourceClientV1beta1.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) if err != nil { // work around "grpc: the client connection is closing" error // TODO: fix the grpc error @@ -111,11 +111,11 @@ func TestInternalVersionIsHandlerVersion(t *testing.T) { t.Logf("patch of handler version v1beta2 (storage version) should fail") i := 0 noxuNamespacedResourceClientV1beta2 := newNamespacedCustomResourceVersionedClient(ns, dynamicClient, noxuDefinition, "v1beta2") // use the storage version v1beta2 - err = wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { + err = wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { patch := []byte(fmt.Sprintf(`{"i": %d}`, i)) i++ - _, err := noxuNamespacedResourceClientV1beta2.Patch(context.TODO(), "foo", types.MergePatchType, patch, metav1.PatchOptions{}) + _, err = noxuNamespacedResourceClientV1beta2.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) assert.Error(t, err) // work around "grpc: the client connection is closing" error From 991651353d66412138eb04ee28729589b4f97ff1 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Tue, 12 Nov 2024 13:12:17 +0200 Subject: [PATCH 2/6] lint Signed-off-by: Omer Aplatony --- .../pkg/apiserver/customresource_discovery_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go index 4d0ae4835d2..b4dab503ee0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go @@ -18,6 +18,7 @@ package apiserver import ( "context" + "errors" "fmt" "sort" "time" @@ -315,7 +316,7 @@ func (c *DiscoveryController) Run(stopCh <-chan struct{}, synchedCh chan<- struc } return true, nil }); err != nil { - if err == context.DeadlineExceeded { + if errors.Is(err, context.Canceled) { utilruntime.HandleError(fmt.Errorf("timed out waiting for initial discovery sync")) return } From 8b90c9e885c0706a8e8ac0f3158393f12248a15a Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Tue, 12 Nov 2024 13:45:26 +0200 Subject: [PATCH 3/6] lint Signed-off-by: Omer Aplatony --- .../pkg/apiserver/customresource_discovery_controller.go | 2 +- .../test/integration/validation_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go index b4dab503ee0..865d9389b3c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go @@ -316,7 +316,7 @@ func (c *DiscoveryController) Run(stopCh <-chan struct{}, synchedCh chan<- struc } return true, nil }); err != nil { - if errors.Is(err, context.Canceled) { + if errors.Is(err, context.DeadlineExceeded) { utilruntime.HandleError(fmt.Errorf("timed out waiting for initial discovery sync")) return } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 11ec1bceb66..6812d53b471 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -18,6 +18,7 @@ package integration import ( "context" + "errors" "fmt" "strings" "testing" @@ -1540,7 +1541,7 @@ properties: } return true, nil }) - if err != context.DeadlineExceeded { + if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("expected no NonStructuralSchema condition, but got one: %v", cond) } return From 80ed375e37344cac1dda856fbdfe0b5db27ace52 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Tue, 12 Nov 2024 14:36:22 +0200 Subject: [PATCH 4/6] Fixed time Signed-off-by: Omer Aplatony --- .../pkg/controller/finalizer/crd_finalizer.go | 2 +- .../test/integration/fixtures/resources.go | 10 +++++----- .../test/integration/listtype_test.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index 2f1cd28fd4f..c569642d953 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -235,7 +235,7 @@ func (c *CRDFinalizer) deleteInstances(crd *apiextensionsv1.CustomResourceDefini // now we need to wait until all the resources are deleted. Start with a simple poll before we do anything fancy. // TODO not all servers are synchronized on caches. It is possible for a stale one to still be creating things. // Once we have a mechanism for servers to indicate their states, we should check that for concurrence. - err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 1*time.Minute, true, func(ctx context.Context) (bool, error) { + err = wait.PollUntilContextTimeout(ctx, 5*time.Second, 1*time.Minute, true, func(ctx context.Context) (bool, error) { listObj, err := crClient.List(ctx, nil) if err != nil { return false, err diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go index 85d7361f52a..6e729cc6aab 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go @@ -347,7 +347,7 @@ func existsInDiscoveryV1(crd *apiextensionsv1.CustomResourceDefinition, apiExten func waitForCRDReadyWatchUnsafe(crd *apiextensionsv1.CustomResourceDefinition, apiExtensionsClient clientset.Interface) (*apiextensionsv1.CustomResourceDefinition, error) { // wait until all resources appears in discovery for _, version := range servedV1Versions(crd) { - err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { return existsInDiscoveryV1(crd, apiExtensionsClient, version) }) if err != nil { @@ -396,7 +396,7 @@ func CreateNewV1CustomResourceDefinitionWatchUnsafe(v1CRD *apiextensionsv1.Custo // wait until all resources appears in discovery for _, version := range servedV1Versions(v1CRD) { - err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { return existsInDiscoveryV1(v1CRD, apiExtensionsClient, version) }) if err != nil { @@ -424,7 +424,7 @@ func CreateNewV1CustomResourceDefinition(v1CRD *apiextensionsv1.CustomResourceDe // For this test, we'll actually cycle, "list/watch/create/delete" until we get an RV from list that observes the create and not an error. // This way all the tests that are checking for watches don't have to worry about RV too old problems because crazy things *could* happen // before like the created RV could be too old to watch. - err = wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + err = wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { return isWatchCachePrimed(v1CRD, dynamicClientSet) }) if err != nil { @@ -518,7 +518,7 @@ func DeleteV1CustomResourceDefinition(crd *apiextensionsv1.CustomResourceDefinit return err } for _, version := range servedV1Versions(crd) { - err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { exists, err := existsInDiscoveryV1(crd, apiExtensionsClient, version) return !exists, err }) @@ -540,7 +540,7 @@ func DeleteV1CustomResourceDefinitions(deleteListOpts metav1.ListOptions, apiExt } for _, crd := range list.Items { for _, version := range servedV1Versions(&crd) { - err := wait.PollUntilContextTimeout(context.Background(), 500*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { exists, err := existsInDiscoveryV1(&crd, apiExtensionsClient, version) return !exists, err }) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go index 9e0dc864411..ddadc18ae42 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go @@ -209,7 +209,7 @@ func TestListTypes(t *testing.T) { } t.Logf("Updating again with invalid values, eventually successfully due to ratcheting logic") - err = wait.PollUntilContextTimeout(context.Background(), time.Microsecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { + err = wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { _, err = fooClient.Update(ctx, modifiedInstance, metav1.UpdateOptions{}) if err == nil { return true, err From ed7f7ce92b06b2452a3d879b4c70d34f5d31ff23 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Sat, 14 Dec 2024 16:35:42 +0200 Subject: [PATCH 5/6] fix race conditions in polling functions Signed-off-by: Omer Aplatony --- .../test/integration/apiapproval_test.go | 6 +++--- .../integration/conversion/conversion_test.go | 4 ++-- .../test/integration/conversion/webhook.go | 4 ++-- .../test/integration/listtype_test.go | 10 +++++----- .../test/integration/validation_test.go | 10 +++++----- .../test/integration/versioning_test.go | 16 ++++++++-------- 6 files changed, 25 insertions(+), 25 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go index 558b0c66410..19e1cf7cdbd 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/apiapproval_test.go @@ -73,9 +73,9 @@ func TestAPIApproval(t *testing.T) { t.Fatal(err) } err = wait.PollUntilContextTimeout(context.Background(), 100*time.Millisecond, 30*time.Second, true, func(ctx context.Context) (bool, error) { - approvedKubeAPI, err = apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, approvedKubeAPI.Name, metav1.GetOptions{}) - if err != nil { - return false, err + approvedKubeAPI, getErr := apiExtensionClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, approvedKubeAPI.Name, metav1.GetOptions{}) + if getErr != nil { + return false, getErr } if approvedKubeAPIApproved := findCRDCondition(approvedKubeAPI, apiextensionsv1.KubernetesAPIApprovalPolicyConformant); approvedKubeAPIApproved == nil || approvedKubeAPIApproved.Status != apiextensionsv1.ConditionTrue { t.Log(approvedKubeAPIApproved) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index cf970fff1ba..9ea69931b82 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -220,12 +220,12 @@ func testWebhookConverter(t *testing.T, watchCache bool) { // wait until new webhook is called the first time if err := wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { - _, err = ctc.versionedClient(marker.GetNamespace(), "v1alpha1").Get(ctx, marker.GetName(), metav1.GetOptions{}) + _, getErr := ctc.versionedClient(marker.GetNamespace(), "v1alpha1").Get(ctx, marker.GetName(), metav1.GetOptions{}) select { case <-upCh: return true, nil default: - t.Logf("Waiting for webhook to become effective, getting marker object: %v", err) + t.Logf("Waiting for webhook to become effective, getting marker object: %v", getErr) return false, nil } }); err != nil { diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go index 6da7c438810..08cfdb9c2dc 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go @@ -65,8 +65,8 @@ func StartConversionWebhookServer(handler http.Handler) (func(), *apiextensionsv // StartTLS returns immediately, there is a small chance of a race to avoid. if err := wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { - _, err = webhookServer.Client().Get(webhookServer.URL) // even a 404 is fine - return err == nil, nil + _, getErr := webhookServer.Client().Get(webhookServer.URL) // even a 404 is fine + return getErr == nil, nil }); err != nil { webhookServer.Close() return nil, nil, err diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go index ddadc18ae42..2e4e6e64694 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/listtype_test.go @@ -210,15 +210,15 @@ func TestListTypes(t *testing.T) { t.Logf("Updating again with invalid values, eventually successfully due to ratcheting logic") err = wait.PollUntilContextTimeout(context.Background(), time.Millisecond*100, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) { - _, err = fooClient.Update(ctx, modifiedInstance, metav1.UpdateOptions{}) - if err == nil { - return true, err + _, updateErr := fooClient.Update(ctx, modifiedInstance, metav1.UpdateOptions{}) + if updateErr == nil { + return true, nil } - if errors.IsInvalid(err) { + if errors.IsInvalid(updateErr) { // wait until modifiedInstance becomes valid again return false, nil } - return false, err + return false, updateErr }) if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 6812d53b471..5f593aae70b 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -779,14 +779,14 @@ func TestCRValidationOnCRDUpdate(t *testing.T) { // CR is now accepted err = wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { - _, err = noxuResourceClient.Create(ctx, instanceToCreate, metav1.CreateOptions{}) - if _, isStatus := err.(*apierrors.StatusError); isStatus { - if apierrors.IsInvalid(err) { + _, createErr := noxuResourceClient.Create(ctx, instanceToCreate, metav1.CreateOptions{}) + if _, isStatus := createErr.(*apierrors.StatusError); isStatus { + if apierrors.IsInvalid(createErr) { return false, nil } } - if err != nil { - return false, err + if createErr != nil { + return false, createErr } return true, nil }) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go index c46e5240d22..2bd46a80621 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go @@ -92,14 +92,14 @@ func TestInternalVersionIsHandlerVersion(t *testing.T) { patch := []byte(fmt.Sprintf(`{"i": %d}`, i)) i++ - _, err = noxuNamespacedResourceClientV1beta1.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) - if err != nil { + _, patchErr := noxuNamespacedResourceClientV1beta1.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) + if patchErr != nil { // work around "grpc: the client connection is closing" error // TODO: fix the grpc error - if err, ok := err.(*errors.StatusError); ok && err.Status().Code == http.StatusInternalServerError { + if statusErr, ok := patchErr.(*errors.StatusError); ok && statusErr.Status().Code == http.StatusInternalServerError { return false, nil } - return false, err + return false, patchErr } return true, nil }) @@ -115,16 +115,16 @@ func TestInternalVersionIsHandlerVersion(t *testing.T) { patch := []byte(fmt.Sprintf(`{"i": %d}`, i)) i++ - _, err = noxuNamespacedResourceClientV1beta2.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) - assert.Error(t, err) + _, patchErr := noxuNamespacedResourceClientV1beta2.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) + assert.Error(t, patchErr) // work around "grpc: the client connection is closing" error // TODO: fix the grpc error - if err, ok := err.(*errors.StatusError); ok && err.Status().Code == http.StatusInternalServerError { + if statusErr, ok := patchErr.(*errors.StatusError); ok && statusErr.Status().Code == http.StatusInternalServerError { return false, nil } - assert.ErrorContains(t, err, "apiVersion") + assert.ErrorContains(t, patchErr, "apiVersion") return true, nil }) assert.NoError(t, err) From 7e578bdb6d939473236614ce923317127e279ecd Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Sat, 14 Dec 2024 17:13:40 +0200 Subject: [PATCH 6/6] lint Signed-off-by: Omer Aplatony --- .../test/integration/validation_test.go | 3 ++- .../test/integration/versioning_test.go | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 5f593aae70b..94378306513 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -780,7 +780,8 @@ func TestCRValidationOnCRDUpdate(t *testing.T) { // CR is now accepted err = wait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (done bool, err error) { _, createErr := noxuResourceClient.Create(ctx, instanceToCreate, metav1.CreateOptions{}) - if _, isStatus := createErr.(*apierrors.StatusError); isStatus { + var statusErr *apierrors.StatusError + if errors.As(createErr, &statusErr) { if apierrors.IsInvalid(createErr) { return false, nil } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go index 2bd46a80621..0b2c82c672a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/versioning_test.go @@ -18,6 +18,7 @@ package integration import ( "context" + stderrors "errors" "fmt" "net/http" "reflect" @@ -25,6 +26,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/test/integration/fixtures" "k8s.io/apimachinery/pkg/api/errors" @@ -96,7 +98,8 @@ func TestInternalVersionIsHandlerVersion(t *testing.T) { if patchErr != nil { // work around "grpc: the client connection is closing" error // TODO: fix the grpc error - if statusErr, ok := patchErr.(*errors.StatusError); ok && statusErr.Status().Code == http.StatusInternalServerError { + var statusErr *errors.StatusError + if stderrors.As(patchErr, &statusErr) && statusErr.Status().Code == http.StatusInternalServerError { return false, nil } return false, patchErr @@ -116,11 +119,12 @@ func TestInternalVersionIsHandlerVersion(t *testing.T) { i++ _, patchErr := noxuNamespacedResourceClientV1beta2.Patch(ctx, "foo", types.MergePatchType, patch, metav1.PatchOptions{}) - assert.Error(t, patchErr) + require.Error(t, patchErr) // work around "grpc: the client connection is closing" error // TODO: fix the grpc error - if statusErr, ok := patchErr.(*errors.StatusError); ok && statusErr.Status().Code == http.StatusInternalServerError { + var statusErr *errors.StatusError + if stderrors.As(patchErr, &statusErr) && statusErr.Status().Code == http.StatusInternalServerError { return false, nil }