Switch to eviction condition, remove eviction status, improve tests

* Instead of duplicated and mostly inconsistent eviction status as a status field, use conditions to display and check for the eviction status.
* More conditions are anticipated to provide insights about current state of the eviction.
* Improve tests by explicitly checking every iteration of a reconciliation. This can be used as a template for other tests to increase coverage.

Author: notandy

api/v1/eviction_types.go

@@ -21,9 +21,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
-// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
-
 // EvictionSpec defines the desired state of Eviction
 type EvictionSpec struct {
 	// +kubebuilder:validation:Required
@@ -40,11 +37,22 @@ type EvictionSpec struct {
 	Reason string `json:"reason"`
 }
 
+const (
+	// ConditionTypeEviction is the type of condition for eviction status
+	ConditionTypeEviction = "Eviction"
+
+	// ConditionReasonRunning means the eviction is currently running
+	ConditionReasonRunning string = "Running"
+
+	// ConditionReasonFailed means the eviction has failed
+	ConditionReasonFailed string = "Failed"
+
+	// ConditionReasonSuceeded means the eviction has succeeded
+	ConditionReasonSuceeded string = "Succeeded"
+)
+
 // EvictionStatus defines the observed state of Eviction
 type EvictionStatus struct {
-	// +kubebuilder:validation:Enum=Pending;Running;Failed;Succeeded
-	// +kubebuilder:default=Pending
-	EvictionState string `json:"evictionState"`
 	// +kubebuilder:validation:Optional
 	HypervisorServiceId string `json:"hypervisorServiceId"`
 	// +kubebuilder:validation:Optional
@@ -62,7 +70,7 @@ type EvictionStatus struct {
 // +kubebuilder:resource:scope=Cluster,shortName=evi
 // +kubebuilder:printcolumn:JSONPath=".spec.hypervisor",name="Hypervisor",type="string"
 // +kubebuilder:printcolumn:JSONPath=".spec.reason",name="Reason",type="string"
-// +kubebuilder:printcolumn:JSONPath=".status.evictionState",name="State",type="string"
+// +kubebuilder:printcolumn:JSONPath=".status.conditions[?(@.type==\"Eviction\")].reason",name="State",type="string"
 // +kubebuilder:printcolumn:JSONPath=".metadata.creationTimestamp",name="Created",type="date"
 
 // Eviction is the Schema for the evictions API

charts/openstack-hypervisor-operator/crds/eviction-crd.yaml

@@ -22,7 +22,7 @@ spec:
     - jsonPath: .spec.reason
       name: Reason
       type: string
-    - jsonPath: .status.evictionState
+    - jsonPath: .status.conditions[?(@.type=="Eviction")].reason
       name: State
       type: string
     - jsonPath: .metadata.creationTimestamp
@@ -128,14 +128,6 @@ spec:
                   - type
                   type: object
                 type: array
-              evictionState:
-                default: Pending
-                enum:
-                - Pending
-                - Running
-                - Failed
-                - Succeeded
-                type: string
               hypervisorServiceId:
                 type: string
               outstandingInstances:
@@ -146,8 +138,6 @@ spec:
                 default: 0
                 format: int64
                 type: integer
-            required:
-            - evictionState
             type: object
         type: object
     served: true

config/crd/bases/kvm.cloud.sap_evictions.yaml

@@ -23,7 +23,7 @@ spec:
     - jsonPath: .spec.reason
       name: Reason
       type: string
-    - jsonPath: .status.evictionState
+    - jsonPath: .status.conditions[?(@.type=="Eviction")].reason
       name: State
       type: string
     - jsonPath: .metadata.creationTimestamp
@@ -129,14 +129,6 @@ spec:
                   - type
                   type: object
                 type: array
-              evictionState:
-                default: Pending
-                enum:
-                - Pending
-                - Running
-                - Failed
-                - Succeeded
-                type: string
               hypervisorServiceId:
                 type: string
               outstandingInstances:
@@ -147,8 +139,6 @@ spec:
                 default: 0
                 format: int64
                 type: integer
-            required:
-            - evictionState
             type: object
         type: object
     served: true

internal/controller/decomission_controller_test.go

@@ -20,6 +20,7 @@ package controller
 import (
 	"context"
 
+	"github.com/gophercloud/gophercloud/v2/testhelper"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
@@ -77,7 +78,10 @@ var _ = Describe("Decommission Controller", func() {
 		})
 
 		It("should successfully reconcile the resource", func() {
-			By("Reconciling the created resource")
+			By("ConditionType the created resource")
+			testhelper.SetupHTTP()
+			defer testhelper.TeardownHTTP()
+
 			_, err := reconcileNodeLoop(1)
 			Expect(err).NotTo(HaveOccurred())
 		})

internal/controller/eviction_controller.go

@@ -54,11 +54,6 @@ type EvictionReconciler struct {
 
 const (
 	evictionFinalizerName = "eviction-controller.cloud.sap/finalizer"
-	Succeeded             = "Succeeded"
-	Running               = "Running"
-	Reconciling           = "Reconciling"
-	Reconciled            = "Reconciled"
-	Failed                = "Failed"
 )
 
 // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions,verbs=get;list;watch;create;update;patch;delete
@@ -75,31 +70,37 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	log := logger.FromContext(ctx).WithName("Eviction").WithValues("hypervisor", eviction.Spec.Hypervisor, "status", eviction.Status.EvictionState)
+	log := logger.FromContext(ctx).
+		WithName("Eviction").
+		WithValues("hypervisor", eviction.Spec.Hypervisor)
 	ctx = logger.IntoContext(ctx, log)
+
 	// Being deleted
 	if !eviction.DeletionTimestamp.IsZero() {
 		err := r.handleFinalizer(ctx, eviction)
 		log.Info("deleted")
 		return ctrl.Result{}, err
 	}
 
-	switch eviction.Status.EvictionState {
-	case Succeeded, Failed:
-		// Nothing left to be done
-		log.Info("finished")
-		return ctrl.Result{}, nil
-	case "Pending", "":
-		// Let's see if we can take it up
-		eviction.Status.EvictionState = "Pending" // "" -> "Pending: Fixes the test
-		log.Info("setup")
+	statusCondition := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeEviction)
+	if statusCondition == nil {
+		// No condition, so we are in the initial state
 		return r.handlePending(ctx, eviction)
-	case "Running":
+	} else if statusCondition.Status == metav1.ConditionTrue {
+		// We are running, so we need to evict the next instance
 		return r.handleRunning(ctx, eviction)
-	default:
-		log.Info("Unknown eviction-state", "EvictionState", eviction.Status.EvictionState)
+	} else if statusCondition.Status == metav1.ConditionFalse {
+		// We are done, so we can just return
+		log.Info("finished")
 		return ctrl.Result{}, nil
+	} else {
+		log.
+			WithValues("reason", statusCondition.Reason).
+			WithValues("msg", statusCondition.Message).
+			Info("unknown status condition")
 	}
+
+	return ctrl.Result{}, nil
 }
 
 func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1.Eviction) (reconcile.Result, error) {
@@ -109,14 +110,13 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1.
 	}
 
 	meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
-		Type:    Reconciling,
-		Status:  metav1.ConditionTrue,
-		Message: Reconciled,
-		Reason:  Reconciled,
+		Type:    kvmv1.ConditionTypeEviction,
+		Status:  metav1.ConditionFalse,
+		Message: "eviction completed successfully",
+		Reason:  kvmv1.ConditionReasonSuceeded,
 	})
 
 	eviction.Status.OutstandingRamMb = 0
-	eviction.Status.EvictionState = Succeeded
 	logger.FromContext(ctx).Info("succeeded")
 	return ctrl.Result{}, r.Status().Update(ctx, eviction)
 }
@@ -160,9 +160,15 @@ func (r *EvictionReconciler) handlePending(ctx context.Context, eviction *kvmv1.
 		} else {
 			// That is (likely) an eviction for a node that never registered
 			// so we are good to go
+			msg := "eviction completed successfully due to expected case of no hypervisor"
+			meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+				Type:    kvmv1.ConditionTypeEviction,
+				Status:  metav1.ConditionFalse,
+				Message: msg,
+				Reason:  kvmv1.ConditionReasonSuceeded,
+			})
 			eviction.Status.OutstandingRamMb = 0
-			eviction.Status.EvictionState = Succeeded
-			logger.FromContext(ctx).Info("succeeded due to expected case of no hypervisor")
+			logger.FromContext(ctx).Info(msg)
 			return ctrl.Result{}, r.Status().Update(ctx, eviction)
 		}
 	}
@@ -172,17 +178,15 @@ func (r *EvictionReconciler) handlePending(ctx context.Context, eviction *kvmv1.
 	if currentHypervisor != hypervisorName {
 		err = fmt.Errorf("hypervisor name %q does not match spec %q", currentHypervisor, hypervisorName)
 		log.Error(err, "Hypervisor name mismatch")
-		if eviction.Status.EvictionState != Failed ||
-			meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
-				Type:    "Eviction",
-				Status:  metav1.ConditionFalse,
-				Message: err.Error(),
-				Reason:  Failed}) {
-			eviction.Status.EvictionState = Failed
-			log.Info("Update", "status", eviction.Status)
-			if err := r.Status().Update(ctx, eviction); err != nil {
-				return ctrl.Result{}, err
-			}
+		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
+			Type:    kvmv1.ConditionTypeEviction,
+			Status:  metav1.ConditionFalse,
+			Message: err.Error(),
+			Reason:  kvmv1.ConditionReasonFailed,
+		})
+		log.Info("Update", "status", eviction.Status)
+		if err = r.Status().Update(ctx, eviction); err != nil {
+			return ctrl.Result{}, err
 		}
 
 		return ctrl.Result{}, nil
@@ -212,13 +216,12 @@ func (r *EvictionReconciler) handlePending(ctx context.Context, eviction *kvmv1.
 
 	// Update status
 	eviction.Status.HypervisorServiceId = hypervisor.ID
-	eviction.Status.EvictionState = Running
 	eviction.Status.OutstandingRamMb = hypervisor.MemoryMbUsed
 	meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
-		Type:    Reconciling,
+		Type:    kvmv1.ConditionTypeEviction,
 		Status:  metav1.ConditionTrue,
-		Message: Reconciling,
-		Reason:  Reconciling,
+		Message: "eviction started",
+		Reason:  kvmv1.ConditionReasonRunning,
 	})
 
 	return ctrl.Result{}, r.Status().Update(ctx, eviction)
@@ -375,7 +378,7 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
 
 	if disabledReason != nil && disabledReason != "" && disabledReason != evictionReason {
 		// Disabled for another reason already
-		return r.addProgressCondition(ctx, eviction, "Found host already disabled", "Update"), nil
+		return r.addProgressCondition(ctx, eviction, "Found host already disabled", kvmv1.ConditionReasonRunning), nil
 	}
 
 	if !controllerutil.ContainsFinalizer(eviction, evictionFinalizerName) {
@@ -396,7 +399,7 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
 		return r.addErrorCondition(ctx, eviction, err), err
 	}
 
-	if r.addProgressCondition(ctx, eviction, "Host disabled", "Update") {
+	if r.addProgressCondition(ctx, eviction, "Host disabled", kvmv1.ConditionReasonRunning) {
 		return true, nil
 	}
 
@@ -438,7 +441,7 @@ func (r *EvictionReconciler) coldMigrate(ctx context.Context, uuid string, evict
 // addCondition adds a condition to the Eviction status and updates the status
 func (r *EvictionReconciler) addCondition(ctx context.Context, eviction *kvmv1.Eviction, status metav1.ConditionStatus, message string, reason string) bool {
 	if !meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
-		Type:    "Eviction",
+		Type:    kvmv1.ConditionTypeEviction,
 		Status:  status,
 		Message: message,
 		Reason:  reason,
@@ -460,15 +463,15 @@ func (r *EvictionReconciler) addProgressCondition(ctx context.Context, eviction
 }
 
 func (r *EvictionReconciler) addErrorCondition(ctx context.Context, eviction *kvmv1.Eviction, err error) bool {
-	return r.addCondition(ctx, eviction, metav1.ConditionFalse, err.Error(), Failed)
+	return r.addCondition(ctx, eviction, metav1.ConditionFalse, err.Error(), kvmv1.ConditionReasonFailed)
 }
 
 // SetupWithManager sets up the controller with the Manager.
 func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	ctx := context.Background()
 
 	var err error
-	if r.computeClient, err = openstack.GetServiceClient(ctx, "compute"); err != nil {
+	if r.computeClient, err = openstack.GetServiceClient(ctx, "compute", nil); err != nil {
 		return err
 	}
 	r.computeClient.Microversion = "2.90" // Xena (or later)