🌱 Remove usage of FailureReason and FailureMessage (baremetal) (#1716)

* 🌱 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

.gitignore

@@ -36,6 +36,7 @@ test/e2e/data/infrastructure-hetzner/v1beta1/env*
 /tilt.d
 tilt_config.json
 baremetalhosts.yaml
+bm-*.yaml
 
 # kubeconfigs
 ./kind.kubeconfig

api/v1beta1/hetznerbaremetalhost_types.go

@@ -342,6 +342,11 @@ func (sts ControllerGeneratedStatus) GetIPAddress() string {
 	return sts.IPv4
 }
 
+// HasFatalError returns true, if the corresponding capi machine should get deleted.
+func (sts ControllerGeneratedStatus) HasFatalError() bool {
+	return sts.ErrorType == FatalError || sts.ErrorType == PermanentError
+}
+
 // GetConditions returns the observations of the operational state of the HetznerBareMetalHost resource.
 func (host *HetznerBareMetalHost) GetConditions() clusterv1.Conditions {
 	return host.Spec.Status.Conditions

api/v1beta1/hetznerbaremetalmachine_types.go

@@ -359,12 +359,6 @@ func (hbmm *HetznerBareMetalMachine) SetConditions(conditions clusterv1.Conditio
 	hbmm.Status.Conditions = conditions
 }
 
-// SetFailure sets a failure reason and message.
-func (hbmm *HetznerBareMetalMachine) SetFailure(reason string, message string) {
-	hbmm.Status.FailureReason = &reason
-	hbmm.Status.FailureMessage = &message
-}
-
 // GetImageSuffix tests whether the suffix is known and outputs it if yes. Otherwise it returns an error.
 func GetImageSuffix(url string) (string, error) {
 	if strings.HasPrefix(url, "oci://") {

api/v1beta1/hetznerbaremetalmachine_types_test.go

@@ -176,35 +176,6 @@ var _ = Describe("Test GetImageSuffix", func() {
 	)
 })
 
-var _ = Describe("Test SetFailure", func() {
-	bmMachine := HetznerBareMetalMachine{}
-	newFailureMessage := "bad failure message"
-	newFailureReason := "bad failure reason"
-
-	It("sets new failure on the machine with existing failure", func() {
-		failureMessage := "first message"
-		failureReason := "first error"
-		bmMachine.Status.FailureMessage = &failureMessage
-		bmMachine.Status.FailureReason = &failureReason
-
-		bmMachine.SetFailure(newFailureReason, newFailureMessage)
-
-		Expect(bmMachine.Status.FailureMessage).ToNot(BeNil())
-		Expect(bmMachine.Status.FailureReason).ToNot(BeNil())
-		Expect(*bmMachine.Status.FailureMessage).To(Equal(newFailureMessage))
-		Expect(*bmMachine.Status.FailureReason).To(Equal(newFailureReason))
-	})
-
-	It("sets new failure on the machine without existing failure", func() {
-		bmMachine.SetFailure(newFailureReason, newFailureMessage)
-
-		Expect(bmMachine.Status.FailureMessage).ToNot(BeNil())
-		Expect(bmMachine.Status.FailureReason).ToNot(BeNil())
-		Expect(*bmMachine.Status.FailureMessage).To(Equal(newFailureMessage))
-		Expect(*bmMachine.Status.FailureReason).To(Equal(newFailureReason))
-	})
-})
-
 var _ = Describe("Test HasHostAnnotation", func() {
 	type testCaseHasHostAnnotation struct {
 		annotations map[string]string

controllers/controllers_suite_test.go

@@ -17,8 +17,10 @@ limitations under the License.
 package controllers
 
 import (
+	"context"
 	"fmt"
 	"os"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -34,6 +36,7 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/metrics"
 
@@ -415,3 +418,21 @@ func isPresentAndTrue(key types.NamespacedName, getter conditions.Getter, condit
 	objectCondition := conditions.Get(getter, condition)
 	return objectCondition.Status == corev1.ConditionTrue
 }
+
+func hasEvent(ctx context.Context, c client.Client, namespace, involvedObjectName, reason, message string) bool {
+	eventList := &corev1.EventList{}
+	if err := c.List(ctx, eventList, client.InNamespace(namespace)); err != nil {
+		return false
+	}
+
+	for i := range eventList.Items {
+		event := eventList.Items[i]
+		if event.Reason == reason &&
+			event.InvolvedObject.Name == involvedObjectName &&
+			strings.Contains(event.Message, message) {
+			return true
+		}
+	}
+
+	return false
+}

controllers/hetznerbaremetalmachine_controller_test.go

@@ -37,9 +37,12 @@ import (
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1"
@@ -480,7 +483,7 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 				}, timeout, time.Second).Should(BeTrue())
 			})
 
-			It("sets a failure reason when maintenance mode is set on the host", func() {
+			It("sets RemediateMachineAnnotation when maintenance mode is set on the host", func() {
 				By("making sure that machine is ready")
 
 				Eventually(func() bool {
@@ -500,14 +503,85 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 
 				Expect(ph.Patch(ctx, host, patch.WithStatusObservedGeneration{})).To(Succeed())
 
-				By("checking that failure message is set on machine")
+				By("checking that RemediateMachineAnnotation is set on machine")
 
-				Eventually(func() bool {
+				Eventually(func() error {
 					if err := testEnv.Get(ctx, key, bmMachine); err != nil {
-						return false
+						return err
 					}
-					return bmMachine.Status.FailureMessage != nil && *bmMachine.Status.FailureMessage == baremetal.FailureMessageMaintenanceMode
-				}, timeout).Should(BeTrue())
+
+					capiMachine, err := util.GetOwnerMachine(ctx, testEnv, bmMachine.ObjectMeta)
+					if err != nil {
+						return err
+					}
+
+					_, exists := capiMachine.Annotations[clusterv1.RemediateMachineAnnotation]
+					if !exists {
+						return fmt.Errorf("RemediateMachineAnnotation not set on capi machine")
+					}
+
+					if !hasEvent(ctx, testEnv, testNs.Name, bmMachine.Name, "MachineWillBeDeleted",
+						baremetal.FailureMessageMaintenanceMode) {
+						return fmt.Errorf("Event not found")
+					}
+
+					return nil
+				}, timeout).Should(Succeed())
+
+				By("Do the job of CAPI: Create a HetznerBareMetalRemediation")
+				rem := &infrav1.HetznerBareMetalRemediation{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      bmMachine.Name,
+						Namespace: bmMachine.Namespace,
+					},
+				}
+				err = controllerutil.SetOwnerReference(capiMachine, rem, testEnv.Scheme())
+				Expect(err).To(BeNil())
+				err = testEnv.Create(ctx, rem)
+				Expect(err).To(BeNil())
+
+				By("Wait for MachineOwnerRemediatedCondition to be set on capiMachine")
+				Eventually(func() error {
+					capiMachine, err = util.GetOwnerMachine(ctx, testEnv, bmMachine.ObjectMeta)
+					if err != nil {
+						return err
+					}
+					if capiMachine == nil {
+						return fmt.Errorf("capiMachine is nil")
+					}
+
+					err = testEnv.Get(ctx, client.ObjectKeyFromObject(bmMachine), bmMachine)
+					Expect(err).ShouldNot(HaveOccurred())
+
+					Expect(capiMachine.Name).To(Equal(bmMachine.Name))
+					Expect(capiMachine.Spec.InfrastructureRef.Name).To(Equal(bmMachine.Name))
+
+					c := conditions.Get(capiMachine, clusterv1.MachineOwnerRemediatedCondition)
+					if c == nil {
+						return fmt.Errorf("condition MachineOwnerRemediatedCondition does not exist %+v", capiMachine.Status)
+					}
+
+					if c.Status != corev1.ConditionFalse {
+						return fmt.Errorf("condition MachineOwnerRemediatedCondition should be False")
+					}
+
+					if c.Message != "Remediation finished (machine will be deleted): exit remediation because host is in maintenance mode" {
+						return fmt.Errorf("Unexpected message: %q", c.Message)
+					}
+
+					return nil
+				}, timeout).Should(Succeed())
+
+				By("Play role of CAPI: Delete capi and bm machine")
+				err = testEnv.Delete(ctx, capiMachine)
+				Expect(err).Should(Succeed())
+				err = testEnv.Delete(ctx, bmMachine)
+				Expect(err).Should(Succeed())
+
+				By("waiting for the successful kubeadm reset event")
+				Eventually(func() bool {
+					return hasEvent(ctx, testEnv, testNs.Name, host.Name, "SuccessfulResetKubeAdm", "Reset was successful.")
+				}, timeout, time.Second).Should(BeTrue())
 			})
 
 			It("checks the hetznerBareMetalMachine status running phase", func() {

pkg/scope/baremetalmachine.go

@@ -25,6 +25,7 @@ import (
 	"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"
@@ -123,3 +124,31 @@ func (m *BareMetalMachineScope) IsControlPlane() bool {
 func (m *BareMetalMachineScope) IsBootstrapReady() bool {
 	return m.Machine.Spec.Bootstrap.DataSecretName != nil
 }
+
+// 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 use HasFatalError() 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 hbmm/hbmh controller has no permission to delete a capi machine. That's why this
+// extra step (via remediate-machine annotation) is needed.
+func (m *BareMetalMachineScope) SetRemediateMachineAnnotationToDeleteMachine(ctx context.Context, message string) error {
+	capiMachine := m.Machine
+
+	// 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 := m.Client.Patch(ctx, capiMachine, patch); err != nil {
+		return err
+	}
+
+	record.Warnf(m.BareMetalMachine, "MachineWillBeDeleted", "Machine will be deleted: %s", message)
+	return nil
+}

pkg/services/baremetal/baremetal/baremetal.go

@@ -113,10 +113,18 @@ func (s *Service) Reconcile(ctx context.Context) (res reconcile.Result, err erro
 	conditions.MarkTrue(s.scope.BareMetalMachine, infrav1.HostAssociateSucceededCondition)
 
 	// update the machine
-	if err := s.update(ctx); err != nil {
+	host, err := s.update(ctx)
+	if err != nil {
 		return checkForRequeueError(err, "failed to update machine")
 	}
 
+	if host.Spec.Status.HasFatalError() {
+		// hbmm will be deleted soon.
+		s.scope.BareMetalMachine.Status.Phase = clusterv1.MachinePhaseDeleting
+		s.scope.BareMetalMachine.Status.Ready = false
+		return reconcile.Result{}, nil
+	}
+
 	// set providerID if necessary
 	if err := s.setProviderID(ctx); err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed to set providerID: %w", err)
@@ -204,14 +212,19 @@ func (s *Service) Delete(ctx context.Context) (reconcile.Result, error) {
 }
 
 // update updates a machine and is invoked by the Machine Controller.
-func (s *Service) update(ctx context.Context) error {
+func (s *Service) update(ctx context.Context) (*infrav1.HetznerBareMetalHost, error) {
 	host, helper, err := s.getAssociatedHostAndPatchHelper(ctx)
 	if err != nil {
-		return fmt.Errorf("failed to get host: %w", err)
+		if apierrors.IsNotFound(err) {
+			msg := fmt.Sprintf("host not found for machine %q. Setting error and deleting machine", s.scope.Machine.Name)
+			err = s.scope.SetRemediateMachineAnnotationToDeleteMachine(ctx, msg)
+			return nil, err
+		}
+		return nil, fmt.Errorf("failed to get host: %w", err)
 	}
 	if host == nil {
-		s.scope.BareMetalMachine.SetFailure("UpdateError", "host not found")
-		return fmt.Errorf("host not found for machine %s: %w", s.scope.Machine.Name, err)
+		err = errors.Join(s.scope.SetRemediateMachineAnnotationToDeleteMachine(ctx, "Reconcile of hbmm: host not found"))
+		return nil, fmt.Errorf("host not found for machine %s: %w", s.scope.Machine.Name, err)
 	}
 
 	readyCondition := conditions.Get(host, clusterv1.ReadyCondition)
@@ -232,28 +245,21 @@ func (s *Service) update(ctx context.Context) error {
 	}
 
 	// maintenance mode on the host is a fatal error for the machine object
-	if host.Spec.MaintenanceMode != nil && *host.Spec.MaintenanceMode && s.scope.BareMetalMachine.Status.FailureReason == nil {
-		s.scope.BareMetalMachine.SetFailure("UpdateError", FailureMessageMaintenanceMode)
-		record.Eventf(
-			s.scope.BareMetalMachine,
-			"BareMetalMachineSetFailure",
-			"set failure reason due to maintenance mode of underlying host",
-		)
-		return nil
-	}
-
-	// if host has a fatal error, then it should be set on the machine object as well
-	if (host.Spec.Status.ErrorType == infrav1.FatalError || host.Spec.Status.ErrorType == infrav1.PermanentError) &&
-		s.scope.BareMetalMachine.Status.FailureReason == nil {
-		s.scope.BareMetalMachine.SetFailure("UpdateError", host.Spec.Status.ErrorMessage)
-		record.Eventf(s.scope.BareMetalMachine, "BareMetalMachineSetFailure", host.Spec.Status.ErrorMessage)
-		return nil
+	if host.Spec.MaintenanceMode != nil && *host.Spec.MaintenanceMode {
+		err := s.scope.SetRemediateMachineAnnotationToDeleteMachine(ctx, FailureMessageMaintenanceMode)
+		if err != nil {
+			return nil, err
+		}
+		return host, nil
 	}
 
-	// if host is healthy, the machine is healthy as well
-	if host.Spec.Status.ErrorType == infrav1.ErrorType("") {
-		s.scope.BareMetalMachine.Status.FailureMessage = nil
-		s.scope.BareMetalMachine.Status.FailureReason = nil
+	// if host has a fatal error, then it should be set on the hbmm object as well
+	if host.Spec.Status.HasFatalError() {
+		err := s.scope.SetRemediateMachineAnnotationToDeleteMachine(ctx, host.Spec.Status.ErrorMessage)
+		if err != nil {
+			return nil, err
+		}
+		return host, nil
 	}
 
 	// ensure that the references are correctly set on host
@@ -266,13 +272,13 @@ func (s *Service) update(ctx context.Context) error {
 	ensureClusterLabel(host, s.scope.Machine.Spec.ClusterName)
 
 	if err := analyzePatchError(helper.Patch(ctx, host), false); err != nil {
-		return fmt.Errorf("failed to patch host: %w", err)
+		return nil, fmt.Errorf("failed to patch host: %w", err)
 	}
 
 	// if machine is a control plane, the host should be set as target of load balancer
 	if s.scope.IsControlPlane() {
 		if err := s.reconcileLoadBalancerAttachment(ctx, host); err != nil {
-			return fmt.Errorf("failed to reconcile load balancer attachment: %w", err)
+			return nil, fmt.Errorf("failed to reconcile load balancer attachment: %w", err)
 		}
 	}
 
@@ -281,7 +287,7 @@ func (s *Service) update(ctx context.Context) error {
 
 	// update status of HetznerBareMetalMachine with infos from host
 	s.updateMachineAddresses(host)
-	return nil
+	return host, nil
 }
 
 func (s *Service) associate(ctx context.Context) error {
@@ -661,8 +667,11 @@ func (s *Service) setProviderID(ctx context.Context) error {
 	}
 
 	if host == nil {
-		s.scope.BareMetalMachine.SetFailure("UpdateError", "host not found")
-		return fmt.Errorf("host not found for machine %s: %w", s.scope.Machine.Name, err)
+		err := s.scope.SetRemediateMachineAnnotationToDeleteMachine(ctx, "setProviderID failed: host not found")
+		if err != nil {
+			return err
+		}
+		return fmt.Errorf("host not found for machine %q", s.scope.Machine.Name)
 	}
 
 	if host.Spec.Status.ProvisioningState != infrav1.StateProvisioned {