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

Issue:
Condition in HetznerBareMetalMachine that mirrors ready condition of associated host
## Description

Have a new condition HostReadyCondition in the HetznerBareMetalMachine that mirrors the "ReadyCondition" of the associated host object.

Please note that mirroring the condition requires to take some decisions:
- if ReadyCondition is not set, then HostReady should also be not set
- if ReadyCondition is set, then the Message and Reason should be copied in case it is false and otherwise HostReady should be set to true. Therefore we need to do an if else statement like if ReadyCondition is true, then set Hostready true. If ReadyCondition is false, then set false. If it is not set at all, then don't set HostReadyCondition

Pull request:
✨ Add new condition for the hetznerbaremetal machine
<!-- If this is your first PR, welcome! Please make sure you read the [contributing guidelines](../CONTRIBUTING.md#contributing-a-patch). -->
<!-- please add an icon to the title of this PR and delete this line and similar ones -->
<!-- the icon will be either ⚠️ (:warning:, major or breaking changes), ✨ (:sparkles:, feature additions), 🐛 (:bug:, patches and bugfixes), 📖 (:book:, documentation or proposals), or 🌱 (:seedling:, minor or other) -->

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #940 

**Special notes for your reviewer**:

_Please confirm that if this PR changes any image versions, then that's the sole change this PR makes._

**TODOs**:

- [ ] squash commits
- [ ] include documentation
- [ ] add unit tests



Patch:
diff --git a/api/v1beta1/conditions_const.go b/api/v1beta1/conditions_const.go
index ffd8d2a7..9f751d5b 100644
--- a/api/v1beta1/conditions_const.go
+++ b/api/v1beta1/conditions_const.go
@@ -98,6 +98,11 @@ const (
 	HCloudCredentialsInvalidReason = "HCloudCredentialsInvalid" // #nosec
 )
 
+const (
+	// HostReadyCondition reports on whether the HetznerBareMetalHost is ready or not.
+	HostReadyCondition clusterv1.ConditionType = "HostReady"
+)
+
 const (
 	// TargetClusterReadyCondition reports on whether the kubeconfig in the target cluster is ready.
 	TargetClusterReadyCondition clusterv1.ConditionType = "TargetClusterReady"
@@ -163,14 +168,12 @@ const (
 	HostAssociateFailedReason = "HostAssociateFailed"
 )
 
-const (
-	// HostProvisionSucceededCondition indicates that a host has been provisioned.
-	HostProvisionSucceededCondition clusterv1.ConditionType = "HostProvisionSucceeded"
-)
-
 // deprecated conditions.
 
 const (
+	// DeprecatedHostProvisionSucceededCondition indicates that a host has been provisioned.
+	DeprecatedHostProvisionSucceededCondition clusterv1.ConditionType = "HostProvisionSucceeded"
+
 	// DeprecatedInstanceReadyCondition reports on current status of the instance. Ready indicates the instance is in a Running state.
 	DeprecatedInstanceReadyCondition clusterv1.ConditionType = "InstanceReady"
 
diff --git a/controllers/hetznerbaremetalhost_controller.go b/controllers/hetznerbaremetalhost_controller.go
index 51e54dd0..793ec3de 100644
--- a/controllers/hetznerbaremetalhost_controller.go
+++ b/controllers/hetznerbaremetalhost_controller.go
@@ -219,15 +219,14 @@ func (r *HetznerBareMetalHostReconciler) getSecrets(
 					clusterv1.ConditionSeverityError,
 					infrav1.ErrorMessageMissingOSSSHSecret,
 				)
-
 				record.Warnf(bmHost, infrav1.OSSSHSecretMissingReason, infrav1.ErrorMessageMissingOSSSHSecret)
-
+				conditions.SetSummary(bmHost)
 				result, err := host.SaveHostAndReturn(ctx, r.Client, bmHost)
 				if result != emptyResult || err != nil {
-					return nil, nil, res, err
+					return nil, nil, result, err
 				}
 
-				return nil, nil, res, nil
+				return nil, nil, reconcile.Result{RequeueAfter: 5 * time.Minute}, nil
 			}
 			return nil, nil, res, fmt.Errorf("failed to get secret: %w", err)
 		}
@@ -245,13 +244,13 @@ func (r *HetznerBareMetalHostReconciler) getSecrets(
 				)
 
 				record.Warnf(bmHost, infrav1.RescueSSHSecretMissingReason, infrav1.ErrorMessageMissingRescueSSHSecret)
-
+				conditions.SetSummary(bmHost)
 				result, err := host.SaveHostAndReturn(ctx, r.Client, bmHost)
 				if result != emptyResult || err != nil {
 					return nil, nil, result, err
 				}
 
-				return nil, nil, res, nil
+				return nil, nil, reconcile.Result{RequeueAfter: 5 * time.Minute}, nil
 			}
 			return nil, nil, res, fmt.Errorf("failed to acquire secret: %w", err)
 		}
@@ -324,7 +323,7 @@ func hetznerSecretErrorResult(
 		)
 
 		record.Warnf(bmHost, infrav1.HetznerSecretUnreachableReason, fmt.Sprintf("%s: %s", infrav1.ErrorMessageMissingHetznerSecret, err.Error()))
-
+		conditions.SetSummary(bmHost)
 		result, err := host.SaveHostAndReturn(ctx, client, bmHost)
 		emptyResult := reconcile.Result{}
 		if result != emptyResult || err != nil {
@@ -345,7 +344,7 @@ func hetznerSecretErrorResult(
 			infrav1.ErrorMessageMissingOrInvalidSecretData,
 		)
 		record.Warnf(bmHost, infrav1.SSHCredentialsInSecretInvalidReason, err.Error())
-
+		conditions.SetSummary(bmHost)
 		return host.SaveHostAndReturn(ctx, client, bmHost)
 	}
 	return ctrl.Result{}, fmt.Errorf("hetznerSecretErrorResult: an unhandled failure occurred: %T %w", err, err)
diff --git a/pkg/services/baremetal/baremetal/baremetal.go b/pkg/services/baremetal/baremetal/baremetal.go
index 5abca0f9..bccac3c7 100644
--- a/pkg/services/baremetal/baremetal/baremetal.go
+++ b/pkg/services/baremetal/baremetal/baremetal.go
@@ -208,6 +208,15 @@ func (s *Service) update(ctx context.Context) error {
 		return fmt.Errorf("host not found for machine %s: %w", s.scope.Machine.Name, err)
 	}
 
+	readyCondition := conditions.Get(host, clusterv1.ReadyCondition)
+	if readyCondition != nil {
+		if readyCondition.Status == corev1.ConditionTrue {
+			conditions.MarkTrue(s.scope.BareMetalMachine, infrav1.HostReadyCondition)
+		} else if readyCondition.Status == corev1.ConditionFalse {
+			conditions.MarkFalse(s.scope.BareMetalMachine, infrav1.HostReadyCondition, readyCondition.Reason, readyCondition.Severity, readyCondition.Message)
+		}
+	}
+
 	// 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(capierrors.UpdateMachineError, FailureMessageMaintenanceMode)
@@ -549,7 +558,6 @@ func (s *Service) getLabelSelector() labels.Selector {
 func (s *Service) setProviderID(ctx context.Context) error {
 	// nothing to do if providerID is set
 	if s.scope.BareMetalMachine.Spec.ProviderID != nil {
-		conditions.MarkTrue(s.scope.BareMetalMachine, infrav1.HostProvisionSucceededCondition)
 		s.scope.BareMetalMachine.Status.Phase = clusterv1.MachinePhaseRunning
 		return nil
 	}
@@ -567,13 +575,6 @@ func (s *Service) setProviderID(ctx context.Context) error {
 
 	if host.Spec.Status.ProvisioningState != infrav1.StateProvisioned {
 		s.scope.BareMetalMachine.Status.Phase = clusterv1.MachinePhaseProvisioning
-		conditions.MarkFalse(
-			s.scope.BareMetalMachine,
-			infrav1.HostProvisionSucceededCondition,
-			infrav1.StillProvisioningReason,
-			clusterv1.ConditionSeverityInfo,
-			"still provisioning - current state %q", host.Spec.Status.ProvisioningState,
-		)
 		// no need for requeue error since host update will trigger a reconciliation
 		return nil
 	}
@@ -581,7 +582,6 @@ func (s *Service) setProviderID(ctx context.Context) error {
 	// set providerID
 	providerID := providerIDFromServerID(host.Spec.ServerID)
 	s.scope.BareMetalMachine.Spec.ProviderID = &providerID
-	conditions.MarkTrue(s.scope.BareMetalMachine, infrav1.HostProvisionSucceededCondition)
 	s.scope.BareMetalMachine.Status.Phase = clusterv1.MachinePhaseRunning
 
 	return nil
diff --git a/pkg/services/baremetal/host/host.go b/pkg/services/baremetal/host/host.go
index 669fa465..b2d0eebb 100644
--- a/pkg/services/baremetal/host/host.go
+++ b/pkg/services/baremetal/host/host.go
@@ -97,6 +97,7 @@ func (s *Service) Reconcile(ctx context.Context) (result reconcile.Result, err e
 	defer func() {
 		// remove deprecated conditions
 		conditions.Delete(s.scope.HetznerBareMetalHost, infrav1.DeprecatedHetznerBareMetalHostReadyCondition)
+		conditions.Delete(s.scope.HetznerBareMetalHost, infrav1.DeprecatedHostProvisionSucceededCondition)
 
 		conditions.SetSummary(s.scope.HetznerBareMetalHost)
 


Test patch:
diff --git a/controllers/hetznerbaremetalmachine_controller_test.go b/controllers/hetznerbaremetalmachine_controller_test.go
index ca43d6dd..b34fe0f1 100644
--- a/controllers/hetznerbaremetalmachine_controller_test.go
+++ b/controllers/hetznerbaremetalmachine_controller_test.go
@@ -54,7 +54,6 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 
 		hetznerSecret   *corev1.Secret
 		rescueSSHSecret *corev1.Secret
-		osSSHSecret     *corev1.Secret
 		bootstrapSecret *corev1.Secret
 
 		key client.ObjectKey
@@ -119,9 +118,6 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 		rescueSSHSecret = helpers.GetDefaultSSHSecret("rescue-ssh-secret", testNs.Name)
 		Expect(testEnv.Create(ctx, rescueSSHSecret)).To(Succeed())
 
-		osSSHSecret = helpers.GetDefaultSSHSecret("os-ssh-secret", testNs.Name)
-		Expect(testEnv.Create(ctx, osSSHSecret)).To(Succeed())
-
 		bootstrapSecret = getDefaultBootstrapSecret(testNs.Name)
 		Expect(testEnv.Create(ctx, bootstrapSecret)).To(Succeed())
 
@@ -167,7 +163,7 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 
 	AfterEach(func() {
 		Expect(testEnv.Cleanup(ctx, testNs, capiCluster, hetznerCluster,
-			hetznerSecret, rescueSSHSecret, osSSHSecret, bootstrapSecret)).To(Succeed())
+			hetznerSecret, rescueSSHSecret, bootstrapSecret)).To(Succeed())
 	})
 
 	Context("Tests with host", func() {
@@ -273,6 +269,8 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 		})
 
 		Context("Basic test", func() {
+			var osSSHSecret *corev1.Secret
+
 			BeforeEach(func() {
 				capiMachine = &clusterv1.Machine{
 					ObjectMeta: metav1.ObjectMeta{
@@ -318,11 +316,14 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 				}
 				Expect(testEnv.Create(ctx, bmMachine)).To(Succeed())
 
+				osSSHSecret = helpers.GetDefaultSSHSecret("os-ssh-secret", testNs.Name)
+				Expect(testEnv.Create(ctx, osSSHSecret)).To(Succeed())
+
 				key = client.ObjectKey{Namespace: testNs.Name, Name: bmMachineName}
 			})
 
 			AfterEach(func() {
-				Expect(testEnv.Cleanup(ctx, capiMachine, bmMachine)).To(Succeed())
+				Expect(testEnv.Cleanup(ctx, capiMachine, bmMachine, osSSHSecret)).To(Succeed())
 			})
 
 			It("creates the bare metal machine", func() {
@@ -370,9 +371,9 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 					return isPresentAndTrue(key, bmMachine, infrav1.HostAssociateSucceededCondition)
 				}, timeout).Should(BeTrue())
 
-				By("checking that the host is provisioned")
+				By("checking that the host is ready")
 				Eventually(func() bool {
-					return isPresentAndTrue(key, bmMachine, infrav1.HostProvisionSucceededCondition)
+					return isPresentAndTrue(key, bmMachine, infrav1.HostReadyCondition)
 				}, timeout).Should(BeTrue())
 
 				By("checking that the bare metal machine is ready")
@@ -444,6 +445,72 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 					return bmMachine.Status.Phase == clusterv1.MachinePhaseRunning
 				}, timeout, time.Second).Should(BeTrue())
 			})
+
+			It("checks that HostReady condition is True for hetznerBareMetalMachine", func() {
+				Eventually(func() bool {
+					return isPresentAndTrue(key, bmMachine, infrav1.HostReadyCondition)
+				}, timeout, time.Second).Should(BeTrue())
+			})
+		})
+
+		Context("Test wrong Host", 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(),
+				}
+				Expect(testEnv.Create(ctx, bmMachine)).To(Succeed())
+
+				key = client.ObjectKey{Namespace: testNs.Name, Name: bmMachineName}
+			})
+
+			AfterEach(func() {
+				Expect(testEnv.Cleanup(ctx, capiMachine, bmMachine)).To(Succeed())
+			})
+
+			It("checks for HostReadyCondition False for hetznerBareMetalMachine with HetznerSecretUnreachableReason", func() {
+				Eventually(func() bool {
+					return isPresentAndFalseWithReason(key, bmMachine, infrav1.HostReadyCondition, infrav1.OSSSHSecretMissingReason)
+				}, timeout, time.Second).Should(BeTrue())
+			})
 		})
 	})