Repository: syself/cluster-api-provider-hetzner (go)

Issue:
Controller should not pick hbmh with one disk, if swraid=true
/kind bug

if the hetzner-bare-metal-machine-template has swraid=true, then the controller should not pick a hbmh which has only one disk.

Up to now the controller picks the hbmh and after several minutes the process fails because installimage fails. 

You can see the error message of installimage in the Kubernetes event, but it is not immediately visible.

**What did you expect to happen:**

I expect the controller to ignore machines which have only one WWN in the hbmh, if the hetzner-bare-metal-machine-template has swraid=true.

**Anything else you would like to add:**

We should do the opposite check, too:

If the  hetzner-bare-metal-machine-template has swraid=false, then the controller should not pick a hbmh which has two WWNs.

[hbmh docs](https://github.com/syself/cluster-api-provider-hetzner/blob/ad6342c684236dda19d85fd12a06066dbb46ed76/docs/reference/hetzner-bare-metal-host.md?plain=1#L33) 

[hbmmt docs](https://github.com/syself/cluster-api-provider-hetzner/blob/ad6342c684236dda19d85fd12a06066dbb46ed76/docs/reference/hetzner-bare-metal-machine-template.md?plain=1#L47)

Implementation: 

There are two lists of WWNs: rootDeviceHints.raid.wwn and rootDeviceHints.wwn.

The controller should only take a machine wher swraid (from the hbmmt) matches the WWN of hbmh.




Pull request:
:seedling: Skip machines with one WWN, if RAID is desired.

**What this PR does / why we need it**:

Skip machines with one WWN, if RAID is desired.

Fixes #1122 

**Special notes for your reviewer**:

**TODOs**:

- [ ] squash commits
- [ ] include documentation
- [ ] add unit tests



Patch:
diff --git a/go.mod b/go.mod
index 543e623c..47fcca82 100644
--- a/go.mod
+++ b/go.mod
@@ -14,6 +14,7 @@ require (
 	github.com/syself/hrobot-go v0.2.5
 	go.uber.org/zap v1.27.0
 	golang.org/x/crypto v0.20.0
+	golang.org/x/exp v0.0.0-20230905200255-921286631fa9
 	golang.org/x/mod v0.15.0
 	k8s.io/api v0.28.4
 	k8s.io/apimachinery v0.28.4
@@ -110,7 +111,6 @@ require (
 	github.com/subosito/gotenv v1.6.0 // indirect
 	github.com/valyala/fastjson v1.6.4 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
-	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect
 	golang.org/x/net v0.21.0 // indirect
 	golang.org/x/oauth2 v0.14.0 // indirect
 	golang.org/x/sync v0.5.0 // indirect
diff --git a/pkg/services/baremetal/baremetal/baremetal.go b/pkg/services/baremetal/baremetal/baremetal.go
index 4783c5d2..2f030b3b 100644
--- a/pkg/services/baremetal/baremetal/baremetal.go
+++ b/pkg/services/baremetal/baremetal/baremetal.go
@@ -29,6 +29,8 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/hetznercloud/hcloud-go/v2/hcloud"
+	"golang.org/x/exp/maps"
+	"golang.org/x/exp/slices"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -283,19 +285,19 @@ func (s *Service) associate(ctx context.Context) error {
 	}
 
 	// choose new host
-	host, helper, err := s.chooseHost(ctx)
+	host, helper, reason, err := s.chooseHost(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to choose host: %w", err)
 	}
 	if host == nil {
 		s.scope.BareMetalMachine.Status.Phase = clusterv1.MachinePhasePending
-		s.scope.V(1).Info("No available host found. Requeuing.")
+		s.scope.Info("No available host found. Requeuing.", "reason", reason)
 		conditions.MarkFalse(
 			s.scope.BareMetalMachine,
 			infrav1.HostAssociateSucceededCondition,
 			infrav1.NoAvailableHostReason,
 			clusterv1.ConditionSeverityWarning,
-			"no available host",
+			fmt.Sprintf("no available host (%s)", reason),
 		)
 		return &scope.RequeueAfterError{RequeueAfter: requeueAfter}
 	}
@@ -363,7 +365,12 @@ func (s *Service) getAssociatedHost(ctx context.Context) (*infrav1.HetznerBareMe
 	return &host, helper, nil
 }
 
-func (s *Service) chooseHost(ctx context.Context) (*infrav1.HetznerBareMetalHost, *patch.Helper, error) {
+// chooseHost tries to find a free hbmh.
+// If no hbmh was found, then hbmh and err are nil, and the string
+// "reason" contains human readable details.
+func (s *Service) chooseHost(ctx context.Context) (
+	hbmh *infrav1.HetznerBareMetalHost, patchHelper *patch.Helper, reason string, err error,
+) {
 	// get list of hosts scoped to namespace of machine
 	hosts := infrav1.HetznerBareMetalHostList{}
 	opts := &client.ListOptions{
@@ -371,63 +378,132 @@ func (s *Service) chooseHost(ctx context.Context) (*infrav1.HetznerBareMetalHost
 	}
 
 	if err := s.scope.Client.List(ctx, &hosts, opts); err != nil {
-		return nil, nil, fmt.Errorf("failed to list hosts: %w", err)
+		return nil, nil, "", fmt.Errorf("failed to list hosts: %w", err)
 	}
 
 	labelSelector := s.getLabelSelector()
 
+	// count all hosts that are not in use already
+	unusedHostsCounter := 0
+
+	// hosts are "available" if they are not in use already by some Kubernetes cluster and do not have
+	// another reason to not be chosen (labels that don't match the selector, maintenance mode, error state, etc.)
 	availableHosts := make([]*infrav1.HetznerBareMetalHost, 0, len(hosts.Items))
 
+	mapOfSkipReasons := make(map[string]int)
+
 	for i, host := range hosts.Items {
 		if host.Spec.ConsumerRef != nil && consumerRefMatches(host.Spec.ConsumerRef, s.scope.BareMetalMachine) {
 			helper, err := patch.NewHelper(&hosts.Items[i], s.scope.Client)
 			if err != nil {
-				return nil, nil, fmt.Errorf("failed to create patch helper: %w", err)
+				return nil, nil, "", fmt.Errorf("failed to create patch helper: %w", err)
 			}
-			return &hosts.Items[i], helper, nil
+			return &hosts.Items[i], helper, "", nil
 		}
 		if host.Spec.ConsumerRef != nil {
 			continue
 		}
-		if host.Spec.MaintenanceMode != nil && *host.Spec.MaintenanceMode {
+
+		// from now on each "continue" should add an entry
+		// to mapOfSkipReasons.
+		unusedHostsCounter++
+
+		// This comes first, because we should not look too deep into machines
+		// which are not in our scope.
+		if !labelSelector.Matches(labels.Set(host.ObjectMeta.Labels)) {
+			mapOfSkipReasons["label-selector-does-not-match"]++
 			continue
 		}
+
 		if host.GetDeletionTimestamp() != nil {
+			mapOfSkipReasons["hbmh-has-deletion-timestamp"]++
 			continue
 		}
-		if host.Spec.Status.ErrorMessage != "" {
+
+		if host.Spec.MaintenanceMode != nil && *host.Spec.MaintenanceMode {
+			mapOfSkipReasons["hbmh-in-maintenance-mode"]++
 			continue
 		}
-
-		if !labelSelector.Matches(labels.Set(host.ObjectMeta.Labels)) {
+		if host.Spec.Status.ErrorMessage != "" {
+			mapOfSkipReasons["hbmh-has-error-message-in-status"]++
 			continue
 		}
 
 		if host.Spec.Status.ProvisioningState != infrav1.StateNone {
+			mapOfSkipReasons["hbmh-in-wrong-provisioning-state"]++
 			continue
 		}
 
+		// This comes last, because now we would choose the machine, but we check
+		// if the config is correct.
+		if host.Spec.RootDeviceHints != nil &&
+			host.Spec.RootDeviceHints.IsValid() {
+			// Even if there are no rootDeviceHints specified, the host should be picked.
+			// After the phase registering, the process to provision the server stops and
+			// waits for the user to specify the rootDeviceHints.
+			// Here (RootDeviceHints exists and is valid) we want to check whether
+			// the specified rootDeviceHints fit with the InstallImage configuration
+			// of the HetznerBareMetalMachine. If not, it is not valid.
+			// Doing that without first choosing the hbmh would be nice, there is a feature request:
+			// https://github.com/syself/cluster-api-provider-hetzner/issues/1166
+			if s.scope.BareMetalMachine.Spec.InstallImage.Swraid == 1 {
+				// Machine should have RAID. Skip machines which have less than two WWNs
+				lenOfWwnSlice := len(host.Spec.RootDeviceHints.Raid.WWN)
+				if lenOfWwnSlice < 2 {
+					mapOfSkipReasons[fmt.Sprintf("machine-should-use-swraid-but-only-%d-RAID-WWN-in-hbmh", lenOfWwnSlice)]++
+					continue
+				}
+			} else { //nolint:gocritic
+				// Machine should have no RAID.
+				if host.Spec.RootDeviceHints.WWN == "" {
+					mapOfSkipReasons["machine-should-use-no-swraid-and-no-non-raid-WWN-in-hbmh"]++
+					continue
+				}
+			}
+		}
+
 		availableHosts = append(availableHosts, &hosts.Items[i])
 	}
 
+	// return if all hosts are in use with a specific message
+	if unusedHostsCounter == 0 {
+		return nil, nil, fmt.Sprintf("all hosts are in use - found %d hosts",
+			len(hosts.Items)), nil
+	}
+
+	// found hosts that are not in use, but all of them had some reason to not be chosen
 	if len(availableHosts) == 0 {
-		return nil, nil, nil
+		return nil, nil, reasonString(mapOfSkipReasons, unusedHostsCounter), nil
 	}
 
-	// choose a host
+	// we found available hosts - choose one
 	randomNumber, err := rand.Int(rand.Reader, big.NewInt(int64(len(availableHosts))))
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to create random number: %w", err)
+		return nil, nil, "", fmt.Errorf("failed to create random number: %w", err)
 	}
 
 	chosenHost := availableHosts[randomNumber.Int64()]
 
 	helper, err := patch.NewHelper(chosenHost, s.scope.Client)
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to create patch helper: %w", err)
+		return nil, nil, "", fmt.Errorf("failed to create patch helper: %w", err)
 	}
 
-	return chosenHost, helper, nil
+	return chosenHost, helper, "", nil
+}
+
+func reasonString(mapOfSkipReasons map[string]int, unusedHostsCounter int) string {
+	reasons := make([]string, 0, len(mapOfSkipReasons))
+	keys := maps.Keys(mapOfSkipReasons)
+	slices.Sort(keys)
+	for _, key := range keys {
+		value := mapOfSkipReasons[key]
+		if value == 0 {
+			continue
+		}
+		reasons = append(reasons, fmt.Sprintf("%s: %d", key, value))
+	}
+	return fmt.Sprintf("No available host of %d found: %s", unusedHostsCounter, strings.Join(reasons, ", "))
 }
 
 func (s *Service) reconcileLoadBalancerAttachment(ctx context.Context, host *infrav1.HetznerBareMetalHost) error {


Test patch:
diff --git a/controllers/hetznerbaremetalhost_controller_test.go b/controllers/hetznerbaremetalhost_controller_test.go
index 4b3b0e6b..b0fc0f09 100644
--- a/controllers/hetznerbaremetalhost_controller_test.go
+++ b/controllers/hetznerbaremetalhost_controller_test.go
@@ -368,7 +368,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 				Expect(testEnv.Cleanup(ctx, host)).To(Succeed())
 			})
 
-			It("gets selected from a bm machine and provisions", func() {
+			It("gets selected from a bm machine and provisions (context 'provision host')", func() {
 				Eventually(func() bool {
 					if err := testEnv.Get(ctx, key, host); err != nil {
 						return false
@@ -382,12 +382,11 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 			})
 		})
 
-		Context("provision host with rootDeviceHints raid", func() {
+		Context("Test secret owner refs", func() {
 			BeforeEach(func() {
 				host = helpers.BareMetalHost(
 					hostName,
 					testNs.Name,
-					helpers.WithRootDeviceHintRaid(),
 					helpers.WithHetznerClusterRef(hetznerClusterName),
 				)
 				Expect(testEnv.Create(ctx, host)).To(Succeed())
@@ -399,24 +398,84 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 				Expect(testEnv.Cleanup(ctx, host)).To(Succeed())
 			})
 
-			It("gets selected from a bm machine and provisions", func() {
+			It("should create an owner ref of the cluster in the rescue ssh secret", func() {
 				Eventually(func() bool {
-					if err := testEnv.Get(ctx, key, host); err != nil {
+					var secret corev1.Secret
+					secretKey := types.NamespacedName{Name: rescueSSHSecret.Name, Namespace: rescueSSHSecret.Namespace}
+					if err := testEnv.Get(ctx, secretKey, &secret); err != nil {
 						return false
 					}
-					if host.Spec.Status.ProvisioningState == infrav1.StateProvisioned {
-						return true
+
+					for _, owner := range secret.GetOwnerReferences() {
+						if owner.UID == hetznerCluster.GetUID() {
+							return true
+						}
 					}
+
 					return false
-				}, timeout).Should(BeTrue())
+				}, timeout, time.Second).Should(BeTrue())
 			})
 		})
+	})
 
-		Context("Test secret owner refs", func() {
+	Context("Tests with bm machine (with RAID)", func() {
+		BeforeEach(func() {
+			capiMachine = &clusterv1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "capi-machine-",
+					Namespace:    testNs.Name,
+					Finalizers:   []string{clusterv1.MachineFinalizer},
+					Labels: map[string]string{
+						clusterv1.ClusterNameLabel: capiCluster.Name,
+					},
+				},
+				Spec: clusterv1.MachineSpec{
+					ClusterName: capiCluster.Name,
+					InfrastructureRef: corev1.ObjectReference{
+						APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
+						Kind:       "HetznerBareMetalMachine",
+						Name:       bmMachineName,
+					},
+					FailureDomain: &defaultFailureDomain,
+					Bootstrap: clusterv1.Bootstrap{
+						DataSecretName: ptr.To("bootstrap-secret"),
+					},
+				},
+			}
+			Expect(testEnv.Create(ctx, capiMachine)).To(Succeed())
+
+			bmMachine = &infrav1.HetznerBareMetalMachine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      bmMachineName,
+					Namespace: testNs.Name,
+					Labels: map[string]string{
+						clusterv1.ClusterNameLabel: capiCluster.Name,
+					},
+					OwnerReferences: []metav1.OwnerReference{
+						{
+							APIVersion: "cluster.x-k8s.io/v1beta1",
+							Kind:       "Machine",
+							Name:       capiMachine.Name,
+							UID:        capiMachine.UID,
+						},
+					},
+				},
+				Spec: getDefaultHetznerBareMetalMachineSpec(),
+			}
+			bmMachine.Spec.InstallImage.Swraid = 1
+			Expect(testEnv.Create(ctx, bmMachine)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			Expect(testEnv.Cleanup(ctx, capiMachine, bmMachine)).To(Succeed())
+		})
+
+		Context("provision host with rootDeviceHints RAID", func() {
 			BeforeEach(func() {
 				host = helpers.BareMetalHost(
 					hostName,
 					testNs.Name,
+					helpers.WithRootDeviceHintRaid(),
 					helpers.WithHetznerClusterRef(hetznerClusterName),
 				)
 				Expect(testEnv.Create(ctx, host)).To(Succeed())
@@ -428,22 +487,17 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 				Expect(testEnv.Cleanup(ctx, host)).To(Succeed())
 			})
 
-			It("should create an owner ref of the cluster in the rescue ssh secret", func() {
+			It("gets selected from a bm machine and provisions (rootDeviceHints RAID)", func() {
+				Expect(len(host.Spec.RootDeviceHints.Raid.WWN)).To(Equal(2))
 				Eventually(func() bool {
-					var secret corev1.Secret
-					secretKey := types.NamespacedName{Name: rescueSSHSecret.Name, Namespace: rescueSSHSecret.Namespace}
-					if err := testEnv.Get(ctx, secretKey, &secret); err != nil {
+					if err := testEnv.Get(ctx, key, host); err != nil {
 						return false
 					}
-
-					for _, owner := range secret.GetOwnerReferences() {
-						if owner.UID == hetznerCluster.GetUID() {
-							return true
-						}
+					if host.Spec.Status.ProvisioningState == infrav1.StateProvisioned {
+						return true
 					}
-
 					return false
-				}, timeout, time.Second).Should(BeTrue())
+				}, timeout).Should(BeTrue())
 			})
 		})
 	})
@@ -519,7 +573,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 				Expect(testEnv.Cleanup(ctx, host)).To(Succeed())
 			})
 
-			It("gets selected from a bm machine and provisions", func() {
+			It("gets selected from a bm machine and provisions (different ports after installImage)", func() {
 				Eventually(func() bool {
 					if err := testEnv.Get(ctx, key, host); err != nil {
 						return false
diff --git a/pkg/services/baremetal/baremetal/baremetal_test.go b/pkg/services/baremetal/baremetal/baremetal_test.go
index 5288e612..5d652f63 100644
--- a/pkg/services/baremetal/baremetal/baremetal_test.go
+++ b/pkg/services/baremetal/baremetal/baremetal_test.go
@@ -177,6 +177,32 @@ var _ = Describe("chooseHost", func() {
 		},
 	}
 
+	hostWithRaidWwnConfig := infrav1.HetznerBareMetalHost{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "hostWithRaidWwnConfig",
+			Namespace: defaultNamespace,
+		},
+		Spec: infrav1.HetznerBareMetalHostSpec{
+			RootDeviceHints: &infrav1.RootDeviceHints{
+				WWN: "",
+				Raid: infrav1.Raid{
+					WWN: []string{"wwnRaid1", "wwnRaid2"},
+				},
+			},
+		},
+	}
+	hostWithNonRaidWwnConfig := infrav1.HetznerBareMetalHost{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "hostWithNonRaidWwnConfig",
+			Namespace: defaultNamespace,
+		},
+		Spec: infrav1.HetznerBareMetalHostSpec{
+			RootDeviceHints: &infrav1.RootDeviceHints{
+				WWN: "wwnNoRaid",
+			},
+		},
+	}
+
 	hostWithLabel := infrav1.HetznerBareMetalHost{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "hostWithLabel",
@@ -208,6 +234,7 @@ var _ = Describe("chooseHost", func() {
 		Hosts            []client.Object
 		HostSelector     infrav1.HostSelector
 		ExpectedHostName string
+		RootDeviceHints  infrav1.RootDeviceHints
 	}
 	DescribeTable("chooseHost",
 		func(tc testCaseChooseHost) {
@@ -217,8 +244,9 @@ var _ = Describe("chooseHost", func() {
 			bmMachine.Spec.HostSelector = tc.HostSelector
 			service := newTestService(bmMachine, c)
 
-			host, _, err := service.chooseHost(context.TODO())
+			host, _, expectedReason, err := service.chooseHost(context.TODO())
 			Expect(err).To(Succeed())
+			Expect(expectedReason).To(Equal(""))
 			if tc.ExpectedHostName == "" {
 				Expect(host).To(BeNil())
 			} else {
@@ -276,6 +304,50 @@ var _ = Describe("chooseHost", func() {
 				ExpectedHostName: "hostWithLabel",
 			}),
 	)
+
+	type testCaseChooseHostWithReason struct {
+		hosts            []client.Object
+		expectedHostName string
+		expectedReason   string
+		swraid           int
+	}
+
+	DescribeTable("chooseHost(): Test with reason, because RAID config does not match.",
+		func(tc testCaseChooseHostWithReason) {
+			scheme := runtime.NewScheme()
+			utilruntime.Must(infrav1.AddToScheme(scheme))
+			c := fakeclient.NewClientBuilder().WithScheme(scheme).WithObjects(tc.hosts...).Build()
+			bmMachine := &infrav1.HetznerBareMetalMachine{
+				TypeMeta:   metav1.TypeMeta{},
+				ObjectMeta: metav1.ObjectMeta{Name: "bmMachine", Namespace: defaultNamespace},
+				Spec: infrav1.HetznerBareMetalMachineSpec{
+					InstallImage: infrav1.InstallImage{
+						Swraid: tc.swraid,
+					},
+				},
+			}
+			service := newTestService(bmMachine, c)
+
+			host, _, reason, err := service.chooseHost(context.TODO())
+			Expect(err).To(Succeed())
+			Expect(reason).To(Equal(tc.expectedReason))
+			Expect(host).To(BeNil())
+		},
+		Entry("No host, because invalid RAID config (want RAID)",
+			testCaseChooseHostWithReason{
+				hosts:            []client.Object{&hostWithNonRaidWwnConfig},
+				expectedHostName: "",
+				expectedReason:   "No available host of 1 found: machine-should-use-swraid-but-only-0-RAID-WWN-in-hbmh: 1",
+				swraid:           1,
+			}),
+		Entry("No host, because invalid RAID config (want no RAID)",
+			testCaseChooseHostWithReason{
+				hosts:            []client.Object{&hostWithRaidWwnConfig},
+				expectedHostName: "",
+				expectedReason:   "No available host of 1 found: machine-should-use-no-swraid-and-no-non-raid-WWN-in-hbmh: 1",
+				swraid:           0,
+			}),
+	)
 })
 
 var _ = Describe("Test NodeAddresses", func() {