Merge branch 'feature-rearchitecture' into feat/chore1

Author: zdtsw

apis/dscinitialization/v1alpha1/dscinitialization_types.go

@@ -22,14 +22,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// List of constants to show different  reconciliation messages and statuses.
-const (
-	ReconcileFailed           = "ReconcileFailed"
-	ReconcileInit             = "ReconcileInit"
-	ReconcileCompleted        = "ReconcileCompleted"
-	ReconcileCompletedMessage = "Reconcile completed successfully"
-)
-
 // DSCInitializationSpec defines the desired state of DSCInitialization
 type DSCInitializationSpec struct {
 	// +kubebuilder:default:=opendatahub

controllers/datasciencecluster/datasciencecluster_controller.go

@@ -115,6 +115,13 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 		}
 	}
 
+	// Ensure all ommited components show up as explicitly disabled
+	instance, err = r.updateComponents(instance)
+	if err != nil {
+		_ = r.reportError(err, instance, "error updating list of components in the CR")
+		return ctrl.Result{}, err
+	}
+
 	// reconcile dashboard component
 	var val ctrl.Result
 	if instance, val, err = r.reconcileSubComponent(instance, dashboard.ComponentName, instance.Spec.Components.Dashboard.Enabled,
@@ -156,15 +163,9 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return val, err
 	}
 
-	// Update final state of spec
-	err = r.Client.Update(ctx, instance)
-	if err != nil {
-		r.Log.Info(fmt.Sprintf("failed to set DataScienceCluster CR :%v", instance.Name), "error", err)
-		// no need to return error as this is not critical and will be reconciled in the next update or reconcile loop
-	}
 	// finalize reconciliation
 	instance, err = r.updateStatus(instance, func(saved *dsc.DataScienceCluster) {
-		status.SetCompleteCondition(&saved.Status.Conditions, status.ReconcileCompleted, "DataScienceCluster resource reconciled successfully.")
+		status.SetCompleteCondition(&saved.Status.Conditions, status.ReconcileCompleted, "DataScienceCluster resource reconciled successfully")
 		saved.Status.Phase = status.PhaseReady
 	})
 	if err != nil {
@@ -180,22 +181,55 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
 
 func (r *DataScienceClusterReconciler) reconcileSubComponent(instance *dsc.DataScienceCluster, componentName string, enabled bool,
 	component components.ComponentInterface, ctx context.Context) (*dsc.DataScienceCluster, ctrl.Result, error) {
-	err := component.ReconcileComponent(instance, r.Client, r.Scheme, enabled, r.ApplicationsNamespace)
+
+	// First set contidions to reflect a component is about to be reconciled
+	instance, err := r.updateStatus(instance, func(saved *dsc.DataScienceCluster) {
+		if enabled {
+			status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileInit, "Component reconciliation started", corev1.ConditionUnknown)
+		} else {
+			status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileInit, "Component removal started", corev1.ConditionUnknown)
+		}
+	})
 	if err != nil {
-		instance = r.reportError(err, instance, "failed to reconcile "+componentName+" on DataScienceCluster")
+		instance = r.reportError(err, instance, "failed to update DataScienceCluster conditions before reconciling "+componentName)
 		return instance, ctrl.Result{
 			// Retry after failure until success.
 			RequeueAfter: time.Second * 10}, err
 	}
-	instance, err = r.updateStatus(instance, func(saved *dsc.DataScienceCluster) {
-		if saved.Status.InstalledComponents == nil {
-			saved.Status.InstalledComponents = make(map[string]bool)
-		}
-		saved.Status.InstalledComponents[componentName] = enabled
-	})
+
+	// Reconcile component
+	err = component.ReconcileComponent(instance, r.Client, r.Scheme, enabled, r.ApplicationsNamespace)
+
 	if err != nil {
-		instance = r.reportError(err, instance, "failed to update DataScienceCluster status after reconciling "+componentName)
-		return instance, ctrl.Result{}, err
+		// reconciliation failed: log errors, raise event and update status accordingly
+		instance = r.reportError(err, instance, "failed to reconcile "+componentName+" on DataScienceCluster")
+		instance, _ = r.updateStatus(instance, func(saved *dsc.DataScienceCluster) {
+			if enabled {
+				status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileFailed, "Component reconciliation failed", corev1.ConditionFalse)
+			} else {
+				status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileFailed, "Component removal failed", corev1.ConditionFalse)
+			}
+		})
+		return instance, ctrl.Result{
+			// Retry after failure until success.
+			RequeueAfter: time.Second * 10}, err
+	} else {
+		// reconciliation succeeded: update status accordingly
+		instance, err = r.updateStatus(instance, func(saved *dsc.DataScienceCluster) {
+			if saved.Status.InstalledComponents == nil {
+				saved.Status.InstalledComponents = make(map[string]bool)
+			}
+			saved.Status.InstalledComponents[componentName] = enabled
+			if enabled {
+				status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileCompleted, "Component reconciled successfully", corev1.ConditionTrue)
+			} else {
+				status.RemoveComponentCondition(&saved.Status.Conditions, componentName)
+			}
+		})
+		if err != nil {
+			instance = r.reportError(err, instance, "failed to update DataScienceCluster status after reconciling "+componentName)
+			return instance, ctrl.Result{}, err
+		}
 	}
 	return instance, ctrl.Result{}, nil
 }
@@ -253,3 +287,21 @@ func (r *DataScienceClusterReconciler) updateStatus(original *dsc.DataScienceClu
 	})
 	return saved, err
 }
+
+func (r *DataScienceClusterReconciler) updateComponents(original *dsc.DataScienceCluster) (*dsc.DataScienceCluster, error) {
+	saved := &dsc.DataScienceCluster{}
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+
+		err := r.Client.Get(context.TODO(), client.ObjectKeyFromObject(original), saved)
+		if err != nil {
+			return err
+		}
+
+		// Try to update
+		err = r.Client.Update(context.TODO(), saved)
+		// Return err itself here (not wrapped inside another error)
+		// so that RetryOnConflict can identify it correctly.
+		return err
+	})
+	return saved, err
+}

controllers/dscinitialization/dscinitialization_controller.go

@@ -85,10 +85,10 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	if instance.Status.Conditions == nil {
 		reason := status.ReconcileInit
 		message := "Initializing DSCInitialization resource"
-		status.SetProgressingCondition(&instance.Status.Conditions, reason, message)
-
-		instance.Status.Phase = status.PhaseProgressing
-		err = r.Client.Status().Update(ctx, instance)
+		instance, err = r.updateStatus(instance, func(saved *dsci.DSCInitialization) {
+			status.SetProgressingCondition(&saved.Status.Conditions, reason, message)
+			saved.Status.Phase = status.PhaseProgressing
+		})
 		if err != nil {
 			r.Log.Error(err, "Failed to add conditions to status of DSCInitialization resource.", "DSCInitialization", req.Namespace, "Request.Name", req.Name)
 			r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError",
@@ -156,24 +156,9 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	}
 
 	// Finish reconciling
-	err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		// refresh the instance in case it was updated during the reconcile
-		err = r.Client.Get(ctx, types.NamespacedName{Name: "default"}, instance)
-		if err != nil {
-			r.Log.Error(err, "failed to retrieve DSCInitialization instance for status update after successfuly completed reconciliation")
-			return err
-		}
-		reason := status.ReconcileCompleted
-		message := status.ReconcileCompletedMessage
-		status.SetCompleteCondition(&instance.Status.Conditions, reason, message)
-
-		instance.Status.Phase = status.PhaseReady
-
-		// Try to update
-		err = r.Client.Status().Update(context.TODO(), instance)
-		// Return err itself here (not wrapped inside another error)
-		// so that RetryOnConflict can identify it correctly.
-		return err
+	_, err = r.updateStatus(instance, func(saved *dsci.DSCInitialization) {
+		status.SetCompleteCondition(&saved.Status.Conditions, status.ReconcileCompleted, status.ReconcileCompletedMessage)
+		saved.Status.Phase = status.PhaseReady
 	})
 	if err != nil {
 		r.Log.Error(err, "failed to update DSCInitialization status after successfuly completed reconciliation")
@@ -235,3 +220,23 @@ var singletonPredicate = predicate.Funcs{
 		return true
 	},
 }
+
+func (r *DSCInitializationReconciler) updateStatus(original *dsci.DSCInitialization, update func(saved *dsci.DSCInitialization)) (*dsci.DSCInitialization, error) {
+	saved := &dsci.DSCInitialization{}
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+
+		err := r.Client.Get(context.TODO(), client.ObjectKeyFromObject(original), saved)
+		if err != nil {
+			return err
+		}
+		// update status here
+		update(saved)
+
+		// Try to update
+		err = r.Client.Status().Update(context.TODO(), saved)
+		// Return err itself here (not wrapped inside another error)
+		// so that RetryOnConflict can identify it correctly.
+		return err
+	})
+	return saved, err
+}

controllers/status/status.go

@@ -56,10 +56,7 @@ const (
 )
 
 const (
-	// TODO: update this list of constants with proper reasons for conditions
-	ReconcileInitiatedReason = "ReconcileInitiated"
-	AReason                  = "AReason"
-	AnotherReason            = "AnotherReason"
+	ReadySuffix = "Ready"
 )
 
 // SetProgressingCondition sets the ProgressingCondition to True and other conditions to
@@ -107,6 +104,45 @@ func SetErrorCondition(conditions *[]conditionsv1.Condition, reason string, mess
 		Reason:  reason,
 		Message: message,
 	})
+	conditionsv1.SetStatusCondition(conditions, conditionsv1.Condition{
+		Type:    conditionsv1.ConditionAvailable,
+		Status:  corev1.ConditionFalse,
+		Reason:  reason,
+		Message: message,
+	})
+	conditionsv1.SetStatusCondition(conditions, conditionsv1.Condition{
+		Type:    conditionsv1.ConditionProgressing,
+		Status:  corev1.ConditionFalse,
+		Reason:  reason,
+		Message: message,
+	})
+	conditionsv1.SetStatusCondition(conditions, conditionsv1.Condition{
+		Type:    conditionsv1.ConditionDegraded,
+		Status:  corev1.ConditionTrue,
+		Reason:  reason,
+		Message: message,
+	})
+	conditionsv1.SetStatusCondition(conditions, conditionsv1.Condition{
+		Type:    conditionsv1.ConditionUpgradeable,
+		Status:  corev1.ConditionUnknown,
+		Reason:  reason,
+		Message: message,
+	})
+}
+
+func SetComponentCondition(conditions *[]conditionsv1.Condition, component string, reason string, message string, status corev1.ConditionStatus) {
+	condtype := component + ReadySuffix
+	conditionsv1.SetStatusCondition(conditions, conditionsv1.Condition{
+		Type:    conditionsv1.ConditionType(condtype),
+		Status:  status,
+		Reason:  reason,
+		Message: message,
+	})
+}
+
+func RemoveComponentCondition(conditions *[]conditionsv1.Condition, component string) {
+	condtype := component + ReadySuffix
+	conditionsv1.RemoveStatusCondition(conditions, conditionsv1.ConditionType(condtype))
 }
 
 // SetCompleteCondition sets the ConditionReconcileComplete to True and other Conditions