Merge pull request metal3-io#70 from dtantsur/status

✨ Rework reporting status to the controller

api/v1alpha1/ironic_webhook.go

@@ -86,13 +86,13 @@ var _ webhook.Validator = &Ironic{}
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *Ironic) ValidateCreate() (warnings admission.Warnings, err error) {
 	ironiclog.Info("validate create", "name", r.Name)
-	return nil, validateIronic(&r.Spec, nil)
+	return nil, ValidateIronic(&r.Spec, nil)
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *Ironic) ValidateUpdate(old runtime.Object) (warnings admission.Warnings, err error) {
 	ironiclog.Info("validate update", "name", r.Name)
-	return nil, validateIronic(&r.Spec, &old.(*Ironic).Spec)
+	return nil, ValidateIronic(&r.Spec, &old.(*Ironic).Spec)
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
@@ -178,7 +178,7 @@ func ValidateDHCP(ironic *IronicSpec, dhcp *DHCP) error {
 	return nil
 }
 
-func validateIronic(ironic *IronicSpec, old *IronicSpec) error {
+func ValidateIronic(ironic *IronicSpec, old *IronicSpec) error {
 	if ironic.HighAvailability && ironic.DatabaseRef.Name == "" {
 		return errors.New("database is required for highly available architecture")
 	}

api/v1alpha1/ironic_webhook_test.go

@@ -135,7 +135,7 @@ func TestValidateIronic(t *testing.T) {
 				tc.OldIronic = &tc.Ironic
 			}
 
-			err := validateIronic(&tc.Ironic, tc.OldIronic)
+			err := ValidateIronic(&tc.Ironic, tc.OldIronic)
 			if tc.ExpectedError == "" {
 				assert.NoError(t, err)
 			} else {

controllers/ironic_controller.go

@@ -113,22 +113,19 @@ func (r *IronicReconciler) handleIronic(cctx ironic.ControllerContext, ironicCon
 		return
 	}
 
-	ready, err := ironic.EnsureIronic(cctx, ironicConf, db, apiSecret)
+	status, err := ironic.EnsureIronic(cctx, ironicConf, db, apiSecret)
 	newStatus := ironicConf.Status.DeepCopy()
 	newStatus.InstalledVersion = nil
 	if err != nil {
-		setCondition(cctx, &newStatus.Conditions, ironicConf.Generation,
-			metal3api.IronicStatusReady, false, metal3api.IronicReasonFailed, err.Error())
-	} else if !ready {
-		cctx.Logger.Info("ironic deployment is still progressing")
-		setCondition(cctx, &newStatus.Conditions, ironicConf.Generation,
-			metal3api.IronicStatusReady, false, metal3api.IronicReasonInProgress, "deployment is not ready yet")
-	} else {
+		cctx.Logger.Error(err, "potentially transient error, will retry")
+		return
+	}
+
+	requeue = setConditionsFromStatus(cctx, status, &newStatus.Conditions, ironicConf.Generation, "ironic")
+	if !requeue {
 		newStatus.InstalledVersion = &metal3api.InstalledVersion{
 			Branch: cctx.VersionInfo.InstalledVersion,
 		}
-		setCondition(cctx, &newStatus.Conditions, ironicConf.Generation,
-			metal3api.IronicStatusReady, true, metal3api.IronicReasonAvailable, "ironic is available")
 	}
 
 	if !apiequality.Semantic.DeepEqual(newStatus, &ironicConf.Status) {

controllers/ironicdatabase_controller.go

@@ -112,21 +112,15 @@ func (r *IronicDatabaseReconciler) handleDatabase(cctx ironic.ControllerContext,
 		return true, nil
 	}
 
-	ready, err := ironic.EnsureDatabase(cctx, db)
+	status, err := ironic.EnsureDatabase(cctx, db)
 	newStatus := db.Status.DeepCopy()
 	if err != nil {
-		cctx.Logger.Error(err, "failed to create or update database")
-		setCondition(cctx, &newStatus.Conditions, db.Generation,
-			metal3api.IronicStatusReady, false, metal3api.IronicReasonFailed, err.Error())
-	} else if !ready {
-		requeue = true
-		setCondition(cctx, &newStatus.Conditions, db.Generation,
-			metal3api.IronicStatusReady, false, metal3api.IronicReasonInProgress, "deployment is not ready yet")
-	} else {
-		setCondition(cctx, &newStatus.Conditions, db.Generation,
-			metal3api.IronicStatusReady, true, metal3api.IronicReasonAvailable, "database is available")
+		cctx.Logger.Error(err, "potentially transient error, will retry")
+		return
 	}
 
+	requeue = setConditionsFromStatus(cctx, status, &newStatus.Conditions, db.Generation, "database")
+
 	if !apiequality.Semantic.DeepEqual(newStatus, &db.Status) {
 		cctx.Logger.Info("updating status", "Status", newStatus)
 		requeue = true

controllers/utils.go

@@ -104,3 +104,24 @@ func generateSecret(cctx ironic.ControllerContext, owner metav1.Object, meta *me
 
 	return
 }
+
+func setConditionsFromStatus(cctx ironic.ControllerContext, status ironic.Status, conditions *[]metav1.Condition, generation int64, resource string) (requeue bool) {
+	message := fmt.Sprintf("%s: %s", resource, status)
+
+	if !status.IsReady() {
+		reason := metal3api.IronicReasonInProgress
+		if status.IsError() {
+			reason = metal3api.IronicReasonFailed
+		}
+
+		cctx.Logger.Info(status.String())
+		setCondition(cctx, conditions, generation, metal3api.IronicStatusReady, false, reason, message)
+
+		requeue = status.Fatal == nil
+		return
+	}
+
+	setCondition(cctx, conditions, generation,
+		metal3api.IronicStatusReady, true, metal3api.IronicReasonAvailable, message)
+	return
+}

pkg/ironic/database.go

@@ -130,7 +130,7 @@ func newDatabasePodTemplate(db *metal3api.IronicDatabase) corev1.PodTemplateSpec
 	}
 }
 
-func ensureDatabaseDeployment(cctx ControllerContext, db *metal3api.IronicDatabase) (bool, error) {
+func ensureDatabaseDeployment(cctx ControllerContext, db *metal3api.IronicDatabase) (Status, error) {
 	deploy := &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{Name: databaseDeploymentName(db), Namespace: db.Namespace},
 	}
@@ -145,15 +145,16 @@ func ensureDatabaseDeployment(cctx ControllerContext, db *metal3api.IronicDataba
 		return controllerutil.SetControllerReference(db, deploy, cctx.Scheme)
 	})
 	if err != nil {
-		return false, err
+		return transientError(err)
 	}
 	if result != controllerutil.OperationResultNone {
 		cctx.Logger.Info("database deployment", "Deployment", deploy.Name, "Status", result)
+		return updated()
 	}
 	return getDeploymentStatus(cctx, deploy)
 }
 
-func ensureDatabaseService(cctx ControllerContext, db *metal3api.IronicDatabase) (bool, error) {
+func ensureDatabaseService(cctx ControllerContext, db *metal3api.IronicDatabase) (Status, error) {
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{Name: databaseDeploymentName(db), Namespace: db.Namespace},
 	}
@@ -178,17 +179,18 @@ func ensureDatabaseService(cctx ControllerContext, db *metal3api.IronicDatabase)
 	})
 	if result != controllerutil.OperationResultNone {
 		cctx.Logger.Info("database service", "Service", service.Name, "Status", result)
+		return updated()
 	}
-	if err != nil || len(service.Spec.ClusterIPs) == 0 {
-		return false, err
+	if err != nil {
+		return transientError(err)
 	}
-	return true, nil
+	return getServiceStatus(service)
 }
 
 // EnsureDatabase ensures MariaDB is running with the current configuration.
-func EnsureDatabase(cctx ControllerContext, db *metal3api.IronicDatabase) (ready bool, err error) {
-	ready, err = ensureDatabaseDeployment(cctx, db)
-	if err != nil || !ready {
+func EnsureDatabase(cctx ControllerContext, db *metal3api.IronicDatabase) (status Status, err error) {
+	status, err = ensureDatabaseDeployment(cctx, db)
+	if err != nil || !status.IsReady() {
 		return
 	}
 

pkg/ironic/ironic.go

@@ -2,7 +2,6 @@ package ironic
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -20,10 +19,10 @@ func ironicDeploymentName(ironic *metal3api.Ironic) string {
 	return fmt.Sprintf("%s-service", ironic.Name)
 }
 
-func ensureIronicDaemonSet(cctx ControllerContext, ironic *metal3api.Ironic, db *metal3api.IronicDatabase, apiSecret *corev1.Secret) (ready bool, err error) {
+func ensureIronicDaemonSet(cctx ControllerContext, ironic *metal3api.Ironic, db *metal3api.IronicDatabase, apiSecret *corev1.Secret) (Status, error) {
 	template, err := newIronicPodTemplate(cctx, ironic, db, apiSecret, cctx.Domain)
 	if err != nil {
-		return
+		return transientError(err)
 	}
 
 	deploy := &appsv1.DaemonSet{
@@ -42,18 +41,20 @@ func ensureIronicDaemonSet(cctx ControllerContext, ironic *metal3api.Ironic, db
 		return controllerutil.SetControllerReference(ironic, deploy, cctx.Scheme)
 	})
 	if err != nil {
-		return
+		return transientError(err)
 	}
 	if result != controllerutil.OperationResultNone {
 		cctx.Logger.Info("ironic daemon set", "DaemonSet", deploy.Name, "Status", result)
+		return updated()
 	}
+
 	return getDaemonSetStatus(cctx, deploy)
 }
 
-func ensureIronicDeployment(cctx ControllerContext, ironic *metal3api.Ironic, db *metal3api.IronicDatabase, apiSecret *corev1.Secret) (ready bool, err error) {
+func ensureIronicDeployment(cctx ControllerContext, ironic *metal3api.Ironic, db *metal3api.IronicDatabase, apiSecret *corev1.Secret) (Status, error) {
 	template, err := newIronicPodTemplate(cctx, ironic, db, apiSecret, cctx.Domain)
 	if err != nil {
-		return
+		return transientError(err)
 	}
 
 	deploy := &appsv1.Deployment{
@@ -77,15 +78,17 @@ func ensureIronicDeployment(cctx ControllerContext, ironic *metal3api.Ironic, db
 		return controllerutil.SetControllerReference(ironic, deploy, cctx.Scheme)
 	})
 	if err != nil {
-		return
+		return transientError(err)
 	}
 	if result != controllerutil.OperationResultNone {
 		cctx.Logger.Info("ironic deployment", "Deployment", deploy.Name, "Status", result)
+		return updated()
 	}
+
 	return getDeploymentStatus(cctx, deploy)
 }
 
-func ensureIronicService(cctx ControllerContext, ironic *metal3api.Ironic) (bool, error) {
+func ensureIronicService(cctx ControllerContext, ironic *metal3api.Ironic) (Status, error) {
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{Name: ironic.Name, Namespace: ironic.Namespace},
 	}
@@ -114,12 +117,13 @@ func ensureIronicService(cctx ControllerContext, ironic *metal3api.Ironic) (bool
 	})
 	if result != controllerutil.OperationResultNone {
 		cctx.Logger.Info("ironic service", "Service", service.Name, "Status", result)
+		return updated()
 	}
-	if err != nil || len(service.Spec.ClusterIPs) == 0 {
-		return false, err
+	if err != nil {
+		return transientError(err)
 	}
 
-	return true, nil
+	return getServiceStatus(service)
 }
 
 func removeIronicDaemonSet(cctx ControllerContext, ironic *metal3api.Ironic) error {
@@ -135,38 +139,39 @@ func removeIronicDeployment(cctx ControllerContext, ironic *metal3api.Ironic) er
 }
 
 // EnsureIronic deploys Ironic either as a Deployment or as a DaemonSet.
-func EnsureIronic(cctx ControllerContext, ironic *metal3api.Ironic, db *metal3api.IronicDatabase, apiSecret *corev1.Secret) (ready bool, err error) {
+func EnsureIronic(cctx ControllerContext, ironic *metal3api.Ironic, db *metal3api.IronicDatabase, apiSecret *corev1.Secret) (status Status, err error) {
 	if db != nil && !isReady(db.Status.Conditions) {
 		cctx.Logger.Info("database is not ready yet")
 		return
 	}
 
-	if ironic.Spec.HighAvailability {
-		if db == nil {
-			return false, errors.New("database is required for highly available architecture")
-		}
+	if validationErr := metal3api.ValidateIronic(&ironic.Spec, nil); validationErr != nil {
+		status = Status{Fatal: validationErr}
+		return
+	}
 
+	if ironic.Spec.HighAvailability {
 		err = removeIronicDeployment(cctx, ironic)
 		if err != nil {
 			return
 		}
-		ready, err = ensureIronicDaemonSet(cctx, ironic, db, apiSecret)
+		status, err = ensureIronicDaemonSet(cctx, ironic, db, apiSecret)
 	} else {
 		err = removeIronicDaemonSet(cctx, ironic)
 		if err != nil {
 			return
 		}
-		ready, err = ensureIronicDeployment(cctx, ironic, db, apiSecret)
+		status, err = ensureIronicDeployment(cctx, ironic, db, apiSecret)
 	}
 
-	if err != nil {
+	if err != nil || status.IsError() {
 		return
 	}
 
 	// Let the service be created while Ironic is being deployed, but do
 	// not report overall success until both are done.
 	serviceStatus, err := ensureIronicService(cctx, ironic)
-	if err != nil || !serviceStatus {
+	if err != nil || !serviceStatus.IsReady() {
 		return serviceStatus, err
 	}
 

pkg/ironic/status.go

@@ -0,0 +1,55 @@
+package ironic
+
+type Status struct {
+	// Object is reconciled but some resources may be in progress.
+	Reconciled bool
+	// Object is reconciled and all resources are ready.
+	Ready bool
+	// Fatal error, further reconciliation is not possible.
+	Fatal error
+}
+
+func (status Status) IsError() bool {
+	return status.Fatal != nil
+}
+
+func (status Status) IsReady() bool {
+	return status.Ready && !status.IsError()
+}
+
+func (status Status) String() string {
+	if status.Fatal != nil {
+		return status.Fatal.Error()
+	}
+
+	if !status.Reconciled {
+		return "resources are being updated"
+	}
+
+	if !status.Ready {
+		return "resources are not ready yet"
+	}
+
+	return "resources are available"
+}
+
+// Everything is done, no more reconciliation required.
+func ready() (Status, error) {
+	return Status{Reconciled: true, Ready: true}, nil
+}
+
+// We have updated dependent resources.
+func updated() (Status, error) {
+	return Status{}, nil
+}
+
+// We are passively waiting for something external to happen.
+func inProgress() (Status, error) {
+	return Status{Reconciled: true}, nil
+}
+
+// Checking or updating status failed, we hope it's going to resolve itself
+// (e.g. a glitch in access to Kube API).
+func transientError(err error) (Status, error) {
+	return Status{}, err
+}