🌱 Refactoring PR for: Remove usage of FailureReason and FailureMessage (baremetal) (#1735)

Author: guettli

.golangci.yaml

@@ -169,6 +169,10 @@ issues:
         - staticcheck
       text: "SA1019: in.(.+) is deprecated"
       path: .*(api|types)\/.*\/conversion.*\.go$
+    - linters:
+        - staticcheck
+      text: "SA1019: .*FailureReason is deprecated"
+      path: pkg/services/baremetal/baremetal/baremetal.go
     - linters:
         - revive
       text: exported (method|function|type|const) (.+) should have comment or be unexported

api/v1beta1/hetznerbaremetalhost_types.go

@@ -40,7 +40,9 @@ const (
 	DeprecatedBareMetalHostFinalizer = "hetznerbaremetalhost.infrastructure.cluster.x-k8s.io"
 
 	// HostAnnotation is the key for an annotation that should go on a HetznerBareMetalMachine to
-	// reference what HetznerBareMetalHost it corresponds to.
+	// reference what HetznerBareMetalHost it corresponds to. The annotation is a string in the
+	// format "namespace/hbmh-name". Note: We should remove the namespace, as cross-namespace
+	// references are not allowed.
 	HostAnnotation = "infrastructure.cluster.x-k8s.io/HetznerBareMetalHost"
 
 	// WipeDiskAnnotation indicates which Disks (WWNs) to erase before provisioning
@@ -120,8 +122,10 @@ const (
 	// RegistrationError is an error condition occurring when the
 	// controller is unable to retrieve information on a specific server via robot.
 	RegistrationError ErrorType = "registration error"
+
 	// PreparationError is an error condition occurring when something fails while preparing host reconciliation.
 	PreparationError ErrorType = "preparation error"
+
 	// ProvisioningError is an error condition occurring when the controller
 	// fails to provision or deprovision the Host.
 	ProvisioningError ErrorType = "provisioning error"
@@ -230,8 +234,9 @@ type HetznerBareMetalHostSpec struct {
 	// +optional
 	ConsumerRef *corev1.ObjectReference `json:"consumerRef,omitempty"`
 
-	// MaintenanceMode indicates that a machine is supposed to be deprovisioned
-	// and won't be selected by any Hetzner bare metal machine.
+	// MaintenanceMode indicates that a machine is supposed to be deprovisioned. The CAPI Machine
+	// will get the cluster.x-k8s.io/remediate-machine annotation, and CAPI will deprovision the
+	// machine. Additionally, the host won't be selected by any Hetzner bare metal machine.
 	MaintenanceMode *bool `json:"maintenanceMode,omitempty"`
 
 	// Description is a human-entered text used to help identify the host.

api/v1beta1/hetznerbaremetalmachine_types.go

@@ -25,7 +25,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/selection"
 	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
 )
 
 const (
@@ -298,8 +297,11 @@ type HetznerBareMetalMachineStatus struct {
 	LastUpdated *metav1.Time `json:"lastUpdated,omitempty"`
 
 	// FailureReason will be set in the event that there is a terminal problem.
+	//
+	// 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"`
+	FailureReason *string `json:"failureReason,omitempty"`
 
 	// FailureMessage will be set in the event that there is a terminal problem.
 	// +optional
@@ -358,7 +360,7 @@ func (hbmm *HetznerBareMetalMachine) SetConditions(conditions clusterv1.Conditio
 }
 
 // SetFailure sets a failure reason and message.
-func (hbmm *HetznerBareMetalMachine) SetFailure(reason capierrors.MachineStatusError, message string) {
+func (hbmm *HetznerBareMetalMachine) SetFailure(reason string, message string) {
 	hbmm.Status.FailureReason = &reason
 	hbmm.Status.FailureMessage = &message
 }

api/v1beta1/hetznerbaremetalmachine_types_test.go

@@ -23,7 +23,6 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/stretchr/testify/require"
-	capierrors "sigs.k8s.io/cluster-api/errors" //nolint:staticcheck // we will handle that, when we update to capi v1.11
 )
 
 var _ = Describe("Test Image.GetDetails", func() {
@@ -179,12 +178,12 @@ var _ = Describe("Test GetImageSuffix", func() {
 
 var _ = Describe("Test SetFailure", func() {
 	bmMachine := HetznerBareMetalMachine{}
-	newFailureMessage := "bad error"
-	newFailureReason := capierrors.CreateMachineError
+	newFailureMessage := "bad failure message"
+	newFailureReason := "bad failure reason"
 
 	It("sets new failure on the machine with existing failure", func() {
 		failureMessage := "first message"
-		failureReason := capierrors.MachineStatusError("first error")
+		failureReason := "first error"
 		bmMachine.Status.FailureMessage = &failureMessage
 		bmMachine.Status.FailureReason = &failureReason
 

api/v1beta1/zz_generated.deepcopy.go

@@ -133,8 +133,9 @@ spec:
                 type: string
               maintenanceMode:
                 description: |-
-                  MaintenanceMode indicates that a machine is supposed to be deprovisioned
-                  and won't be selected by any Hetzner bare metal machine.
+                  MaintenanceMode indicates that a machine is supposed to be deprovisioned. The CAPI Machine
+                  will get the cluster.x-k8s.io/remediate-machine annotation, and CAPI will deprovision the
+                  machine. Additionally, the host won't be selected by any Hetzner bare metal machine.
                 type: boolean
               rootDeviceHints:
                 description: |-

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

@@ -399,8 +399,10 @@ spec:
                   a terminal problem.
                 type: string
               failureReason:
-                description: FailureReason will be set in the event that there is
-                  a terminal problem.
+                description: |-
+                  FailureReason will be set in the event that there is a terminal problem.
+
+                  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
               lastUpdated:
                 description: LastUpdated identifies when this status was last observed.

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

@@ -96,7 +96,7 @@ func (r *HCloudRemediationReconciler) Reconcile(ctx context.Context, req reconci
 
 	key := client.ObjectKey{
 		Name:      machine.Spec.InfrastructureRef.Name,
-		Namespace: machine.Spec.InfrastructureRef.Namespace,
+		Namespace: hcloudRemediation.Namespace,
 	}
 
 	if err := r.Get(ctx, key, hcloudMachine); err != nil {

controllers/hcloudremediation_controller.go

@@ -199,6 +199,9 @@ func (r *HetznerBareMetalHostReconciler) Reconcile(ctx context.Context, req ctrl
 		return res, nil
 	}
 
+	// Case "Delete" was handled in reconcileSelectedStates. From now we know that the host has no
+	// DeletionTimestamp set. But the hbmm could be in Deprovisioning.
+
 	hetznerCluster := &infrav1.HetznerCluster{}
 
 	hetznerClusterName := client.ObjectKey{
@@ -240,9 +243,13 @@ func (r *HetznerBareMetalHostReconciler) Reconcile(ctx context.Context, req ctrl
 	}
 
 	log = log.WithValues("HetznerBareMetalMachine", klog.KObj(hetznerBareMetalMachine))
-
 	ctx = ctrl.LoggerInto(ctx, log)
 
+	// check whether rate limit has been reached and if so, then wait.
+	if wait := reconcileRateLimit(bmHost, r.RateLimitWaitTime); wait {
+		return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
+	}
+
 	// Get Hetzner robot api credentials
 	secretManager := secretutil.NewSecretManager(log, r.Client, r.APIReader)
 	robotCreds, err := getAndValidateRobotCredentials(ctx, req.Namespace, hetznerCluster, secretManager)
@@ -280,11 +287,6 @@ func (r *HetznerBareMetalHostReconciler) Reconcile(ctx context.Context, req ctrl
 		return reconcile.Result{}, fmt.Errorf("failed to create scope: %w", err)
 	}
 
-	// check whether rate limit has been reached and if so, then wait.
-	if wait := reconcileRateLimit(bmHost, r.RateLimitWaitTime); wait {
-		return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
-	}
-
 	return r.reconcile(ctx, hostScope)
 }
 
@@ -321,7 +323,7 @@ func (r *HetznerBareMetalHostReconciler) reconcileSelectedStates(ctx context.Con
 
 		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 
-		// Handle StateDeleting
+	// Handle StateDeleting
 	case infrav1.StateDeleting:
 		if controllerutil.RemoveFinalizer(bmHost, infrav1.HetznerBareMetalHostFinalizer) ||
 			controllerutil.RemoveFinalizer(bmHost, infrav1.DeprecatedBareMetalHostFinalizer) {

controllers/hetznerbaremetalhost_controller.go

@@ -44,8 +44,7 @@ import (
 )
 
 const (
-	bmMachineName = "bm-machine-host-testing"
-	hostName      = "test-host"
+	hostName = "test-host"
 )
 
 func verifyError(host *infrav1.HetznerBareMetalHost, errorType infrav1.ErrorType, errorMessage string) bool {
@@ -62,6 +61,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 	var (
 		host           *infrav1.HetznerBareMetalHost
 		bmMachine      *infrav1.HetznerBareMetalMachine
+		machineName    string
 		hetznerCluster *infrav1.HetznerCluster
 
 		capiCluster *clusterv1.Cluster
@@ -91,6 +91,8 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 
 		hetznerClusterName = utils.GenerateName(nil, "hetzner-cluster-test")
 
+		machineName = utils.GenerateName(nil, "machine")
+
 		capiCluster = &clusterv1.Cluster{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "test1-",
@@ -169,15 +171,15 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 		osSSHClientAfterInstallImage.On("CheckCloudInitLogsForSigTerm").Return(sshclient.Output{})
 		osSSHClientAfterInstallImage.On("ResetKubeadm").Return(sshclient.Output{})
 		osSSHClientAfterInstallImage.On("GetHostName").Return(sshclient.Output{
-			StdOut: infrav1.BareMetalHostNamePrefix + bmMachineName,
+			StdOut: infrav1.BareMetalHostNamePrefix + machineName,
 			StdErr: "",
 			Err:    nil,
 		})
 		osSSHClientAfterInstallImage.On("GetCloudInitOutput").Return(sshclient.Output{StdOut: "dummy content of /var/log/cloud-init-output.log"})
 
 		osSSHClientAfterCloudInit.On("Reboot").Return(sshclient.Output{})
 		osSSHClientAfterCloudInit.On("GetHostName").Return(sshclient.Output{
-			StdOut: infrav1.BareMetalHostNamePrefix + bmMachineName,
+			StdOut: infrav1.BareMetalHostNamePrefix + machineName,
 			StdErr: "",
 			Err:    nil,
 		})
@@ -251,9 +253,9 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 		BeforeEach(func() {
 			capiMachine = &clusterv1.Machine{
 				ObjectMeta: metav1.ObjectMeta{
-					GenerateName: "capi-machine-",
-					Namespace:    testNs.Name,
-					Finalizers:   []string{clusterv1.MachineFinalizer},
+					Name:       machineName,
+					Namespace:  testNs.Name,
+					Finalizers: []string{clusterv1.MachineFinalizer},
 					Labels: map[string]string{
 						clusterv1.ClusterNameLabel: capiCluster.Name,
 					},
@@ -263,7 +265,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 					InfrastructureRef: corev1.ObjectReference{
 						APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
 						Kind:       "HetznerBareMetalMachine",
-						Name:       bmMachineName,
+						Name:       machineName,
 					},
 					FailureDomain: &defaultFailureDomain,
 					Bootstrap: clusterv1.Bootstrap{
@@ -275,7 +277,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 
 			bmMachine = &infrav1.HetznerBareMetalMachine{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      bmMachineName,
+					Name:      machineName,
 					Namespace: testNs.Name,
 					Labels: map[string]string{
 						clusterv1.ClusterNameLabel: capiCluster.Name,
@@ -425,9 +427,9 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 		BeforeEach(func() {
 			capiMachine = &clusterv1.Machine{
 				ObjectMeta: metav1.ObjectMeta{
-					GenerateName: "capi-machine-",
-					Namespace:    testNs.Name,
-					Finalizers:   []string{clusterv1.MachineFinalizer},
+					Name:       machineName,
+					Namespace:  testNs.Name,
+					Finalizers: []string{clusterv1.MachineFinalizer},
 					Labels: map[string]string{
 						clusterv1.ClusterNameLabel: capiCluster.Name,
 					},
@@ -437,7 +439,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 					InfrastructureRef: corev1.ObjectReference{
 						APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
 						Kind:       "HetznerBareMetalMachine",
-						Name:       bmMachineName,
+						Name:       machineName,
 					},
 					FailureDomain: &defaultFailureDomain,
 					Bootstrap: clusterv1.Bootstrap{
@@ -449,7 +451,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 
 			bmMachine = &infrav1.HetznerBareMetalMachine{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      bmMachineName,
+					Name:      machineName,
 					Namespace: testNs.Name,
 					Labels: map[string]string{
 						clusterv1.ClusterNameLabel: capiCluster.Name,
@@ -509,9 +511,9 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 		BeforeEach(func() {
 			capiMachine = &clusterv1.Machine{
 				ObjectMeta: metav1.ObjectMeta{
-					GenerateName: "capi-machine-",
-					Namespace:    testNs.Name,
-					Finalizers:   []string{clusterv1.MachineFinalizer},
+					Name:       machineName,
+					Namespace:  testNs.Name,
+					Finalizers: []string{clusterv1.MachineFinalizer},
 					Labels: map[string]string{
 						clusterv1.ClusterNameLabel: capiCluster.Name,
 					},
@@ -521,7 +523,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 					InfrastructureRef: corev1.ObjectReference{
 						APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
 						Kind:       "HetznerBareMetalMachine",
-						Name:       bmMachineName,
+						Name:       machineName,
 					},
 					FailureDomain: &defaultFailureDomain,
 					Bootstrap: clusterv1.Bootstrap{
@@ -533,7 +535,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 
 			bmMachine = &infrav1.HetznerBareMetalMachine{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      bmMachineName,
+					Name:      machineName,
 					Namespace: testNs.Name,
 					Labels: map[string]string{
 						clusterv1.ClusterNameLabel: capiCluster.Name,
@@ -594,6 +596,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 	var (
 		host           *infrav1.HetznerBareMetalHost
 		bmMachine      *infrav1.HetznerBareMetalMachine
+		machineName    string
 		hetznerCluster *infrav1.HetznerCluster
 		capiCluster    *clusterv1.Cluster
 		capiMachine    *clusterv1.Machine
@@ -619,6 +622,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		hetznerClusterName = utils.GenerateName(nil, "hetzner-cluster-test")
+		machineName = utils.GenerateName(nil, "machine")
 
 		capiCluster = &clusterv1.Cluster{
 			ObjectMeta: metav1.ObjectMeta{
@@ -657,9 +661,9 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 
 		capiMachine = &clusterv1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				GenerateName: "capi-machine-",
-				Namespace:    testNs.Name,
-				Finalizers:   []string{clusterv1.MachineFinalizer},
+				Name:       machineName,
+				Namespace:  testNs.Name,
+				Finalizers: []string{clusterv1.MachineFinalizer},
 				Labels: map[string]string{
 					clusterv1.ClusterNameLabel: capiCluster.Name,
 				},
@@ -669,7 +673,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 				InfrastructureRef: corev1.ObjectReference{
 					APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
 					Kind:       "HetznerBareMetalMachine",
-					Name:       bmMachineName,
+					Name:       machineName,
 				},
 				FailureDomain: &defaultFailureDomain,
 				Bootstrap: clusterv1.Bootstrap{
@@ -681,7 +685,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 
 		bmMachine = &infrav1.HetznerBareMetalMachine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      bmMachineName,
+				Name:      machineName,
 				Namespace: testNs.Name,
 				Labels: map[string]string{
 					clusterv1.ClusterNameLabel: capiCluster.Name,