Fix inconsistencies between MCAD and MicroMCAD (#37)

* Separate status ""/pending/running/completed/failed and step ""/creating/created/deleting

* Adjust MinAvailable/MaxNumRequeuings behavior

* Tweak logging

* Fix tests

* Implement configurable requeuing delays

Author: tardieu

api/v1beta1/appwrapper_types.go

@@ -47,6 +47,13 @@ type SchedulingSpec struct {
 }
 
 type RequeuingSpec struct {
+	// Initial waiting time before requeuing conditions are checked
+	// +kubebuilder:default=300
+	TimeInSeconds int64 `json:"timeInSeconds,omitempty"`
+
+	// Wait time before trying to dispatch again after requeuing
+	PauseTimeInSeconds int64 `json:"pauseTimeInSeconds,omitempty"`
+
 	// Max requeuings permitted
 	MaxNumRequeuings int32 `json:"maxNumRequeuings,omitempty"`
 }
@@ -56,6 +63,9 @@ type AppWrapperStatus struct {
 	// Phase
 	Phase AppWrapperPhase `json:"state,omitempty"`
 
+	// Status of wrapped resources
+	Step AppWrapperStep `json:"step,omitempty"`
+
 	// When last dispatched
 	DispatchTimestamp metav1.Time `json:"dispatchTimestamp,omitempty"`
 
@@ -72,27 +82,36 @@ type AppWrapperStatus struct {
 // AppWrapperPhase is the label for the AppWrapper status
 type AppWrapperPhase string
 
+// AppWrapperState is the status of wrapped resources
+type AppWrapperStep string
+
 const (
-	// no resource reservation
+	// Initial state upon creation of the AppWrapper object
 	Empty AppWrapperPhase = ""
 
-	// no resource reservation
+	// AppWrapper has not been dispatched yet or has been requeued
 	Queued AppWrapperPhase = "Pending"
 
-	// resources are reserved
-	Dispatching AppWrapperPhase = "Dispatching"
-
-	// resources are reserved
+	// AppWrapper has been dispatched and not requeued
 	Running AppWrapperPhase = "Running"
 
-	// no resource reservation
+	// AppWrapper completed successfully
 	Succeeded AppWrapperPhase = "Completed"
 
-	// resources are reserved (some pods may still be running)
+	// AppWrapper failed and is not requeued
 	Failed AppWrapperPhase = "Failed"
 
-	// resources are reserved (some pods may still be running)
-	Requeuing AppWrapperPhase = "Requeuing"
+	// Resources are not deployed
+	Idle AppWrapperStep = ""
+
+	// MCAD is in the process of creating the wrapped resources
+	Creating AppWrapperStep = "creating"
+
+	// The wrapped resources have been deployed successfully
+	Created AppWrapperStep = "created"
+
+	// MCAD is in the process of deleting the wrapped resources
+	Deleting AppWrapperStep = "deleting"
 )
 
 // AppWrapper resources
@@ -137,6 +156,9 @@ type AppWrapperTransition struct {
 
 	// Phase entered
 	Phase AppWrapperPhase `json:"state"`
+
+	// Status of wrapped resources
+	Step AppWrapperStep `json:"step,omitempty"`
 }
 
 //+kubebuilder:object:root=true

config/crd/bases/workload.codeflare.dev_appwrappers.yaml

@@ -127,6 +127,17 @@ spec:
                         description: Max requeuings permitted
                         format: int32
                         type: integer
+                      pauseTimeInSeconds:
+                        description: Wait time before trying to dispatch again after
+                          requeuing
+                        format: int64
+                        type: integer
+                      timeInSeconds:
+                        default: 300
+                        description: Initial waiting time before requeuing conditions
+                          are checked
+                        format: int64
+                        type: integer
                     type: object
                 type: object
             required:
@@ -150,6 +161,9 @@ spec:
               state:
                 description: Phase
                 type: string
+              step:
+                description: Status of wrapped resources
+                type: string
               transitions:
                 description: Transition log
                 items:
@@ -161,6 +175,9 @@ spec:
                     state:
                       description: Phase entered
                       type: string
+                    step:
+                      description: Status of wrapped resources
+                      type: string
                     time:
                       description: Timestamp
                       format: date-time

internal/controller/appwrapper_controller.go

@@ -91,8 +91,8 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	// handle deletion
 	if !appWrapper.DeletionTimestamp.IsZero() {
 		// delete wrapped resources
-		if r.deleteResources(ctx, appWrapper) != 0 {
-			// deletion is pending, requeue reconciliation after delay
+		if !r.delete(ctx, appWrapper, *appWrapper.DeletionTimestamp) {
+			// requeue reconciliation after delay
 			return ctrl.Result{RequeueAfter: deletionDelay}, nil
 		}
 		// remove finalizer
@@ -103,7 +103,7 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		}
 		// remove AppWrapper from cache
 		r.deleteCachedPhase(appWrapper)
-		log.FromContext(ctx).Info("Deleted", "state", "Deleted")
+		log.FromContext(ctx).Info("Deleted")
 		return ctrl.Result{}, nil
 	}
 
@@ -116,63 +116,70 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 				return ctrl.Result{}, err
 			}
 		}
-		// set queued status only after adding finalizer
-		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Queued)
+		// set queued/idle status only after adding finalizer
+		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Queued, mcadv1beta1.Idle)
 
 	case mcadv1beta1.Queued, mcadv1beta1.Succeeded:
 		r.triggerDispatch()
 		return ctrl.Result{}, nil
 
-	case mcadv1beta1.Dispatching:
-		// create wrapped resources
-		err, fatal := r.createResources(ctx, appWrapper)
-		if err != nil {
-			if fatal {
-				return r.updateStatus(ctx, appWrapper, mcadv1beta1.Failed, err.Error())
+	case mcadv1beta1.Running:
+		switch appWrapper.Status.Step {
+		case mcadv1beta1.Creating:
+			// create wrapped resources
+			if err, fatal := r.createResources(ctx, appWrapper); err != nil {
+				return r.requeueOrFail(ctx, appWrapper, fatal, err.Error())
 			}
-			return r.requeueOrFail(ctx, appWrapper, err.Error())
-		}
-		// set running status only after successfully requesting the creation of all resources
-		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Running)
+			// set running/created status only after successfully requesting the creation of all resources
+			return r.updateStatus(ctx, appWrapper, mcadv1beta1.Running, mcadv1beta1.Created)
 
-	case mcadv1beta1.Running:
-		// count AppWrapper pods
-		counts, err := r.countPods(ctx, appWrapper)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-		// check pod count if dispatched for a while
-		if isSlowDispatching(appWrapper) && counts.Running+counts.Succeeded < int(appWrapper.Spec.Scheduling.MinAvailable) {
+		case mcadv1beta1.Created:
+			// count AppWrapper pods
+			counts, err := r.countPods(ctx, appWrapper)
+			if err != nil {
+				return ctrl.Result{}, err
+			}
+			// check pod count if dispatched for a while
+			if metav1.Now().After(appWrapper.Status.DispatchTimestamp.Add(time.Duration(appWrapper.Spec.Scheduling.Requeuing.TimeInSeconds)*time.Second)) &&
+				counts.Running+counts.Succeeded < int(appWrapper.Spec.Scheduling.MinAvailable) {
+				customMessage := "expected pods " + strconv.Itoa(int(appWrapper.Spec.Scheduling.MinAvailable)) + " but found pods " + strconv.Itoa(counts.Running+counts.Succeeded)
+				// requeue or fail if max retries exhausted with custom error message
+				return r.requeueOrFail(ctx, appWrapper, false, customMessage)
+			}
+			// check for successful completion by looking at pods and wrapped resources
+			success, err := r.isSuccessful(ctx, appWrapper, counts)
+			if err != nil {
+				return ctrl.Result{}, err
+			}
+			// set succeeded/idle status if done
+			if success {
+				return r.updateStatus(ctx, appWrapper, mcadv1beta1.Succeeded, mcadv1beta1.Idle)
+			}
+			// AppWrapper is healthy, requeue reconciliation after delay
+			return ctrl.Result{RequeueAfter: runDelay}, nil
 
-			customMessage := "expected pods " + strconv.Itoa(int(appWrapper.Spec.Scheduling.MinAvailable)) + " but found pods " + strconv.Itoa(counts.Running+counts.Succeeded)
-			// requeue or fail if max retries exhausted with custom error message
-			return r.requeueOrFail(ctx, appWrapper, customMessage)
-		}
-		// check for successful completion by looking at pods and wrapped resources
-		success, err := r.isSuccessful(ctx, appWrapper, counts)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-		// set succeeded status if done
-		if success {
-			return r.updateStatus(ctx, appWrapper, mcadv1beta1.Succeeded)
+		case mcadv1beta1.Deleting:
+			// delete wrapped resources
+			if !r.delete(ctx, appWrapper, appWrapper.Status.RequeueTimestamp) {
+				// requeue reconciliation after delay
+				return ctrl.Result{RequeueAfter: deletionDelay}, nil
+			}
+			// reset status to queued/idle
+			appWrapper.Status.Restarts += 1
+			return r.updateStatus(ctx, appWrapper, mcadv1beta1.Queued, mcadv1beta1.Idle)
 		}
-		// AppWrapper is healthy, requeue reconciliation after delay
-		return ctrl.Result{RequeueAfter: runDelay}, nil
 
-	case mcadv1beta1.Requeuing:
-		// delete wrapped resources
-		if r.deleteResources(ctx, appWrapper) != 0 {
-			if !isSlowRequeuing(appWrapper) {
+	case mcadv1beta1.Failed:
+		switch appWrapper.Status.Step {
+		case mcadv1beta1.Deleting:
+			// delete wrapped resources
+			if !r.delete(ctx, appWrapper, appWrapper.Status.RequeueTimestamp) {
 				// requeue reconciliation after delay
 				return ctrl.Result{RequeueAfter: deletionDelay}, nil
 			}
-			// forcefully delete wrapped resources and pods
-			r.forceDelete(ctx, appWrapper)
+			// set status to failed/idle
+			return r.updateStatus(ctx, appWrapper, mcadv1beta1.Failed, mcadv1beta1.Idle)
 		}
-		// reset status to queued
-		appWrapper.Status.Restarts += 1
-		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Queued)
 	}
 	return ctrl.Result{}, nil
 }
@@ -208,32 +215,39 @@ func (r *AppWrapperReconciler) podMapFunc(ctx context.Context, obj client.Object
 }
 
 // Update AppWrapper status
-func (r *AppWrapperReconciler) updateStatus(ctx context.Context, appWrapper *mcadv1beta1.AppWrapper, phase mcadv1beta1.AppWrapperPhase, reason ...string) (ctrl.Result, error) {
+func (r *AppWrapperReconciler) updateStatus(ctx context.Context, appWrapper *mcadv1beta1.AppWrapper, phase mcadv1beta1.AppWrapperPhase, step mcadv1beta1.AppWrapperStep, reason ...string) (ctrl.Result, error) {
 	// log transition
 	now := metav1.Now()
-	transition := mcadv1beta1.AppWrapperTransition{Time: now, Phase: phase}
+	transition := mcadv1beta1.AppWrapperTransition{Time: now, Phase: phase, Step: step}
 	if len(reason) > 0 {
 		transition.Reason = reason[0]
 	}
 	appWrapper.Status.Transitions = append(appWrapper.Status.Transitions, transition)
 	appWrapper.Status.Phase = phase
+	appWrapper.Status.Step = step
 	// update AppWrapper status in etcd, requeue reconciliation on failure
 	if err := r.Status().Update(ctx, appWrapper); err != nil {
 		return ctrl.Result{}, err
 	}
 	// cache AppWrapper status
 	r.addCachedPhase(appWrapper)
-	log.FromContext(ctx).Info(string(phase), "state", string(phase))
+	log.FromContext(ctx).Info(string(phase), "state", phase, "step", step)
 	return ctrl.Result{}, nil
 }
 
-// Set requeuing or failed status depending on restarts count
-func (r *AppWrapperReconciler) requeueOrFail(ctx context.Context, appWrapper *mcadv1beta1.AppWrapper, reason string) (ctrl.Result, error) {
-	if appWrapper.Status.Restarts < appWrapper.Spec.Scheduling.Requeuing.MaxNumRequeuings {
+// Set requeuing or failed status depending on error, configuration, and restarts count
+func (r *AppWrapperReconciler) requeueOrFail(ctx context.Context, appWrapper *mcadv1beta1.AppWrapper, fatal bool, reason string) (ctrl.Result, error) {
+	if appWrapper.Spec.Scheduling.MinAvailable == 0 {
+		// set failed status and leave resources as is
+		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Failed, appWrapper.Status.Step, reason)
+	} else if fatal || appWrapper.Spec.Scheduling.Requeuing.MaxNumRequeuings > 0 && appWrapper.Status.Restarts >= appWrapper.Spec.Scheduling.Requeuing.MaxNumRequeuings {
+		// set failed/deleting status (request deletion of wrapped resources)
 		appWrapper.Status.RequeueTimestamp = metav1.Now()
-		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Requeuing, reason)
+		return r.updateStatus(ctx, appWrapper, mcadv1beta1.Failed, mcadv1beta1.Deleting, reason)
 	}
-	return r.updateStatus(ctx, appWrapper, mcadv1beta1.Failed, reason)
+	// requeue AppWrapper
+	appWrapper.Status.RequeueTimestamp = metav1.Now()
+	return r.updateStatus(ctx, appWrapper, mcadv1beta1.Running, mcadv1beta1.Deleting, reason)
 }
 
 // Trigger dispatch by means of "*/*" request
@@ -270,18 +284,22 @@ func (r *AppWrapperReconciler) dispatch(ctx context.Context) (ctrl.Result, error
 		}
 		// set dispatching time and status
 		appWrapper.Status.DispatchTimestamp = metav1.Now()
-		if _, err := r.updateStatus(ctx, appWrapper, mcadv1beta1.Dispatching); err != nil {
+		if _, err := r.updateStatus(ctx, appWrapper, mcadv1beta1.Running, mcadv1beta1.Creating); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
 }
 
-// Is dispatching too slow?
-func isSlowDispatching(appWrapper *mcadv1beta1.AppWrapper) bool {
-	return metav1.Now().After(appWrapper.Status.DispatchTimestamp.Add(runningTimeout))
-}
-
-// Is requeuing too slow?
-func isSlowRequeuing(appWrapper *mcadv1beta1.AppWrapper) bool {
-	return metav1.Now().After(appWrapper.Status.RequeueTimestamp.Add(requeuingTimeout))
+// Delete wrapped resources, forcing deletion after a delay
+func (r *AppWrapperReconciler) delete(ctx context.Context, appWrapper *mcadv1beta1.AppWrapper, time metav1.Time) bool {
+	if r.deleteResources(ctx, appWrapper) != 0 {
+		// resources still exist
+		if !metav1.Now().After(time.Add(deletionTimeout)) {
+			// there is still time
+			return false
+		}
+		// force deletion
+		r.forceDelete(ctx, appWrapper)
+	}
+	return true
 }

internal/controller/cache.go

@@ -40,6 +40,9 @@ type CachedAppWrapper struct {
 	// AppWrapper phase
 	Phase mcadv1beta1.AppWrapperPhase
 
+	// AppWrapper step
+	Step mcadv1beta1.AppWrapperStep
+
 	// Number of transitions
 	Transitions int
 
@@ -49,7 +52,7 @@ type CachedAppWrapper struct {
 
 // Add AppWrapper to cache
 func (r *AppWrapperReconciler) addCachedPhase(appWrapper *mcadv1beta1.AppWrapper) {
-	r.Cache[appWrapper.UID] = &CachedAppWrapper{Phase: appWrapper.Status.Phase, Transitions: len(appWrapper.Status.Transitions)}
+	r.Cache[appWrapper.UID] = &CachedAppWrapper{Phase: appWrapper.Status.Phase, Step: appWrapper.Status.Step, Transitions: len(appWrapper.Status.Transitions)}
 }
 
 // Remove AppWrapper from cache
@@ -58,11 +61,11 @@ func (r *AppWrapperReconciler) deleteCachedPhase(appWrapper *mcadv1beta1.AppWrap
 }
 
 // Get AppWrapper phase from cache if available or from AppWrapper if not
-func (r *AppWrapperReconciler) getCachedPhase(appWrapper *mcadv1beta1.AppWrapper) mcadv1beta1.AppWrapperPhase {
+func (r *AppWrapperReconciler) getCachedPhase(appWrapper *mcadv1beta1.AppWrapper) (mcadv1beta1.AppWrapperPhase, mcadv1beta1.AppWrapperStep) {
 	if cached, ok := r.Cache[appWrapper.UID]; ok && cached.Transitions > len(appWrapper.Status.Transitions) {
-		return cached.Phase // our cache is more up-to-date than the reconciler cache
+		return cached.Phase, cached.Step // our cache is more up-to-date than the reconciler cache
 	}
-	return appWrapper.Status.Phase
+	return appWrapper.Status.Phase, appWrapper.Status.Step
 }
 
 // Check whether reconciler cache and our cache appear to be in sync
@@ -91,7 +94,7 @@ func (r *AppWrapperReconciler) isStale(ctx context.Context, appWrapper *mcadv1be
 			}
 			return true
 		}
-		if cached.Phase != status.Phase {
+		if cached.Phase != status.Phase || cached.Step != status.Step {
 			// assume something is wrong with our cache
 			delete(r.Cache, appWrapper.UID)
 			log.FromContext(ctx).Error(errors.New("cache conflict"), "Internal error")