fix: openstackserver reconciliation when cluster is paused

Signed-off-by: Bharath Nallapeta <[email protected]>

Author: bnallapeta

controllers/openstackserver_controller.go

@@ -39,6 +39,7 @@ import (
 	"sigs.k8s.io/cluster-api/util/annotations"
 	v1beta1conditions "sigs.k8s.io/cluster-api/util/deprecated/v1beta1/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
+	"sigs.k8s.io/cluster-api/util/predicates"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -50,7 +51,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	orcv1alpha1 "github.com/k-orc/openstack-resource-controller/v2/api/v1alpha1"
-	"github.com/k-orc/openstack-resource-controller/v2/pkg/predicates"
+	orcpredicates "github.com/k-orc/openstack-resource-controller/v2/pkg/predicates"
 
 	infrav1alpha1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha1"
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
@@ -78,6 +79,7 @@ type OpenStackServerReconciler struct {
 
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackservers,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackservers/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch
 // +kubebuilder:rbac:groups=ipam.cluster.x-k8s.io,resources=ipaddressclaims;ipaddressclaims/status,verbs=get;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=ipam.cluster.x-k8s.io,resources=ipaddresses;ipaddresses/status,verbs=get;list;watch
 // +kubebuilder:rbac:groups=openstack.k-orc.cloud,resources=images,verbs=get;list;watch
@@ -105,17 +107,6 @@ func (r *OpenStackServerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 
 	scope.Logger().Info("Reconciling OpenStackServer")
 
-	cluster, err := getClusterFromMetadata(ctx, r.Client, openStackServer.ObjectMeta)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
-	if cluster != nil {
-		if annotations.IsPaused(cluster, openStackServer) {
-			scope.Logger().Info("OpenStackServer linked to a Cluster that is paused. Won't reconcile")
-			return reconcile.Result{}, nil
-		}
-	}
-
 	patchHelper, err := patch.NewHelper(openStackServer, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -134,6 +125,7 @@ func (r *OpenStackServerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		}
 	}()
 
+	// Handle deleted servers
 	if !openStackServer.DeletionTimestamp.IsZero() {
 		// When moving a cluster, we need to populate the server status with the resources
 		// that were in another object's status.
@@ -146,6 +138,18 @@ func (r *OpenStackServerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return reconcile.Result{}, r.reconcileDelete(scope, openStackServer)
 	}
 
+	// Handle non-deleted servers
+	cluster, err := getClusterFromMetadata(ctx, r.Client, openStackServer.ObjectMeta)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+	if cluster != nil {
+		if annotations.IsPaused(cluster, openStackServer) {
+			scope.Logger().Info("OpenStackServer linked to a Cluster that is paused. Won't reconcile")
+			return reconcile.Result{}, nil
+		}
+	}
+
 	return r.reconcileNormal(ctx, scope, openStackServer)
 }
 
@@ -218,7 +222,12 @@ func (r *OpenStackServerReconciler) SetupWithManager(ctx context.Context, mgr ct
 				}
 				return requests
 			}),
-			builder.WithPredicates(predicates.NewBecameAvailable(mgr.GetLogger(), &orcv1alpha1.Image{})),
+			builder.WithPredicates(orcpredicates.NewBecameAvailable(mgr.GetLogger(), &orcv1alpha1.Image{})),
+		).
+		Watches(
+			&clusterv1.Cluster{},
+			handler.EnqueueRequestsFromMapFunc(r.requeueOpenStackServersForCluster(ctx)),
+			builder.WithPredicates(predicates.ClusterPausedTransitionsOrInfrastructureProvisioned(mgr.GetScheme(), log)),
 		).
 		Watches(
 			&ipamv1.IPAddressClaim{},
@@ -752,3 +761,50 @@ func OpenStackServerReconcileComplete(log logr.Logger) predicate.Funcs {
 		GenericFunc: func(event.GenericEvent) bool { return false },
 	}
 }
+
+// requeueOpenStackServersForCluster returns a handler.MapFunc that watches for
+// Cluster changes and triggers reconciliation of all OpenStackServers in that cluster.
+func (r *OpenStackServerReconciler) requeueOpenStackServersForCluster(ctx context.Context) handler.MapFunc {
+	log := ctrl.LoggerFrom(ctx)
+	return func(ctx context.Context, o client.Object) []ctrl.Request {
+		c, ok := o.(*clusterv1.Cluster)
+		if !ok {
+			panic(fmt.Sprintf("Expected a Cluster but got a %T", o))
+		}
+
+		log := log.WithValues("objectMapper", "clusterToOpenStackServer", "namespace", c.Namespace, "cluster", c.Name)
+
+		// Don't handle deleted clusters - servers will be cleaned up via their own deletion flow
+		if !c.DeletionTimestamp.IsZero() {
+			log.V(4).Info("Cluster has a deletion timestamp, skipping mapping.")
+			return nil
+		}
+
+		// List all OpenStackServers in the cluster
+		serverList := &infrav1alpha1.OpenStackServerList{}
+		if err := r.Client.List(
+			ctx,
+			serverList,
+			client.InNamespace(c.Namespace),
+			client.MatchingLabels{clusterv1.ClusterNameLabel: c.Name},
+		); err != nil {
+			log.Error(err, "Failed to list OpenStackServers for cluster")
+			return nil
+		}
+
+		// Create reconcile requests for all servers
+		requests := make([]ctrl.Request, 0, len(serverList.Items))
+		for i := range serverList.Items {
+			server := &serverList.Items[i]
+			requests = append(requests, ctrl.Request{
+				NamespacedName: client.ObjectKey{
+					Namespace: server.Namespace,
+					Name:      server.Name,
+				},
+			})
+			log.V(5).Info("Queueing OpenStackServer for reconciliation", "server", server.Name)
+		}
+
+		return requests
+	}
+}

controllers/openstackserver_controller_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controllers
 
 import (
+	"context"
 	"fmt"
 	"reflect"
 	"testing"
@@ -33,9 +34,13 @@ import (
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
 	clusterv1beta1 "sigs.k8s.io/cluster-api/api/core/v1beta1"
+	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	"sigs.k8s.io/cluster-api/util/conditions"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	infrav1alpha1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha1"
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
@@ -194,6 +199,191 @@ var deleteRootVolume = func(r *recorders) {
 	r.volume.DeleteVolume(rootVolumeUUID, volumes.DeleteOpts{}).Return(nil)
 }
 
+func TestOpenStackServerReconciler_requeueOpenStackServersForCluster(t *testing.T) {
+	tests := []struct {
+		name            string
+		cluster         *clusterv1.Cluster
+		servers         []*infrav1alpha1.OpenStackServer
+		clusterDeleting bool
+		wantRequests    int
+		wantServerNames []string
+	}{
+		{
+			name: "returns reconcile requests for all servers in cluster",
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: "test-ns",
+				},
+			},
+			servers: []*infrav1alpha1.OpenStackServer{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "server-1",
+						Namespace: "test-ns",
+						Labels: map[string]string{
+							clusterv1beta1.ClusterNameLabel: "test-cluster",
+						},
+					},
+					Spec: infrav1alpha1.OpenStackServerSpec{
+						Flavor: ptr.To("m1.small"),
+						Image: infrav1.ImageParam{
+							Filter: &infrav1.ImageFilter{Name: ptr.To("test-image")},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "server-2",
+						Namespace: "test-ns",
+						Labels: map[string]string{
+							clusterv1beta1.ClusterNameLabel: "test-cluster",
+						},
+					},
+					Spec: infrav1alpha1.OpenStackServerSpec{
+						Flavor: ptr.To("m1.small"),
+						Image: infrav1.ImageParam{
+							Filter: &infrav1.ImageFilter{Name: ptr.To("test-image")},
+						},
+					},
+				},
+			},
+			wantRequests:    2,
+			wantServerNames: []string{"server-1", "server-2"},
+		},
+		{
+			name: "returns empty for deleted cluster",
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: "test-ns",
+				},
+			},
+			servers: []*infrav1alpha1.OpenStackServer{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "server-1",
+						Namespace: "test-ns",
+						Labels: map[string]string{
+							clusterv1beta1.ClusterNameLabel: "test-cluster",
+						},
+					},
+					Spec: infrav1alpha1.OpenStackServerSpec{
+						Flavor: ptr.To("m1.small"),
+						Image: infrav1.ImageParam{
+							Filter: &infrav1.ImageFilter{Name: ptr.To("test-image")},
+						},
+					},
+				},
+			},
+			clusterDeleting: true,
+			wantRequests:    0,
+		},
+		{
+			name: "returns empty when no servers exist",
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: "test-ns",
+				},
+			},
+			servers:      []*infrav1alpha1.OpenStackServer{},
+			wantRequests: 0,
+		},
+		{
+			name: "only returns servers from same cluster",
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: "test-ns",
+				},
+			},
+			servers: []*infrav1alpha1.OpenStackServer{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "server-1",
+						Namespace: "test-ns",
+						Labels: map[string]string{
+							clusterv1beta1.ClusterNameLabel: "test-cluster",
+						},
+					},
+					Spec: infrav1alpha1.OpenStackServerSpec{
+						Flavor: ptr.To("m1.small"),
+						Image: infrav1.ImageParam{
+							Filter: &infrav1.ImageFilter{Name: ptr.To("test-image")},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "server-2",
+						Namespace: "test-ns",
+						Labels: map[string]string{
+							clusterv1beta1.ClusterNameLabel: "other-cluster",
+						},
+					},
+					Spec: infrav1alpha1.OpenStackServerSpec{
+						Flavor: ptr.To("m1.small"),
+						Image: infrav1.ImageParam{
+							Filter: &infrav1.ImageFilter{Name: ptr.To("test-image")},
+						},
+					},
+				},
+			},
+			wantRequests:    1,
+			wantServerNames: []string{"server-1"},
+		},
+	}
+
+	for i := range tests {
+		tt := &tests[i]
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			ctx := context.TODO()
+
+			// Set deletion timestamp and finalizers if needed
+			if tt.clusterDeleting {
+				now := metav1.Now()
+				tt.cluster.DeletionTimestamp = &now
+				tt.cluster.Finalizers = []string{"test-finalizer"}
+			}
+
+			// Create a fake client with the test data
+			scheme := runtime.NewScheme()
+			g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed())
+			g.Expect(infrav1alpha1.AddToScheme(scheme)).To(Succeed())
+
+			objs := []client.Object{tt.cluster}
+			for _, server := range tt.servers {
+				objs = append(objs, server)
+			}
+
+			fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build()
+
+			// Create reconciler and call mapper function
+			reconciler := &OpenStackServerReconciler{
+				Client: fakeClient,
+			}
+			mapFunc := reconciler.requeueOpenStackServersForCluster(ctx)
+			requests := mapFunc(ctx, tt.cluster)
+
+			// Verify results
+			if tt.wantRequests == 0 {
+				g.Expect(requests).To(Or(BeNil(), BeEmpty()))
+			} else {
+				g.Expect(requests).To(HaveLen(tt.wantRequests))
+				if len(tt.wantServerNames) > 0 {
+					gotNames := make([]string, len(requests))
+					for i, req := range requests {
+						gotNames[i] = req.Name
+					}
+					g.Expect(gotNames).To(ConsistOf(tt.wantServerNames))
+				}
+			}
+		})
+	}
+}
+
 func TestOpenStackServer_serverToInstanceSpec(t *testing.T) {
 	tests := []struct {
 		name            string