diff --git a/controller/controller_test_helpers_test.go b/controller/controller_test_helpers_test.go index fd95fb3..d2a6fad 100644 --- a/controller/controller_test_helpers_test.go +++ b/controller/controller_test_helpers_test.go @@ -133,9 +133,9 @@ func updateMachineBootstrapSecret(ctx context.Context, name, bootstrapSecretName Expect(k8sClient.Update(ctx, machine)).To(Succeed()) } -func updateMachineControlPlaneLabel(ctx context.Context, name, namespace string) { +func updateMachineControlPlaneLabel(ctx context.Context, name string) { machine := &clusterv1.Machine{} - Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, machine)).To(Succeed()) + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: "default"}, machine)).To(Succeed()) if machine.Labels == nil { machine.Labels = map[string]string{} } diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index ed72106..90ce37a 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -157,8 +157,9 @@ var _ = Describe("StackitMachine Controller", func() { Expect(fakeCloud.ServerCount()).To(Equal(0)) By("reconciling again") - _, err = reconciler.Reconcile(ctx, request) - Expect(err).To(HaveOccurred(), "reconcile must surface the missing server instead of papering over it") + result, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) Expect(fakeCloud.CreateServerCalls).To(Equal(1), "a replacement server was created for an already-provisioned machine") @@ -167,7 +168,8 @@ var _ = Describe("StackitMachine Controller", func() { By("reporting a consistent readiness state") degraded := &infrav1.StackitMachine{} Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) - expectCondition(degraded.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(degraded.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") Expect(degraded.Status.Ready).To(BeFalse(), "legacy status.ready must follow the Ready condition, not contradict it") }) @@ -319,9 +321,25 @@ var _ = Describe("StackitMachine Controller", func() { expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") }) + It("returns an error when VM creation is rejected", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + fakeCloud.FailNextCreateServer = fmt.Errorf("create server rejected: %w", cloud.ErrInvalidInput) + + result, err := reconciler.Reconcile(ctx, request) + Expect(err).To(HaveOccurred()) + Expect(result).To(Equal(reconcile.Result{})) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + expectCondition(got.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceError") + expectCondition(got.Status.Conditions, infrav1.MachineReadyCondition, metav1.ConditionFalse, "InstanceError") + }) + It("registers control plane VMs as API server load balancer targets", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName, namespace) + updateMachineControlPlaneLabel(ctx, machineName) enableStackitClusterLoadBalancer(ctx, clusterName, namespace) reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) createBootstrapSecret(ctx, bootstrapName) @@ -341,7 +359,7 @@ var _ = Describe("StackitMachine Controller", func() { It("requeues when load balancer target registration returns a transient error", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName, namespace) + updateMachineControlPlaneLabel(ctx, machineName) createBootstrapSecret(ctx, bootstrapName) loadBalancerID := createAPIServerLoadBalancer(ctx, fakeCloud) updateStackitClusterLoadBalancer(ctx, clusterName, namespace, loadBalancerID) @@ -358,7 +376,7 @@ var _ = Describe("StackitMachine Controller", func() { It("deletes the VM and removes the finalizer", func() { updateMachineBootstrapSecret(ctx, machineName, bootstrapName) - updateMachineControlPlaneLabel(ctx, machineName, namespace) + updateMachineControlPlaneLabel(ctx, machineName) enableStackitClusterLoadBalancer(ctx, clusterName, namespace) reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) createBootstrapSecret(ctx, bootstrapName) @@ -386,6 +404,85 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + It("removes the finalizer when the server is already gone at deletion time", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + updateMachineControlPlaneLabel(ctx, machineName) + enableStackitClusterLoadBalancer(ctx, clusterName, namespace) + reconcileStackitClusterOnce(ctx, clusterName, namespace, fakeCloud) + createBootstrapSecret(ctx, bootstrapName) + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + stackitCluster := &infrav1.StackitCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: clusterName, Namespace: namespace}, stackitCluster)).To(Succeed()) + loadBalancerID := stackitCluster.Status.APIServerLoadBalancerID + Expect(loadBalancerID).NotTo(BeEmpty()) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1)) + + provisioned := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, provisioned)).To(Succeed()) + instanceID := provisioned.Status.InstanceID + Expect(instanceID).NotTo(BeEmpty()) + + By("removing the server behind the provider's back") + Expect(fakeCloud.DeleteServer(ctx, instanceID)).To(Succeed()) + Expect(fakeCloud.ServerCount()).To(Equal(0)) + + By("reconciling into the terminal state") + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + + degraded := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, degraded)).To(Succeed()) + expectCondition(degraded.Status.Conditions, infrav1.MachineInstanceReadyCondition, metav1.ConditionFalse, "InstanceNotFound") + Expect(degraded.Status.InstanceID).To(Equal(instanceID)) + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(1), + "the deletion must still find the load balancer target to remove") + + By("deleting the object while the cloud reports the server as gone") + // The fake deletes an unknown ID without an error, so the not-found answer + // of the cloud must be injected. + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrNotFound) + + Expect(k8sClient.Delete(ctx, degraded)).To(Succeed()) + + _, err = reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + // The reconcile clears FailNextDeleteServer when it calls DeleteServer, + // so a nil field proves the call happened. + Expect(fakeCloud.FailNextDeleteServer).ToNot(HaveOccurred(), + "the machine deletion did not ask the cloud to remove the server") + Expect(fakeCloud.LoadBalancerTargetCount(loadBalancerID)).To(Equal(0)) + Eventually(func() bool { + err := k8sClient.Get(ctx, stackitKey, &infrav1.StackitMachine{}) + return apierrors.IsNotFound(err) + }).Should(BeTrue()) + }) + + It("keeps the finalizer when the server deletion fails", func() { + updateMachineBootstrapSecret(ctx, machineName, bootstrapName) + createBootstrapSecret(ctx, bootstrapName) + + _, err := reconciler.Reconcile(ctx, request) + Expect(err).NotTo(HaveOccurred()) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + + got := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, got)).To(Succeed()) + fakeCloud.FailNextDeleteServer = fmt.Errorf("delete server: %w", cloud.ErrTransient) + + Expect(k8sClient.Delete(ctx, got)).To(Succeed()) + _, err = reconciler.Reconcile(ctx, request) + + Expect(err).To(MatchError(cloud.ErrTransient)) + Expect(fakeCloud.ServerCount()).To(Equal(1)) + stillThere := &infrav1.StackitMachine{} + Expect(k8sClient.Get(ctx, stackitKey, stillThere)).To(Succeed()) + Expect(stillThere.Finalizers).To(ContainElement(infrav1.MachineFinalizer)) + }) + It("maps owning Machine events to StackitMachine reconcile requests", func() { machine := &clusterv1.Machine{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: machineName, Namespace: namespace}, machine)).To(Succeed()) diff --git a/controller/stackitmachine_infrastructure.go b/controller/stackitmachine_infrastructure.go index 863b4e2..2eeef61 100644 --- a/controller/stackitmachine_infrastructure.go +++ b/controller/stackitmachine_infrastructure.go @@ -12,6 +12,7 @@ package controller import ( "context" + "errors" "fmt" "time" @@ -32,6 +33,12 @@ import ( "github.com/stackitcloud/cluster-api-provider-stackit/util" ) +var ( + // If the server was there and is gone, cloud.ErrNotFound shouldn't be used, + // as the state is final. + errInstanceGone = errors.New("instance gone") +) + func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope.MachineScope) (ctrl.Result, error) { log := logf.FromContext(ctx) sm := s.StackitMachine @@ -72,6 +79,16 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, s *scope s.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition) server, created, err := r.ensureServer(ctx, cloudClient, s, bootstrapData) + if errors.Is(err, errInstanceGone) { + s.SetNotReady("InstanceNotFound", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) + if r.Recorder != nil { + r.Recorder.Eventf( + sm, nil, corev1.EventTypeWarning, "InstanceNotFound", "Reconcile", + "Server %s no longer exists; the Machine must be replaced", sm.Status.InstanceID, + ) + } + return ctrl.Result{}, nil + } if err != nil { s.SetNotReady("InstanceError", err.Error(), infrav1.MachineInstanceReadyCondition, infrav1.MachineReadyCondition) return util.CloudFailureResult( @@ -241,8 +258,8 @@ func (r *StackitMachineReconciler) ensureServer( // replace the Machine. if sm.Status.Initialization.Provisioned { return nil, false, fmt.Errorf( - "%w: server %s for already-provisioned machine no longer exists; the Machine must be replaced", - cloud.ErrNotFound, sm.Status.InstanceID, + "%w: server %s; the Machine must be replaced", + errInstanceGone, sm.Status.InstanceID, ) }