Fix a bug in olm install

When installing OLM, retry creating resources, if the CRD is not ready
yet.

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

Author: nunnatsa

changelog/fragments/01-olm-install-retry.yaml

@@ -0,0 +1,10 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      Fix a bug where `olm install` command is failed fo "no-match" error.
+
+    kind: "bugfix"
+
+    # Is this a breaking change?
+    breaking: false

internal/olm/client/client.go

@@ -112,16 +112,52 @@ func NewClientForConfig(cfg *rest.Config) (*Client, error) {
 func (c Client) DoCreate(ctx context.Context, objs ...client.Object) error {
 	for _, obj := range objs {
 		kind := obj.GetObjectKind().GroupVersionKind().Kind
-		log.Infof("  Creating %s %q", kind, getName(obj.GetNamespace(), obj.GetName()))
-		err := c.KubeClient.Create(ctx, obj)
+		resourceName := getName(obj.GetNamespace(), obj.GetName())
+
+		log.Infof("  Creating %s %q", kind, resourceName)
+
+		if err := c.safeCreateOneResource(ctx, obj, kind, resourceName); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// try to create 10 times before giving up
+func (c Client) safeCreateOneResource(ctx context.Context, obj client.Object, kind string, resourceName string) error {
+	for i := 0; i < 10; i++ { // try to create 10 times before giving up
+		retry, err := c.tryCreatingOneResource(ctx, obj, kind)
 		if err != nil {
-			if !apierrors.IsAlreadyExists(err) {
-				return err
-			}
+			return err
+		}
+
+		if !retry {
+			log.Infof("    %s %q created", kind, resourceName)
+			return nil
+		}
+
+		log.Infof("    the CRD for %s %q is not ready yet. Retrying in one second", kind, resourceName)
+		time.Sleep(time.Second)
+	}
+
+	return fmt.Errorf("failed to create the %s %q resource", kind, resourceName)
+}
+
+// tryCreatingOneResource makes one try to create a resource in the cluster. It returns true if retry is needed, and an
+// error if something went wrong
+func (c Client) tryCreatingOneResource(ctx context.Context, obj client.Object, kind string) (bool, error) {
+	err := c.KubeClient.Create(ctx, obj)
+	if err != nil {
+		if apierrors.IsAlreadyExists(err) {
 			log.Infof("    %s %q already exists", kind, getName(obj.GetNamespace(), obj.GetName()))
+			return false, nil
+		} else if meta.IsNoMatchError(err) {
+			return true, nil
 		}
+		return false, err
 	}
-	return nil
+
+	return false, nil
 }
 
 func (c Client) DoDelete(ctx context.Context, objs ...client.Object) error {

internal/olm/client/client_test.go

@@ -16,18 +16,19 @@ package client
 
 import (
 	"context"
+	"errors"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	olmapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	fake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 var _ = Describe("Client", func() {
@@ -258,4 +259,148 @@ var _ = Describe("Client", func() {
 			})
 		})
 	})
+
+	Describe("test DoCreate", func() {
+		var fakeClient client.Client
+
+		BeforeEach(func() {
+			fakeClient = &errClient{cli: fake.NewClientBuilder().Build()}
+		})
+
+		AfterEach(func() {
+			fakeClient.(*errClient).reset()
+		})
+
+		It("should create all the resources successfully", func() {
+			cli := Client{KubeClient: fakeClient}
+
+			Expect(cli.DoCreate(context.Background(),
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-ns"},
+				},
+				&corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-pod", Namespace: "test-ns"},
+				},
+			)).To(Succeed())
+
+			ns := &corev1.Namespace{}
+			Expect(fakeClient.Get(context.Background(), client.ObjectKey{Name: "test-ns"}, ns)).To(Succeed())
+
+			pod := &corev1.Pod{}
+			Expect(fakeClient.Get(context.Background(), client.ObjectKey{Namespace: "test-ns", Name: "test-pod"}, pod)).To(Succeed())
+		})
+
+		It("should eventually create all the resources successfully", func() {
+			cli := Client{KubeClient: fakeClient}
+
+			Expect(cli.DoCreate(context.Background(),
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-ns"},
+				},
+				&corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{Name: "eventually-match", Namespace: "test-ns"},
+				},
+			)).To(Succeed())
+
+			ns := &corev1.Namespace{}
+			Expect(fakeClient.Get(context.Background(), client.ObjectKey{Name: "test-ns"}, ns)).To(Succeed())
+
+			pod := &corev1.Pod{}
+			Expect(fakeClient.Get(context.Background(), client.ObjectKey{Namespace: "test-ns", Name: "eventually-match"}, pod)).To(Succeed())
+		})
+
+		It("should fail with no-match error", func() {
+			cli := Client{KubeClient: fakeClient}
+
+			Expect(cli.DoCreate(context.Background(),
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-ns"},
+				},
+				&corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{Name: "no-match", Namespace: "test-ns"},
+				},
+			)).ToNot(Succeed())
+		})
+
+		It("should fail with unknown-error error", func() {
+			cli := Client{KubeClient: fakeClient}
+
+			Expect(cli.DoCreate(context.Background(),
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-ns"},
+				},
+				&corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{Name: "no-match", Namespace: "unknown-error"},
+				},
+			)).ToNot(Succeed())
+		})
+	})
 })
+
+type errClient struct {
+	cli            client.Client
+	noMatchCounter int
+}
+
+func (c *errClient) reset() {
+	c.noMatchCounter = 0
+}
+
+func (c *errClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	return c.cli.Get(ctx, key, obj, opts...)
+}
+
+func (c *errClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	return c.cli.List(ctx, list, opts...)
+}
+func (c *errClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
+	switch obj.GetName() {
+	case "no-match":
+		return &meta.NoResourceMatchError{}
+
+	case "eventually-match":
+		if c.noMatchCounter > 5 {
+			return c.cli.Create(ctx, obj, opts...)
+		}
+		c.noMatchCounter++
+		return &meta.NoResourceMatchError{}
+
+	case "unknown-error":
+		return errors.New("fake error")
+
+	default:
+		return c.cli.Create(ctx, obj, opts...)
+	}
+}
+
+func (c *errClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+	return c.cli.Delete(ctx, obj, opts...)
+}
+
+func (c *errClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	return c.cli.Update(ctx, obj, opts...)
+}
+
+func (c *errClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+	return c.cli.Patch(ctx, obj, patch, opts...)
+}
+
+func (c *errClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
+	return c.cli.DeleteAllOf(ctx, obj, opts...)
+}
+
+func (c *errClient) SubResource(subResource string) client.SubResourceClient {
+	return c.cli.SubResource(subResource)
+}
+
+func (c *errClient) Scheme() *runtime.Scheme {
+	return c.cli.Scheme()
+}
+
+func (c *errClient) RESTMapper() meta.RESTMapper {
+	return c.cli.RESTMapper()
+}
+
+func (c *errClient) Status() client.SubResourceWriter {
+	return c.cli.Status()
+}