fix: report a missing instance as a final state

When the server of an already provisioned machine was gone, the reconcile
used the reason InstanceError and returned an error, so the controller
retried for ever and wrote an error line for a state that no retry changes.

The provider now reports that state with its own error value. The reconcile turns
it into the reason InstanceNotFound on the conditions and into a warning event,
and it gives no error back to the controller runtime, so the retry stops. It
still creates no replacement server.

test: cover the error return of the machine reconcile

The regression test for the gone server asserted this before. It now asserts the opposite,
so a rejected VM creation covers it instead.

test: cover the deletion of a machine whose server is gone

The spec drives a provisioned control plane machine into the terminal
state of a missing instance, then deletes it while the cloud answers the
server delete with not found. The load balancer target and the finalizer
must go.

The spec is the fourth caller of updateMachineControlPlaneLabel, so
unparam reports the namespace parameter that always receives "default".
The parameter has been refactored away, and the helper writes the
namespace like the helper above it.

test: cover a failed server delete during machine deletion

The delete path removes the finalizer only after the cloud reports the
server as deleted or as already gone. The other exit had no spec: a
deletion that answers with any other error must keep the object.

The new spec injects a transient error into the server delete, then
asserts that the reconcile returns that error, that the server is still
there, and that the finalizer still holds the object, so the next
attempt can find the instance ID.

test: drop the excessive comment of the failed delete spec

Author: corioliskraft

controller/controller_test_helpers_test.go

@@ -130,9 +130,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{}
 	}

controller/stackitmachine_controller_test.go

@@ -154,8 +154,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")
@@ -164,7 +165,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")
 	})
@@ -328,9 +330,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)
@@ -350,7 +368,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)
@@ -367,7 +385,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)
@@ -416,6 +434,63 @@ var _ = Describe("StackitMachine Controller", func() {
 			"the server was created while the API server had no finalizer to clean it up")
 	})
 
+	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())
+	})
+
 	// A lost status patch leaves a running, tagged server that no field on the
 	// object names, so an empty status.instanceID is not proof that none exists.
 	It("deletes a tagged server whose instance ID was lost from the status", func() {
@@ -442,7 +517,6 @@ var _ = Describe("StackitMachine Controller", func() {
 
 		_, err = reconciler.Reconcile(ctx, request)
 		Expect(err).NotTo(HaveOccurred())
-
 		Expect(fakeCloud.ServerCount()).To(Equal(0),
 			"the tagged server leaked because deletion trusted the empty status")
 		Eventually(func() bool {
@@ -451,6 +525,28 @@ var _ = Describe("StackitMachine Controller", func() {
 		}).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())

controller/stackitmachine_infrastructure.go

@@ -12,6 +12,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -32,6 +33,8 @@ import (
 	"github.com/stackitcloud/cluster-api-provider-stackit/util"
 )
 
+var errInstanceGone = errors.New("instance gone")
+
 func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineScope *scope.MachineScope) (ctrl.Result, error) {
 	log := logf.FromContext(ctx)
 	stackitMachine := machineScope.StackitMachine
@@ -83,6 +86,21 @@ func (r *StackitMachineReconciler) reconcileNormal(ctx context.Context, machineS
 	machineScope.SetConditions(metav1.ConditionTrue, "Available", "", infrav1.MachineCredentialsReadyCondition)
 
 	server, created, err := r.ensureServer(ctx, cloudClient, machineScope, bootstrapData)
+	if errors.Is(err, errInstanceGone) {
+		machineScope.SetNotReady(
+			"InstanceNotFound",
+			err.Error(),
+			infrav1.MachineInstanceReadyCondition,
+			infrav1.MachineReadyCondition,
+		)
+		if r.Recorder != nil {
+			r.Recorder.Eventf(
+				stackitMachine, nil, corev1.EventTypeWarning, "InstanceNotFound", "Reconcile",
+				"Server %s no longer exists; the Machine must be replaced", stackitMachine.Status.InstanceID,
+			)
+		}
+		return ctrl.Result{}, nil
+	}
 	if err != nil {
 		machineScope.SetNotReady(
 			"InstanceError",
@@ -285,8 +303,8 @@ func (r *StackitMachineReconciler) ensureServer(
 	// identity, so surface the loss and let Cluster API replace the Machine.
 	if stackitMachine.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, stackitMachine.Status.InstanceID,
+			"%w: server %s; the Machine must be replaced",
+			errInstanceGone, stackitMachine.Status.InstanceID,
 		)
 	}