Better handling of Topology Finalizers

When a GlanceAPI references a Topology, a finalizer is added to such resource. By doing this, we can clearly identify which topology is applied to which API. For this reason, and because Glance is potentially made by a list of APIs that can independently reference the same Topology CR, the finalizer string is based on the concatenation between the Glance service (prefix), and the API name. It is possible, as a day2 operation, to add, update or remove a Topology reference. When doing this, especially for update and removal operations, a pointer to the CR must be updated. To properly support this action, the information about the LastAppliedTopology is saved in the Status, and it allows to properly clean up unused resources and not locking them indefinitely. Signed-off-by: Francesco Pantano <[email protected]>
openstack-k8s-operators · Jan 15, 2025 · 262fd74 · 262fd74
1 parent acbc792
commit 262fd74
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 19 deletions.
diff --git a/api/bases/glance.openstack.org_glanceapis.yaml b/api/bases/glance.openstack.org_glanceapis.yaml
@@ -759,6 +759,8 @@ spec:
                 additionalProperties:
                   type: string
                 type: object
+              lastAppliedTopology:
+                type: string
               networkAttachments:
                 additionalProperties:
                   items:

diff --git a/api/go.mod b/api/go.mod
@@ -71,6 +71,6 @@ require (
 // must consistent within modules and service operators
 replace github.com/openshift/api => github.com/openshift/api v0.0.0-20240830023148-b7d0481c9094 //allow-merging
 
-replace github.com/openstack-k8s-operators/infra-operator/apis => github.com/fmount/infra-operator/apis v0.0.0-20241217210734-91376cf9eb3f //allow-merging
+replace github.com/openstack-k8s-operators/infra-operator/apis => github.com/fmount/infra-operator/apis v0.0.0-20250109124018-4262fdefc70b //allow-merging
 
 replace github.com/openstack-k8s-operators/lib-common/modules/common => github.com/fmount/lib-common/modules/common v0.0.0-20241217100632-a2c8ea43c395 //allow-merging
diff --git a/api/v1beta1/glanceapi_types.go b/api/v1beta1/glanceapi_types.go
@@ -115,6 +115,9 @@ type GlanceAPIStatus struct {
 	// then the controller has not processed the latest changes injected by
 	// the opentack-operator in the top-level CR (e.g. the ContainerImage)
 	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+
+	// LastAppliedTopology - the last applied Topology
+	LastAppliedTopology string `json:"lastAppliedTopology,omitempty"`
 }
 
 // +kubebuilder:object:root=true

diff --git a/config/crd/bases/glance.openstack.org_glanceapis.yaml b/config/crd/bases/glance.openstack.org_glanceapis.yaml
@@ -759,6 +759,8 @@ spec:
                 additionalProperties:
                   type: string
                 type: object
+              lastAppliedTopology:
+                type: string
               networkAttachments:
                 additionalProperties:
                   items:

diff --git a/controllers/glanceapi_controller.go b/controllers/glanceapi_controller.go
@@ -347,7 +347,6 @@ func (r *GlanceAPIReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 	topologyFn := func(_ context.Context, o client.Object) []reconcile.Request {
 		result := []reconcile.Request{}
-
 		// get all GlanceAPIs CRs
 		glanceAPIs := &glancev1.GlanceAPIList{}
 		listOpts := []client.ListOption{
@@ -855,6 +854,15 @@ func (r *GlanceAPIReconciler) reconcileNormal(
 	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
 
 	var topology *topologyv1.Topology
+	// When the Topology CR reference is updated and the current GlanceAPI
+	// switches to a new Topology, remove the finalizer from the previous
+	// Topology
+	if instance.Status.LastAppliedTopology != "" {
+		_, err = r.ensureDeletedTopology(ctx, instance, helper)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+	}
 	if instance.Spec.Topology != nil {
 		topology, err = r.ensureGlanceAPITopology(ctx, helper, instance)
 		if err != nil {
@@ -867,6 +875,9 @@ func (r *GlanceAPIReconciler) reconcileNormal(
 			r.Log.Info("Glance config is waiting for Topology requirements, requeueing...")
 			return ctrl.Result{}, err
 		}
+		// update the Status with the last retrieved Topology name as the finalizer
+		// is now removed from the previous topologyRef
+		instance.Status.LastAppliedTopology = instance.Spec.Topology.Name
 	}
 
 	// At this point Glance has a Topology CR (or not in case it's not referenced in the
@@ -1532,7 +1543,7 @@ func (r *GlanceAPIReconciler) ensureGlanceAPITopology(
 
 	// Add finalizer to the resource because it is going to be consumed by GlanceAPI
 	if !controllerutil.ContainsFinalizer(topology, h.GetFinalizer()) {
-		controllerutil.AddFinalizer(topology, h.GetFinalizer())
+		controllerutil.AddFinalizer(topology, fmt.Sprintf("%s-%s", h.GetFinalizer(), instance.APIName()))
 	}
 	// Update the resource
 	if err := h.GetClient().Update(ctx, topology); err != nil {
@@ -1553,17 +1564,24 @@ func (r *GlanceAPIReconciler) ensureDeletedTopology(
 ) (ctrl.Result, error) {
 	ns := instance.Namespace
 
-	if instance.Spec.Topology == nil {
+	// no Topology is currently passed to the GlanceAPI, and it was not used
+	// before
+	if instance.Spec.Topology == nil && instance.Status.LastAppliedTopology == "" {
 		return ctrl.Result{}, nil
 	}
-	if instance.Spec.Topology.Namespace != "" {
-		ns = instance.Spec.Topology.Namespace
+	if instance.Spec.Topology != nil {
+		// Check namespace and set name
+		if instance.Spec.Topology.Namespace != "" {
+			ns = instance.Spec.Topology.Namespace
+		}
 	}
+
+	name := instance.Status.LastAppliedTopology
 	// Remove the finalizer from the Topology CR
 	topology, _, err := topologyv1.GetTopologyByName(
 		ctx,
 		h,
-		instance.Spec.Topology.Name,
+		name,
 		ns,
 	)
 
@@ -1574,7 +1592,7 @@ func (r *GlanceAPIReconciler) ensureDeletedTopology(
 		return ctrl.Result{}, err
 	}
 	if err == nil {
-		if controllerutil.RemoveFinalizer(topology, h.GetFinalizer()) {
+		if controllerutil.RemoveFinalizer(topology, fmt.Sprintf("%s-%s", h.GetFinalizer(), instance.APIName())) {
 			err = r.Update(ctx, topology)
 			if err != nil && !k8s_errors.IsNotFound(err) {
 				return ctrl.Result{}, err

diff --git a/go.mod b/go.mod
@@ -90,6 +90,6 @@ replace github.com/openstack-k8s-operators/glance-operator/api => ./api
 // must consistent within modules and service operators
 replace github.com/openshift/api => github.com/openshift/api v0.0.0-20240830023148-b7d0481c9094 //allow-merging
 
-replace github.com/openstack-k8s-operators/infra-operator/apis => github.com/fmount/infra-operator/apis v0.0.0-20241217210734-91376cf9eb3f //allow-merging
+replace github.com/openstack-k8s-operators/infra-operator/apis => github.com/fmount/infra-operator/apis v0.0.0-20250109124018-4262fdefc70b //allow-merging
 
 replace github.com/openstack-k8s-operators/lib-common/modules/common => github.com/fmount/lib-common/modules/common v0.0.0-20241217100632-a2c8ea43c395 //allow-merging
diff --git a/pkg/glanceapi/statefulset.go b/pkg/glanceapi/statefulset.go
@@ -76,6 +76,16 @@ func StatefulSet(
 	glanceURIScheme := corev1.URISchemeHTTP
 	tlsEnabled := instance.Spec.TLS.API.Enabled(service.EndpointPublic)
 
+	// initialize an Affinity Object
+	aff := &affinity.OverrideSpec{
+		PodAffinity:     nil,
+		PodAntiAffinity: nil,
+		NodeAffinity:    nil,
+	}
+
+	// initialize a Topology Object
+	tplg := []corev1.TopologySpreadConstraint{}
+
 	if instance.Spec.APIType == glancev1.APIInternal ||
 		instance.Spec.APIType == glancev1.APIEdge {
 		port = int32(glance.GlanceInternalPort)
@@ -297,16 +307,6 @@ func StatefulSet(
 		statefulset.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
 
-	// initialize an Affinity Object
-	aff := &affinity.OverrideSpec{
-		PodAffinity:     nil,
-		PodAntiAffinity: nil,
-		NodeAffinity:    nil,
-	}
-
-	// initialize a Topology Object
-	tplg := []corev1.TopologySpreadConstraint{}
-
 	if topologyOverride != nil {
 		ts := topologyOverride.Spec
 		if ts.TopologySpreadConstraint == nil {