🌱 Remove usage of FailureReason and FailureMessage (hcloud, PR2) (#1770)

* 🌱 Remove usage of FailureReason and FailureMessage (baremetal)

> Deprecated: This field is deprecated and is
> going to be removed when support for v1beta1
> will be dropped. Please see
> https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md
> for more details.

Author: guettli

api/v1beta1/hcloudmachine_types.go

@@ -120,12 +120,18 @@ type HCloudMachineStatus struct {
 	// FailureReason will be set in the event that there is a terminal problem
 	// reconciling the Machine and will contain a succinct value suitable
 	// for machine interpretation.
+	//
+	// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
+	//
 	// +optional
 	FailureReason *capierrors.MachineStatusError `json:"failureReason,omitempty"`
 
 	// FailureMessage will be set in the event that there is a terminal problem
 	// reconciling the Machine and will contain a more verbose string suitable
 	// for logging and human consumption.
+	//
+	// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
+	//
 	// +optional
 	FailureMessage *string `json:"failureMessage,omitempty"`
 

api/v1beta1/types.go

@@ -292,4 +292,8 @@ const (
 
 	// HCloudBootStateOperatingSystemRunning indicates that the server is successfully running.
 	HCloudBootStateOperatingSystemRunning HCloudBootState = "OperatingSystemRunning"
+
+	// HCloudBootStateProvisioningFailed indicates that provisioning failed. The capi machine, and
+	// the hcloud machine will get deleted.
+	HCloudBootStateProvisioningFailed HCloudBootState = "ProvisioningFailed"
 )

config/crd/bases/infrastructure.cluster.x-k8s.io_hcloudmachines.yaml

@@ -262,12 +262,16 @@ spec:
                   FailureMessage will be set in the event that there is a terminal problem
                   reconciling the Machine and will contain a more verbose string suitable
                   for logging and human consumption.
+
+                  Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
                 type: string
               failureReason:
                 description: |-
                   FailureReason will be set in the event that there is a terminal problem
                   reconciling the Machine and will contain a succinct value suitable
                   for machine interpretation.
+
+                  Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
                 type: string
               instanceState:
                 description: InstanceState is the state of the server for this machine.

controllers/hcloudmachine_controller.go

@@ -241,10 +241,12 @@ func (r *HCloudMachineReconciler) Reconcile(ctx context.Context, req reconcile.R
 		return r.reconcileDelete(ctx, machineScope)
 	}
 
-	if hcloudMachine.Status.FailureReason != nil {
-		// This machine will be removed.
+	if hcloudMachine.Status.BootState == infrav1.HCloudBootStateProvisioningFailed {
+		// This hcloud machine will be removed soon.
+		log.Info("hcloudmachine: ProvisioningFailed. Not reconciling this machine.")
 		return reconcile.Result{}, nil
 	}
+
 	return r.reconcileNormal(ctx, machineScope)
 }
 

controllers/hcloudremediation_controller_test.go

@@ -27,10 +27,13 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1"
+	"github.com/syself/cluster-api-provider-hetzner/pkg/scope"
 	hcloudutil "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/util"
 	"github.com/syself/cluster-api-provider-hetzner/pkg/utils"
 )
@@ -147,9 +150,8 @@ var _ = Describe("HCloudRemediationReconciler", func() {
 				},
 			},
 			Spec: infrav1.HCloudMachineSpec{
-				ImageName:          "my-control-plane",
-				Type:               "cpx31",
-				PlacementGroupName: &defaultPlacementGroupName,
+				ImageName: "my-control-plane",
+				Type:      "cpx31",
 			},
 		}
 		Expect(testEnv.Create(ctx, hcloudMachine)).To(Succeed())
@@ -227,14 +229,18 @@ var _ = Describe("HCloudRemediationReconciler", func() {
 			Expect(testEnv.Create(ctx, hcloudRemediation)).To(Succeed())
 
 			By("checking if hcloudRemediation is in deleting phase and capiMachine has the MachineOwnerRemediatedCondition")
-			Eventually(func() bool {
+			Eventually(func() error {
 				if err := testEnv.Get(ctx, hcloudRemediationkey, hcloudRemediation); err != nil {
-					return false
+					return err
 				}
-
-				return hcloudRemediation.Status.Phase == infrav1.PhaseDeleting &&
-					isPresentAndFalseWithReason(capiMachineKey, capiMachine, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason)
-			}, timeout).Should(BeTrue())
+				if hcloudRemediation.Status.Phase != infrav1.PhaseDeleting {
+					return fmt.Errorf("hcloudRemediation.Status.Phase is not infrav1.PhaseDeleting")
+				}
+				if !isPresentAndFalseWithReason(capiMachineKey, capiMachine, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason) {
+					return fmt.Errorf("MachineOwnerRemediatedCondition not set")
+				}
+				return nil
+			}, timeout).Should(Succeed())
 		})
 
 		It("checks that, under normal conditions, a reboot is carried out and retryCount and lastRemediated are set", func() {
@@ -318,5 +324,92 @@ var _ = Describe("HCloudRemediationReconciler", func() {
 					isPresentAndFalseWithReason(capiMachineKey, capiMachine, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason)
 			}, timeout).Should(BeTrue())
 		})
+		It("should delete machine if SetErrorAndRemediate() was called", func() {
+			By("Creating Server")
+
+			hcloudClient := testEnv.HCloudClientFactory.NewClient("dummy-token")
+
+			server, err := hcloudClient.CreateServer(ctx, hcloud.ServerCreateOpts{
+				Name: "myserver",
+			})
+			Expect(err).ShouldNot(HaveOccurred())
+			providerID := hcloudutil.ProviderIDFromServerID(int(server.ID))
+			hcloudMachine.Spec.ProviderID = &providerID
+			err = testEnv.Update(ctx, hcloudMachine)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			By("Call SetRemediateMachineAnnotationToDeleteMachine")
+			Eventually(func() error {
+				err = testEnv.Get(ctx, client.ObjectKeyFromObject(hcloudMachine), hcloudMachine)
+				if err != nil {
+					return err
+				}
+				err = scope.SetRemediateMachineAnnotationToDeleteMachine(ctx, testEnv, capiMachine, hcloudMachine, "test-of-set-error-and-remediate")
+				if err != nil {
+					return err
+				}
+				err = testEnv.Status().Update(ctx, hcloudMachine)
+				if err != nil {
+					return err
+				}
+				return nil
+			}).Should(Succeed())
+
+			By("Wait until HCloudBootStateProvisioningFailed is set.")
+			Eventually(func() error {
+				err := testEnv.Get(ctx, client.ObjectKeyFromObject(hcloudMachine), hcloudMachine)
+				if err != nil {
+					return err
+				}
+				if hcloudMachine.Status.BootState != infrav1.HCloudBootStateProvisioningFailed {
+					return fmt.Errorf("BootState is not HCloudBootStateProvisioningFailed, but %q",
+						hcloudMachine.Status.BootState)
+				}
+				return nil
+			}).Should(Succeed())
+
+			By("Do the job of CAPI: Create a HCloudRemediation")
+			rem := &infrav1.HCloudRemediation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      hcloudMachine.Name,
+					Namespace: hcloudMachine.Namespace,
+				},
+				Spec: infrav1.HCloudRemediationSpec{
+					Strategy: &infrav1.RemediationStrategy{
+						Type:       infrav1.RemediationTypeReboot,
+						RetryLimit: 5,
+						Timeout: &metav1.Duration{
+							Duration: time.Minute,
+						},
+					},
+				},
+			}
+
+			err = controllerutil.SetOwnerReference(capiMachine, rem, testEnv.GetScheme())
+			Expect(err).Should(Succeed())
+
+			err = testEnv.Create(ctx, rem)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			By("Wait until our remediation controller has set condition on capi machine")
+			Eventually(func() error {
+				err := testEnv.Get(ctx, client.ObjectKeyFromObject(capiMachine), capiMachine)
+				if err != nil {
+					return err
+				}
+
+				c := conditions.Get(capiMachine, clusterv1.MachineOwnerRemediatedCondition)
+				if c == nil {
+					return fmt.Errorf("not set: MachineOwnerRemediatedCondition")
+				}
+				if c.Status != corev1.ConditionFalse {
+					return fmt.Errorf("status not set yet")
+				}
+				if c.Message != "Remediation finished (machine will be deleted): exit remediation because infra machine is in BootState ProvisioningFailed (no need to try a reboot)" {
+					return fmt.Errorf("Message is not as expected: %q", c.Message)
+				}
+				return nil
+			}).Should(Succeed())
+		})
 	})
 })

pkg/scope/hcloudremediation.go

@@ -72,33 +72,26 @@ func NewHCloudRemediationScope(params HCloudRemediationScopeParams) (*HCloudReme
 		return nil, fmt.Errorf("failed to init patch helper: %w", err)
 	}
 
-	machinePatchHelper, err := patch.NewHelper(params.Machine, params.Client)
-	if err != nil {
-		return nil, fmt.Errorf("failed to init machine patch helper: %w", err)
-	}
-
 	return &HCloudRemediationScope{
-		Logger:             params.Logger,
-		Client:             params.Client,
-		HCloudClient:       params.HCloudClient,
-		patchHelper:        patchHelper,
-		machinePatchHelper: machinePatchHelper,
-		Machine:            params.Machine,
-		HCloudMachine:      params.HCloudMachine,
-		HCloudRemediation:  params.HCloudRemediation,
+		Logger:            params.Logger,
+		Client:            params.Client,
+		HCloudClient:      params.HCloudClient,
+		patchHelper:       patchHelper,
+		Machine:           params.Machine,
+		HCloudMachine:     params.HCloudMachine,
+		HCloudRemediation: params.HCloudRemediation,
 	}, nil
 }
 
 // HCloudRemediationScope defines the basic context for an actuator to operate upon.
 type HCloudRemediationScope struct {
 	logr.Logger
-	Client             client.Client
-	patchHelper        *patch.Helper
-	machinePatchHelper *patch.Helper
-	HCloudClient       hcloudclient.Client
-	Machine            *clusterv1.Machine
-	HCloudMachine      *infrav1.HCloudMachine
-	HCloudRemediation  *infrav1.HCloudRemediation
+	Client            client.Client
+	patchHelper       *patch.Helper
+	HCloudClient      hcloudclient.Client
+	Machine           *clusterv1.Machine
+	HCloudMachine     *infrav1.HCloudMachine
+	HCloudRemediation *infrav1.HCloudRemediation
 }
 
 // Close closes the current scope persisting the cluster configuration and status.
@@ -126,8 +119,3 @@ func (m *HCloudRemediationScope) ServerIDFromProviderID() (int64, error) {
 func (m *HCloudRemediationScope) PatchObject(ctx context.Context, opts ...patch.Option) error {
 	return m.patchHelper.Patch(ctx, m.HCloudRemediation, opts...)
 }
-
-// PatchMachine persists the machine spec and status.
-func (m *HCloudRemediationScope) PatchMachine(ctx context.Context, opts ...patch.Option) error {
-	return m.machinePatchHelper.Patch(ctx, m.Machine, opts...)
-}

pkg/scope/machine.go

@@ -28,10 +28,11 @@ import (
 
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	capierrors "sigs.k8s.io/cluster-api/errors" //nolint:staticcheck // we will handle that, when we update to capi v1.11
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
+	"sigs.k8s.io/cluster-api/util/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1"
 	secretutil "github.com/syself/cluster-api-provider-hetzner/pkg/secrets"
@@ -126,13 +127,44 @@ func (m *MachineScope) PatchObject(ctx context.Context) error {
 	return m.patchHelper.Patch(ctx, m.HCloudMachine)
 }
 
-// SetError sets the ErrorMessage and ErrorReason fields on the machine and logs
-// the message. It assumes the reason is invalid configuration, since that is
-// currently the only relevant MachineStatusError choice.
-// CAPI will delete the machine and create a new one.
-func (m *MachineScope) SetError(message string, reason capierrors.MachineStatusError) {
-	m.HCloudMachine.Status.FailureMessage = &message
-	m.HCloudMachine.Status.FailureReason = &reason
+// SetErrorAndRemediate sets "cluster.x-k8s.io/remediate-machine" annotation on the corresponding
+// CAPI machine. CAPI will remediate that machine. Additionally, an event of type Warning will be
+// created, and the DeleteMachineSucceededCondition will be set to False on the hcloud-machine. It
+// gets used, when a not-recoverable error happens. Example: hcloud server was deleted by hand in
+// the hcloud UI.
+func (m *MachineScope) SetErrorAndRemediate(ctx context.Context, message string) error {
+	return SetRemediateMachineAnnotationToDeleteMachine(ctx, m.Client, m.Machine, m.HCloudMachine, message)
+}
+
+// SetRemediateMachineAnnotationToDeleteMachine sets "cluster.x-k8s.io/remediate-machine" annotation
+// on the corresponding CAPI machine. This will trigger CAPI to start remediation. Our remediation
+// contoller will inspect BootState to differentiate between a remediate (with reboot) and delete
+// (no reboot gets tried). Finally the capi machine and the infra machine will be deleted.
+//
+// Background: the hcloudmachine controller has no permission to delete a capi machine. That's why
+// this extra step (via remediate-machine annotation) is needed.
+func SetRemediateMachineAnnotationToDeleteMachine(ctx context.Context, crClient client.Client, capiMachine *clusterv1.Machine, hcloudMachine *infrav1.HCloudMachine, message string) error {
+	// Create a patch base
+	patch := client.MergeFrom(capiMachine.DeepCopy())
+
+	// Modify only annotations on the in-memory copy
+	if capiMachine.Annotations == nil {
+		capiMachine.Annotations = map[string]string{}
+	}
+	capiMachine.Annotations[clusterv1.RemediateMachineAnnotation] = ""
+
+	// Apply patch – only the diff (annotations) is sent to the API server
+	if err := crClient.Patch(ctx, capiMachine, patch); err != nil {
+		return fmt.Errorf("patch failed in SetErrorAndRemediate: %w", err)
+	}
+
+	record.Warnf(hcloudMachine,
+		"HCloudMachineWillBeRemediated",
+		"HCloudMachine will be remediated: %s", message)
+
+	hcloudMachine.SetBootState(infrav1.HCloudBootStateProvisioningFailed)
+
+	return nil
 }
 
 // SetRegion sets the region field on the machine.

pkg/services/baremetal/remediation/remediation.go

@@ -194,7 +194,7 @@ func (s *Service) timeUntilNextRemediation(now time.Time) time.Duration {
 }
 
 // setOwnerRemediatedConditionToFailed sets MachineOwnerRemediatedCondition on CAPI machine object
-// that have failed a healthcheck.
+// that have failed a healthcheck. This will make capi delete the capi and baremetal machine.
 func (s *Service) setOwnerRemediatedConditionToFailed(ctx context.Context, msg string) error {
 	capiMachine, err := util.GetOwnerMachine(ctx, s.scope.Client, s.scope.BareMetalRemediation.ObjectMeta)
 	if err != nil {