Address sporadic in ServerClaim test

ironcore-dev · Aug 15, 2024 · f0bf450 · f0bf450
1 parent 7a70879
commit f0bf450
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 30 deletions.
diff --git a/internal/controller/server_controller.go b/internal/controller/server_controller.go
@@ -645,7 +645,11 @@ func (r *ServerReconciler) ensureServerPowerState(ctx context.Context, log logr.
 	}
 
 	bmcClient, err := GetBMCClientForServer(ctx, r.Client, server, r.Insecure)
-	defer bmcClient.Logout()
+	defer func() {
+		if bmcClient != nil {
+			bmcClient.Logout()
+		}
+	}()
 	if err != nil {
 		return fmt.Errorf("failed to get BMC client: %w", err)
 	}

diff --git a/internal/controller/server_controller_test.go b/internal/controller/server_controller_test.go
@@ -40,6 +40,7 @@ var _ = Describe("Server Controller", func() {
 			},
 		}
 		Expect(k8sClient.Create(ctx, endpoint)).To(Succeed())
+		DeferCleanup(k8sClient.Delete, endpoint)
 
 		By("Ensuring that the BMC resource has been created for an endpoint")
 		bmc = &metalv1alpha1.BMC{
@@ -48,6 +49,16 @@ var _ = Describe("Server Controller", func() {
 			},
 		}
 		Eventually(Get(bmc)).Should(Succeed())
+		DeferCleanup(k8sClient.Delete, bmc)
+
+		By("Ensuring that the BMCSecret will be removed")
+		bmcSecret := &metalv1alpha1.BMCSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: bmc.Name,
+			},
+		}
+		Eventually(Get(bmcSecret)).Should(Succeed())
+		DeferCleanup(k8sClient.Delete, bmcSecret)
 
 		By("Ensuring that the Server resource has been created")
 		server := &metalv1alpha1.Server{
@@ -76,6 +87,7 @@ var _ = Describe("Server Controller", func() {
 			HaveField("Status.State", metalv1alpha1.ServerStateInitial),
 			HaveField("Status.PowerState", metalv1alpha1.ServerOffPowerState),
 		))
+		DeferCleanup(k8sClient.Delete, server)
 
 		By("Ensuring the boot configuration has been created")
 		bootConfig := &metalv1alpha1.ServerBootConfiguration{

diff --git a/internal/controller/serverclaim_controller.go b/internal/controller/serverclaim_controller.go
@@ -153,30 +153,33 @@ func (r *ServerClaimReconciler) reconcile(ctx context.Context, log logr.Logger,
 		return ctrl.Result{}, nil
 	}
 
-	if modified, err := r.patchServerRef(ctx, claim, server.Name); err != nil || modified {
+	if modified, err := r.patchServerRef(ctx, claim, server); err != nil || modified {
 		return ctrl.Result{}, err
 	}
 	log.V(1).Info("Patched ServerRef in Claim")
 
 	if err := r.applyBootConfiguration(ctx, log, server, claim); err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to apply boot configuration: %w", err)
 	}
+	log.V(1).Info("Applied BootConfiguration for ServerClaim")
 
-	serverBase := server.DeepCopy()
-	server.Spec.ServerClaimRef = &v1.ObjectReference{
-		APIVersion: "metal.ironcore.dev/v1alpha1",
-		Kind:       "ServerClaim",
-		Namespace:  claim.Namespace,
-		Name:       claim.Name,
-		UID:        claim.UID,
-	}
-	server.Spec.Power = claim.Spec.Power
-	if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to patch claim ref for server: %w", err)
+	if server.Spec.ServerClaimRef == nil {
+		serverBase := server.DeepCopy()
+		server.Spec.ServerClaimRef = &v1.ObjectReference{
+			APIVersion: "metal.ironcore.dev/v1alpha1",
+			Kind:       "ServerClaim",
+			Namespace:  claim.Namespace,
+			Name:       claim.Name,
+			UID:        claim.UID,
+		}
+		server.Spec.Power = claim.Spec.Power
+		if err := r.Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to patch claim ref for server: %w", err)
+		}
+		log.V(1).Info("Patched ServerClaim reference on Server", "Server", server.Name, "ServerClaimRef", claim.Name)
 	}
-	log.V(1).Info("Patched ServerClaim reference on Server", "Server", server.Name, "ServerClaimRef", claim.Name)
 
-	if modified, err := r.patchServerClaimPhase(ctx, claim, metalv1alpha1.PhaseBound); err != nil || modified {
+	if _, err := r.patchServerClaimPhase(ctx, claim, metalv1alpha1.PhaseBound); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -196,17 +199,17 @@ func (r *ServerClaimReconciler) patchServerClaimPhase(ctx context.Context, claim
 	return true, nil
 }
 
-func (r *ServerClaimReconciler) patchServerRef(ctx context.Context, claim *metalv1alpha1.ServerClaim, server string) (bool, error) {
+func (r *ServerClaimReconciler) patchServerRef(ctx context.Context, claim *metalv1alpha1.ServerClaim, server *metalv1alpha1.Server) (bool, error) {
 	if claim.Spec.ServerRef == nil {
 		claimBase := claim.DeepCopy()
-		claim.Spec.ServerRef = &v1.LocalObjectReference{Name: server}
+		claim.Spec.ServerRef = &v1.LocalObjectReference{Name: server.Name}
 		if err := r.Patch(ctx, claim, client.MergeFrom(claimBase)); err != nil {
 			return false, err
 		}
 		return true, nil
 	}
 
-	if claim.Spec.ServerRef != nil && claim.Spec.ServerRef.Name == server {
+	if claim.Spec.ServerRef != nil && claim.Spec.ServerRef.Name == server.Name {
 		return false, nil
 	}
 

diff --git a/internal/controller/serverclaim_controller_test.go b/internal/controller/serverclaim_controller_test.go
@@ -46,6 +46,15 @@ var _ = Describe("ServerClaim Controller", func() {
 		Eventually(Get(bmc)).Should(Succeed())
 		DeferCleanup(k8sClient.Delete, bmc)
 
+		By("Ensuring that the BMCSecret will be removed")
+		bmcSecret := &metalv1alpha1.BMCSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: bmc.Name,
+			},
+		}
+		Eventually(Get(bmcSecret)).Should(Succeed())
+		DeferCleanup(k8sClient.Delete, bmcSecret)
+
 		By("Ensuring that the Server resource has been created")
 		server = &metalv1alpha1.Server{
 			ObjectMeta: metav1.ObjectMeta{
@@ -247,6 +256,13 @@ var _ = Describe("ServerClaim Controller", func() {
 			server.Status.State = metalv1alpha1.ServerStateAvailable
 		})).Should(Succeed())
 
+		By("Ensuring that the ServerClaim is bound")
+		Eventually(Object(claim)).Should(SatisfyAll(
+			HaveField("Finalizers", ContainElement(ServerClaimFinalizer)),
+			HaveField("Spec.ServerRef", Equal(&v1.LocalObjectReference{Name: server.Name})),
+			HaveField("Status.Phase", metalv1alpha1.PhaseBound),
+		))
+
 		By("Ensuring that the Server has the correct claim ref")
 		Eventually(Object(server)).Should(SatisfyAll(
 			HaveField("Spec.ServerClaimRef", &v1.ObjectReference{
@@ -258,14 +274,7 @@ var _ = Describe("ServerClaim Controller", func() {
 			}),
 			HaveField("Spec.Power", metalv1alpha1.PowerOff),
 			HaveField("Status.State", metalv1alpha1.ServerStateReserved),
-		))
-
-		By("Ensuring that the ServerClaim is bound")
-		Eventually(Object(claim)).Should(SatisfyAll(
-			HaveField("Finalizers", ContainElement(ServerClaimFinalizer)),
-			HaveField("Status.Phase", metalv1alpha1.PhaseBound),
-			HaveField("Spec.ServerRef", Not(BeNil())),
-			HaveField("Spec.ServerRef.Name", server.Name),
+			HaveField("Status.PowerState", metalv1alpha1.ServerOffPowerState),
 		))
 
 		By("Deleting the ServerClaim")

diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go
@@ -32,8 +32,8 @@ import (
 )
 
 const (
-	pollingInterval      = 50 * time.Millisecond
-	eventuallyTimeout    = 3 * time.Second
+	pollingInterval      = 100 * time.Millisecond
+	eventuallyTimeout    = 5 * time.Second
 	consistentlyDuration = 1 * time.Second
 )
 

diff --git a/internal/probe/probe_suite_test.go b/internal/probe/probe_suite_test.go
@@ -19,8 +19,8 @@ var (
 	probeAgent     *probe.Agent
 	registryServer *registry.Server
 
-	registryAddr = ":5432"
-	registryURL  = "http://localhost:5432"
+	registryAddr = ":15432"
+	registryURL  = "http://localhost:15432"
 	systemUUID   = "1234-5678"
 )