From b8a79f6c75c356efa358dd217268cbb1863af927 Mon Sep 17 00:00:00 2001 From: Anik Bhattacharjee Date: Mon, 7 Mar 2022 11:37:28 -0500 Subject: [PATCH 1/2] (catsrc) set status reason/message on incorrect polling interval This PR sets the status reason as InvalidIntervalError and a status message if updateStrategy.RegistryPoll.ParsingError is set for the catsrc. Signed-off-by: Anik Bhattacharjee --- pkg/controller/operators/catalog/operator.go | 17 +- test/e2e/catalog_e2e_test.go | 160 ++++++++++++------- 2 files changed, 116 insertions(+), 61 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 703d76d284..deed0565f1 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -727,10 +727,19 @@ func (o *Operator) syncRegistryServer(logger *logrus.Entry, in *v1alpha1.Catalog // requeue the catalog sync based on the polling interval, for accurate syncs of catalogs with polling enabled if out.Spec.UpdateStrategy != nil { - logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String()) - resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now()) - o.catsrcQueueSet.RequeueAfter(out.GetNamespace(), out.GetName(), queueinformer.ResyncWithJitter(resyncPeriod, 0.1)()) - return + if out.Spec.UpdateStrategy.RegistryPoll != nil { + if out.Spec.UpdateStrategy.RegistryPoll.ParsingError != "" && out.Status.Reason != v1alpha1.CatalogSourceIntervalInvalidError { + out.SetError(v1alpha1.CatalogSourceIntervalInvalidError, fmt.Errorf(out.Spec.UpdateStrategy.RegistryPoll.ParsingError)) + if _, err := o.client.OperatorsV1alpha1().CatalogSources(out.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}); err != nil { + logger.Errorf("error while updating catalogsource status for an invalid interval - %v", err) + return + } + } + logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String()) + resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now()) + o.catsrcQueueSet.RequeueAfter(out.GetNamespace(), out.GetName(), queueinformer.ResyncWithJitter(resyncPeriod, 0.1)()) + return + } } if err := o.sources.Remove(sourceKey); err != nil { diff --git a/test/e2e/catalog_e2e_test.go b/test/e2e/catalog_e2e_test.go index ac14002c4a..57ef1e31f8 100644 --- a/test/e2e/catalog_e2e_test.go +++ b/test/e2e/catalog_e2e_test.go @@ -42,7 +42,7 @@ const ( catsrcImage = "docker://quay.io/olmtest/catsrc-update-test:" ) -var _ = Describe("Catalog represents a store of bundles which OLM can use to install Operators", func() { +var _ = Describe("Starting CatalogSource e2e tests", func() { var ( c operatorclient.ClientInterface crc versioned.Interface @@ -1074,76 +1074,122 @@ var _ = Describe("Catalog represents a store of bundles which OLM can use to ins Expect(err).ShouldNot(HaveOccurred()) Expect(csv.Spec.Replaces).To(Equal("busybox-dependency.v1.0.0")) }) - It("registry polls on the correct interval", func() { - // Create a catalog source with polling enabled - // Confirm the following - // a) the new update pod is spun up roughly in line with the registry polling interval - // b) the update pod is removed quickly when the image is found to not have changed - // This is more of a behavioral test that ensures the feature is working as designed. - - c := newKubeClient() - crc := newCRClient() + When("A catalogSource is created with correct interval", func() { + var source *v1alpha1.CatalogSource + singlePod := podCount(1) sourceName := genName("catalog-") - source := &v1alpha1.CatalogSource{ - TypeMeta: metav1.TypeMeta{ - Kind: v1alpha1.CatalogSourceKind, - APIVersion: v1alpha1.CatalogSourceCRDAPIVersion, - }, - ObjectMeta: metav1.ObjectMeta{ - Name: sourceName, - Namespace: ns.GetName(), - Labels: map[string]string{"olm.catalogSource": sourceName}, - }, - Spec: v1alpha1.CatalogSourceSpec{ - SourceType: v1alpha1.SourceTypeGrpc, - Image: "quay.io/olmtest/catsrc-update-test:new", - UpdateStrategy: &v1alpha1.UpdateStrategy{ - RegistryPoll: &v1alpha1.RegistryPoll{ - RawInterval: "45s", + + BeforeEach(func() { + source = &v1alpha1.CatalogSource{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.CatalogSourceKind, + APIVersion: v1alpha1.CatalogSourceCRDAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: sourceName, + Namespace: testNamespace, + Labels: map[string]string{"olm.catalogSource": sourceName}, + }, + Spec: v1alpha1.CatalogSourceSpec{ + SourceType: v1alpha1.SourceTypeGrpc, + Image: "quay.io/olmtest/catsrc-update-test:new", + UpdateStrategy: &v1alpha1.UpdateStrategy{ + RegistryPoll: &v1alpha1.RegistryPoll{ + RawInterval: "45s", + }, }, }, - }, - } + } - source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.Background(), source, metav1.CreateOptions{}) - Expect(err).ToNot(HaveOccurred()) + source, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) - // wait for new catalog source pod to be created and report ready - selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": source.GetName()}) - singlePod := podCount(1) - catalogPods, err := awaitPods(GinkgoT(), c, source.GetNamespace(), selector.String(), singlePod) - Expect(err).ToNot(HaveOccurred()) - Expect(catalogPods).ToNot(BeNil()) + // wait for new catalog source pod to be created and report ready + selector := labels.SelectorFromSet(map[string]string{"olm.catalogSource": source.GetName()}) - Eventually(func() (bool, error) { - podList, err := c.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).List(context.Background(), metav1.ListOptions{LabelSelector: selector.String()}) - if err != nil { - return false, err - } + catalogPods, err := awaitPods(GinkgoT(), c, source.GetNamespace(), selector.String(), singlePod) + Expect(err).ToNot(HaveOccurred()) + Expect(catalogPods).ToNot(BeNil()) - for _, p := range podList.Items { - if podReady(&p) { - return true, nil + Eventually(func() (bool, error) { + podList, err := c.KubernetesInterface().CoreV1().Pods(source.GetNamespace()).List(context.TODO(), metav1.ListOptions{LabelSelector: selector.String()}) + if err != nil { + return false, err } + + for _, p := range podList.Items { + if podReady(&p) { + return true, nil + } + return false, nil + } + return false, nil + }).Should(BeTrue()) + }) + + It("registry polls on the correct interval", func() { + // Wait roughly the polling interval for update pod to show up + updateSelector := labels.SelectorFromSet(map[string]string{"catalogsource.operators.coreos.com/update": source.GetName()}) + updatePods, err := awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), updateSelector.String(), 5*time.Second, 2*time.Minute, singlePod) + Expect(err).ToNot(HaveOccurred()) + Expect(updatePods).ToNot(BeNil()) + Expect(updatePods.Items).To(HaveLen(1)) + + // No update to image: update pod should be deleted quickly + noPod := podCount(0) + updatePods, err = awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), updateSelector.String(), 1*time.Second, 30*time.Second, noPod) + Expect(err).ToNot(HaveOccurred()) + Expect(updatePods.Items).To(HaveLen(0)) + }) + + }) + + When("A catalogSource is created with incorrect interval", func() { + + var source *v1alpha1.CatalogSource + + BeforeEach(func() { + sourceName := genName("catalog-") + source = &v1alpha1.CatalogSource{ + TypeMeta: metav1.TypeMeta{ + Kind: v1alpha1.CatalogSourceKind, + APIVersion: v1alpha1.CatalogSourceCRDAPIVersion, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: sourceName, + Namespace: testNamespace, + Labels: map[string]string{"olm.catalogSource": sourceName}, + }, + Spec: v1alpha1.CatalogSourceSpec{ + SourceType: v1alpha1.SourceTypeGrpc, + Image: "quay.io/olmtest/catsrc-update-test:new", + UpdateStrategy: &v1alpha1.UpdateStrategy{ + RegistryPoll: &v1alpha1.RegistryPoll{ + RawInterval: "45mError.code", + }, + }, + }, } - return false, nil - }).Should(BeTrue()) + _, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Create(context.TODO(), source, metav1.CreateOptions{}) + Expect(err).ToNot(HaveOccurred()) - // Wait roughly the polling interval for update pod to show up - updateSelector := labels.SelectorFromSet(map[string]string{"catalogsource.operators.coreos.com/update": source.GetName()}) - updatePods, err := awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), updateSelector.String(), 5*time.Second, 2*time.Minute, singlePod) - Expect(err).ToNot(HaveOccurred()) - Expect(updatePods).ToNot(BeNil()) - Expect(updatePods.Items).To(HaveLen(1)) + }) - // No update to image: update pod should be deleted quickly - noPod := podCount(0) - updatePods, err = awaitPodsWithInterval(GinkgoT(), c, source.GetNamespace(), updateSelector.String(), 1*time.Second, 30*time.Second, noPod) - Expect(err).ToNot(HaveOccurred()) - Expect(updatePods.Items).To(HaveLen(0)) + It("the catalogsource status communicates that a default interval time is being used instead", func() { + Eventually(func() bool { + catsrc, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(context.TODO(), source.GetName(), metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + if catsrc.Status.Reason == v1alpha1.CatalogSourceIntervalInvalidError { + if catsrc.Status.Message == "error parsing spec.updateStrategy.registryPoll.interval. Using the default value of 15m0s instead. Error: time: unknown unit \"mError\" in duration \"45mError.code\"" { + return true + } + } + return false + }).Should(BeTrue()) + }) }) It("adding catalog template adjusts image used", func() { From 03c2e6b8d6e17308b9b69ae04c06d00d217ecd15 Mon Sep 17 00:00:00 2001 From: Anik Bhattacharjee Date: Wed, 6 Apr 2022 13:15:10 -0400 Subject: [PATCH 2/2] add test to verify error clears on correct update Signed-off-by: Anik Bhattacharjee --- pkg/controller/operators/catalog/operator.go | 4 -- test/e2e/catalog_e2e_test.go | 48 +++++++++++++++++--- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index deed0565f1..455d9f4646 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -730,10 +730,6 @@ func (o *Operator) syncRegistryServer(logger *logrus.Entry, in *v1alpha1.Catalog if out.Spec.UpdateStrategy.RegistryPoll != nil { if out.Spec.UpdateStrategy.RegistryPoll.ParsingError != "" && out.Status.Reason != v1alpha1.CatalogSourceIntervalInvalidError { out.SetError(v1alpha1.CatalogSourceIntervalInvalidError, fmt.Errorf(out.Spec.UpdateStrategy.RegistryPoll.ParsingError)) - if _, err := o.client.OperatorsV1alpha1().CatalogSources(out.GetNamespace()).UpdateStatus(context.TODO(), out, metav1.UpdateOptions{}); err != nil { - logger.Errorf("error while updating catalogsource status for an invalid interval - %v", err) - return - } } logger.Debugf("requeuing registry server sync based on polling interval %s", out.Spec.UpdateStrategy.Interval.Duration.String()) resyncPeriod := reconciler.SyncRegistryUpdateInterval(out, time.Now()) diff --git a/test/e2e/catalog_e2e_test.go b/test/e2e/catalog_e2e_test.go index 57ef1e31f8..997e07c54d 100644 --- a/test/e2e/catalog_e2e_test.go +++ b/test/e2e/catalog_e2e_test.go @@ -1074,7 +1074,7 @@ var _ = Describe("Starting CatalogSource e2e tests", func() { Expect(err).ShouldNot(HaveOccurred()) Expect(csv.Spec.Replaces).To(Equal("busybox-dependency.v1.0.0")) }) - When("A catalogSource is created with correct interval", func() { + When("A catalogSource is created with correct polling interval", func() { var source *v1alpha1.CatalogSource singlePod := podCount(1) @@ -1146,12 +1146,18 @@ var _ = Describe("Starting CatalogSource e2e tests", func() { }) - When("A catalogSource is created with incorrect interval", func() { - - var source *v1alpha1.CatalogSource + When("A catalogSource is created with incorrect polling interval", func() { + var ( + source *v1alpha1.CatalogSource + sourceName string + ) + const ( + incorrectInterval = "45mError.code" + correctInterval = "45m" + ) BeforeEach(func() { - sourceName := genName("catalog-") + sourceName = genName("catalog-") source = &v1alpha1.CatalogSource{ TypeMeta: metav1.TypeMeta{ Kind: v1alpha1.CatalogSourceKind, @@ -1167,7 +1173,7 @@ var _ = Describe("Starting CatalogSource e2e tests", func() { Image: "quay.io/olmtest/catsrc-update-test:new", UpdateStrategy: &v1alpha1.UpdateStrategy{ RegistryPoll: &v1alpha1.RegistryPoll{ - RawInterval: "45mError.code", + RawInterval: incorrectInterval, }, }, }, @@ -1177,7 +1183,10 @@ var _ = Describe("Starting CatalogSource e2e tests", func() { Expect(err).ToNot(HaveOccurred()) }) - + AfterEach(func() { + err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Delete(context.TODO(), source.GetName(), metav1.DeleteOptions{}) + Expect(err).ToNot(HaveOccurred()) + }) It("the catalogsource status communicates that a default interval time is being used instead", func() { Eventually(func() bool { catsrc, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(context.TODO(), source.GetName(), metav1.GetOptions{}) @@ -1190,6 +1199,31 @@ var _ = Describe("Starting CatalogSource e2e tests", func() { return false }).Should(BeTrue()) }) + When("the catalogsource is updated with a valid polling interval", func() { + BeforeEach(func() { + catsrc, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(context.TODO(), source.GetName(), metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + catsrc.Spec.UpdateStrategy.RegistryPoll.RawInterval = correctInterval + _, err = crc.OperatorsV1alpha1().CatalogSources(catsrc.GetNamespace()).Update(context.TODO(), catsrc, metav1.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + }) + It("the catalogsource spec shows the updated polling interval, and the error message in the status is cleared", func() { + Eventually(func() error { + catsrc, err := crc.OperatorsV1alpha1().CatalogSources(source.GetNamespace()).Get(context.TODO(), source.GetName(), metav1.GetOptions{}) + if err != nil { + return err + } + expectedTime, err := time.ParseDuration(correctInterval) + if err != nil { + return err + } + if catsrc.Status.Reason != "" || (catsrc.Spec.UpdateStrategy.Interval != &metav1.Duration{expectedTime}) { + return err + } + return nil + }).Should(Succeed()) + }) + }) }) It("adding catalog template adjusts image used", func() {