diff --git a/apis/metal3.io/v1alpha1/firmwareschema_types.go b/apis/metal3.io/v1alpha1/firmwareschema_types.go index df76ff5dad..67620b8890 100644 --- a/apis/metal3.io/v1alpha1/firmwareschema_types.go +++ b/apis/metal3.io/v1alpha1/firmwareschema_types.go @@ -18,6 +18,7 @@ package v1alpha1 import ( "fmt" + "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -71,6 +72,10 @@ func (schema *SettingSchema) Validate(name string, value intstr.IntOrString) err return SchemaSettingError{name: name, message: "it is ReadOnly"} } + if strings.Contains(name, "Password") { + return SchemaSettingError{name: name, message: "Password fields can't be set"} + } + // Check if valid based on type switch schema.AttributeType { case "Enumeration": diff --git a/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go b/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go index 8bca68d148..c170e967e8 100644 --- a/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go +++ b/apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go @@ -37,19 +37,16 @@ type SettingsConditionType string const ( // Indicates that the settings in the Spec are different than Status - UpdateRequested SettingsConditionType = "UpdateRequested" + FirmwareSettingsChangeDetected SettingsConditionType = "ChangeDetected" // Indicates if the settings are valid and can be configured on the host - SettingsValid SettingsConditionType = "Valid" + FirmwareSettingsValid SettingsConditionType = "Valid" ) // HostFirmwareSettingsSpec defines the desired state of HostFirmwareSettings type HostFirmwareSettingsSpec struct { // Settings are the desired firmware settings stored as name/value pairs. - // This will be populated with the actual firmware settings and only - // contain the settings that can be modified (i.e. not ReadOnly), to - // facilitate making changes. // +patchStrategy=merge Settings DesiredSettingsMap `json:"settings" required:"true"` } @@ -61,9 +58,13 @@ type HostFirmwareSettingsStatus struct { // Namespace as the settings but it can be overwritten in the Spec FirmwareSchema *SchemaReference `json:"schema,omitempty"` - // Settings are the actual firmware settings stored as name/value pairs + // Settings are the firmware settings stored as name/value pairs Settings SettingsMap `json:"settings" required:"true"` + // Time that the status was last updated + // +optional + LastUpdated *metav1.Time `json:"lastUpdated,omitempty"` + // Track whether settings stored in the spec are valid based on the schema // +patchMergeKey=type // +patchStrategy=merge diff --git a/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go b/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go index 44f8b914d1..03d546bb69 100644 --- a/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go +++ b/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go @@ -584,6 +584,10 @@ func (in *HostFirmwareSettingsStatus) DeepCopyInto(out *HostFirmwareSettingsStat (*out)[key] = val } } + if in.LastUpdated != nil { + in, out := &in.LastUpdated, &out.LastUpdated + *out = (*in).DeepCopy() + } if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]metav1.Condition, len(*in)) diff --git a/config/crd/bases/metal3.io_hostfirmwaresettings.yaml b/config/crd/bases/metal3.io_hostfirmwaresettings.yaml index 62e084047e..0c8ab5c326 100644 --- a/config/crd/bases/metal3.io_hostfirmwaresettings.yaml +++ b/config/crd/bases/metal3.io_hostfirmwaresettings.yaml @@ -46,9 +46,7 @@ spec: - type: string x-kubernetes-int-or-string: true description: Settings are the desired firmware settings stored as - name/value pairs. This will be populated with the actual firmware - settings and only contain the settings that can be modified (i.e. - not ReadOnly), to facilitate making changes. + name/value pairs. type: object required: - settings @@ -131,6 +129,10 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + lastUpdated: + description: Time that the status was last updated + format: date-time + type: string schema: description: FirmwareSchema is a reference to the Schema used to describe each FirmwareSetting. By default, this will be a Schema in the same @@ -150,7 +152,7 @@ spec: settings: additionalProperties: type: string - description: Settings are the actual firmware settings stored as name/value + description: Settings are the firmware settings stored as name/value pairs type: object required: diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml index 7c994b33d1..d3342668a2 100644 --- a/config/render/capm3.yaml +++ b/config/render/capm3.yaml @@ -1243,9 +1243,7 @@ spec: - type: string x-kubernetes-int-or-string: true description: Settings are the desired firmware settings stored as - name/value pairs. This will be populated with the actual firmware - settings and only contain the settings that can be modified (i.e. - not ReadOnly), to facilitate making changes. + name/value pairs. type: object required: - settings @@ -1328,6 +1326,10 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + lastUpdated: + description: Time that the status was last updated + format: date-time + type: string schema: description: FirmwareSchema is a reference to the Schema used to describe each FirmwareSetting. By default, this will be a Schema in the same @@ -1347,7 +1349,7 @@ spec: settings: additionalProperties: type: string - description: Settings are the actual firmware settings stored as name/value + description: Settings are the firmware settings stored as name/value pairs type: object required: diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index bf43c8eed8..712c24b2b1 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/controllers/metal3.io/baremetalhost_controller.go @@ -56,6 +56,7 @@ const ( unmanagedRetryDelay = time.Minute * 10 preprovImageRetryDelay = time.Minute * 5 provisionerNotReadyRetryDelay = time.Second * 30 + subResourceNotReadyRetryDelay = time.Second * 60 rebootAnnotationPrefix = "reboot.metal3.io" inspectAnnotationPrefix = "inspect.metal3.io" hardwareDetailsAnnotation = inspectAnnotationPrefix + "/hardwaredetails" @@ -808,6 +809,14 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf return result } + // Create the hostFirmwareSettings resource with same host name/namespace if it doesn't exist + if info.host.Name != "" { + if err = r.createHostFirmwareSettings(info); err != nil { + info.log.Info("failed creating hostfirmwaresettings") + return actionError{errors.Wrap(err, "failed creating hostFirmwareSettings")} + } + } + // Reaching this point means the credentials are valid and worked, // so clear any previous error and record the success in the // status block. @@ -944,7 +953,8 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner, // Use settings in hostFirmwareSettings if available hfs, err := r.getHostFirmwareSettings(info) if err != nil { - info.log.Info("hostFirmwareSettings not available for cleaning") + // wait until hostFirmwareSettings are ready + return actionContinue{subResourceNotReadyRetryDelay} } if hfs != nil { prepareData.ActualFirmwareSettings = hfs.Status.Settings.DeepCopy() @@ -1318,6 +1328,38 @@ func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty boo return } +func (r *BareMetalHostReconciler) createHostFirmwareSettings(info *reconcileInfo) error { + + // Check if HostFirmwareSettings already exists + hfs := &metal3v1alpha1.HostFirmwareSettings{} + if err := r.Get(context.TODO(), info.request.NamespacedName, hfs); err != nil { + if k8serrors.IsNotFound(err) { + // A resource doesn't exist, create one + hfs.ObjectMeta = metav1.ObjectMeta{ + Name: info.host.Name, + Namespace: info.host.Namespace} + hfs.Status.Settings = make(metal3v1alpha1.SettingsMap) + hfs.Spec.Settings = make(metal3v1alpha1.DesiredSettingsMap) + + // Set bmh as owner, this makes sure the resource is deleted when bmh is deleted + if err = controllerutil.SetControllerReference(info.host, hfs, r.Scheme()); err != nil { + return errors.Wrap(err, "could not set bmh as controller") + } + if err = r.Create(context.TODO(), hfs); err != nil { + return errors.Wrap(err, "failure creating hostFirmwareSettings resource") + } + + info.log.Info("created new hostFirmwareSettings resource") + + } else { + // Error reading the object + return errors.Wrap(err, "could not load hostFirmwareSettings resource") + } + } + + return nil +} + // Get the stored firmware settings if there are valid changes func (r *BareMetalHostReconciler) getHostFirmwareSettings(info *reconcileInfo) (hfs *metal3v1alpha1.HostFirmwareSettings, err error) { @@ -1335,9 +1377,9 @@ 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.UpdateRequested)) { + if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsChangeDetected)) { - if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.SettingsValid)) { + if meta.IsStatusConditionTrue(hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsValid)) { return hfs, nil } diff --git a/controllers/metal3.io/hostfirmwaresettings_controller.go b/controllers/metal3.io/hostfirmwaresettings_controller.go index 566480e78a..a3370a488a 100644 --- a/controllers/metal3.io/hostfirmwaresettings_controller.go +++ b/controllers/metal3.io/hostfirmwaresettings_controller.go @@ -21,7 +21,9 @@ import ( "crypto/sha256" "fmt" "sort" + "strconv" "strings" + "time" "github.com/go-logr/logr" "github.com/pkg/errors" @@ -30,21 +32,21 @@ 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/types" "k8s.io/apimachinery/pkg/util/intstr" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/source" metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" "github.com/metal3-io/baremetal-operator/pkg/provisioner" ) +const ( + provisionerRetryDelay = time.Second * 30 + resourceNotAvailableRetryDelay = time.Second * 30 + reconcilerRequeueDelay = time.Minute * 5 +) + // HostFirmwareSettingsReconciler reconciles a HostFirmwareSettings object type HostFirmwareSettingsReconciler struct { client.Client @@ -115,24 +117,20 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl bmh := &metal3v1alpha1.BareMetalHost{} if err = r.Get(context.TODO(), req.NamespacedName, bmh); err != nil { reqLogger.Info("could not get baremetalhost, not running reconciler") - // only run again if created - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true, RequeueAfter: resourceNotAvailableRetryDelay}, nil } // Fetch the HostFirmwareSettings hfs := &metal3v1alpha1.HostFirmwareSettings{} info := &rInfo{log: reqLogger, hfs: hfs, bmh: bmh} if err = r.Get(ctx, req.NamespacedName, hfs); err != nil { + // The HFS resource may have been deleted if k8serrors.IsNotFound(err) { - // A resource doesn't exist, create one - if err = r.newHostFirmwareSettings(info, req.NamespacedName); err != nil { - - return ctrl.Result{}, errors.Wrap(err, "could not create firmware settings") - } - } else { - // Error reading the object - requeue the request. - return ctrl.Result{}, errors.Wrap(err, "could not load host firmware settings") + reqLogger.Info("hostFirmwareSettings not found") + return ctrl.Result{Requeue: true, RequeueAfter: resourceNotAvailableRetryDelay}, nil } + // Error reading the object - requeue the request. + return ctrl.Result{}, errors.Wrap(err, "could not load hostFirmwareSettings") } // Create a provisioner that can access Ironic API @@ -143,13 +141,20 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl ready, err := prov.IsReady() if err != nil || !ready { - reqLogger.Info("provisioner is not ready", "RequeueAfter:", provisionerNotReadyRetryDelay) - return ctrl.Result{Requeue: true, RequeueAfter: provisionerNotReadyRetryDelay}, nil + reqLogger.Info("provisioner is not ready", "RequeueAfter:", provisionerRetryDelay) + return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil + } + + info.log.Info("retrieving firmware settings and saving to resource", "node", bmh.Status.Provisioning.ID) + + // Get the current settings and schema, retry if provisioner returns error + currentSettings, schema, err := prov.GetFirmwareSettings(true) + if err != nil { + reqLogger.Info("provisioner returns error", "RequeueAfter:", provisionerRetryDelay) + return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil } - // Get the data from Ironic and update HFS, the call to provisioner may fail if settings can't - // be retrieved for the bios_interface. - if err = r.updateHostFirmwareSettings(prov, info); err != nil { + if err = r.updateHostFirmwareSettings(currentSettings, schema, info); err != nil { return ctrl.Result{}, errors.Wrap(err, "Could not update hostFirmwareSettings") } @@ -157,20 +162,12 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl r.publishEvent(req, e) } - // only run again on changes - return ctrl.Result{}, nil + // requeue to run again after delay + return ctrl.Result{Requeue: true, RequeueAfter: reconcilerRequeueDelay}, nil } // Get the firmware settings from the provisioner and update hostFirmwareSettings -func (r *HostFirmwareSettingsReconciler) updateHostFirmwareSettings(prov provisioner.Provisioner, info *rInfo) (err error) { - - info.log.Info("retrieving firmware settings and saving to resource", "node", info.bmh.Status.Provisioning.ID) - - // Get the current settings and schema - currentSettings, schema, err := prov.GetFirmwareSettings(true) - if err != nil { - return errors.Wrap(err, "could not get firmware settings from provisioner") - } +func (r *HostFirmwareSettingsReconciler) updateHostFirmwareSettings(currentSettings metal3v1alpha1.SettingsMap, schema map[string]metal3v1alpha1.SettingSchema, info *rInfo) (err error) { // get or create a firmwareSchema to hold schema firmwareSchema, err := r.getOrCreateFirmwareSchema(info, schema) @@ -197,12 +194,13 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta info.hfs.Status.Settings = make(metal3v1alpha1.SettingsMap) } - // Update Status + // Update Status on changes for k, v := range settings { // Some vendors include encrypted password fields, don't add these if strings.Contains(k, "Password") { continue } + info.hfs.Status.Settings[k] = v } @@ -214,6 +212,10 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta specMismatch = true break } + } else { + // Spec setting is not in Status, this will be handled by validateHostFirmwareSettings + specMismatch = true + break } } @@ -222,26 +224,33 @@ func (r *HostFirmwareSettingsReconciler) updateStatus(info *rInfo, settings meta generation := info.hfs.GetGeneration() if specMismatch { - setCondition(generation, &info.hfs.Status, metal3v1alpha1.UpdateRequested, metav1.ConditionTrue, reason, "") + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsChangeDetected, metav1.ConditionTrue, reason, "") - } else { - setCondition(generation, &info.hfs.Status, metal3v1alpha1.UpdateRequested, metav1.ConditionFalse, reason, "") - } + // Run validation on the Spec to detect invalid values entered by user, including Spec settings not in Status + // Eventually this will be handled by a webhook + errors := r.validateHostFirmwareSettings(info, schema) + if len(errors) == 0 { + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionTrue, reason, "") + } else { + for _, error := range errors { + info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid BIOS setting: %v", error)) + } - // Run validation on the Spec to detect invalid values entered by user, including Spec settings not in Status - // Eventually this will be handled by a webhook - errors := r.validateHostFirmwareSettings(info, schema) - if len(errors) == 0 { - setCondition(generation, &info.hfs.Status, metal3v1alpha1.SettingsValid, metav1.ConditionTrue, reason, "") + reason = reasonConfigurationError + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionFalse, reason, "Invalid BIOS setting") + } } else { - for _, error := range errors { - info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid BIOS setting: %v", error)) + // Reset conditions + if meta.IsStatusConditionTrue(info.hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsChangeDetected)) { + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsChangeDetected, metav1.ConditionFalse, reason, "") + } + if !meta.IsStatusConditionTrue(info.hfs.Status.Conditions, string(metal3v1alpha1.FirmwareSettingsValid)) { + setCondition(generation, &info.hfs.Status, metal3v1alpha1.FirmwareSettingsValid, metav1.ConditionTrue, reason, "") } - - reason = reasonConfigurationError - setCondition(generation, &info.hfs.Status, metal3v1alpha1.SettingsValid, metav1.ConditionFalse, reason, "Invalid BIOS setting") } + t := metav1.Now() + info.hfs.Status.LastUpdated = &t return r.Status().Update(context.TODO(), info.hfs) } @@ -259,12 +268,19 @@ func (r *HostFirmwareSettingsReconciler) getOrCreateFirmwareSchema(info *rInfo, info.log.Info("found existing firmwareSchema resource") + // Add hfs as owner so can be garbage collected on delete, if already an owner it will just be overwritten + if err = controllerutil.SetOwnerReference(info.hfs, firmwareSchema, r.Scheme()); err != nil { + return nil, errors.Wrap(err, "could not set owner of existing firmwareSchema") + } + if err = r.Update(context.TODO(), firmwareSchema); err != nil { + return nil, err + } + return firmwareSchema, nil } if !k8serrors.IsNotFound(err) { // Error reading the object return nil, err - } firmwareSchema = &metal3v1alpha1.FirmwareSchema{ @@ -285,15 +301,12 @@ func (r *HostFirmwareSettingsReconciler) getOrCreateFirmwareSchema(info *rInfo, for k, v := range schema { firmwareSchema.Spec.Schema[k] = v } - - if err = r.Create(context.TODO(), firmwareSchema); err != nil { - return nil, err - } - // Set hfs as owner after the create - if controllerutil.SetOwnerReference(info.hfs, firmwareSchema, r.Scheme()); err != nil { + // Set hfs as owner + if err = controllerutil.SetOwnerReference(info.hfs, firmwareSchema, r.Scheme()); err != nil { return nil, errors.Wrap(err, "could not set owner of firmwareSchema") } - if err = r.Update(context.TODO(), firmwareSchema); err != nil { + + if err = r.Create(context.TODO(), firmwareSchema); err != nil { return nil, err } @@ -302,91 +315,11 @@ func (r *HostFirmwareSettingsReconciler) getOrCreateFirmwareSchema(info *rInfo, return firmwareSchema, nil } -// Create a hostFirmwareSettings -func (r *HostFirmwareSettingsReconciler) newHostFirmwareSettings(info *rInfo, namespacedName types.NamespacedName) (err error) { - - info.hfs.ObjectMeta = metav1.ObjectMeta{ - Name: namespacedName.Name, Namespace: namespacedName.Namespace} - info.hfs.Status.Settings = make(metal3v1alpha1.SettingsMap) - info.hfs.Spec.Settings = make(metal3v1alpha1.DesiredSettingsMap) - - if err = r.Create(context.TODO(), info.hfs); err != nil { - return errors.Wrap(err, "failure creating hostFirmwareSettings resource") - } - - // Set bmh as owner, this makes sure the resource is deleted when bmh is deleted - if controllerutil.SetControllerReference(info.bmh, info.hfs, r.Scheme()); err != nil { - return errors.Wrap(err, "could not set bmh as controller") - } - if err = r.Update(context.TODO(), info.hfs); err != nil { - return errors.Wrap(err, "could not update hostfirmwaresettings") - } - - info.log.Info("created new hostFirmwareSettings resource") - - return nil -} - -// EventHandler for updates to both the baremetalhost and hostfirmwarettings -func (r *HostFirmwareSettingsReconciler) updateEventHandler(e event.UpdateEvent) bool { - - r.Log.Info("hostfirmwaresettings in event handler") - - // If this is a baremetalhost, only return true on certain state transitions - var oldState metal3v1alpha1.ProvisioningState = metal3v1alpha1.StateNone - newState := oldState - - oldHost, oldHostExists := e.ObjectOld.(*metal3v1alpha1.BareMetalHost) - if oldHostExists { - oldState = oldHost.Status.Provisioning.State - } - newHost, newHostExists := e.ObjectNew.(*metal3v1alpha1.BareMetalHost) - if newHostExists { - newState = newHost.Status.Provisioning.State - } - - if newHostExists && !oldHostExists { - // The baremetalhost has been created - return true - } - if oldHostExists || newHostExists { - // Data needs to be retrieved from Ironic when node is moved to Ready e.g. after cleaning and when - // provisioned, its not necessary on every state change as data from Ironic will be the same. - if (newState == metal3v1alpha1.StateReady || newState == metal3v1alpha1.StateProvisioned) && oldState != newState { - r.Log.Info("baremetalhost event state update", "oldstate", oldState, "newstate", newState) - return true - } - - return false - } - - _, oldHFS := e.ObjectOld.(*metal3v1alpha1.HostFirmwareSettings) - _, newHFS := e.ObjectNew.(*metal3v1alpha1.HostFirmwareSettings) - if !(oldHFS || newHFS) { - // Don't process if not a hostFirmwareSetting - return false - } - - // This is a hostfirmwaresettings resource, if the update increased the resource generation then process it - // changes to LastUpdated will not increase the resource generation - if e.ObjectNew.GetGeneration() != e.ObjectOld.GetGeneration() { - r.Log.Info("hostfirmwaresettings resource generation changed, updating") - return true - } - - return false -} - // SetupWithManager sets up the controller with the Manager. func (r *HostFirmwareSettingsReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&metal3v1alpha1.HostFirmwareSettings{}). - WithEventFilter( - predicate.Funcs{ - UpdateFunc: r.updateEventHandler, - }). - Watches(&source.Kind{Type: &metal3v1alpha1.BareMetalHost{}}, &handler.EnqueueRequestForObject{}, builder.Predicates{}). Complete(r) } @@ -447,19 +380,37 @@ func setCondition(generation int64, status *metal3v1alpha1.HostFirmwareSettingsS meta.SetStatusCondition(&status.Conditions, newCondition) } -// Generate a name based on the schema keys which should be the same for similar hardware +// Generate a name based on the schema key and values which should be the same for similar hardware func GetSchemaName(schema map[string]metal3v1alpha1.SettingSchema) string { // Schemas from the same vendor and model should be identical for both keys and values. - // Hash the keys of the map to get a identifier unique to this schema. - keys := make([]string, 0, len(schema)) - for k := range schema { - keys = append(keys, k) + hashkeys := make([]string, 0, len(schema)) + for k, v := range schema { + hashkeys = append(hashkeys, k) + hashkeys = append(hashkeys, v.AttributeType) + for _, av := range v.AllowableValues { + hashkeys = append(hashkeys, av) + } + if v.LowerBound != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.LowerBound)) + } + if v.UpperBound != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.UpperBound)) + } + if v.MinLength != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.MinLength)) + } + if v.MaxLength != nil { + hashkeys = append(hashkeys, strconv.Itoa(*v.MaxLength)) + } + if v.ReadOnly != nil { + hashkeys = append(hashkeys, strconv.FormatBool(*v.ReadOnly)) + } } - sort.Strings(keys) + sort.Strings(hashkeys) h := sha256.New() - h.Write([]byte(fmt.Sprintf("%v", keys))) + h.Write([]byte(fmt.Sprintf("%v", hashkeys))) hash := fmt.Sprintf("%x", h.Sum(nil))[:8] return "schema-" + hash diff --git a/controllers/metal3.io/hostfirmwaresettings_test.go b/controllers/metal3.io/hostfirmwaresettings_test.go index c87d5a0dc0..9e99c9564f 100644 --- a/controllers/metal3.io/hostfirmwaresettings_test.go +++ b/controllers/metal3.io/hostfirmwaresettings_test.go @@ -21,7 +21,7 @@ import ( const ( hostName string = "myHostName" hostNamespace string = "myHostNamespace" - schemaName string = "schema-17e7ebad" // Hash generated from schema, change this if the schema is changed + schemaName string = "schema-4bcc035f" // Hash generated from schema, change this if the schema is changed ) var ( @@ -67,10 +67,6 @@ func (m *hsfMockProvisioner) ValidateManagementAccess(data provisioner.Managemen return } -func (m *hsfMockProvisioner) PreprovisioningImageFormats() ([]metal3v1alpha1.ImageFormat, error) { - return nil, nil -} - func (m *hsfMockProvisioner) InspectHardware(data provisioner.InspectData, force, refresh bool) (result provisioner.Result, started bool, details *metal3v1alpha1.HardwareDetails, err error) { return } @@ -133,7 +129,7 @@ func getSchema() *metal3v1alpha1.FirmwareSchema { { APIVersion: "metal3.io/v1alpha1", Kind: "HostFirmwareSettings", - Name: hostName, + Name: "dummyhfs", }, }, }, @@ -214,8 +210,36 @@ func createBaremetalHost() *metal3v1alpha1.BareMetalHost { return bmh } -func getExpectedSchemaResource() *metal3v1alpha1.FirmwareSchema { +func getExpectedSchema() *metal3v1alpha1.FirmwareSchema { + firmwareSchema := getSchema() + firmwareSchema.ObjectMeta.ResourceVersion = "1" + firmwareSchema.ObjectMeta.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: "metal3.io/v1alpha1", + Kind: "HostFirmwareSettings", + Name: hostName, + }, + } + firmwareSchema.Spec.Schema = getCurrentSchemaSettings() + + return firmwareSchema +} + +func getExpectedSchemaTwoOwners() *metal3v1alpha1.FirmwareSchema { firmwareSchema := getSchema() + firmwareSchema.ObjectMeta.ResourceVersion = "2" + firmwareSchema.ObjectMeta.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: "metal3.io/v1alpha1", + Kind: "HostFirmwareSettings", + Name: "dummyhfs", + }, + { + APIVersion: "metal3.io/v1alpha1", + Kind: "HostFirmwareSettings", + Name: hostName, + }, + } firmwareSchema.Spec.Schema = getCurrentSchemaSettings() return firmwareSchema @@ -295,7 +319,6 @@ func TestStoreHostFirmwareSettings(t *testing.T) { "SecureBoot": "Enabled", }, Conditions: []metav1.Condition{ - {Type: "UpdateRequested", Status: "False", Reason: "Success"}, {Type: "Valid", Status: "True", Reason: "Success"}, }, }, @@ -335,7 +358,6 @@ func TestStoreHostFirmwareSettings(t *testing.T) { "SecureBoot": "Enabled", }, Conditions: []metav1.Condition{ - {Type: "UpdateRequested", Status: "False", Reason: "Success"}, {Type: "Valid", Status: "True", Reason: "Success"}, }, }, @@ -356,6 +378,7 @@ func TestStoreHostFirmwareSettings(t *testing.T) { Settings: metal3v1alpha1.DesiredSettingsMap{ "NetworkBootRetryCount": intstr.FromString("10"), "ProcVirtualization": intstr.FromString("Enabled"), + "AssetTag": intstr.FromString("Z98765432"), }, }, Status: metal3v1alpha1.HostFirmwareSettingsStatus{ @@ -364,6 +387,7 @@ func TestStoreHostFirmwareSettings(t *testing.T) { Namespace: hostNamespace, }, Settings: metal3v1alpha1.SettingsMap{ + "AssetTag": "Z98765432", "L2Cache": "10x256 KB", "NetworkBootRetryCount": "10", "ProcVirtualization": "Enabled", @@ -376,6 +400,7 @@ func TestStoreHostFirmwareSettings(t *testing.T) { Settings: metal3v1alpha1.DesiredSettingsMap{ "NetworkBootRetryCount": intstr.FromString("10"), "ProcVirtualization": intstr.FromString("Enabled"), + "AssetTag": intstr.FromString("Z98765432"), }, }, Status: metal3v1alpha1.HostFirmwareSettingsStatus{ @@ -391,7 +416,7 @@ func TestStoreHostFirmwareSettings(t *testing.T) { "SecureBoot": "Enabled", }, Conditions: []metav1.Condition{ - {Type: "UpdateRequested", Status: "True", Reason: "Success"}, + {Type: "ChangeDetected", Status: "True", Reason: "Success"}, {Type: "Valid", Status: "True", Reason: "Success"}, }, }, @@ -447,7 +472,7 @@ func TestStoreHostFirmwareSettings(t *testing.T) { "SecureBoot": "Enabled", }, Conditions: []metav1.Condition{ - {Type: "UpdateRequested", Status: "True", Reason: "Success"}, + {Type: "ChangeDetected", Status: "True", Reason: "Success"}, {Type: "Valid", Status: "False", Reason: "ConfigurationError", Message: "Invalid BIOS setting"}, }, }, @@ -482,14 +507,17 @@ func TestStoreHostFirmwareSettings(t *testing.T) { } if tc.CreateSchemaResource { + // Create an existing schema with different hfs owner firmwareSchema := getSchema() firmwareSchema.Spec.Schema = getCurrentSchemaSettings() r.Client.Create(ctx, firmwareSchema) - r.Client.Update(ctx, firmwareSchema) // in order to set resource version } - err := r.updateHostFirmwareSettings(prov, info) + currentSettings, schema, err := prov.GetFirmwareSettings(true) + assert.Equal(t, nil, err) + + err = r.updateHostFirmwareSettings(currentSettings, schema, info) assert.Equal(t, nil, err) // Check that resources get created or updated @@ -501,11 +529,12 @@ func TestStoreHostFirmwareSettings(t *testing.T) { // Use the same time for expected and actual currentTime := metav1.Now() - for i := 0; i < 2; i++ { + tc.ExpectedSettings.Status.LastUpdated = ¤tTime + actualSettings.Status.LastUpdated = ¤tTime + for i := range tc.ExpectedSettings.Status.Conditions { tc.ExpectedSettings.Status.Conditions[i].LastTransitionTime = currentTime actualSettings.Status.Conditions[i].LastTransitionTime = currentTime } - assert.Equal(t, tc.ExpectedSettings, actualSettings) key = client.ObjectKey{ @@ -513,8 +542,12 @@ func TestStoreHostFirmwareSettings(t *testing.T) { actualSchema := &metal3v1alpha1.FirmwareSchema{} err = r.Client.Get(ctx, key, actualSchema) assert.Equal(t, nil, err) - expectedSchema := getExpectedSchemaResource() - expectedSchema.ObjectMeta.ResourceVersion = "2" + var expectedSchema *metal3v1alpha1.FirmwareSchema + if tc.CreateSchemaResource { + expectedSchema = getExpectedSchemaTwoOwners() + } else { + expectedSchema = getExpectedSchema() + } assert.Equal(t, expectedSchema, actualSchema) }) } @@ -605,7 +638,7 @@ func TestValidateHostFirmwareSettings(t *testing.T) { hfs: hfs, } - errors := r.validateHostFirmwareSettings(info, getExpectedSchemaResource()) + errors := r.validateHostFirmwareSettings(info, getExpectedSchema()) if len(errors) == 0 { assert.Equal(t, tc.ExpectedError, "") } else { diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index 4431701ddc..f7e358b73a 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -1161,7 +1161,7 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails bmcConfig := bmc.FirmwareConfig(*data.FirmwareConfig) firmwareConfig = &bmcConfig } - bmcsettings, err := bmcAccess.BuildBIOSSettings(firmwareConfig) + fwConfigSettings, err := bmcAccess.BuildBIOSSettings(firmwareConfig) if err != nil { return nil, err } @@ -1170,13 +1170,13 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails 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 _, bmcsetting := range bmcsettings { - if val, exists := data.ActualFirmwareSettings[bmcsetting["name"]]; exists { - if bmcsetting["value"] != val { - newSettings = buildFirmwareSettings(newSettings, bmcsetting["name"], bmcsetting["value"]) + for _, fwConfigSetting := range fwConfigSettings { + if val, exists := data.ActualFirmwareSettings[fwConfigSetting["name"]]; exists { + if fwConfigSetting["value"] != val { + newSettings = buildFirmwareSettings(newSettings, fwConfigSetting["name"], fwConfigSetting["value"]) } } else { - p.log.Info("name converted from bmc driver not found in firmware settings", "name", bmcsetting["name"], "node", p.nodeID) + p.log.Info("name converted from bmc driver not found in firmware settings", "name", fwConfigSetting["name"], "node", p.nodeID) } } @@ -1184,14 +1184,20 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails if data.TargetFirmwareSettings != nil { for k, v := range data.TargetFirmwareSettings { if data.ActualFirmwareSettings[k] != v.String() { + // Skip changing this setting if it was defined in the vendor specific settings + for _, fwConfigSetting := range fwConfigSettings { + if fwConfigSetting["name"] == k { + continue + } + } newSettings = buildFirmwareSettings(newSettings, k, v.String()) } } } } else { // use only the settings converted by bmc driver - for _, bmcsetting := range bmcsettings { - newSettings = append(newSettings, bmcsetting) + for _, fwConfigSetting := range fwConfigSettings { + newSettings = append(newSettings, fwConfigSetting) } }