Replace Delivery retries with Informers for Templates

[#23]

Co-authored-by: Rasheed Abdul-Aziz <rabdulaziz@vmware.com>

config/crd/bases/carto.run_clusterdeliveries.yaml

@@ -92,6 +92,7 @@ spec:
                     templateRef:
                       properties:
                         kind:
+                          description: Kind THIS MUST MATCH ValidDeliveryTemplates
                           enum:
                           - ClusterSourceTemplate
                           - ClusterDeploymentTemplate

pkg/apis/v1alpha1/cluster_delivery.go

@@ -23,6 +23,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -64,7 +65,15 @@ type ClusterDeliveryResource struct {
 	Configs     []ResourceReference              `json:"configs,omitempty"`
 }
 
+// ValidDeliveryTemplates THIS MUST MATCH DeliveryClusterTemplateReference Kind Enum
+var ValidDeliveryTemplates = []client.Object{
+	&ClusterSourceTemplate{},
+	&ClusterDeploymentTemplate{},
+	&ClusterTemplate{},
+}
+
 type DeliveryClusterTemplateReference struct {
+	// Kind THIS MUST MATCH ValidDeliveryTemplates
 	// +kubebuilder:validation:Enum=ClusterSourceTemplate;ClusterDeploymentTemplate;ClusterTemplate
 	Kind string `json:"kind"`
 	// +kubebuilder:validation:MinLength=1

pkg/controller/delivery/reconciler.go

@@ -17,10 +17,9 @@ package delivery
 import (
 	"context"
 	"fmt"
-	"strings"
-	"time"
 
 	"github.com/go-logr/logr"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -29,8 +28,6 @@ import (
 	"github.com/vmware-tanzu/cartographer/pkg/repository"
 )
 
-const reconcileInterval = 5 * time.Second
-
 type Reconciler struct {
 	repo             repository.Repository
 	conditionManager conditions.ConditionManager
@@ -67,23 +64,29 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl
 }
 
 func (r *Reconciler) reconcileDelivery(delivery *v1alpha1.ClusterDelivery) error {
-	var missing []string
+	var (
+		err               error
+		resourcesNotFound []string
+	)
+
 	for _, resource := range delivery.Spec.Resources {
-		_, err := r.repo.GetDeliveryClusterTemplate(resource.TemplateRef)
-		// FIXME: should be checking if err IsNotFound
+		_, err = r.repo.GetDeliveryClusterTemplate(resource.TemplateRef)
 		if err != nil {
-			missing = append(missing, resource.Name)
-			r.logger.Error(err, "retrieving cluster template")
+			if !kerrors.IsNotFound(err) {
+				return err
+			}
+
+			resourcesNotFound = append(resourcesNotFound, resource.Name)
 		}
 	}
 
-	if len(missing) == 0 {
-		r.conditionManager.AddPositive(TemplatesFoundCondition())
-		return nil
+	if len(resourcesNotFound) > 0 {
+		r.conditionManager.AddPositive(TemplatesNotFoundCondition(resourcesNotFound))
 	} else {
-		r.conditionManager.AddPositive(TemplatesNotFoundCondition(missing))
-		return fmt.Errorf("encountered errors fetching resources: %s", strings.Join(missing, ", "))
+		r.conditionManager.AddPositive(TemplatesFoundCondition())
 	}
+
+	return nil
 }
 
 func (r *Reconciler) completeReconciliation(delivery *v1alpha1.ClusterDelivery, reconcileError error) (ctrl.Result, error) {
@@ -99,5 +102,5 @@ func (r *Reconciler) completeReconciliation(delivery *v1alpha1.ClusterDelivery,
 		return ctrl.Result{}, reconcileError
 	}
 
-	return ctrl.Result{RequeueAfter: reconcileInterval}, nil
+	return ctrl.Result{}, nil
 }

pkg/controller/delivery/reconciler_test.go

@@ -17,14 +17,15 @@ package delivery_test
 import (
 	"context"
 	"errors"
-	"time"
 
 	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gbytes"
 	. "github.com/onsi/gomega/gstruct"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -143,32 +144,37 @@ var _ = Describe("delivery reconciler", func() {
 				}))
 			})
 
-			It("reschedules for 5 seconds", func() {
-				result, err := reconciler.Reconcile(ctx, req)
+			It("does not return an error", func() {
+				_, err := reconciler.Reconcile(ctx, req)
 				Expect(err).NotTo(HaveOccurred())
-
-				Expect(result).To(Equal(ctrl.Result{RequeueAfter: 5 * time.Second}))
 			})
 		})
 
-		Context("a referenced template is not found", func() {
+		Context("get cluster template fails", func() {
 			BeforeEach(func() {
-				repo.GetDeliveryClusterTemplateReturnsOnCall(0, nil, nil)
-				repo.GetDeliveryClusterTemplateReturnsOnCall(1, nil, errors.New("second-resource not found"))
+				repo.GetDeliveryClusterTemplateReturnsOnCall(0, nil, errors.New("getting templates is hard"))
 			})
 
-			It("returns an error without a requeue value", func() {
+			It("returns an error", func() {
 				_, err := reconciler.Reconcile(ctx, req)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(MatchError(ContainSubstring("getting templates is hard")))
+			})
+
+			It("does not requeue", func() {
+				result, _ := reconciler.Reconcile(ctx, req)
 
-				Expect(err).To(MatchError(ContainSubstring("encountered errors fetching resources: second-resource")))
+				Expect(result).To(Equal(ctrl.Result{Requeue: false}))
 			})
+		})
 
-			It("Sets the status for TemplateNotFound", func() {
-				_, err := reconciler.Reconcile(ctx, req)
-				Expect(err).To(HaveOccurred())
+		Context("cannot find cluster template", func() {
+			BeforeEach(func() {
+				repo.GetDeliveryClusterTemplateReturnsOnCall(0, nil, kerrors.NewNotFound(schema.GroupResource{}, ""))
+			})
+
+			It("adds a positive templates NOT found condition", func() {
+				_, _ = reconciler.Reconcile(ctx, req)
 
-				Expect(repo.StatusUpdateCallCount()).To(Equal(1))
 				deliveryObject, ok := repo.StatusUpdateArgsForCall(0).(*v1alpha1.ClusterDelivery)
 				Expect(ok).To(BeTrue())
 
@@ -191,14 +197,6 @@ var _ = Describe("delivery reconciler", func() {
 					),
 				))
 			})
-
-			It("logs all GetTemplate errors encountered", func() {
-				_, err := reconciler.Reconcile(ctx, req)
-				Expect(err).To(HaveOccurred())
-
-				Expect(out).To(Say(`"msg":"retrieving cluster template"`))
-				Expect(out).To(Say(`"error":"second-resource not found"`))
-			})
 		})
 
 		It("Starts and Finishes cleanly", func() {
@@ -208,7 +206,6 @@ var _ = Describe("delivery reconciler", func() {
 			Eventually(out).Should(Say(`"msg":"started","name":"my-new-delivery"`))
 			Eventually(out).Should(Say(`"msg":"finished","name":"my-new-delivery"`))
 		})
-
 	})
 
 	Context("repo.GetDelivery fails", func() {