From 1fb877c7480d4a439959fa0b9ac6c2a16c073b5b Mon Sep 17 00:00:00 2001 From: nikolayzakharov Date: Wed, 2 Sep 2026 16:12:28 +0200 Subject: [PATCH 1/5] 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. --- controller/stackitmachine_controller_test.go | 8 +++++--- controller/stackitmachine_infrastructure.go | 21 ++++++++++++++++++-- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index ed72106..6b5ce0f 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") }) 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, ) } From 7611afe27db76b02fde3b96fed97e37759379b0b Mon Sep 17 00:00:00 2001 From: nikolayzakharov Date: Wed, 2 Sep 2026 18:18:46 +0200 Subject: [PATCH 2/5] 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. --- controller/stackitmachine_controller_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 6b5ce0f..0d97551 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -321,6 +321,22 @@ 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) From c17ac7d68eb5bd855daf4abddcdf4de52e46d694 Mon Sep 17 00:00:00 2001 From: nikolayzakharov Date: Fri, 4 Sep 2026 15:39:44 +0200 Subject: [PATCH 3/5] 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. --- controller/controller_test_helpers_test.go | 4 +- controller/stackitmachine_controller_test.go | 63 +++++++++++++++++++- 2 files changed, 62 insertions(+), 5 deletions(-) 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 0d97551..0d520ab 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -339,7 +339,7 @@ var _ = Describe("StackitMachine Controller", func() { 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) @@ -359,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) @@ -376,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) @@ -404,6 +404,63 @@ 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("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()) From e575fe7b4e14bdb80fb1b52fd119e609043d6816 Mon Sep 17 00:00:00 2001 From: nikolayzakharov Date: Sat, 5 Sep 2026 12:02:02 +0200 Subject: [PATCH 4/5] 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. --- controller/stackitmachine_controller_test.go | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 0d520ab..31856b4 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -461,6 +461,33 @@ var _ = Describe("StackitMachine Controller", func() { }).Should(BeTrue()) }) + It("keeps the finalizer when the server deletion fails", func() { + // The delete path removes the finalizer only after the cloud reports the + // server as deleted or as already gone. Any other delete error leaves the + // server running, so the object must stay and the reconcile must retry. + // Without the object, nothing holds the instance ID and the VM keeps + // running unnoticed. + 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()) From 1eb54836bb7c1133fd7526bf5cdfb2279611a4dd Mon Sep 17 00:00:00 2001 From: nikolayzakharov Date: Sat, 5 Sep 2026 12:35:56 +0200 Subject: [PATCH 5/5] test: drop the excessive comment of the failed delete spec --- controller/stackitmachine_controller_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/controller/stackitmachine_controller_test.go b/controller/stackitmachine_controller_test.go index 31856b4..90ce37a 100644 --- a/controller/stackitmachine_controller_test.go +++ b/controller/stackitmachine_controller_test.go @@ -462,11 +462,6 @@ var _ = Describe("StackitMachine Controller", func() { }) It("keeps the finalizer when the server deletion fails", func() { - // The delete path removes the finalizer only after the cloud reports the - // server as deleted or as already gone. Any other delete error leaves the - // server running, so the object must stay and the reconcile must retry. - // Without the object, nothing holds the instance ID and the VM keeps - // running unnoticed. updateMachineBootstrapSecret(ctx, machineName, bootstrapName) createBootstrapSecret(ctx, bootstrapName)