Repository: syself/cluster-api-provider-hetzner
Pull request #901 (2023-09-11 13:09:52)

:seedling: Refactor conditions for HetznerBareMetalHosts
**What this PR does / why we need it**:
Refactoring conditions for HetznerBareMetalHost objects to make them more precise, to improve the namings and to include a summary condition

**TODOs**:
- [x] squash commits
- [ ] include documentation
- [ ] add unit tests




Patch:
diff --git a/api/v1beta1/conditions_const.go b/api/v1beta1/conditions_const.go
index 53760601..3d44af1b 100644
--- a/api/v1beta1/conditions_const.go
+++ b/api/v1beta1/conditions_const.go
@@ -109,8 +109,8 @@ const (
 )
 
 const (
-	// HetznerBareMetalHostReadyCondition reports on whether the Hetzner cluster is in ready state.
-	HetznerBareMetalHostReadyCondition clusterv1.ConditionType = "HetznerBareMetalHostReady"
+	// CredentialsAvailableCondition reports on whether the Hetzner cluster is in ready state.
+	CredentialsAvailableCondition clusterv1.ConditionType = "CredentialsAvailable"
 	// RobotCredentialsInvalidReason indicates that credentials for Robot are invalid.
 	RobotCredentialsInvalidReason = "RobotCredentialsInvalid" // #nosec
 	// SSHCredentialsInSecretInvalidReason indicates that ssh credentials are invalid.
@@ -123,6 +123,17 @@ const (
 	RescueSSHSecretMissingReason = "RescueSSHSecretMissing"
 )
 
+const (
+	// ProvisionSucceededCondition indicates that a host has been provisioned.
+	ProvisionSucceededCondition clusterv1.ConditionType = "ProvisionSucceeded"
+	// StillProvisioningReason indicates that the server is still provisioning.
+	StillProvisioningReason = "StillProvisioning"
+	// SSHConnectionRefusedReason indicates that the server cannot be reached via SSH.
+	SSHConnectionRefusedReason = "SSHConnectionRefused"
+	// RescueSystemUnavailableReason indicates that the server has no rescue system.
+	RescueSystemUnavailableReason = "RescueSystemUnavailable"
+)
+
 const (
 	// HostAssociateSucceededCondition indicates that a host has been associated.
 	HostAssociateSucceededCondition clusterv1.ConditionType = "HostAssociateSucceeded"
@@ -135,27 +146,17 @@ const (
 const (
 	// HostProvisionSucceededCondition indicates that a host has been provisioned.
 	HostProvisionSucceededCondition clusterv1.ConditionType = "HostProvisionSucceeded"
-	// StillProvisioningReason indicates that asssociating a host is still provisioning.
-	StillProvisioningReason = "StillProvisioning"
 )
 
-// deprecated .
+// deprecated conditions.
 
 const (
 	// InstanceReadyCondition reports on current status of the instance. Ready indicates the instance is in a Running state.
 	InstanceReadyCondition clusterv1.ConditionType = "InstanceReady"
+
 	// InstanceBootstrapReadyCondition reports on current status of the instance. BootstrapReady indicates the bootstrap is ready.
 	InstanceBootstrapReadyCondition clusterv1.ConditionType = "InstanceBootstrapReady"
-)
-
-const (
-	// AssociateBMHCondition reports on whether the Hetzner cluster is in ready state.
-	AssociateBMHCondition clusterv1.ConditionType = "AssociateBMHCondition"
-)
 
-// deprecated conditions.
-
-const (
 	// HetznerClusterTargetClusterReadyCondition reports on whether the kubeconfig in the target cluster is ready.
 	HetznerClusterTargetClusterReadyCondition clusterv1.ConditionType = "HetznerClusterTargetClusterReady"
 
@@ -164,4 +165,10 @@ const (
 
 	// LoadBalancerAttachedToNetworkCondition reports on whether the load balancer is attached to a network.
 	LoadBalancerAttachedToNetworkCondition clusterv1.ConditionType = "LoadBalancerAttachedToNetwork"
+
+	// HetznerBareMetalHostReadyCondition reports on whether the Hetzner cluster is in ready state.
+	HetznerBareMetalHostReadyCondition clusterv1.ConditionType = "HetznerBareMetalHostReady"
+
+	// AssociateBMHCondition reports on whether the Hetzner cluster is in ready state.
+	AssociateBMHCondition clusterv1.ConditionType = "AssociateBMHCondition"
 )
diff --git a/controllers/hetznerbaremetalhost_controller.go b/controllers/hetznerbaremetalhost_controller.go
index f0d13bfe..72a22514 100644
--- a/controllers/hetznerbaremetalhost_controller.go
+++ b/controllers/hetznerbaremetalhost_controller.go
@@ -214,7 +214,7 @@ func (r *HetznerBareMetalHostReconciler) getSecrets(
 			if apierrors.IsNotFound(err) {
 				conditions.MarkFalse(
 					bmHost,
-					infrav1.HetznerBareMetalHostReadyCondition,
+					infrav1.CredentialsAvailableCondition,
 					infrav1.OSSSHSecretMissingReason,
 					clusterv1.ConditionSeverityError,
 					infrav1.ErrorMessageMissingOSSSHSecret,
@@ -238,7 +238,7 @@ func (r *HetznerBareMetalHostReconciler) getSecrets(
 			if apierrors.IsNotFound(err) {
 				conditions.MarkFalse(
 					bmHost,
-					infrav1.HetznerBareMetalHostReadyCondition,
+					infrav1.CredentialsAvailableCondition,
 					infrav1.RescueSSHSecretMissingReason,
 					clusterv1.ConditionSeverityError,
 					infrav1.ErrorMessageMissingRescueSSHSecret,
@@ -317,7 +317,7 @@ func hetznerSecretErrorResult(
 		// at some point in the future.
 		conditions.MarkFalse(
 			bmHost,
-			infrav1.HetznerBareMetalHostReadyCondition,
+			infrav1.CredentialsAvailableCondition,
 			infrav1.HetznerSecretUnreachableReason,
 			clusterv1.ConditionSeverityError,
 			infrav1.ErrorMessageMissingHetznerSecret,
@@ -339,7 +339,7 @@ func hetznerSecretErrorResult(
 	if errors.As(err, &credValidationErr) {
 		conditions.MarkFalse(
 			bmHost,
-			infrav1.HetznerBareMetalHostReadyCondition,
+			infrav1.CredentialsAvailableCondition,
 			infrav1.RobotCredentialsInvalidReason,
 			clusterv1.ConditionSeverityError,
 			infrav1.ErrorMessageMissingOrInvalidSecretData,
diff --git a/pkg/services/baremetal/host/host.go b/pkg/services/baremetal/host/host.go
index 5ae5288c..dce685d9 100644
--- a/pkg/services/baremetal/host/host.go
+++ b/pkg/services/baremetal/host/host.go
@@ -100,6 +100,11 @@ func (s *Service) Reconcile(ctx context.Context) (result reconcile.Result, err e
 		return reconcile.Result{Requeue: true}, fmt.Errorf("action %q failed: %w", initialState, err)
 	}
 
+	// remove deprecated conditions
+	conditions.Delete(s.scope.HetznerBareMetalHost, infrav1.HetznerBareMetalHostReadyCondition)
+
+	conditions.SetSummary(s.scope.HetznerBareMetalHost)
+
 	// save host if it changed during reconciliation
 	if !reflect.DeepEqual(oldHost, *s.scope.HetznerBareMetalHost) {
 		return SaveHostAndReturn(ctx, s.scope.Client, s.scope.HetznerBareMetalHost)
@@ -166,6 +171,13 @@ func (s *Service) actionPreparing() actionResult {
 	// if there is no rescue system, we cannot provision the server
 	if !server.Rescue {
 		errMsg := fmt.Sprintf("bm server %v has no rescue system", server.ServerNumber)
+		conditions.MarkFalse(
+			s.scope.HetznerBareMetalHost,
+			infrav1.ProvisionSucceededCondition,
+			infrav1.RescueSystemUnavailableReason,
+			clusterv1.ConditionSeverityError,
+			errMsg,
+		)
 		record.Warnf(s.scope.HetznerBareMetalHost, "NoRescueSystemAvailable", errMsg)
 		s.scope.HetznerBareMetalHost.SetError(infrav1.FatalError, errMsg)
 		return s.recordActionFailure(infrav1.RegistrationError, errMsg)
@@ -251,7 +263,7 @@ func (s *Service) ensureSSHKey(sshSecretRef infrav1.SSHSecretRef, sshSecret *cor
 				msg := fmt.Sprintf("cannot upload ssh key %s - exists already under a different name", string(sshSecret.Data[sshSecretRef.Key.Name]))
 				conditions.MarkFalse(
 					s.scope.HetznerBareMetalHost,
-					infrav1.HetznerBareMetalHostReadyCondition,
+					infrav1.CredentialsAvailableCondition,
 					infrav1.SSHKeyAlreadyExistsReason,
 					clusterv1.ConditionSeverityError,
 					msg,
@@ -276,8 +288,15 @@ func (s *Service) handleIncompleteBoot(isRebootIntoRescue, isTimeout, isConnecti
 		if s.scope.HetznerBareMetalHost.Spec.Status.ErrorType == infrav1.ErrorTypeConnectionError {
 			// if error has occurred before, check the timeout
 			if hasTimedOut(s.scope.HetznerBareMetalHost.Spec.Status.LastUpdated, time.Minute) {
-				record.Warnf(s.scope.HetznerBareMetalHost, "SSHConnectionError",
-					"Connection error when targeting server with ssh that might be due to a wrong ssh port. Please check.")
+				msg := "Connection error when targeting server with ssh that miht be due to a wrong ssh port. Please check."
+				conditions.MarkFalse(
+					s.scope.HetznerBareMetalHost,
+					infrav1.ProvisionSucceededCondition,
+					infrav1.SSHConnectionRefusedReason,
+					clusterv1.ConditionSeverityError,
+					msg,
+				)
+				record.Warnf(s.scope.HetznerBareMetalHost, "SSHConnectionError", msg)
 				return fmt.Errorf("%w - might be due to wrong port", errSSHConnectionRefused)
 			}
 		} else {
@@ -983,6 +1002,8 @@ func (s *Service) actionProvisioning() actionResult {
 	}
 
 	host.ClearError()
+	conditions.MarkTrue(host, infrav1.ProvisionSucceededCondition)
+
 	return actionComplete{}
 }
 
@@ -1273,6 +1294,9 @@ func analyzeSSHOutputProvisioned(out sshclient.Output) (isTimeout, isConnectionR
 }
 
 func (s *Service) actionProvisioned() actionResult {
+	// set host to provisioned
+	conditions.MarkTrue(s.scope.HetznerBareMetalHost, infrav1.ProvisionSucceededCondition)
+
 	rebootDesired := s.scope.HetznerBareMetalHost.HasRebootAnnotation()
 	isRebooted := s.scope.HetznerBareMetalHost.Spec.Status.Rebooted
 	creds := sshclient.CredentialsFromSecret(s.scope.OSSSHSecret, s.scope.HetznerBareMetalHost.Spec.Status.SSHSpec.SecretRef)
diff --git a/pkg/services/baremetal/host/state_machine.go b/pkg/services/baremetal/host/state_machine.go
index 735ff467..2e295a30 100644
--- a/pkg/services/baremetal/host/state_machine.go
+++ b/pkg/services/baremetal/host/state_machine.go
@@ -84,8 +84,17 @@ func (hsm *hostStateMachine) ReconcileState() (actionRes actionResult) {
 		return actResult
 	}
 
-	// Set host ready now. This can be changed while the state is handled.
-	conditions.MarkTrue(hsm.host, infrav1.HetznerBareMetalHostReadyCondition)
+	// Assume credentials are ready for now. This can be changed while the state is handled.
+	conditions.MarkTrue(hsm.host, infrav1.CredentialsAvailableCondition)
+
+	// Set condition on false. It will be set on true if host is provisioned during reconcilement
+	conditions.MarkFalse(
+		hsm.host,
+		infrav1.ProvisionSucceededCondition,
+		infrav1.StillProvisioningReason,
+		clusterv1.ConditionSeverityInfo,
+		"host is provisioning - current state: %q", hsm.host.Spec.Status.ProvisioningState,
+	)
 
 	if stateHandler, found := hsm.handlers()[initialState]; found {
 		return stateHandler()
@@ -163,7 +172,7 @@ func (hsm *hostStateMachine) updateOSSSHStatusAndValidateKey(osSSHSecret *corev1
 	}
 	if err := validateSSHKey(osSSHSecret, hsm.host.Spec.Status.SSHSpec.SecretRef); err != nil {
 		msg := fmt.Sprintf("ssh credentials are invalid: %s", err.Error())
-		conditions.MarkFalse(hsm.host, infrav1.HetznerBareMetalHostReadyCondition, infrav1.SSHCredentialsInSecretInvalidReason, clusterv1.ConditionSeverityError, msg)
+		conditions.MarkFalse(hsm.host, infrav1.CredentialsAvailableCondition, infrav1.SSHCredentialsInSecretInvalidReason, clusterv1.ConditionSeverityError, msg)
 		record.Warnf(hsm.host, infrav1.SSHKeyAlreadyExistsReason, msg)
 		return hsm.reconciler.recordActionFailure(infrav1.PreparationError, infrav1.ErrorMessageMissingOrInvalidSecretData)
 	}
@@ -193,7 +202,7 @@ func (hsm *hostStateMachine) updateRescueSSHStatusAndValidateKey(rescueSSHSecret
 	}
 	if err := validateSSHKey(rescueSSHSecret, hsm.reconciler.scope.HetznerCluster.Spec.SSHKeys.RobotRescueSecretRef); err != nil {
 		msg := fmt.Sprintf("ssh credentials for rescue system are invalid: %s", err.Error())
-		conditions.MarkFalse(hsm.host, infrav1.HetznerBareMetalHostReadyCondition, infrav1.SSHCredentialsInSecretInvalidReason, clusterv1.ConditionSeverityError, msg)
+		conditions.MarkFalse(hsm.host, infrav1.CredentialsAvailableCondition, infrav1.SSHCredentialsInSecretInvalidReason, clusterv1.ConditionSeverityError, msg)
 		return hsm.reconciler.recordActionFailure(infrav1.PreparationError, infrav1.ErrorMessageMissingOrInvalidSecretData)
 	}
 	return nil


Test patch:
diff --git a/controllers/hetznerbaremetalhost_controller_test.go b/controllers/hetznerbaremetalhost_controller_test.go
index 686c21e6..dd05e683 100644
--- a/controllers/hetznerbaremetalhost_controller_test.go
+++ b/controllers/hetznerbaremetalhost_controller_test.go
@@ -376,10 +376,11 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 					if err := testEnv.Get(ctx, key, host); err != nil {
 						return false
 					}
-					if host.Spec.Status.ProvisioningState == infrav1.StateProvisioned {
-						return true
+					if host.Spec.Status.ProvisioningState != infrav1.StateProvisioned {
+						return false
 					}
-					return false
+
+					return isPresentAndTrue(key, host, infrav1.ProvisionSucceededCondition)
 				}, timeout).Should(BeTrue())
 			})
 		})
@@ -724,7 +725,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 				if err := testEnv.Get(ctx, key, host); err != nil {
 					return false
 				}
-				return isPresentAndFalseWithReason(key, host, infrav1.HetznerBareMetalHostReadyCondition, infrav1.RescueSSHSecretMissingReason)
+				return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.RescueSSHSecretMissingReason)
 			}, timeout).Should(BeTrue())
 		})
 
@@ -749,7 +750,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 				if err := testEnv.Get(ctx, key, host); err != nil {
 					return false
 				}
-				return isPresentAndFalseWithReason(key, host, infrav1.HetznerBareMetalHostReadyCondition, infrav1.SSHCredentialsInSecretInvalidReason)
+				return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.SSHCredentialsInSecretInvalidReason)
 			}, timeout).Should(BeTrue())
 		})
 	})
@@ -782,7 +783,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 				if err := testEnv.Get(ctx, key, host); err != nil {
 					return false
 				}
-				return isPresentAndFalseWithReason(key, host, infrav1.HetznerBareMetalHostReadyCondition, infrav1.OSSSHSecretMissingReason)
+				return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.OSSSHSecretMissingReason)
 			}, timeout).Should(BeTrue())
 		})
 
@@ -807,7 +808,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 				if err := testEnv.Get(ctx, key, host); err != nil {
 					return false
 				}
-				return isPresentAndFalseWithReason(key, host, infrav1.HetznerBareMetalHostReadyCondition, infrav1.SSHCredentialsInSecretInvalidReason)
+				return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.SSHCredentialsInSecretInvalidReason)
 			}, timeout).Should(BeTrue())
 		})
 	})