unpack retry e2e tests

Signed-off-by: Ankita Thomas <[email protected]>

Author: ankitathomas

Diff for: pkg/controller/bundle/bundle_unpacker.go

@@ -18,6 +18,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8slabels "k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/client-go/kubernetes"
 	listersbatchv1 "k8s.io/client-go/listers/batch/v1"
 	listerscorev1 "k8s.io/client-go/listers/core/v1"
@@ -687,10 +688,10 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath
 
 			if failed {
 				if time.Now().After(lastFailureTime.Add(unpackRetryInterval)) {
-					fresh.SetName(fmt.Sprintf("%s-%d", fresh.GetName(), len(jobs)))
+					fresh.SetName(names.SimpleNameGenerator.GenerateName(fresh.GetName()))
 					job, err = c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{})
-					return
 				}
+				return
 			}
 		}
 	}
@@ -880,7 +881,7 @@ func OperatorGroupBundleUnpackRetryInterval(ogLister v1listers.OperatorGroupName
 
 	d, err := time.ParseDuration(timeoutStr)
 	if err != nil {
-		return 0, fmt.Errorf("failed to parse unpack timeout annotation(%s: %s): %w", BundleUnpackRetryMinimumIntervalAnnotationKey, timeoutStr, err)
+		return 0, fmt.Errorf("failed to parse unpack retry annotation(%s: %s): %w", BundleUnpackRetryMinimumIntervalAnnotationKey, timeoutStr, err)
 	}
 
 	return d, nil

Diff for: pkg/controller/bundle/bundle_unpacker_test.go

@@ -1820,3 +1820,115 @@ func TestOperatorGroupBundleUnpackTimeout(t *testing.T) {
 		})
 	}
 }
+
+func TestOperatorGroupBundleUnpackRetryInterval(t *testing.T) {
+	nsName := "fake-ns"
+
+	for _, tc := range []struct {
+		name            string
+		operatorGroups  []*operatorsv1.OperatorGroup
+		expectedTimeout time.Duration
+		expectedError   error
+	}{
+		{
+			name:            "No operator groups exist",
+			expectedTimeout: 0,
+			expectedError:   errors.New("found 0 operatorGroups, expected 1"),
+		},
+		{
+			name: "Multiple operator groups exist",
+			operatorGroups: []*operatorsv1.OperatorGroup{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       operatorsv1.OperatorGroupKind,
+						APIVersion: operatorsv1.GroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "og1",
+						Namespace: nsName,
+					},
+				},
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       operatorsv1.OperatorGroupKind,
+						APIVersion: operatorsv1.GroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "og2",
+						Namespace: nsName,
+					},
+				},
+			},
+			expectedTimeout: 0,
+			expectedError:   errors.New("found 2 operatorGroups, expected 1"),
+		},
+		{
+			name: "One operator group exists with valid unpack retry annotation",
+			operatorGroups: []*operatorsv1.OperatorGroup{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       operatorsv1.OperatorGroupKind,
+						APIVersion: operatorsv1.GroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "og",
+						Namespace:   nsName,
+						Annotations: map[string]string{BundleUnpackRetryMinimumIntervalAnnotationKey: "1m"},
+					},
+				},
+			},
+			expectedTimeout: 1 * time.Minute,
+			expectedError:   nil,
+		},
+		{
+			name: "One operator group exists with no unpack retry annotation",
+			operatorGroups: []*operatorsv1.OperatorGroup{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       operatorsv1.OperatorGroupKind,
+						APIVersion: operatorsv1.GroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "og",
+						Namespace: nsName,
+					},
+				},
+			},
+			expectedTimeout: 0,
+			expectedError:   nil,
+		},
+		{
+			name: "One operator group exists with invalid unpack retry annotation",
+			operatorGroups: []*operatorsv1.OperatorGroup{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       operatorsv1.OperatorGroupKind,
+						APIVersion: operatorsv1.GroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "og",
+						Namespace:   nsName,
+						Annotations: map[string]string{BundleUnpackRetryMinimumIntervalAnnotationKey: "invalid"},
+					},
+				},
+			},
+			expectedTimeout: 0,
+			expectedError:   fmt.Errorf("failed to parse unpack retry annotation(operatorframework.io/bundle-unpack-min-retry-interval: invalid): %w", errors.New("time: invalid duration \"invalid\"")),
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			ogIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+			ogLister := v1listers.NewOperatorGroupLister(ogIndexer).OperatorGroups(nsName)
+
+			for _, og := range tc.operatorGroups {
+				err := ogIndexer.Add(og)
+				assert.NoError(t, err)
+			}
+
+			timeout, err := OperatorGroupBundleUnpackRetryInterval(ogLister)
+
+			assert.Equal(t, tc.expectedTimeout, timeout)
+			assert.Equal(t, tc.expectedError, err)
+		})
+	}
+}

Diff for: test/e2e/fbc_provider.go

@@ -31,3 +31,9 @@ func NewFileBasedFiledBasedCatalogProvider(path string) (FileBasedCatalogProvide
 func (f *fileBasedFileBasedCatalogProvider) GetCatalog() string {
 	return f.fbc
 }
+
+func NewRawFileBasedCatalogProvider(data string) (FileBasedCatalogProvider, error) {
+	return &fileBasedFileBasedCatalogProvider{
+		fbc: string(data),
+	}, nil
+}

Diff for: test/e2e/subscription_e2e_test.go

@@ -2520,6 +2520,172 @@ var _ = Describe("Subscription", func() {
 				_, err = fetchSubscription(crc, generatedNamespace.GetName(), subName, subscriptionHasCurrentCSV("example-operator.v0.2.0"))
 				Expect(err).Should(BeNil())
 			})
+
+			It("should recreate failing unpacking jobs when unpack retry is specified", func() {
+				By("Ensuring a registry to host bundle images")
+				local, err := Local(c)
+				Expect(err).NotTo(HaveOccurred(), "cannot determine if test running locally or on CI: %s", err)
+
+				var registryURL string
+				var copyImage func(src, srcTag, dst, dstTag string) error
+				if local {
+					registryURL, err = createDockerRegistry(c, generatedNamespace.GetName())
+					Expect(err).NotTo(HaveOccurred(), "error creating container registry: %s", err)
+					defer deleteDockerRegistry(c, generatedNamespace.GetName())
+
+					// ensure registry pod is ready before attempting port-forwarding
+					_ = awaitPod(GinkgoT(), c, generatedNamespace.GetName(), registryName, podReady)
+
+					err = registryPortForward(generatedNamespace.GetName())
+					Expect(err).NotTo(HaveOccurred(), "port-forwarding local registry: %s", err)
+					copyImage = func(src, srcTag, dst, dstTag string) error {
+						_, err := skopeoLocalCopy(src, srcTag, dst, dstTag)
+						return err
+					}
+				} else {
+					registryURL = fmt.Sprintf("%s/%s", openshiftregistryFQDN, generatedNamespace.GetName())
+					registryAuth, err := openshiftRegistryAuth(c, generatedNamespace.GetName())
+					Expect(err).NotTo(HaveOccurred(), "error getting openshift registry authentication: %s", err)
+					copyImage = func(src, srcTag, dst, dstTag string) error {
+						skopeoArgs := skopeoCopyCmd(src, srcTag, dst, dstTag, registryAuth)
+						err = createSkopeoPod(c, skopeoArgs, generatedNamespace.GetName())
+						if err != nil {
+							return fmt.Errorf("error creating skopeo pod: %v", err)
+						}
+
+						// wait for skopeo pod to exit successfully
+						awaitPod(GinkgoT(), c, generatedNamespace.GetName(), skopeo, func(pod *corev1.Pod) bool {
+							return pod.Status.Phase == corev1.PodSucceeded
+						})
+
+						if err := deleteSkopeoPod(c, generatedNamespace.GetName()); err != nil {
+							return fmt.Errorf("error deleting skopeo pod: %s", err)
+						}
+						return nil
+					}
+				}
+
+				// testImage is the name of the image used throughout the test - the image overwritten by skopeo
+				// the tag is generated randomly and appended to the end of the testImage
+				srcImage := "quay.io/olmtest/example-operator-bundle:0.1.0"
+				srcTag := "0.1.0"
+				bundleImage := fmt.Sprint("docker://", registryURL, "/unpack-retry-bundle", ":")
+				bundleTag := genName("x")
+
+				unpackRetryCatalog := fmt.Sprintf(`
+---
+schema: olm.package
+name: unpack-retry-package
+defaultChannel: stable
+---
+schema: olm.channel
+package: unpack-retry-package
+name: stable
+entries:
+  - name: unpack-retry-operator.v1.0.0
+---
+schema: olm.bundle
+name: unpack-retry-operator.v1.0.0
+package: unpack-retry-package
+image: %s%s
+properties:
+  - type: olm.package
+    value:
+      packageName: unpack-retry-package
+      version: 1.0.0
+`, bundleImage, bundleTag)
+
+				By("creating a catalog referencing a non-existent bundle image")
+				unpackRetryProvider, err := NewRawFileBasedCatalogProvider(unpackRetryCatalog)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(magicCatalog.UpdateCatalog(context.Background(), unpackRetryProvider)).To(Succeed())
+
+				By("creating a subscription for the missing bundle")
+				unpackRetrySubName := fmt.Sprintf("%s-unpack-retry-package-sub", generatedNamespace.GetName())
+				createSubscriptionForCatalog(crc, generatedNamespace.GetName(), unpackRetrySubName, catalogSourceName, "unpack-retry-package", stableChannel, "", operatorsv1alpha1.ApprovalAutomatic)
+
+				By("waiting for bundle unpack to fail")
+				Eventually(
+					func() error {
+						fetched, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), unpackRetrySubName, metav1.GetOptions{})
+						if err != nil {
+							return err
+						}
+						if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpackFailed); cond.Status != corev1.ConditionTrue || cond.Reason != "BundleUnpackFailed" {
+							return fmt.Errorf("%s condition not found", v1alpha1.SubscriptionBundleUnpackFailed)
+						}
+						return nil
+					},
+					5*time.Minute,
+					interval,
+				).Should(Succeed())
+
+				By("patching operator group to enable unpack retries")
+				ogNN := types.NamespacedName{Name: operatorGroup.GetName(), Namespace: generatedNamespace.GetName()}
+				setBundleUnpackRetryMinimumIntervalAnnotation(context.Background(), ctx.Ctx().Client(), ogNN, "1s")
+
+				By("ensuring failing bundle unpack jobs are retried")
+				Eventually(func() error {
+					fetched, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), unpackRetrySubName, metav1.GetOptions{})
+					if err != nil {
+						return err
+					}
+					if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionTrue {
+						return fmt.Errorf("missing expected condition %s on subscription %s", v1alpha1.SubscriptionBundleUnpacking, unpackRetrySubName)
+					}
+					return nil
+				}).Should(Succeed())
+
+				By("Ensuring successful bundle unpack jobs are not retried")
+				Consistently(func() error {
+					fetched, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), subName, metav1.GetOptions{})
+					if err != nil {
+						return err
+					}
+					if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionUnknown {
+						return fmt.Errorf("unexpected unpack condition %s on subscription %s", v1alpha1.SubscriptionBundleUnpacking, subName)
+					}
+					if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpackFailed); cond.Status != corev1.ConditionUnknown {
+						return fmt.Errorf("unexpected unpack condition %s on subscription %s", v1alpha1.SubscriptionBundleUnpackFailed, subName)
+					}
+					return nil
+				}).Should(Succeed())
+
+				By("patching operator group to disable unpack retries")
+				setBundleUnpackRetryMinimumIntervalAnnotation(context.Background(), ctx.Ctx().Client(), ogNN, "")
+
+				By("ensuring failing bundle unpack jobs are no longer retried")
+				Eventually(func() error {
+					fetched, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), unpackRetrySubName, metav1.GetOptions{})
+					if err != nil {
+						return err
+					}
+					if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpackFailed); cond.Status != corev1.ConditionTrue {
+						return fmt.Errorf("missing expected condition %s on subscription %s", v1alpha1.SubscriptionBundleUnpacking, unpackRetrySubName)
+					}
+					return nil
+				}).Should(Succeed())
+
+				Consistently(func() error {
+					fetched, err := crc.OperatorsV1alpha1().Subscriptions(generatedNamespace.GetName()).Get(context.Background(), unpackRetrySubName, metav1.GetOptions{})
+					if err != nil {
+						return err
+					}
+					if cond := fetched.Status.GetCondition(v1alpha1.SubscriptionBundleUnpacking); cond.Status != corev1.ConditionUnknown {
+						return fmt.Errorf("mnexpected condition %s on subscription %s", v1alpha1.SubscriptionBundleUnpacking, unpackRetrySubName)
+					}
+					return nil
+				}).Should(Succeed())
+
+				By("pushing missing bundle image")
+				Expect(copyImage(srcImage, srcTag, bundleImage, bundleTag)).To(Succeed())
+
+				By("patching operator group to enable unpack retries")
+				setBundleUnpackRetryMinimumIntervalAnnotation(context.Background(), ctx.Ctx().Client(), ogNN, "1s")
+
+				By("waiting for checking for installPlan to indicate unpack succeeds")
+				Expect(fetchSubscription(crc, magicCatalog.GetNamespace(), unpackRetrySubName, subscriptionHasInstallPlanChecker)).To(Succeed())
+			})
 		})
 	})
 })

Diff for: test/e2e/util.go

@@ -93,13 +93,25 @@ func objectRefToNamespacedName(ip *corev1.ObjectReference) types.NamespacedName
 // adding the "operatorframework.io/bundle-unpack-timeout" annotation to an OperatorGroup
 // resource.
 func addBundleUnpackTimeoutOGAnnotation(ctx context.Context, c k8scontrollerclient.Client, ogNN types.NamespacedName, timeout string) {
+	setOGAnnotation(ctx, c, ogNN, bundle.BundleUnpackTimeoutAnnotationKey, timeout)
+}
+
+func setBundleUnpackRetryMinimumIntervalAnnotation(ctx context.Context, c k8scontrollerclient.Client, ogNN types.NamespacedName, interval string) {
+	setOGAnnotation(ctx, c, ogNN, bundle.BundleUnpackRetryMinimumIntervalAnnotationKey, interval)
+}
+
+func setOGAnnotation(ctx context.Context, c k8scontrollerclient.Client, ogNN types.NamespacedName, key, value string) {
 	Eventually(func() error {
 		og := &operatorsv1.OperatorGroup{}
 		if err := c.Get(ctx, ogNN, og); err != nil {
 			return err
 		}
 		annotations := og.GetAnnotations()
-		annotations[bundle.BundleUnpackTimeoutAnnotationKey] = timeout
+		if len(value) == 0 {
+			delete(annotations, key)
+		} else {
+			annotations[key] = value
+		}
 		og.SetAnnotations(annotations)
 
 		return c.Update(ctx, og)