From b4ab7da398101209e9a34d99ace6f169ba46df1a Mon Sep 17 00:00:00 2001 From: tliu2021 Date: Wed, 23 Mar 2022 09:34:55 -0400 Subject: [PATCH 1/2] Check for empty HFS status settings If the HFS status settings have not been populated, does not generate the Invalid BIOS Setting events. If the changes are detected, but the HFS status settings are empty, it waits until hostFirmwareSettings are read. Signed-off-by: tliu2021 --- .../metal3.io/baremetalhost_controller.go | 5 +++ .../baremetalhost_controller_test.go | 42 +++++++++++++++++++ .../hostfirmwaresettings_controller.go | 7 +++- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index 7de67bde8f..a6d95d5ffd 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/controllers/metal3.io/baremetalhost_controller.go @@ -1428,6 +1428,11 @@ func (r *BareMetalHostReconciler) getHostFirmwareSettings(info *reconcileInfo) ( // Check if there are settings in the Spec that are different than the Status if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsChangeDetected)) { + // Check if the status settings have been populated + if len(hfs.Status.Settings) == 0 { + return false, nil, errors.New("host firmware status settings not available") + } + if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsValid)) { info.log.Info("hostFirmwareSettings indicating ChangeDetected", "namespacename", info.request.NamespacedName) return true, hfs, nil diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go index e5659d3ba5..60b08d3eb0 100644 --- a/controllers/metal3.io/baremetalhost_controller_test.go +++ b/controllers/metal3.io/baremetalhost_controller_test.go @@ -2736,9 +2736,51 @@ func TestHFSTransitionToPreparing(t *testing.T) { {Type: "ChangeDetected", Status: "True", Reason: "Success"}, {Type: "Valid", Status: "True", Reason: "Success"}, }, + Settings: metal3v1alpha1.SettingsMap{ + "ProcVirtualization": "Enabled", + "SecureBoot": "Enabled", + }, } r.Update(goctx.TODO(), hfs) waitForProvisioningState(t, r, host, metal3v1alpha1.StatePreparing) } + +// TestHFSEmptyStatusSettings ensures that BMH does not move to the next state in the +// following two senarios: +// 1. when a user provides the BIOS settings on a hardware server that does not +// have the required license to configure BIOS +// 2. when it is waiting to get data from Ironic +func TestHFSEmptyStatusSettings(t *testing.T) { + host := newDefaultHost(t) + host.Spec.Online = true + host.Spec.ConsumerRef = &corev1.ObjectReference{} + host.Spec.ExternallyProvisioned = false + r := newTestReconciler(host) + + waitForProvisioningState(t, r, host, metal3v1alpha1.StatePreparing) + + // Update HFS so host will go through cleaning + hfs := &metal3v1alpha1.HostFirmwareSettings{} + key := client.ObjectKey{ + Namespace: host.ObjectMeta.Namespace, Name: host.ObjectMeta.Name} + if err := r.Get(goctx.TODO(), key, hfs); err != nil { + t.Fatal(err) + } + + hfs.Status = metal3v1alpha1.HostFirmwareSettingsStatus{ + Conditions: []metav1.Condition{ + {Type: "ChangeDetected", Status: "True", Reason: "Success"}, + {Type: "Valid", Status: "True", Reason: "Success"}, + }, + } + + r.Update(goctx.TODO(), hfs) + + tryReconcile(t, r, host, + func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool { + return host.Status.Provisioning.State == metal3v1alpha1.StatePreparing + }, + ) +} diff --git a/controllers/metal3.io/hostfirmwaresettings_controller.go b/controllers/metal3.io/hostfirmwaresettings_controller.go index 090536c8bd..7f49277639 100644 --- a/controllers/metal3.io/hostfirmwaresettings_controller.go +++ b/controllers/metal3.io/hostfirmwaresettings_controller.go @@ -247,8 +247,11 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta dirty = true } } else { - for _, error := range errors { - info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid BIOS setting: %v", error)) + // If the status settings are empty, don't raise events + if len(newStatus.Settings) != 0 { + for _, error := range errors { + info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid BIOS setting: %v", error)) + } } reason = reasonConfigurationError if setCondition(generation, &newStatus, info, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionFalse, reason, "Invalid BIOS setting") { From b503d6094fef08b1d578b80133421247b84ee4fb Mon Sep 17 00:00:00 2001 From: tliu2021 Date: Fri, 1 Apr 2022 16:20:12 -0400 Subject: [PATCH 2/2] Address code review comments Signed-off-by: tliu2021 --- .../baremetalhost_controller_test.go | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go index 60b08d3eb0..004cadc13c 100644 --- a/controllers/metal3.io/baremetalhost_controller_test.go +++ b/controllers/metal3.io/baremetalhost_controller_test.go @@ -2747,11 +2747,9 @@ func TestHFSTransitionToPreparing(t *testing.T) { waitForProvisioningState(t, r, host, metal3v1alpha1.StatePreparing) } -// TestHFSEmptyStatusSettings ensures that BMH does not move to the next state in the -// following two senarios: -// 1. when a user provides the BIOS settings on a hardware server that does not +// TestHFSEmptyStatusSettings ensures that BMH does not move to the next state +// when a user provides the BIOS settings on a hardware server that does not // have the required license to configure BIOS -// 2. when it is waiting to get data from Ironic func TestHFSEmptyStatusSettings(t *testing.T) { host := newDefaultHost(t) host.Spec.Online = true @@ -2783,4 +2781,19 @@ func TestHFSEmptyStatusSettings(t *testing.T) { return host.Status.Provisioning.State == metal3v1alpha1.StatePreparing }, ) + + // Clear the change, it will no longer be blocked + hfs.Status = metal3v1alpha1.HostFirmwareSettingsStatus{ + Conditions: []metav1.Condition{ + {Type: "ChangeDetected", Status: "False", Reason: "Success"}, + {Type: "Valid", Status: "True", Reason: "Success"}, + }, + } + + r.Update(goctx.TODO(), hfs) + tryReconcile(t, r, host, + func(host *metal3v1alpha1.BareMetalHost, result reconcile.Result) bool { + return host.Status.Provisioning.State == metal3v1alpha1.StateAvailable + }, + ) }