From 6dfdf2d5df67be09ce4dccb34d443e360035d84c Mon Sep 17 00:00:00 2001
From: Christian Schwede <cschwede@redhat.com>
Date: Mon, 4 Mar 2024 10:30:07 +0100
Subject: [PATCH] Fix status issue and ensure NetworkAttachments are created

SwiftStorage status was set to Ready before the network
attachments were created. This could result in a deployment
with missing network attachments.

At the same time add missing status defer function.
---
 controllers/swiftstorage_controller.go | 74 +++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 13 deletions(-)

diff --git a/controllers/swiftstorage_controller.go b/controllers/swiftstorage_controller.go
index f7415792..5f28c2f2 100644
--- a/controllers/swiftstorage_controller.go
+++ b/controllers/swiftstorage_controller.go
@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	helper "github.com/openstack-k8s-operators/lib-common/modules/common/helper"
 	service "github.com/openstack-k8s-operators/lib-common/modules/common/service"
@@ -86,7 +87,7 @@ type Netconfig struct {
 //
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.12.2/pkg/reconcile
-func (r *SwiftStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+func (r *SwiftStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) {
 	_ = r.Log.WithValues("swiftstorage", req.NamespacedName)
 
 	instance := &swiftv1beta1.SwiftStorage{}
@@ -103,6 +104,43 @@ func (r *SwiftStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return ctrl.Result{}, err
 	}
 
+	helper, err := helper.NewHelper(
+		instance,
+		r.Client,
+		r.Kclient,
+		r.Scheme,
+		r.Log,
+	)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// Always patch the instance status when exiting this function so we can persist any changes.
+	defer func() {
+		// update the Ready condition based on the sub conditions
+		if instance.Status.Conditions.AllSubConditionIsTrue() {
+			instance.Status.Conditions.MarkTrue(
+				condition.ReadyCondition, condition.ReadyMessage)
+		} else {
+			// something is not ready so reset the Ready condition
+			instance.Status.Conditions.MarkUnknown(
+				condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage)
+			// and recalculate it based on the state of the rest of the conditions
+			instance.Status.Conditions.Set(
+				instance.Status.Conditions.Mirror(condition.ReadyCondition))
+		}
+		err := helper.PatchInstance(ctx, instance)
+		if err != nil {
+			_err = err
+			return
+		}
+	}()
+
+	// If we're not deleting this and the service object doesn't have our finalizer, add it.
+	if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) {
+		return ctrl.Result{}, nil
+	}
+
 	if instance.Status.Conditions == nil {
 		instance.Status.Conditions = condition.Conditions{}
 		cl := condition.CreateList(
@@ -123,20 +161,10 @@ func (r *SwiftStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	}
 
 	if !instance.DeletionTimestamp.IsZero() {
+		controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
 		return ctrl.Result{}, nil
 	}
 
-	helper, err := helper.NewHelper(
-		instance,
-		r.Client,
-		r.Kclient,
-		r.Scheme,
-		r.Log,
-	)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	serviceLabels := swiftstorage.Labels()
 	envVars := make(map[string]env.Setter)
 
@@ -182,7 +210,6 @@ func (r *SwiftStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request
 				err.Error()))
 			return ctrl.Result{}, err
 		}
-
 		// Get the storage network subnet range to include it in the
 		// NetworkPolicy for the storage pods
 		config := Netconfig{}
@@ -218,6 +245,27 @@ func (r *SwiftStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return ctrlResult, nil
 	}
 
+	// verify if network attachment matches expectations
+	networkReady, networkAttachmentStatus, err := networkattachment.VerifyNetworkStatusFromAnnotation(ctx, helper, instance.Spec.NetworkAttachments, serviceLabels, instance.Status.ReadyCount)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	instance.Status.NetworkAttachments = networkAttachmentStatus
+	if networkReady {
+		instance.Status.Conditions.MarkTrue(condition.NetworkAttachmentsReadyCondition, condition.NetworkAttachmentsReadyMessage)
+	} else {
+		err := fmt.Errorf("not all pods have interfaces with ips as configured in NetworkAttachments: %s", instance.Spec.NetworkAttachments)
+		instance.Status.Conditions.Set(condition.FalseCondition(
+			condition.NetworkAttachmentsReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			condition.NetworkAttachmentsReadyErrorMessage,
+			err.Error()))
+
+		return ctrl.Result{}, err
+	}
+
 	instance.Status.ReadyCount = sset.GetStatefulSet().Status.ReadyReplicas
 	if instance.Status.ReadyCount == *instance.Spec.Replicas {
 		// When the cluster is attached to an external network, create DNS record for every