Add missing conditions in both the top-level and glanceAPI controller

Most of the code ofter returns ctrl.Result{}, err in case there's
something wrong with a specific component that is supposed to be
ready before move forward with the reconcile loop. However, an
aspect of this evaluation is the missing update of the condition
for that particular component. This commit fixes it and introduces
the missing updates during the reconcile loop.

Signed-off-by: Francesco Pantano <[email protected]>

Author: fmount

controllers/glance_controller.go

@@ -156,16 +156,18 @@ func (r *GlanceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 		instance.Status.Conditions = condition.Conditions{}
 	}
 
-	// initialize conditions used later as Status=Unknown
+	// initialize conditions used later as Status=Unknown, except the ReadyCondition
+	// that should be False when we start
 	cl := condition.CreateList(
+		// Mark ReadyCondition as False from the beginning
+		condition.FalseCondition(condition.ReadyCondition, condition.InitReason, condition.SeverityInfo, condition.ReadyInitMessage),
 		condition.UnknownCondition(condition.DBReadyCondition, condition.InitReason, condition.DBReadyInitMessage),
 		condition.UnknownCondition(condition.DBSyncReadyCondition, condition.InitReason, condition.DBSyncReadyInitMessage),
 		condition.UnknownCondition(condition.MemcachedReadyCondition, condition.InitReason, condition.MemcachedReadyInitMessage),
 		condition.UnknownCondition(condition.InputReadyCondition, condition.InitReason, condition.InputReadyInitMessage),
 		condition.UnknownCondition(condition.ServiceConfigReadyCondition, condition.InitReason, condition.ServiceConfigReadyInitMessage),
 		condition.UnknownCondition(glancev1.GlanceAPIReadyCondition, condition.InitReason, glancev1.GlanceAPIReadyInitMessage),
 		condition.UnknownCondition(condition.KeystoneServiceReadyCondition, condition.InitReason, ""),
-		condition.UnknownCondition(condition.NetworkAttachmentsReadyCondition, condition.InitReason, condition.NetworkAttachmentsReadyInitMessage),
 		// service account, role, rolebinding conditions
 		condition.UnknownCondition(condition.ServiceAccountReadyCondition, condition.InitReason, condition.ServiceAccountReadyInitMessage),
 		condition.UnknownCondition(condition.RoleReadyCondition, condition.InitReason, condition.RoleReadyInitMessage),

controllers/glanceapi_controller.go

@@ -147,7 +147,12 @@ func (r *GlanceAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	//
 	// initialize status
 	//
-	instance.Status.Conditions = condition.Conditions{}
+	// initialize status if Conditions is nil, but do not reset if it
+	// already exists
+	isNewInstance := instance.Status.Conditions == nil
+	if isNewInstance {
+		instance.Status.Conditions = condition.Conditions{}
+	}
 	// initialize conditions used later as Status=Unknown
 	cl := condition.CreateList(
 		condition.UnknownCondition(glancev1.CinderCondition, condition.InitReason, glancev1.CinderInitMessage),
@@ -163,6 +168,10 @@ func (r *GlanceAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	)
 
 	instance.Status.Conditions.Init(&cl)
+	if isNewInstance {
+		// Register overall status immediately to have an early feedback e.g. in the cli
+		return ctrl.Result{}, nil
+	}
 
 	if instance.Status.Hash == nil {
 		instance.Status.Hash = map[string]string{}
@@ -471,6 +480,8 @@ func (r *GlanceAPIReconciler) reconcileInit(
 			serviceLabels,
 		)
 		if err != nil {
+			// The ExposeServiceReadyCondition is already marked as False
+			// within the GetHeadlessService function: we can return
 			return ctrlResult, err
 		}
 
@@ -485,6 +496,12 @@ func (r *GlanceAPIReconciler) reconcileInit(
 		apiEndpoints[string(endpointType)], err = svc.GetAPIEndpoint(
 			svcOverride.EndpointURL, data.Protocol, data.Path)
 		if err != nil {
+			instance.Status.Conditions.MarkFalse(
+				condition.ExposeServiceReadyCondition,
+				condition.ErrorReason,
+				condition.SeverityWarning,
+				condition.ExposeServiceReadyErrorMessage,
+				err.Error())
 			return ctrl.Result{}, err
 		}
 	}
@@ -503,9 +520,14 @@ func (r *GlanceAPIReconciler) reconcileInit(
 	// Create/Patch the KeystoneEndpoints
 	ctrlResult, err := r.ensureKeystoneEndpoints(ctx, helper, instance, serviceLabels)
 	if err != nil {
+		instance.Status.Conditions.MarkFalse(
+			condition.KeystoneEndpointReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			"Creating KeyStoneEndpoint CR %s",
+			err.Error())
 		return ctrlResult, err
 	}
-
 	r.Log.Info(fmt.Sprintf("Reconciled Service '%s' init successfully", instance.Name))
 	return ctrl.Result{}, nil
 }
@@ -566,6 +588,12 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 	availableBackends := glancev1.GetEnabledBackends(instance.Spec.CustomServiceConfig)
 	_, hashChanged, err := r.createHashOfBackendConfig(instance, availableBackends)
 	if err != nil {
+		instance.Status.Conditions.Set(condition.FalseCondition(
+			condition.InputReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			condition.InputReadyErrorMessage,
+			err.Error()))
 		return ctrl.Result{}, err
 	}
 	// Update the current StateFulSet (by recreating it) only when a backend is
@@ -642,9 +670,15 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 				err.Error()))
 			return ctrlResult, err
 		} else if (ctrlResult != ctrl.Result{}) {
+			// Marking the condition as Unknown because we are not returining
+			// an err, but comparing the ctrlResult: this represents an in
+			// progress operation rather than something that failed
+			instance.Status.Conditions.MarkUnknown(
+				condition.TLSInputReadyCondition,
+				condition.InitReason,
+				condition.InputReadyInitMessage)
 			return ctrlResult, nil
 		}
-
 		if hash != "" {
 			configVars[tls.CABundleKey] = env.SetValue(hash)
 		}
@@ -661,6 +695,13 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 			err.Error()))
 		return ctrlResult, err
 	} else if (ctrlResult != ctrl.Result{}) {
+		// Marking the condition as Unknown because we are not returining
+		// an err, but comparing the ctrlResult: this represents an in
+		// progress operation rather than something that failed
+		instance.Status.Conditions.MarkUnknown(
+			condition.TLSInputReadyCondition,
+			condition.InitReason,
+			condition.InputReadyInitMessage)
 		return ctrlResult, nil
 	}
 	configVars[tls.TLSHashName] = env.SetValue(certsHash)
@@ -680,10 +721,6 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 			condition.ServiceConfigReadyErrorMessage,
 			err.Error()))
 		return ctrl.Result{}, err
-	} else if hashChanged {
-		// Hash changed and instance status should be updated (which will be done by main defer func),
-		// so we need to return and reconcile again
-		return ctrl.Result{}, nil
 	}
 	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
 	// Create Secrets - end
@@ -695,6 +732,13 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 	// networks to attach to
 	serviceAnnotations, ctrlResult, err = ensureNAD(ctx, &instance.Status.Conditions, instance.Spec.NetworkAttachments, helper)
 	if err != nil {
+		instance.Status.Conditions.MarkFalse(
+			condition.NetworkAttachmentsReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			condition.NetworkAttachmentsReadyErrorMessage,
+			err,
+		)
 		return ctrlResult, err
 	}
 
@@ -764,6 +808,13 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 	// verify if network attachment matches expectations
 	networkReady, networkAttachmentStatus, err := nad.VerifyNetworkStatusFromAnnotation(ctx, helper, instance.Spec.NetworkAttachments, GetServiceLabels(instance), instance.Status.ReadyCount)
 	if err != nil {
+		err = fmt.Errorf("verifying API NetworkAttachments (%s) %w", instance.Spec.NetworkAttachments, err)
+		instance.Status.Conditions.MarkFalse(
+			condition.NetworkAttachmentsReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			condition.NetworkAttachmentsReadyErrorMessage,
+			err)
 		return ctrl.Result{}, err
 	}
 
@@ -830,8 +881,11 @@ func (r *GlanceAPIReconciler) reconcileNormal(ctx context.Context, instance *gla
 		}
 	}
 
-	// Mark the CronJobReadyCondition as True because there's nothing to do here
-	instance.Status.Conditions.MarkTrue(condition.CronJobReadyCondition, condition.CronJobReadyMessage)
+	// If we reach this point, we can mark the CronJobReadyCondition as True
+	instance.Status.Conditions.MarkTrue(
+		condition.CronJobReadyCondition,
+		condition.CronJobReadyMessage,
+	)
 
 	// create ImageCache cronJobs - end
 	r.Log.Info(fmt.Sprintf("Reconciled Service '%s' successfully", instance.Name))
@@ -1044,13 +1098,11 @@ func (r *GlanceAPIReconciler) ensureKeystoneEndpoints(
 		}
 		return ctrlResult, nil
 	}
-
 	// Build the keystoneEndpoints Spec
 	ksEndpointSpec := keystonev1.KeystoneEndpointSpec{
 		ServiceName: glance.ServiceName,
 		Endpoints:   instance.Status.APIEndpoints,
 	}
-
 	ksSvc := keystonev1.NewKeystoneEndpoint(
 		instance.Name,
 		instance.Namespace,
@@ -1062,14 +1114,12 @@ func (r *GlanceAPIReconciler) ensureKeystoneEndpoints(
 	if err != nil {
 		return ctrlResult, err
 	}
-
 	// mirror the Status, Reason, Severity and Message of the latest keystoneendpoint condition
 	// into a local condition with the type condition.KeystoneEndpointReadyCondition
 	c := ksSvc.GetConditions().Mirror(condition.KeystoneEndpointReadyCondition)
 	if c != nil {
 		instance.Status.Conditions.Set(c)
 	}
-
 	return ctrlResult, nil
 }
 

docs/dev/design-decisions.md

@@ -416,3 +416,30 @@ instance that should be registered in Keystone.
 An update to the `keystoneEndpoint` parameter results in a reconciliation execution
 that switches the two affected instances (the one registered in keystone and the
 one proposed by the new update).
+
+## How Conditions are managed
+
+Conditions represent a critical aspect for reconciliation because they allow:
+
+1. to evaluate the status of a particular component validating a set of conditions
+2. give a feedback to the end user about the current state of the Deployment
+3. identify the status of the underlying GlanceAPIs and report their healhy to
+   the upper level CR
+
+The document [docs/conditions]() describes the meaning of the k8s-operators
+shared conditions.
+Conditions are re-evaluated when a new Reconciliation loop starts, and if one of
+them is different from what has been previously registered, an update is performed,
+and it allows to give immediate feedback to the user.
+Conditions are initially marked as `Unknown`, while the general `ReadyCondition`
+is marked as `False` because we assume that the Deployment is in progress but
+not `Ready` at the same time, until the entire set of conditions are evaluated.
+Conditions can be seen as a checklist or steps within the reconciliation loop,
+and while the loop proceed to the end, each of them is evaluated.
+If a particular component with an associated condition is not Ready, an `error`
+is returned from the operator, and its failing condition is marked as `False`
+with an appropriate error message.
+If the end of the loop is reached, it means we passed through all the steps and
+the Conditions are marked to `True`: it is possible, at this point, to mark the
+overall `ReadyCondition` to `Status=True` as well and `Mirror` the result to the
+top-level CR.

test/functional/glance_controller_test.go

@@ -53,7 +53,7 @@ var _ = Describe("Glance controller", func() {
 		It("initializes the status fields", func() {
 			Eventually(func(g Gomega) {
 				glance := GetGlance(glanceName)
-				g.Expect(glance.Status.Conditions).To(HaveLen(13))
+				g.Expect(glance.Status.Conditions).To(HaveLen(12))
 				g.Expect(glance.Status.DatabaseHostname).To(Equal(""))
 				g.Expect(glance.Status.APIEndpoints).To(BeEmpty())
 			}, timeout, interval).Should(Succeed())

test/functional/sample_test.go

@@ -61,7 +61,7 @@ var _ = Describe("Samples", func() {
 			Eventually(func(g Gomega) {
 				name := CreateGlanceFromSample("glance_v1beta1_glance.yaml", glanceTest.Instance)
 				glance := GetGlance(name)
-				g.Expect(glance.Status.Conditions).To(HaveLen(13))
+				g.Expect(glance.Status.Conditions).To(HaveLen(12))
 				g.Expect(glance.Status.DatabaseHostname).To(Equal(""))
 				g.Expect(glance.Status.APIEndpoints).To(BeEmpty())
 			}, timeout, interval).Should(Succeed())