From 9163ac66c410ab727f1c51d0652056e359134b86 Mon Sep 17 00:00:00 2001 From: timflannagan Date: Wed, 6 Oct 2021 11:00:33 -0400 Subject: [PATCH] test/e2e: Wrap flake-prone garbage collection tests in eventually assertions Update the test/e2e/gc_e2e_test.go and wrap any error-prone test blocks in eventually assertions to reduce the number of flakes that occur when attempting to create/update/delete/etc. Kubernetes resources during individual CI runs on these garbage collection specs. Signed-off-by: timflannagan --- test/e2e/gc_e2e_test.go | 186 +++++++++++++++++++++++++--------------- 1 file changed, 118 insertions(+), 68 deletions(-) diff --git a/test/e2e/gc_e2e_test.go b/test/e2e/gc_e2e_test.go index bfe8fda885c..2c9d62b6581 100644 --- a/test/e2e/gc_e2e_test.go +++ b/test/e2e/gc_e2e_test.go @@ -47,9 +47,7 @@ var _ = Describe("Garbage collection for dependent resources", func() { BeforeEach(func() { group := fmt.Sprintf("%s.com", rand.String(16)) - // Create a CustomResourceDefinition - var err error - crd, err = kubeClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Create(context.TODO(), &apiextensionsv1.CustomResourceDefinition{ + crd := &apiextensionsv1.CustomResourceDefinition{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("plural.%s", group), }, @@ -73,22 +71,32 @@ var _ = Describe("Garbage collection for dependent resources", func() { ListKind: "KindList", }, }, - }, metav1.CreateOptions{}) - Expect(err).NotTo(HaveOccurred()) + } - // Create a ClusterRole for the crd - cr, err = kubeClient.CreateClusterRole(&rbacv1.ClusterRole{ + // Create a CustomResourceDefinition + var err error + Eventually(func() error { + crd, err = kubeClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{}) + return err + }).Should(Succeed()) + + cr := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "clusterrole-", OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(crd)}, }, - }) - Expect(err).NotTo(HaveOccurred()) + } + // Create a ClusterRole for the crd + Eventually(func() error { + cr, err = kubeClient.CreateClusterRole(cr) + return err + }).Should(Succeed()) }) AfterEach(func() { + // TODO(tflannag): Hmmm.... // Clean up cluster role IgnoreError(kubeClient.DeleteClusterRole(cr.GetName(), &metav1.DeleteOptions{})) @@ -100,11 +108,13 @@ var _ = Describe("Garbage collection for dependent resources", func() { BeforeEach(func() { // Delete CRD - Expect(kubeClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Delete(context.TODO(), crd.GetName(), metav1.DeleteOptions{})).To(Succeed()) + Eventually(func() bool { + err := kubeClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Delete(context.TODO(), crd.GetName(), metav1.DeleteOptions{}) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) }) It("should delete the associated ClusterRole", func() { - Eventually(func() bool { _, err := kubeClient.GetClusterRole(cr.GetName()) return k8serrors.IsNotFound(err) @@ -124,9 +134,7 @@ var _ = Describe("Garbage collection for dependent resources", func() { BeforeEach(func() { group := rand.String(16) - // Create an API Service - var err error - as, err = kubeClient.CreateAPIService(&apiregistrationv1.APIService{ + apiService := &apiregistrationv1.APIService{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("v1.%s", group), }, @@ -136,17 +144,28 @@ var _ = Describe("Garbage collection for dependent resources", func() { GroupPriorityMinimum: 1, VersionPriority: 1, }, - }) - Expect(err).NotTo(HaveOccurred()) + } + // Create an API Service + var err error + Eventually(func() error { + // TODO(tflannag): Is comparing error to apierrors.IsAlreadyExists prone to masking + // incorrect testing assumptions? + as, err = kubeClient.CreateAPIService(apiService) + return err + }).Should(Succeed()) - // Create a ClusterRole - cr, err = kubeClient.CreateClusterRole(&rbacv1.ClusterRole{ + cr := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "clusterrole-", OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(as)}, }, - }) - Expect(err).NotTo(HaveOccurred()) + } + + Eventually(func() error { + // Create a ClusterRole + cr, err = kubeClient.CreateClusterRole(cr) + return err + }).Should(Succeed()) }) AfterEach(func() { @@ -161,7 +180,10 @@ var _ = Describe("Garbage collection for dependent resources", func() { BeforeEach(func() { // Delete API service - Expect(kubeClient.DeleteAPIService(as.GetName(), &metav1.DeleteOptions{})).To(Succeed()) + Eventually(func() bool { + err := kubeClient.DeleteAPIService(as.GetName(), &metav1.DeleteOptions{}) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) }) It("should delete the associated ClusterRole", func() { @@ -193,7 +215,6 @@ var _ = Describe("Garbage collection for dependent resources", func() { propagation metav1.DeletionPropagation options metav1.DeleteOptions ) - BeforeEach(func() { ownerA = newCSV("ownera", testNamespace, "", semver.MustParse("0.0.0"), nil, nil, nil) @@ -201,10 +222,15 @@ var _ = Describe("Garbage collection for dependent resources", func() { // create all owners var err error - fetchedA, err = operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Create(context.TODO(), &ownerA, metav1.CreateOptions{}) - Expect(err).NotTo(HaveOccurred()) - fetchedB, err = operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Create(context.TODO(), &ownerB, metav1.CreateOptions{}) - Expect(err).NotTo(HaveOccurred()) + Eventually(func() error { + fetchedA, err = operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Create(context.TODO(), &ownerA, metav1.CreateOptions{}) + return err + }).Should(Succeed()) + + Eventually(func() error { + fetchedB, err = operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Create(context.TODO(), &ownerB, metav1.CreateOptions{}) + return err + }).Should(Succeed()) dependent = &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -218,8 +244,10 @@ var _ = Describe("Garbage collection for dependent resources", func() { ownerutil.AddOwner(dependent, fetchedB, true, false) // create ConfigMap dependent - _, err = kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Create(context.TODO(), dependent, metav1.CreateOptions{}) - Expect(err).NotTo(HaveOccurred(), "dependent could not be created") + Eventually(func() error { + _, err = kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Create(context.TODO(), dependent, metav1.CreateOptions{}) + return err + }).Should(Succeed(), "dependent could not be created") propagation = metav1.DeletePropagationForeground options = metav1.DeleteOptions{PropagationPolicy: &propagation} @@ -229,32 +257,35 @@ var _ = Describe("Garbage collection for dependent resources", func() { BeforeEach(func() { // delete ownerA in the foreground (to ensure any "blocking" dependents are deleted before ownerA) - err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), fetchedA.GetName(), options) - Expect(err).NotTo(HaveOccurred()) + Eventually(func() bool { + err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), fetchedA.GetName(), options) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) // wait for deletion of ownerA Eventually(func() bool { _, err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Get(context.TODO(), ownerA.GetName(), metav1.GetOptions{}) return k8serrors.IsNotFound(err) }).Should(BeTrue()) - }) It("should not have deleted the dependent since ownerB CSV is still present", func() { - _, err := kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Get(context.TODO(), dependent.GetName(), metav1.GetOptions{}) - Expect(err).NotTo(HaveOccurred(), "dependent deleted after one of the owner was deleted") + Eventually(func() error { + _, err := kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Get(context.TODO(), dependent.GetName(), metav1.GetOptions{}) + return err + }).Should(Succeed(), "dependent deleted after one of the owner was deleted") ctx.Ctx().Logf("dependent still exists after one owner was deleted") - }) - }) When("removing both the owners using 'Foreground' deletion policy", func() { BeforeEach(func() { // delete ownerA in the foreground (to ensure any "blocking" dependents are deleted before ownerA) - err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), fetchedA.GetName(), options) - Expect(err).NotTo(HaveOccurred()) + Eventually(func() bool { + err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), fetchedA.GetName(), options) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) // wait for deletion of ownerA Eventually(func() bool { @@ -263,8 +294,10 @@ var _ = Describe("Garbage collection for dependent resources", func() { }).Should(BeTrue()) // delete ownerB in the foreground (to ensure any "blocking" dependents are deleted before ownerB) - err = operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), fetchedB.GetName(), options) - Expect(err).NotTo(HaveOccurred()) + Eventually(func() bool { + err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), fetchedB.GetName(), options) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) // wait for deletion of ownerB Eventually(func() bool { @@ -274,9 +307,10 @@ var _ = Describe("Garbage collection for dependent resources", func() { }) It("should have deleted the dependent since both the owners were deleted", func() { - _, err := kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Get(context.TODO(), dependent.GetName(), metav1.GetOptions{}) - Expect(err).To(HaveOccurred()) - Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + Eventually(func() bool { + _, err := kubeClient.KubernetesInterface().CoreV1().ConfigMaps(testNamespace).Get(context.TODO(), dependent.GetName(), metav1.GetOptions{}) + return k8serrors.IsNotFound(err) + }).Should(BeTrue(), "expected dependency configmap would be properly garabage collected") ctx.Ctx().Logf("dependent successfully garbage collected after both owners were deleted") }) @@ -359,8 +393,10 @@ var _ = Describe("Garbage collection for dependent resources", func() { BeforeEach(func() { // Delete subscription first - err := operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Delete(context.TODO(), subName, metav1.DeleteOptions{}) - Expect(err).To(BeNil()) + Eventually(func() bool { + err := operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Delete(context.TODO(), subName, metav1.DeleteOptions{}) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) // wait for deletion Eventually(func() bool { @@ -369,8 +405,10 @@ var _ = Describe("Garbage collection for dependent resources", func() { }).Should(BeTrue()) // Delete CSV - err = operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), csvName, metav1.DeleteOptions{}) - Expect(err).To(BeNil()) + Eventually(func() bool { + err := operatorClient.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Delete(context.TODO(), csvName, metav1.DeleteOptions{}) + return k8serrors.IsNotFound(err) + }).Should(BeTrue()) // wait for deletion Eventually(func() bool { @@ -427,8 +465,11 @@ var _ = Describe("Garbage collection for dependent resources", func() { }, } - source, err := operatorClient.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred(), "could not create catalog source") + var err error + Eventually(func() error { + source, err = operatorClient.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{}) + return err + }).Should(Succeed(), "could not create catalog source") // Create a Subscription for package _ = createSubscriptionForCatalog(operatorClient, source.GetNamespace(), subName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic) @@ -457,17 +498,20 @@ var _ = Describe("Garbage collection for dependent resources", func() { var installPlanRef string BeforeEach(func() { - // update subscription first - sub, err := operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.TODO(), subName, metav1.GetOptions{}) - Expect(err).ToNot(HaveOccurred(), "could not get subscription") - - // update channel on sub - sub.Spec.Channel = upgradeChannelName - _, err = operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Update(context.TODO(), sub, metav1.UpdateOptions{}) - Expect(err).ToNot(HaveOccurred(), "could not update subscription") + Eventually(func() error { + // update subscription first + sub, err := operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.TODO(), subName, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("could not get subscription") + } + // update channel on sub + sub.Spec.Channel = upgradeChannelName + _, err = operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Update(context.TODO(), sub, metav1.UpdateOptions{}) + return err + }).Should(Succeed(), "could not update subscription") // Wait for the Subscription to succeed - sub, err = fetchSubscription(operatorClient, testNamespace, subName, subscriptionStateAtLatestChecker) + sub, err := fetchSubscription(operatorClient, testNamespace, subName, subscriptionStateAtLatestChecker) Expect(err).ToNot(HaveOccurred(), "could not get subscription at latest status") installPlanRef = sub.Status.InstallPlanRef.Name @@ -530,8 +574,11 @@ var _ = Describe("Garbage collection for dependent resources", func() { }, } - source, err := operatorClient.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred(), "could not create catalog source") + var err error + Eventually(func() error { + source, err = operatorClient.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{}) + return err + }).Should(Succeed()) // Create a Subscription for package _ = createSubscriptionForCatalog(operatorClient, source.GetNamespace(), subName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic) @@ -561,17 +608,20 @@ var _ = Describe("Garbage collection for dependent resources", func() { var installPlanRef string BeforeEach(func() { - // update subscription first - sub, err := operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.TODO(), subName, metav1.GetOptions{}) - Expect(err).ToNot(HaveOccurred(), "could not get subscription") - - // update channel on sub - sub.Spec.Channel = upgradeChannelName - _, err = operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Update(context.TODO(), sub, metav1.UpdateOptions{}) - Expect(err).ToNot(HaveOccurred(), "could not update subscription") + Eventually(func() error { + // update subscription first + sub, err := operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Get(context.TODO(), subName, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("could not get subscription") + } + // update channel on sub + sub.Spec.Channel = upgradeChannelName + _, err = operatorClient.OperatorsV1alpha1().Subscriptions(testNamespace).Update(context.TODO(), sub, metav1.UpdateOptions{}) + return err + }).Should(Succeed(), "could not update subscription") // Wait for the Subscription to succeed - sub, err = fetchSubscription(operatorClient, testNamespace, subName, subscriptionStateAtLatestChecker) + sub, err := fetchSubscription(operatorClient, testNamespace, subName, subscriptionStateAtLatestChecker) Expect(err).ToNot(HaveOccurred(), "could not get subscription at latest status") installPlanRef = sub.Status.InstallPlanRef.Name