Skip to content

Commit

Permalink
Use integer value in clean steps for HostFirmwareSettings Integer type
Browse files Browse the repository at this point in the history 
For these types the value in clean steps should be stored as an integer,
otherwise vendors reject the Redfish request.
  • Loading branch information
@bfournie
bfournie committed Jan 6, 2022
1 parent 5cc03c5 commit b00f767
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 23 deletions.
3 changes: 1 addition & 2 deletions controllers/metal3.io/hostfirmwaresettings_controller.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down
57 changes: 56 additions & 1 deletion controllers/metal3.io/hostfirmwaresettings_test.go
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down
25 changes: 14 additions & 11 deletions pkg/provisioner/ironic/ironic.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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"]))
}
}

Expand All @@ -1208,19 +1209,21 @@ 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 {
return settings
}
}

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) {
Expand Down
18 changes: 9 additions & 9 deletions pkg/provisioner/ironic/provision_test.go
View file
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -496,18 +496,18 @@ 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"),
},
firmwareConfig: &v1alpha1.FirmwareConfig{
VirtualizationEnabled: &True,
SimultaneousMultithreadingEnabled: &False,
},
expectedSettings: []map[string]string{
expectedSettings: []map[string]interface{}{
{
"name": "NetworkBootRetryCount",
"value": "10",
"value": 10,
},
{
"name": "ProcVirtualization",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit b00f767

Please sign in to comment.