Merge pull request #3133 from jacob-anders/fix-healthy-condition

🐛 Fix Healthy condition staying Unknown despite Ironic reporting health status

Author: metal3-io-bot

internal/controller/metal3.io/baremetalhost_controller.go

@@ -240,16 +240,21 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 		return result, err
 	}
 
+	// Always compute conditions since some (e.g. Healthy) can change
+	// based on external state from Ironic regardless of state machine
+	// transitions.
+	conditionsBefore := slices.Clone(host.GetConditions())
+	computeConditions(ctx, host, prov)
+	conditionsChanged := !reflect.DeepEqual(conditionsBefore, host.GetConditions())
+
 	// Only save status when we're told to, otherwise we
 	// introduce an infinite loop reconciling the same object over and
 	// over when there is an unrecoverable error (tracked through the
 	// error state of the host).
-	if actResult.Dirty() {
-		// Save Host
+	if actResult.Dirty() || conditionsChanged {
 		info.log.Info("saving host status",
 			"operational status", host.OperationalStatus(),
 			"provisioning state", host.Status.Provisioning.State)
-		computeConditions(ctx, host, prov)
 		err = r.saveHostStatus(ctx, host)
 		if err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to save host status after %q: %w", initialState, err)
@@ -2301,7 +2306,11 @@ func computeConditions(ctx context.Context, host *metal3api.BareMetalHost, prov
 		setConditionUnknown(host, metal3api.HealthyCondition, metal3api.UnknownHealthReason)
 		return
 	}
-	switch prov.GetHealth(ctx) {
+	switch health := prov.GetHealth(ctx); health {
+	case "":
+		if meta.FindStatusCondition(host.Status.Conditions, string(metal3api.HealthyCondition)) == nil {
+			setConditionUnknown(host, metal3api.HealthyCondition, metal3api.UnknownHealthReason)
+		}
 	case provisioner.HealthOK:
 		setConditionTrue(host, metal3api.HealthyCondition, metal3api.HealthyReason)
 	case provisioner.HealthWarning:

internal/controller/metal3.io/baremetalhost_controller_test.go

@@ -3187,12 +3187,6 @@ func TestComputeHealthyCondition(t *testing.T) {
 		ExpectedStatus metav1.ConditionStatus
 		ExpectedReason string
 	}{
-		{
-			Scenario:       "empty health reports unknown",
-			Health:         "",
-			ExpectedStatus: metav1.ConditionUnknown,
-			ExpectedReason: metal3api.UnknownHealthReason,
-		},
 		{
 			Scenario:       "OK health",
 			Health:         provisioner.HealthOK,
@@ -3234,6 +3228,27 @@ func TestComputeHealthyCondition(t *testing.T) {
 	}
 }
 
+func TestComputeHealthyConditionEmptyHealth(t *testing.T) {
+	host := bmhWithStatus(metal3api.OperationalStatusOK, metal3api.StateAvailable)
+	fix := &fixture.Fixture{Health: ""}
+	prov, err := fix.NewProvisioner(t.Context(), provisioner.BuildHostData(*host, bmc.Credentials{}), nil)
+	require.NoError(t, err)
+
+	// On a fresh host with no existing condition, empty health should
+	// initialise the condition to Unknown.
+	computeConditions(t.Context(), host, prov)
+	cond := conditions.Get(host, metal3api.HealthyCondition)
+	require.NotNil(t, cond, "empty health on new host should set Unknown condition")
+	assert.Equal(t, metav1.ConditionUnknown, cond.Status)
+
+	// Set a real health value, then verify empty health preserves it.
+	setConditionTrue(host, metal3api.HealthyCondition, metal3api.HealthyReason)
+	computeConditions(t.Context(), host, prov)
+	cond = conditions.Get(host, metal3api.HealthyCondition)
+	require.NotNil(t, cond)
+	assert.Equal(t, metav1.ConditionTrue, cond.Status, "empty health should not overwrite existing condition")
+}
+
 func TestComputeConditions(t *testing.T) {
 	fix := fixture.Fixture{PowerFailed: true}
 	provisionerWithPowerFailure, err := fix.NewProvisioner(t.Context(), provisioner.HostData{}, nil)
@@ -3332,7 +3347,7 @@ func TestComputeConditions(t *testing.T) {
 			}
 			cond := conditions.Get(tc.BareMetalHost, metal3api.HealthyCondition)
 			require.NotNil(t, cond, "Healthy condition should always be set")
-			assert.Equal(t, metav1.ConditionUnknown, cond.Status, "Healthy should be Unknown when health is not reported")
+			assert.Equal(t, metav1.ConditionUnknown, cond.Status, "Healthy should be Unknown when no health data is available")
 		})
 	}
 }

pkg/provisioner/ironic/clients/features.go

@@ -45,7 +45,8 @@ func (af AvailableFeatures) Log(logger logr.Logger) {
 		"maxVersion", fmt.Sprintf("1.%d", af.MaxVersion),
 		"chosenVersion", af.ChooseMicroversion(),
 		"virtualMediaGET", af.HasVirtualMediaGetAPI(),
-		"disablePowerOff", af.HasDisablePowerOff())
+		"disablePowerOff", af.HasDisablePowerOff(),
+		"healthAPI", af.HasHealthAPI())
 }
 
 func (af AvailableFeatures) HasVirtualMediaGetAPI() bool {
@@ -56,7 +57,15 @@ func (af AvailableFeatures) HasDisablePowerOff() bool {
 	return af.MaxVersion >= 95 //nolint:mnd
 }
 
+func (af AvailableFeatures) HasHealthAPI() bool {
+	return af.MaxVersion >= 109 //nolint:mnd
+}
+
 func (af AvailableFeatures) ChooseMicroversion() string {
+	if af.HasHealthAPI() {
+		return "1.109"
+	}
+
 	if af.HasDisablePowerOff() {
 		return "1.95"
 	}

pkg/provisioner/ironic/clients/features_test.go

@@ -6,7 +6,6 @@ import (
 )
 
 func TestAvailableFeatures_ChooseMicroversion(t *testing.T) {
-	microVersion := "1.95"
 	type fields struct {
 		MaxVersion int
 	}
@@ -23,18 +22,32 @@ func TestAvailableFeatures_ChooseMicroversion(t *testing.T) {
 			want: baselineVersionString,
 		},
 		{
-			name: fmt.Sprintf("MaxVersion = %d return %s", 89, microVersion),
+			name: "MaxVersion = 95 return 1.95",
 			feature: fields{
 				MaxVersion: 95,
 			},
-			want: microVersion,
+			want: "1.95",
 		},
 		{
-			name: fmt.Sprintf("MaxVersion > %d return %s", 89, microVersion),
+			name: "MaxVersion = 100 return 1.95",
 			feature: fields{
 				MaxVersion: 100,
 			},
-			want: microVersion,
+			want: "1.95",
+		},
+		{
+			name: "MaxVersion = 109 return 1.109",
+			feature: fields{
+				MaxVersion: 109,
+			},
+			want: "1.109",
+		},
+		{
+			name: "MaxVersion > 109 return 1.109",
+			feature: fields{
+				MaxVersion: 115,
+			},
+			want: "1.109",
 		},
 	}
 	for _, tt := range tests {

pkg/provisioner/ironic/ironic.go

@@ -1997,7 +1997,9 @@ func (p *ironicProvisioner) HasPowerFailure(ctx context.Context) bool {
 func (p *ironicProvisioner) GetHealth(ctx context.Context) string {
 	node, err := p.getNode(ctx)
 	if err != nil {
-		p.log.Error(err, "ignored error while checking health status")
+		if !errors.Is(err, provisioner.ErrNeedsRegistration) {
+			p.log.Error(err, "ignored error while checking health status")
+		}
 		return ""
 	}
 	return node.Health