Cleanup e2e tests and add case for invalid bundle image

Author: hasbro17

cmd/catalog/main.go

@@ -72,7 +72,7 @@ var (
 
 	installPlanTimeout = flag.Duration("install-plan-retry-timeout", 1*time.Minute, "time since first attempt at which plan execution errors are considered fatal")
 	// TODO(haseeb): Change to a more realistic timeout for bundle unpack jobs
-	bundleUnpackTimeout = flag.Duration("bundle-unpack-timeout", 60*time.Second, "The time duration after which the bundle unpack Job for an installplan will be aborted and the installplan will be Failed. 0 is considered as having no timeout.")
+	bundleUnpackTimeout = flag.Duration("bundle-unpack-timeout", catalog.DefaultBundleUnpackTimeout, "The time duration after which the bundle unpack Job for an installplan will be aborted and the installplan will be Failed. 0 is considered as having no timeout.")
 )
 
 func init() {

pkg/controller/operators/catalog/operator.go

@@ -77,6 +77,8 @@ const (
 	maxInstallPlanCount    = 5
 	maxDeletesPerSweep     = 5
 	RegistryFieldManager   = "olm.registry"
+	// TODO(haseeb): Change to a more realistic timeout for bundle unpack jobs
+	DefaultBundleUnpackTimeout = 2 * time.Minute
 )
 
 // Operator represents a Kubernetes operator that executes InstallPlans by

test/e2e/installplan_e2e_test.go

@@ -2999,108 +2999,140 @@ var _ = Describe("Install Plan", func() {
 		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
 	})
 
-	It("should fail an InstallPlan when an InstallPlan has a bundle unpack job timeout", func() {
-		ns := &corev1.Namespace{}
-		ns.SetName(genName("ns-"))
+	When("waiting on the bundle unpacking job", func() {
+		var (
+			ns         *corev1.Namespace
+			catsrcName string
+			crc        versioned.Interface
+			ip         *operatorsv1alpha1.InstallPlan
+		)
+		BeforeEach(func() {
+			crc = newCRClient()
+			ns = &corev1.Namespace{}
+			ns.SetName(genName("ns-"))
+			Expect(ctx.Ctx().Client().Create(context.Background(), ns)).To(Succeed())
 
-		c := newKubeClient()
-		crc := newCRClient()
-		now := metav1.Now()
+			// Create the single (kiali) bundle catalog source
+			catsrc := &operatorsv1alpha1.CatalogSource{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      genName("kiali-"),
+					Namespace: ns.GetName(),
+					Labels:    map[string]string{"olm.catalogSource": "kaili-catalog"},
+				},
+				Spec: operatorsv1alpha1.CatalogSourceSpec{
+					Image:      "quay.io/olmtest/single-bundle-index:1.0.0",
+					SourceType: operatorsv1alpha1.SourceTypeGrpc,
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), catsrc)).To(Succeed())
 
-		// Create a namespace
-		ns, err := c.KubernetesInterface().CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		deleteOpts := &metav1.DeleteOptions{}
-		defer func() {
-			err = c.KubernetesInterface().CoreV1().Namespaces().Delete(context.TODO(), ns.GetName(), *deleteOpts)
+			// Wait for the CatalogSource to be ready
+			catsrc, err := fetchCatalogSourceOnStatus(crc, catsrc.GetName(), catsrc.GetNamespace(), catalogSourceRegistryPodSynced)
 			Expect(err).ToNot(HaveOccurred())
-		}()
-
-		// Create the single (kiali) bundle catalog source
-		catsrc := &operatorsv1alpha1.CatalogSource{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      genName("kiali-"),
-				Namespace: ns.GetName(),
-				Labels:    map[string]string{"olm.catalogSource": "kaili-catalog"},
-			},
-			Spec: operatorsv1alpha1.CatalogSourceSpec{
-				Image:      "quay.io/olmtest/single-bundle-index:1.0.0",
-				SourceType: operatorsv1alpha1.SourceTypeGrpc,
-			},
-		}
-		catsrc, err = crc.OperatorsV1alpha1().CatalogSources(catsrc.GetNamespace()).Create(context.TODO(), catsrc, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
 
-		// Wait for the CatalogSource to be ready
-		catsrc, err = fetchCatalogSourceOnStatus(crc, catsrc.GetName(), catsrc.GetNamespace(), catalogSourceRegistryPodSynced)
-		Expect(err).ToNot(HaveOccurred())
+			catsrcName = catsrc.GetName()
 
-		// Create the OperatorGroup
-		og := &operatorsv1.OperatorGroup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "og",
-			},
-			Spec: operatorsv1.OperatorGroupSpec{
-				TargetNamespaces: []string{ns.GetName()},
-			},
-		}
-		_, err = crc.OperatorsV1().OperatorGroups(ns.GetName()).Create(context.TODO(), og, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
+			// Create the OperatorGroup
+			og := &operatorsv1.OperatorGroup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "og",
+					Namespace: ns.GetName(),
+				},
+				Spec: operatorsv1.OperatorGroupSpec{
+					TargetNamespaces: []string{ns.GetName()},
+				},
+			}
+			Expect(ctx.Ctx().Client().Create(context.Background(), og)).To(Succeed())
 
-		// Create an InstallPlan in the Planning phase with status.bundleLookups specified for a non-existing bundle
-		ip := &operatorsv1alpha1.InstallPlan{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "ip",
-				Namespace: ns.GetName(),
-			},
-			Spec: operatorsv1alpha1.InstallPlanSpec{
-				ClusterServiceVersionNames: []string{"foobar"},
-				Approval:                   v1alpha1.ApprovalAutomatic,
-				Approved:                   true,
-				Generation:                 1,
-			},
-			Status: operatorsv1alpha1.InstallPlanStatus{
-				Phase:          operatorsv1alpha1.InstallPlanPhasePlanning,
-				CatalogSources: []string{},
-				BundleLookups: []operatorsv1alpha1.BundleLookup{
-					{
-						Path:       "quay.io/foo/bar:v0.0.1",
-						Identifier: "foobar.v0.0.1",
-						CatalogSourceRef: &corev1.ObjectReference{
-							Namespace: ns.GetName(),
-							Name:      catsrc.GetName(),
-						},
-						Conditions: []operatorsv1alpha1.BundleLookupCondition{
-							{
-								Type:               operatorsv1alpha1.BundleLookupPending,
-								Status:             corev1.ConditionTrue,
-								Reason:             "JobIncomplete",
-								Message:            "unpack job not completed",
-								LastTransitionTime: &now,
+			now := metav1.Now()
+			ip = &operatorsv1alpha1.InstallPlan{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "ip",
+					Namespace: ns.GetName(),
+				},
+				Spec: operatorsv1alpha1.InstallPlanSpec{
+					ClusterServiceVersionNames: []string{"foobar"},
+					Approval:                   v1alpha1.ApprovalAutomatic,
+					Approved:                   true,
+				},
+				Status: operatorsv1alpha1.InstallPlanStatus{
+					Phase:          operatorsv1alpha1.InstallPlanPhasePlanning,
+					CatalogSources: []string{},
+					BundleLookups: []operatorsv1alpha1.BundleLookup{
+						{
+							Identifier: "foobar.v0.0.1",
+							CatalogSourceRef: &corev1.ObjectReference{
+								Namespace: ns.GetName(),
+								Name:      catsrcName,
+							},
+							Conditions: []operatorsv1alpha1.BundleLookupCondition{
+								{
+									Type:               operatorsv1alpha1.BundleLookupPending,
+									Status:             corev1.ConditionTrue,
+									Reason:             "JobIncomplete",
+									Message:            "unpack job not completed",
+									LastTransitionTime: &now,
+								},
 							},
 						},
 					},
 				},
-			},
-		}
+			}
+		})
 
-		outIP, err := crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).Create(context.TODO(), ip, metav1.CreateOptions{})
-		Expect(err).NotTo(HaveOccurred())
-		Expect(outIP).NotTo(BeNil())
+		AfterEach(func() {
+			Expect(ctx.Ctx().Client().Delete(context.Background(), ns)).To(Succeed())
+		})
 
-		// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
-		// InstallPlans without any steps or bundle lookups
-		outIP.Status = ip.Status
-		_, err = crc.OperatorsV1alpha1().InstallPlans(ns.GetName()).UpdateStatus(context.TODO(), outIP, metav1.UpdateOptions{})
-		Expect(err).NotTo(HaveOccurred())
+		It("should timeout and fail the InstallPlan for a non-existent bundle image", func() {
+			// Create an InstallPlan status.bundleLookups.Path specified for a non-existent bundle image
+			ip.Status.BundleLookups[0].Path = "quay.io/foo/bar:v0.0.1"
 
-		// The InstallPlan should be Failed due to the bundle unpack job timeout
-		// TODO(haseeb): Use Eventually() to set a Duration just over the default bundle-unpack-timeout duration. Else this could poll for a long time.
-		fetchedInstallPlan, err := fetchInstallPlanWithNamespace(GinkgoT(), crc, ip.Name, ns.GetName(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
-		Expect(err).NotTo(HaveOccurred())
-		Expect(fetchedInstallPlan).NotTo(BeNil())
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with phase %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase))
-		ctx.Ctx().Logf(fmt.Sprintf("Install plan %s fetched with conditions %+v", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Conditions))
+			outIP := ip.DeepCopy()
+
+			Expect(ctx.Ctx().Client().Create(context.Background(), outIP)).To(Succeed())
+
+			// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
+			// InstallPlans without any steps or bundle lookups
+			outIP.Status = ip.Status
+			Expect(ctx.Ctx().Client().Status().Update(context.Background(), outIP)).To(Succeed())
+
+			// The InstallPlan should be Failed due to the bundle unpack job timeout
+			// We wait for some time over the bundle unpack Job's timeout (i.e ActiveDeadlineSeconds) so that the Job can fail
+			waitFor := catalog.DefaultBundleUnpackTimeout + 30*time.Second
+			Eventually(
+				func() (*operatorsv1alpha1.InstallPlan, error) {
+					err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(outIP), outIP)
+					return outIP, err
+				},
+				waitFor,
+			).Should(HavePhase(operatorsv1alpha1.InstallPlanPhaseFailed))
+		})
+
+		It("should timeout and fail the InstallPlan for an invalid bundle image", func() {
+			// Create an InstallPlan status.bundleLookups.Path specified for an invalid bundle image
+			ip.Status.BundleLookups[0].Path = "alpine:3.13"
+
+			outIP := ip.DeepCopy()
+
+			Expect(ctx.Ctx().Client().Create(context.Background(), outIP)).To(Succeed())
+
+			// The status gets ignored on create so we need to update it else the InstallPlan sync ignores
+			// InstallPlans without any steps or bundle lookups
+			outIP.Status = ip.Status
+			Expect(ctx.Ctx().Client().Status().Update(context.Background(), outIP)).To(Succeed())
+
+			// The InstallPlan should be Failed due to the bundle unpack job timeout
+			// We wait for some time over the bundle unpack Job's timeout (i.e ActiveDeadlineSeconds) so that the Job can fail
+			waitFor := catalog.DefaultBundleUnpackTimeout + 30*time.Second
+			Eventually(
+				func() (*operatorsv1alpha1.InstallPlan, error) {
+					err := ctx.Ctx().Client().Get(context.Background(), client.ObjectKeyFromObject(outIP), outIP)
+					return outIP, err
+				},
+				waitFor,
+			).Should(HavePhase(operatorsv1alpha1.InstallPlanPhaseFailed))
+		})
 
 	})