From f0bf45015615e330e30ef4d53193395598524dec Mon Sep 17 00:00:00 2001 From: Andreas Fritzler Date: Thu, 15 Aug 2024 15:23:11 +0200 Subject: [PATCH] Address sporadic in ServerClaim test --- internal/controller/server_controller.go | 6 ++- internal/controller/server_controller_test.go | 12 ++++++ internal/controller/serverclaim_controller.go | 37 ++++++++++--------- .../controller/serverclaim_controller_test.go | 25 +++++++++---- internal/controller/suite_test.go | 4 +- internal/probe/probe_suite_test.go | 4 +- 6 files changed, 58 insertions(+), 30 deletions(-) diff --git a/internal/controller/server_controller.go b/internal/controller/server_controller.go index 9c5b232..65ccb5a 100644 --- 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 index cc14f19..aa0ac37 100644 --- 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 index 034bfbc..8c024bd 100644 --- a/internal/controller/serverclaim_controller.go +++ b/internal/controller/serverclaim_controller.go @@ -153,7 +153,7 @@ 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") @@ -161,22 +161,25 @@ func (r *ServerClaimReconciler) reconcile(ctx context.Context, log logr.Logger, 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 index dd96946..64577ad 100644 --- 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 index da35aca..80c092d 100644 --- 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 index 3190e19..0b4c959 100644 --- 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" )