Merge pull request #123 from pravisankar/add-controller-retry

Requeue requests during controller reconciliation if needed

Author: openshift-merge-robot

pkg/operator/controller/controller.go

@@ -21,6 +21,7 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -90,6 +91,9 @@ type Config struct {
 // events.
 type reconciler struct {
 	Config
+
+	dnsConfig   *configv1.DNS
+	infraConfig *configv1.Infrastructure
 }
 
 // Reconcile expects request to refer to a clusteringress in the operator
@@ -134,38 +138,48 @@ func (r *reconciler) reconcile(request reconcile.Request) (reconcile.Result, err
 
 	// Collect errors as we go.
 	errs := []error{}
+	result := reconcile.Result{}
 
 	if ingress != nil {
 		// Only reconcile the ingress itself if it exists.
 		dnsConfig := &configv1.DNS{}
-		err = r.Client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, dnsConfig)
-		if err != nil {
-			return reconcile.Result{}, fmt.Errorf("failed to get dns 'cluster': %v", err)
+		if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, dnsConfig); err != nil {
+			errs = append(errs, fmt.Errorf("failed to get dns 'cluster': %v", err))
+			r.dnsConfig = nil
+			if errors.IsNotFound(err) {
+				result.RequeueAfter = 10 * time.Second
+			}
+		} else {
+			r.dnsConfig = dnsConfig
 		}
 
 		infraConfig := &configv1.Infrastructure{}
-		err = r.Client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig)
-		if err != nil {
-			return reconcile.Result{}, fmt.Errorf("failed to get infrastructure 'cluster': %v", err)
+		if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig); err != nil {
+			errs = append(errs, fmt.Errorf("failed to get infrastructure 'cluster': %v", err))
+			r.infraConfig = nil
+			if errors.IsNotFound(err) {
+				result.RequeueAfter = 10 * time.Second
+			}
+		} else {
+			r.infraConfig = infraConfig
 		}
 
 		// Ensure we have all the necessary scaffolding on which to place router
 		// instances.
 		err = r.ensureRouterNamespace()
 		if err != nil {
-			return reconcile.Result{}, err
+			errs = append(errs, err)
+			return result, utilerrors.NewAggregate(errs)
 		}
 
 		if ingress.DeletionTimestamp != nil {
 			// Handle deletion.
-			err := r.ensureIngressDeleted(ingress, dnsConfig)
-			if err != nil {
+			if err := r.ensureIngressDeleted(ingress); err != nil {
 				errs = append(errs, fmt.Errorf("failed to ensure ingress deletion: %v", err))
 			}
 		} else {
 			// Handle everything else.
-			err := r.ensureRouterForIngress(ingress, dnsConfig, infraConfig)
-			if err != nil {
+			if err := r.ensureRouterForIngress(ingress, &result); err != nil {
 				errs = append(errs, fmt.Errorf("failed to ensure clusteringress: %v", err))
 			}
 		}
@@ -192,23 +206,25 @@ func (r *reconciler) reconcile(request reconcile.Request) (reconcile.Result, err
 		}
 	}
 
-	return reconcile.Result{}, utilerrors.NewAggregate(errs)
+	return result, utilerrors.NewAggregate(errs)
 }
 
 // ensureIngressDeleted tries to delete ingress, and if successful, will remove
 // the finalizer.
-func (r *reconciler) ensureIngressDeleted(ingress *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.DNS) error {
+func (r *reconciler) ensureIngressDeleted(ingress *ingressv1alpha1.ClusterIngress) error {
 	// TODO: This should also be tied to the HA type.
-	err := r.finalizeLoadBalancerService(ingress, dnsConfig)
+	err := r.finalizeLoadBalancerService(ingress)
 	if err != nil {
 		return fmt.Errorf("failed to finalize load balancer service for %s: %v", ingress.Name, err)
 	}
 	logrus.Infof("finalized load balancer service for ingress %s", ingress.Name)
+
 	err = r.ensureRouterDeleted(ingress)
 	if err != nil {
 		return fmt.Errorf("failed to delete deployment for ingress %s: %v", ingress.Name, err)
 	}
 	logrus.Infof("deleted deployment for ingress %s", ingress.Name)
+
 	// Clean up the finalizer to allow the clusteringress to be deleted.
 	updated := ingress.DeepCopy()
 	if slice.ContainsString(ingress.Finalizers, ClusterIngressFinalizer) {
@@ -297,7 +313,7 @@ func (r *reconciler) ensureRouterNamespace() error {
 
 // ensureRouterForIngress ensures all necessary router resources exist for a
 // given clusteringress.
-func (r *reconciler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.DNS, infraConfig *configv1.Infrastructure) error {
+func (r *reconciler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress, result *reconcile.Result) error {
 	expected, err := r.ManifestFactory.RouterDeployment(ci)
 	if err != nil {
 		return fmt.Errorf("failed to build router deployment: %v", err)
@@ -336,42 +352,49 @@ func (r *reconciler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress,
 		Controller: &trueVar,
 	}
 
-	lbService, err := r.ensureLoadBalancerService(ci, infraConfig, current)
+	errs := []error{}
+	lbService, err := r.ensureLoadBalancerService(ci, current)
 	if err != nil {
-		return fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err)
+		errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err))
 	}
-
-	err = r.ensureDNS(ci, dnsConfig, lbService)
-	if err != nil {
-		return fmt.Errorf("failed to ensure DNS for %s: %v", ci.Name, err)
+	if err = r.ensureDNS(ci, lbService); err != nil {
+		errs = append(errs, fmt.Errorf("failed to ensure DNS for %s: %v", ci.Name, err))
 	}
 
 	internalSvc, err := r.ensureInternalRouterServiceForIngress(ci, deploymentRef)
 	if err != nil {
-		return fmt.Errorf("failed to create internal router service for clusteringress %s: %v", ci.Name, err)
+		errs = append(errs, fmt.Errorf("failed to create internal router service for clusteringress %s: %v", ci.Name, err))
+		return utilerrors.NewAggregate(errs)
 	}
 
 	if ci.Spec.DefaultCertificateSecret == nil {
 		if err := r.ensureDefaultCertificateForIngress(current, ci); err != nil {
-			return fmt.Errorf("failed to create default certificate for clusteringress %s: %v", ci.Name, err)
+			errs = append(errs, fmt.Errorf("failed to create default certificate for clusteringress %s: %v", ci.Name, err))
+			return utilerrors.NewAggregate(errs)
 		}
 	} else {
 		if err := r.ensureDefaultCertificateDeleted(current, ci); err != nil {
-			return fmt.Errorf("failed to delete operator-generated default certificate for clusteringress %s: %v", ci.Name, err)
+			errs = append(errs, fmt.Errorf("failed to delete operator-generated default certificate for clusteringress %s: %v", ci.Name, err))
+			return utilerrors.NewAggregate(errs)
 		}
 	}
 
 	if err := r.ensureMetricsIntegration(ci, internalSvc, deploymentRef); err != nil {
-		return fmt.Errorf("failed to integrate metrics with openshift-monitoring for clusteringress %s: %v", ci.Name, err)
+		errs = append(errs, fmt.Errorf("failed to integrate metrics with openshift-monitoring for clusteringress %s: %v", ci.Name, err))
+		return utilerrors.NewAggregate(errs)
 	}
 
 	_, err = r.ensureServiceMonitor(ci, internalSvc, current)
 	if err != nil {
-		return fmt.Errorf("failed to ensure servicemonitor for %s: %v", ci.Name, err)
+		errs = append(errs, fmt.Errorf("failed to ensure servicemonitor for %s: %v", ci.Name, err))
+		if meta.IsNoMatchError(err) {
+			result.RequeueAfter = 10 * time.Second
+		}
 	}
 
 	if err := r.syncClusterIngressStatus(current, ci); err != nil {
-		return fmt.Errorf("failed to update status of clusteringress %s/%s: %v", current.Namespace, current.Name, err)
+		errs = append(errs, fmt.Errorf("failed to update status of clusteringress %s/%s: %v", current.Namespace, current.Name, err))
+		return utilerrors.NewAggregate(errs)
 	}
 
 	return nil

pkg/operator/controller/controller_dns.go

@@ -15,12 +15,12 @@ import (
 
 // ensureDNS will create DNS records for the given LB service. If service is
 // nil, nothing is done.
-func (r *reconciler) ensureDNS(ci *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.DNS, service *corev1.Service) error {
-	desiredDNSRecords, err := desiredDNSRecords(ci, dnsConfig, service)
+func (r *reconciler) ensureDNS(ci *ingressv1alpha1.ClusterIngress, service *corev1.Service) error {
+	dnsRecords, err := r.desiredDNSRecords(ci, service)
 	if err != nil {
 		return err
 	}
-	for _, record := range desiredDNSRecords {
+	for _, record := range dnsRecords {
 		err := r.DNSManager.Ensure(record)
 		if err != nil {
 			return fmt.Errorf("failed to ensure DNS record %v for %s: %v", record, ci.Name, err)
@@ -34,7 +34,7 @@ func (r *reconciler) ensureDNS(ci *ingressv1alpha1.ClusterIngress, dnsConfig *co
 // If service is nil, no records are desired. If an ingress domain is in use,
 // records are desired in every specified zone present in the cluster DNS
 // configuration.
-func desiredDNSRecords(ci *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.DNS, service *corev1.Service) ([]*dns.Record, error) {
+func (r *reconciler) desiredDNSRecords(ci *ingressv1alpha1.ClusterIngress, service *corev1.Service) ([]*dns.Record, error) {
 	records := []*dns.Record{}
 
 	// If no service exists, no DNS records should exist.
@@ -49,8 +49,11 @@ func desiredDNSRecords(ci *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.D
 		return records, nil
 	}
 
+	if r.dnsConfig == nil {
+		return records, fmt.Errorf("dns config not found")
+	}
 	// If no zones are configured, there's nothing to do.
-	if dnsConfig.Spec.PrivateZone == nil && dnsConfig.Spec.PublicZone == nil {
+	if r.dnsConfig.Spec.PrivateZone == nil && r.dnsConfig.Spec.PublicZone == nil {
 		return records, nil
 	}
 
@@ -72,11 +75,11 @@ func desiredDNSRecords(ci *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.D
 			},
 		}
 	}
-	if dnsConfig.Spec.PrivateZone != nil {
-		records = append(records, makeRecord(dnsConfig.Spec.PrivateZone))
+	if r.dnsConfig.Spec.PrivateZone != nil {
+		records = append(records, makeRecord(r.dnsConfig.Spec.PrivateZone))
 	}
-	if dnsConfig.Spec.PublicZone != nil {
-		records = append(records, makeRecord(dnsConfig.Spec.PublicZone))
+	if r.dnsConfig.Spec.PublicZone != nil {
+		records = append(records, makeRecord(r.dnsConfig.Spec.PublicZone))
 	}
 	return records, nil
 }

pkg/operator/controller/controller_lb.go

@@ -34,8 +34,8 @@ const (
 // ensureLoadBalancerService creates an LB service if one is desired but absent.
 // Always returns the current LB service if one exists (whether it already
 // existed or was created during the course of the function).
-func (r *reconciler) ensureLoadBalancerService(ci *ingressv1alpha1.ClusterIngress, infra *configv1.Infrastructure, deployment *appsv1.Deployment) (*corev1.Service, error) {
-	desiredLBService, err := desiredLoadBalancerService(ci, infra, deployment)
+func (r *reconciler) ensureLoadBalancerService(ci *ingressv1alpha1.ClusterIngress, deployment *appsv1.Deployment) (*corev1.Service, error) {
+	desiredLBService, err := r.desiredLoadBalancerService(ci, deployment)
 	if err != nil {
 		return nil, err
 	}
@@ -66,7 +66,7 @@ func loadBalancerServiceName(ci *ingressv1alpha1.ClusterIngress) types.Namespace
 // clusteringress, or nil if an LB service isn't desired. An LB service is
 // desired if the high availability type is Cloud. An LB service will declare an
 // owner reference to the given deployment.
-func desiredLoadBalancerService(ci *ingressv1alpha1.ClusterIngress, infra *configv1.Infrastructure, deployment *appsv1.Deployment) (*corev1.Service, error) {
+func (r *reconciler) desiredLoadBalancerService(ci *ingressv1alpha1.ClusterIngress, deployment *appsv1.Deployment) (*corev1.Service, error) {
 	if ci.Spec.HighAvailability == nil || ci.Spec.HighAvailability.Type != ingressv1alpha1.CloudClusterIngressHA {
 		return nil, nil
 	}
@@ -96,7 +96,10 @@ func desiredLoadBalancerService(ci *ingressv1alpha1.ClusterIngress, infra *confi
 	}
 	service.Spec.Selector["router"] = name.Name
 
-	if infra.Status.Platform == configv1.AWSPlatform {
+	if r.infraConfig == nil {
+		return nil, fmt.Errorf("infra config not found")
+	}
+	if r.infraConfig.Status.Platform == configv1.AWSPlatform {
 		if service.Annotations == nil {
 			service.Annotations = map[string]string{}
 		}
@@ -124,15 +127,15 @@ func (r *reconciler) currentLoadBalancerService(ci *ingressv1alpha1.ClusterIngre
 // finalizeLoadBalancerService deletes any DNS entries associated with any
 // current LB service associated with the clusteringress and then finalizes the
 // service.
-func (r *reconciler) finalizeLoadBalancerService(ci *ingressv1alpha1.ClusterIngress, dnsConfig *configv1.DNS) error {
+func (r *reconciler) finalizeLoadBalancerService(ci *ingressv1alpha1.ClusterIngress) error {
 	service, err := r.currentLoadBalancerService(ci)
 	if err != nil {
 		return err
 	}
 	if service == nil {
 		return nil
 	}
-	records, err := desiredDNSRecords(ci, dnsConfig, service)
+	records, err := r.desiredDNSRecords(ci, service)
 	if err != nil {
 		return err
 	}