Fix ServerClaim with label selector claiming multiple servers (#230)

* Add tests for multiple Servers and ServerClaims

* Fix claiming multiple Servers for a single label-selector Claim

The claimServerBySelector code path missed the
checkForPrevUsedServer previously

* Ensure no server before testing server claiming

internal/controller/serverclaim_controller.go

@@ -297,13 +297,21 @@ func (r *ServerClaimReconciler) claimServer(ctx context.Context, log logr.Logger
 		server *metalv1alpha1.Server
 		err    error
 	)
+	serverList := &metalv1alpha1.ServerList{}
+	if err := r.List(ctx, serverList); err != nil {
+		return nil, false, err
+	}
+	if server := checkForPrevUsedServer(log, serverList.Items, claim); server != nil {
+		return server, false, nil
+	}
+
 	switch {
 	case claim.Spec.ServerRef != nil:
 		server, err = r.claimServerByReference(ctx, log, claim)
 	case claim.Spec.ServerSelector != nil:
 		server, err = r.claimServerBySelector(ctx, log, claim)
 	default:
-		server, err = r.claimFirstBestServer(ctx, log, claim)
+		server, err = r.claimFirstBestServer(ctx, log)
 	}
 	if err != nil {
 		return nil, false, err
@@ -378,7 +386,6 @@ func (r *ServerClaimReconciler) claimServerBySelector(ctx context.Context, log l
 			continue
 		}
 		return &server, nil
-
 	}
 	return nil, nil
 }
@@ -395,16 +402,11 @@ func checkForPrevUsedServer(log logr.Logger, servers []metalv1alpha1.Server, cla
 	return nil
 }
 
-func (r *ServerClaimReconciler) claimFirstBestServer(ctx context.Context, log logr.Logger, claim *metalv1alpha1.ServerClaim) (*metalv1alpha1.Server, error) {
+func (r *ServerClaimReconciler) claimFirstBestServer(ctx context.Context, log logr.Logger) (*metalv1alpha1.Server, error) {
 	serverList := &metalv1alpha1.ServerList{}
 	if err := r.List(ctx, serverList); err != nil {
 		return nil, err
 	}
-
-	if server := checkForPrevUsedServer(log, serverList.Items, claim); server != nil {
-		return server, nil
-	}
-
 	log.V(1).Info("Trying to claim first best server")
 	for _, server := range serverList.Items {
 		if server.Spec.ServerClaimRef != nil {

internal/controller/serverclaim_controller_test.go

@@ -4,11 +4,14 @@
 package controller
 
 import (
+	"context"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/utils/ptr"
 	. "sigs.k8s.io/controller-runtime/pkg/envtest/komega"
 
@@ -548,3 +551,100 @@ var _ = Describe("ServerClaim Validation", func() {
 			HaveField("Spec.ServerSelector.MatchLabels", Equal(map[string]string{"foo": "bar"}))))
 	})
 })
+
+var _ = Describe("Server Claiming", MustPassRepeatedly(5), func() {
+	ns := SetupTest()
+
+	makeServer := func(ctx context.Context) {
+		server := metalv1alpha1.Server{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "server-",
+				Annotations: map[string]string{
+					metalv1alpha1.OperationAnnotation: metalv1alpha1.OperationAnnotationIgnore,
+				},
+				Labels: map[string]string{"foo": "bar"},
+			},
+		}
+		ExpectWithOffset(1, k8sClient.Create(ctx, &server)).To(Succeed())
+		DeferCleanup(k8sClient.Delete, &server)
+		server.Status.State = metalv1alpha1.ServerStateAvailable
+		server.Status.PowerState = metalv1alpha1.ServerOffPowerState
+		ExpectWithOffset(1, k8sClient.Status().Update(ctx, &server)).To(Succeed())
+	}
+
+	makeClaim := func(ctx context.Context, labelSelector *metav1.LabelSelector) {
+		claim := metalv1alpha1.ServerClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: "claim-",
+				Namespace:    ns.Name,
+			},
+			Spec: metalv1alpha1.ServerClaimSpec{
+				ServerSelector: labelSelector,
+			},
+		}
+		ExpectWithOffset(1, k8sClient.Create(ctx, &claim)).To(Succeed())
+		DeferCleanup(k8sClient.Delete, &claim)
+	}
+
+	countBoundServer := func(ctx context.Context, serverCount int) func(Gomega) int {
+		return func(g Gomega) int {
+			var serverList metalv1alpha1.ServerList
+			g.Expect(k8sClient.List(ctx, &serverList)).To(Succeed())
+			g.Expect(serverList.Items).To(HaveLen(serverCount))
+			count := 0
+			for _, server := range serverList.Items {
+				if server.Spec.ServerClaimRef != nil {
+					count++
+				}
+			}
+			return count
+		}
+	}
+
+	BeforeEach(func(ctx SpecContext) {
+		var server metalv1alpha1.Server
+		Expect(k8sClient.DeleteAllOf(ctx, &server)).To(Succeed())
+	})
+
+	It("binds four out of ten server for four best effort claims", func(ctx SpecContext) {
+		for range 10 {
+			makeServer(ctx)
+		}
+		for range 4 {
+			makeClaim(ctx, nil)
+		}
+		Eventually(countBoundServer(ctx, 10)).Should(Equal(4))
+		Consistently(countBoundServer(ctx, 10)).Should(Equal(4))
+	})
+
+	It("binds four out of ten server for four label selector claims", func(ctx SpecContext) {
+		for range 10 {
+			makeServer(ctx)
+		}
+		for range 4 {
+			makeClaim(ctx, metav1.SetAsLabelSelector(labels.Set{"foo": "bar"}))
+		}
+		Eventually(countBoundServer(ctx, 10)).Should(Equal(4))
+		Consistently(countBoundServer(ctx, 10)).Should(Equal(4))
+	})
+
+	It("should not bind the same server to multiple best effort claims", func(ctx SpecContext) {
+		By("Creating eight ServerClaims")
+		for range 8 {
+			makeClaim(ctx, nil)
+		}
+		makeServer(ctx)
+		Eventually(countBoundServer(ctx, 1)).Should(Equal(1))
+		Consistently(countBoundServer(ctx, 1)).Should(Equal(1))
+	})
+
+	It("should not bind the same server to multiple label selector claims", func(ctx SpecContext) {
+		By("Creating eight ServerClaims")
+		for range 8 {
+			makeClaim(ctx, metav1.SetAsLabelSelector(labels.Set{"foo": "bar"}))
+		}
+		makeServer(ctx)
+		Eventually(countBoundServer(ctx, 1)).Should(Equal(1))
+		Consistently(countBoundServer(ctx, 1)).Should(Equal(1))
+	})
+})