Repository: metal3-io/baremetal-operator
Pull request #1064 (2022-01-06 23:17:23)

Use integer value in clean steps for HostFirmwareSettings Integer type
For these types the value in clean steps should be stored as an integer, otherwise vendors reject the Redfish request.


Patch:
diff --git a/controllers/metal3.io/hostfirmwaresettings_controller.go b/controllers/metal3.io/hostfirmwaresettings_controller.go
index cbe955aa..4fe3f6e6 100644
--- a/controllers/metal3.io/hostfirmwaresettings_controller.go
+++ b/controllers/metal3.io/hostfirmwaresettings_controller.go
@@ -32,7 +32,6 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -208,7 +207,7 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta
 	specMismatch := false
 	for k, v := range info.hfs.Spec.Settings {
 		if statusVal, ok := info.hfs.Status.Settings[k]; ok {
-			if v != intstr.FromString(statusVal) {
+			if v.String() != statusVal {
 				specMismatch = true
 				break
 			}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 5ba4e9ce..20ec3c19 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -14,6 +14,7 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/yaml"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
@@ -1154,14 +1155,14 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails
 		return nil, err
 	}
 
-	var newSettings []map[string]string
+	var newSettings []map[string]interface{}
 	if data.ActualFirmwareSettings != nil {
 		// If we have the current settings from Ironic, update the settings to contain:
 		// 1. settings converted by BMC drivers that are different than current settings
 		for _, fwConfigSetting := range fwConfigSettings {
 			if val, exists := data.ActualFirmwareSettings[fwConfigSetting["name"]]; exists {
 				if fwConfigSetting["value"] != val {
-					newSettings = buildFirmwareSettings(newSettings, fwConfigSetting["name"], fwConfigSetting["value"])
+					newSettings = buildFirmwareSettings(newSettings, fwConfigSetting["name"], intstr.FromString(fwConfigSetting["value"]))
 				}
 			} else {
 				p.log.Info("name converted from bmc driver not found in firmware settings", "name", fwConfigSetting["name"], "node", p.nodeID)
@@ -1178,14 +1179,14 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails
 							continue
 						}
 					}
-					newSettings = buildFirmwareSettings(newSettings, k, v.String())
+					newSettings = buildFirmwareSettings(newSettings, k, v)
 				}
 			}
 		}
 	} else {
-		// use only the settings converted by bmc driver
+		// use only the settings converted by bmc driver. Note that these settings are all strings
 		for _, fwConfigSetting := range fwConfigSettings {
-			newSettings = append(newSettings, fwConfigSetting)
+			newSettings = buildFirmwareSettings(newSettings, fwConfigSetting["name"], intstr.FromString(fwConfigSetting["value"]))
 		}
 	}
 
@@ -1208,7 +1209,7 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails
 	return
 }
 
-func buildFirmwareSettings(settings []map[string]string, name string, value string) []map[string]string {
+func buildFirmwareSettings(settings []map[string]interface{}, name string, value intstr.IntOrString) []map[string]interface{} {
 	// if name already exists, don't add it
 	for _, setting := range settings {
 		if setting["name"] == name {
@@ -1216,11 +1217,13 @@ func buildFirmwareSettings(settings []map[string]string, name string, value stri
 		}
 	}
 
-	return append(settings,
-		map[string]string{
-			"name":  name,
-			"value": value},
-	)
+	if value.Type == intstr.Int {
+		settings = append(settings, map[string]interface{}{"name": name, "value": value.IntValue()})
+	} else {
+		settings = append(settings, map[string]interface{}{"name": name, "value": value.String()})
+	}
+
+	return settings
 }
 
 func (p *ironicProvisioner) startManualCleaning(bmcAccess bmc.AccessDetails, ironicNode *nodes.Node, data provisioner.PrepareData) (success bool, result provisioner.Result, err error) {


Test patch:
diff --git a/controllers/metal3.io/hostfirmwaresettings_test.go b/controllers/metal3.io/hostfirmwaresettings_test.go
index 94757e0a..428c29c3 100644
--- a/controllers/metal3.io/hostfirmwaresettings_test.go
+++ b/controllers/metal3.io/hostfirmwaresettings_test.go
@@ -487,6 +487,61 @@ func TestStoreHostFirmwareSettings(t *testing.T) {
 			},
 			SpecIsValid: false,
 		},
+		{
+			Scenario: "spec has same settings as current",
+			CurrentHFSResource: &metal3v1alpha1.HostFirmwareSettings{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "HostFirmwareSettings",
+					APIVersion: "metal3.io/v1alpha1"},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            hostName,
+					Namespace:       hostNamespace,
+					ResourceVersion: "1"},
+				Spec: metal3v1alpha1.HostFirmwareSettingsSpec{
+					Settings: metal3v1alpha1.DesiredSettingsMap{
+						"NetworkBootRetryCount": intstr.FromInt(20),
+						"ProcVirtualization":    intstr.FromString("Disabled"),
+					},
+				},
+				Status: metal3v1alpha1.HostFirmwareSettingsStatus{
+					FirmwareSchema: &metal3v1alpha1.SchemaReference{
+						Name:      schemaName,
+						Namespace: hostNamespace,
+					},
+					Settings: metal3v1alpha1.SettingsMap{
+						"L2Cache":               "10x256 KB",
+						"NetworkBootRetryCount": "10",
+						"ProcVirtualization":    "Enabled",
+					},
+				},
+			},
+			CreateSchemaResource: true,
+			ExpectedSettings: &metal3v1alpha1.HostFirmwareSettings{
+				Spec: metal3v1alpha1.HostFirmwareSettingsSpec{
+					Settings: metal3v1alpha1.DesiredSettingsMap{
+						"NetworkBootRetryCount": intstr.FromInt(20),
+						"ProcVirtualization":    intstr.FromString("Disabled"),
+					},
+				},
+				Status: metal3v1alpha1.HostFirmwareSettingsStatus{
+					FirmwareSchema: &metal3v1alpha1.SchemaReference{
+						Name:      schemaName,
+						Namespace: hostNamespace,
+					},
+					Settings: metal3v1alpha1.SettingsMap{
+						"AssetTag":              "X45672917",
+						"L2Cache":               "10x512 KB",
+						"NetworkBootRetryCount": "20",
+						"ProcVirtualization":    "Disabled",
+						"SecureBoot":            "Enabled",
+					},
+					Conditions: []metav1.Condition{
+						{Type: "Valid", Status: "True", Reason: "Success"},
+					},
+				},
+			},
+			SpecIsValid: false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -597,7 +652,7 @@ func TestValidateHostFirmwareSettings(t *testing.T) {
 				Settings: metal3v1alpha1.DesiredSettingsMap{
 					"CustomPostMessage":     intstr.FromString("All tests passed"),
 					"ProcVirtualization":    intstr.FromString("Disabled"),
-					"NetworkBootRetryCount": intstr.FromString("2000"),
+					"NetworkBootRetryCount": intstr.FromInt(2000),
 				},
 			},
 			ExpectedError: "Setting NetworkBootRetryCount is invalid, integer 2000 is above maximum value 20",
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index 725941e4..9330510c 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -410,7 +410,7 @@ func TestBuildCleanSteps(t *testing.T) {
 		currentSettings  v1alpha1.SettingsMap
 		desiredSettings  v1alpha1.DesiredSettingsMap
 		firmwareConfig   *v1alpha1.FirmwareConfig
-		expectedSettings []map[string]string
+		expectedSettings []map[string]interface{}
 	}{
 		{
 			name: "no current settings",
@@ -424,7 +424,7 @@ func TestBuildCleanSteps(t *testing.T) {
 				VirtualizationEnabled:             &True,
 				SimultaneousMultithreadingEnabled: &False,
 			},
-			expectedSettings: []map[string]string{
+			expectedSettings: []map[string]interface{}{
 				{
 					"name":  "ProcVirtualization",
 					"value": "Enabled",
@@ -471,7 +471,7 @@ func TestBuildCleanSteps(t *testing.T) {
 				VirtualizationEnabled:             &True,
 				SimultaneousMultithreadingEnabled: &False,
 			},
-			expectedSettings: []map[string]string{
+			expectedSettings: []map[string]interface{}{
 				{
 					"name":  "ProcVirtualization",
 					"value": "Enabled",
@@ -496,7 +496,7 @@ func TestBuildCleanSteps(t *testing.T) {
 				"ProcHyperthreading":    "Disabled",
 			},
 			desiredSettings: v1alpha1.DesiredSettingsMap{
-				"NetworkBootRetryCount": intstr.FromString("10"),
+				"NetworkBootRetryCount": intstr.FromInt(10),
 				"ProcVirtualization":    intstr.FromString("Disabled"),
 				"ProcHyperthreading":    intstr.FromString("Enabled"),
 			},
@@ -504,10 +504,10 @@ func TestBuildCleanSteps(t *testing.T) {
 				VirtualizationEnabled:             &True,
 				SimultaneousMultithreadingEnabled: &False,
 			},
-			expectedSettings: []map[string]string{
+			expectedSettings: []map[string]interface{}{
 				{
 					"name":  "NetworkBootRetryCount",
-					"value": "10",
+					"value": 10,
 				},
 				{
 					"name":  "ProcVirtualization",
@@ -541,7 +541,7 @@ func TestBuildCleanSteps(t *testing.T) {
 				VirtualizationEnabled:             &False,
 				SimultaneousMultithreadingEnabled: &True,
 			},
-			expectedSettings: []map[string]string{
+			expectedSettings: []map[string]interface{}{
 				{
 					"name":  "ProcVirtualization",
 					"value": "Disabled",
@@ -576,7 +576,7 @@ func TestBuildCleanSteps(t *testing.T) {
 				VirtualizationEnabled:             &False,
 				SimultaneousMultithreadingEnabled: &True,
 			},
-			expectedSettings: []map[string]string{
+			expectedSettings: []map[string]interface{}{
 				{
 					"name":  "ProcHyperthreading",
 					"value": "Enabled",
@@ -624,7 +624,7 @@ func TestBuildCleanSteps(t *testing.T) {
 
 			assert.Equal(t, nil, err)
 			if cleanSteps == nil {
-				assert.Equal(t, tc.expectedSettings, []map[string]string(nil))
+				assert.Equal(t, tc.expectedSettings, []map[string]interface{}(nil))
 			} else {
 				settings := cleanSteps[0].Args["settings"]
 				assert.ElementsMatch(t, tc.expectedSettings, settings)