Repository: metal3-io/baremetal-operator
Pull request #852 (2021-04-12 20:57:33)

Always lazily update Node options in Ironic
It became clear from #839 and #849 that in order to ensure consistent behaviour, we must ensure that the Ironic Node's image data is up to date on every reconcile, and this was implemented in #845 by unconditionally updating it. This will result in a write to Ironic on every reconcile, which is excessive.

We have the ability to check whether the data we want to set is already present, but we largely don't because of the huge amount of messy boilerplate currently required to update each option. This PR refactors the code so that we can just pass a map of the options we want to set to a function and lazy updates come for free, along with standardised logging.

metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/852#" title="Author self-approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)~~ [zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
zaneb: /test-integration
zaneb: /test-integration

Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 75bcf635..82a9fae3 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -403,7 +403,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 	}
 
 	var ironicNode *nodes.Node
-	var updates nodes.UpdateOpts
+	updater := updateOptsBuilder(p.debugLog)
 
 	p.debugLog.Info("validating management access")
 
@@ -499,72 +499,29 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 		// if there are differences.
 		provID = ironicNode.UUID
 
-		if ironicNode.Name != ironicNodeName(p.objectMeta) {
-			updates = append(
-				updates,
-				nodes.UpdateOperation{
-					Op:    nodes.ReplaceOp,
-					Path:  "/name",
-					Value: ironicNodeName(p.objectMeta),
-				},
-			)
-		}
+		updater.SetTopLevelOpt("name", ironicNodeName(p.objectMeta), ironicNode.Name)
 
 		// Look for the case where we previously enrolled this node
 		// and now the credentials have changed.
 		if credentialsChanged {
-			updates = append(
-				updates,
-				nodes.UpdateOperation{
-					Op:    nodes.ReplaceOp,
-					Path:  "/driver_info",
-					Value: driverInfo,
-				},
-			)
-			// We don't return here because we also have to set the
-			// target provision state to manageable, which happens
-			// below.
+			updater.SetTopLevelOpt("driver_info", driverInfo, nil)
 		}
+
+		// We don't return here because we also have to set the
+		// target provision state to manageable, which happens
+		// below.
 	}
 	if data.CurrentImage != nil {
-		updatesImage, optsErr := p.getImageUpdateOptsForNode(ironicNode, data.CurrentImage, data.BootMode)
-		if optsErr != nil {
-			result, err = transientError(errors.Wrap(optsErr, "Could not get Image options for node"))
-			return
-		}
-		if len(updatesImage) != 0 {
-			updates = append(updates, updatesImage...)
-		}
+		p.getImageUpdateOptsForNode(ironicNode, data.CurrentImage, data.BootMode, updater)
 	}
-	if ironicNode.AutomatedClean == nil ||
-		(data.AutomatedCleaningMode == metal3v1alpha1.CleaningModeDisabled && *ironicNode.AutomatedClean) ||
-		(data.AutomatedCleaningMode == metal3v1alpha1.CleaningModeMetadata && !*ironicNode.AutomatedClean) {
-		p.log.Info("setting automated cleaning mode to",
-			"ID", ironicNode.UUID,
-			"mode", data.AutomatedCleaningMode)
-
-		value := data.AutomatedCleaningMode != metal3v1alpha1.CleaningModeDisabled
-		updates = append(
-			updates,
-			nodes.UpdateOperation{
-				Op:    nodes.ReplaceOp,
-				Path:  "/automated_clean",
-				Value: value,
-			},
-		)
-	}
-	if len(updates) != 0 {
-		_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
-		switch err.(type) {
-		case nil:
-		case gophercloud.ErrDefault409:
-			p.log.Info("could not update host driver settings, busy")
-			result, err = retryAfterDelay(provisionRequeueDelay)
-			return
-		default:
-			result, err = transientError(errors.Wrap(err, "failed to update host settings in ironic"))
-			return
-		}
+	updater.SetTopLevelOpt("automated_clean",
+		data.AutomatedCleaningMode != metal3v1alpha1.CleaningModeDisabled,
+		ironicNode.AutomatedClean)
+
+	var success bool
+	success, result, err = p.tryUpdateNode(ironicNode, updater)
+	if !success {
+		return
 	}
 	// ironicNode, err = nodes.Get(p.client, p.status.ID).Extract()
 	// if err != nil {
@@ -625,6 +582,26 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 	}
 }
 
+func (p *ironicProvisioner) tryUpdateNode(ironicNode *nodes.Node, updater *nodeUpdater) (success bool, result provisioner.Result, err error) {
+	if len(updater.Updates) == 0 {
+		success = true
+		return
+	}
+
+	p.log.Info("updating node settings in ironic")
+	_, err = nodes.Update(p.client, ironicNode.UUID, updater.Updates).Extract()
+	switch err.(type) {
+	case nil:
+		success = true
+	case gophercloud.ErrDefault409:
+		p.log.Info("could not update node settings in ironic, busy")
+		result, err = retryAfterDelay(provisionRequeueDelay)
+	default:
+		result, err = transientError(errors.Wrap(err, "failed to update host settings in ironic"))
+	}
+	return
+}
+
 func (p *ironicProvisioner) tryChangeNodeProvisionState(ironicNode *nodes.Node, opts nodes.ProvisionStateOpts) (success bool, result provisioner.Result, err error) {
 	p.log.Info("changing provisioning state",
 		"current", ironicNode.ProvisionState,
@@ -688,29 +665,19 @@ func (p *ironicProvisioner) InspectHardware(data provisioner.InspectData, force,
 				}
 				fallthrough
 			default:
-				p.log.Info("updating boot mode before hardware inspection")
-				op, value := buildCapabilitiesValue(ironicNode, data.BootMode)
-				updates := nodes.UpdateOpts{
-					nodes.UpdateOperation{
-						Op:    op,
-						Path:  "/properties/capabilities",
-						Value: value,
-					},
-				}
-				_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
-				switch err.(type) {
-				case nil:
-				case gophercloud.ErrDefault409:
-					p.log.Info("could not update host settings in ironic, busy")
-					result, err = retryAfterDelay(provisionRequeueDelay)
-					return
-				default:
-					result, err = transientError(errors.Wrap(err, "failed to update host boot mode settings in ironic"))
+				var success bool
+				success, result, err = p.tryUpdateNode(
+					ironicNode,
+					updateOptsBuilder(p.debugLog).
+						SetPropertiesOpts(optionsData{
+							"capabilities": buildCapabilitiesValue(ironicNode, data.BootMode),
+						}, ironicNode),
+				)
+				if !success {
 					return
 				}
 
 				p.log.Info("starting new hardware inspection")
-				var success bool
 				success, result, err = p.tryChangeNodeProvisionState(
 					ironicNode,
 					nodes.ProvisionStateOpts{Target: nodes.TargetInspect},
@@ -775,297 +742,99 @@ func (p *ironicProvisioner) UpdateHardwareState() (hwState provisioner.HardwareS
 	return
 }
 
-func (p *ironicProvisioner) setLiveIsoUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image, updates nodes.UpdateOpts) (nodes.UpdateOpts, error) {
-	var op nodes.UpdateOp
+func (p *ironicProvisioner) setLiveIsoUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image, updater *nodeUpdater) {
+	optValues := optionsData{
+		"boot_iso": imageData.URL,
 
-	if _, ok := ironicNode.InstanceInfo["boot_iso"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding boot_iso")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating boot_iso")
-	}
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/instance_info/boot_iso",
-			Value: imageData.URL,
-		},
-	)
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    nodes.ReplaceOp,
-			Path:  "/deploy_interface",
-			Value: "ramdisk",
-		},
-	)
-	// remove any image_source or checksum options
-	removals := []string{
-		"image_source", "image_os_hash_value", "image_os_hash_algo", "image_checksum"}
-	op = nodes.RemoveOp
-	for _, item := range removals {
-		if _, ok := ironicNode.InstanceInfo[item]; ok {
-			p.log.Info("removing " + item)
-			updates = append(
-				updates,
-				nodes.UpdateOperation{
-					Op:   op,
-					Path: "/instance_info/" + item,
-				},
-			)
-		}
+		// remove any image_source or checksum options
+		"image_source":        nil,
+		"image_os_hash_value": nil,
+		"image_os_hash_algo":  nil,
+		"image_checksum":      nil,
 	}
-	return updates, nil
+	updater.
+		SetInstanceInfoOpts(optValues, ironicNode).
+		SetTopLevelOpt("deploy_interface", "ramdisk", ironicNode.DeployInterface)
 }
 
-func (p *ironicProvisioner) setDirectDeployUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image, updates nodes.UpdateOpts) (nodes.UpdateOpts, error) {
+func (p *ironicProvisioner) setDirectDeployUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image, updater *nodeUpdater) {
 	checksum, checksumType, ok := imageData.GetChecksum()
 	if !ok {
 		p.log.Info("image/checksum not found for host")
-		return updates, nil
-	}
-
-	var op nodes.UpdateOp
-
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    nodes.ReplaceOp,
-			Path:  "/deploy_interface",
-			Value: "direct",
-		},
-	)
-	// Remove any boot_iso field
-	if _, ok := ironicNode.InstanceInfo["boot_iso"]; ok {
-		p.log.Info("removing boot_iso")
-		updates = append(
-			updates,
-			nodes.UpdateOperation{
-				Op:   nodes.RemoveOp,
-				Path: "/instance_info/boot_iso",
-			},
-		)
+		return
 	}
-	// image_source
-	if _, ok := ironicNode.InstanceInfo["image_source"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding image_source")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating image_source")
-	}
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/instance_info/image_source",
-			Value: imageData.URL,
-		},
-	)
-
-	// image_os_hash_algo
-	if _, ok := ironicNode.InstanceInfo["image_os_hash_algo"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding image_os_hash_algo")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating image_os_hash_algo")
-	}
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/instance_info/image_os_hash_algo",
-			Value: checksumType,
-		},
-	)
-
-	// image_os_hash_value
-	if _, ok := ironicNode.InstanceInfo["image_os_hash_value"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding image_os_hash_value")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating image_os_hash_value")
-	}
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/instance_info/image_os_hash_value",
-			Value: checksum,
-		},
-	)
 
-	// image_checksum
-	//
 	// FIXME: For older versions of ironic that do not have
 	// https://review.opendev.org/#/c/711816/ failing to include the
 	// 'image_checksum' causes ironic to refuse to provision the
 	// image, even if the other hash value parameters are given. We
 	// only want to do that for MD5, however, because those versions
 	// of ironic only support MD5 checksums.
+	var legacyChecksum *string
 	if checksumType == string(metal3v1alpha1.MD5) {
-		if _, ok := ironicNode.InstanceInfo["image_checksum"]; !ok {
-			op = nodes.AddOp
-			p.log.Info("adding image_checksum")
-		} else {
-			op = nodes.ReplaceOp
-			p.log.Info("updating image_checksum")
-		}
-		updates = append(
-			updates,
-			nodes.UpdateOperation{
-				Op:    op,
-				Path:  "/instance_info/image_checksum",
-				Value: checksum,
-			},
-		)
+		legacyChecksum = &checksum
 	}
 
-	if imageData.DiskFormat != nil {
-		updates = append(updates, nodes.UpdateOperation{
-			Op:    nodes.AddOp,
-			Path:  "/instance_info/image_disk_format",
-			Value: *imageData.DiskFormat,
-		})
-	}
+	optValues := optionsData{
+		// Remove any boot_iso field
+		"boot_iso": nil,
 
-	return updates, nil
+		"image_source":        imageData.URL,
+		"image_os_hash_algo":  checksumType,
+		"image_os_hash_value": checksum,
+		"image_checksum":      legacyChecksum,
+		"image_disk_format":   imageData.DiskFormat,
+	}
+	updater.
+		SetInstanceInfoOpts(optValues, ironicNode).
+		SetTopLevelOpt("deploy_interface", "direct", ironicNode.DeployInterface)
 }
 
-func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image, bootMode metal3v1alpha1.BootMode) (updates nodes.UpdateOpts, err error) {
+func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image, bootMode metal3v1alpha1.BootMode, updater *nodeUpdater) {
 	// instance_uuid
-	p.log.Info("setting instance_uuid")
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    nodes.ReplaceOp,
-			Path:  "/instance_uuid",
-			Value: string(p.objectMeta.UID),
-		},
-	)
+	updater.SetTopLevelOpt("instance_uuid", string(p.objectMeta.UID), ironicNode.InstanceUUID)
 
 	// Secure boot is a normal capability that goes into instance_info (we
 	// also put it to properties for consistency, although it's not
 	// strictly required in our case).
 
+	// Instance info capabilities were invented later and
+	// use a normal JSON mapping instead of a custom
+	// string value.
+	capabilitiesII := map[string]string{}
 	if bootMode == metal3v1alpha1.UEFISecureBoot {
-		updates = append(updates, nodes.UpdateOperation{
-			Op:   nodes.AddOp,
-			Path: "/instance_info/capabilities",
-			// Instance info capabilities were invented later and
-			// use a normal JSON mapping instead of a custom
-			// string value.
-			Value: map[string]string{
-				"secure_boot": "true",
-			},
-		})
-	} else {
-		updates = append(updates, nodes.UpdateOperation{
-			Op:    nodes.AddOp,
-			Path:  "/instance_info/capabilities",
-			Value: map[string]string{},
-		})
+		capabilitiesII["secure_boot"] = "true"
 	}
 
-	// Set live-iso format options
+	updater.SetInstanceInfoOpts(optionsData{"capabilities": capabilitiesII}, ironicNode)
+
 	if imageData.DiskFormat != nil && *imageData.DiskFormat == "live-iso" {
-		return p.setLiveIsoUpdateOptsForNode(ironicNode, imageData, updates)
+		// Set live-iso format options
+		p.setLiveIsoUpdateOptsForNode(ironicNode, imageData, updater)
+	} else {
+		// Set deploy_interface direct options when not booting a live-iso
+		p.setDirectDeployUpdateOptsForNode(ironicNode, imageData, updater)
 	}
-
-	// Set deploy_interface direct options when not booting a live-iso
-	return p.setDirectDeployUpdateOptsForNode(ironicNode, imageData, updates)
 }
 
-func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node, data provisioner.ProvisionData) (updates nodes.UpdateOpts, err error) {
-	imageOpts, err := p.getImageUpdateOptsForNode(ironicNode, &data.Image, data.BootMode)
-	if err != nil {
-		return updates, errors.Wrap(err, "Could not get Image options for node")
-	}
-	updates = append(updates, imageOpts...)
-
-	// root_device
-	//
-	// FIXME(dhellmann): We need to specify the root device to receive
-	// the image. That should come from some combination of inspecting
-	// the host to see what is available and the hardware profile to
-	// give us instructions.
-	var op nodes.UpdateOp
-	if _, ok := ironicNode.Properties["root_device"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding root_device")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating root_device")
-	}
-
-	// hints
-	//
-	// If the user has provided explicit root device hints, they take
-	// precedence. Otherwise use the values from the hardware profile.
-	hints := devicehints.MakeHintMap(data.RootDeviceHints)
-	p.log.Info("using root device", "hints", hints)
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/properties/root_device",
-			Value: hints,
-		},
-	)
+func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node, data provisioner.ProvisionData) *nodeUpdater {
+	updater := updateOptsBuilder(p.debugLog)
 
-	// cpu_arch
-	//
-	// FIXME(dhellmann): This should come from inspecting the
-	// host.
-	if _, ok := ironicNode.Properties["cpu_arch"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding cpu_arch")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating cpu_arch")
-	}
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/properties/cpu_arch",
-			Value: data.HardwareProfile.CPUArch,
-		},
-	)
+	p.getImageUpdateOptsForNode(ironicNode, &data.Image, data.BootMode, updater)
 
-	// local_gb
-	if _, ok := ironicNode.Properties["local_gb"]; !ok {
-		op = nodes.AddOp
-		p.log.Info("adding local_gb")
-	} else {
-		op = nodes.ReplaceOp
-		p.log.Info("updating local_gb")
-	}
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/properties/local_gb",
-			Value: data.HardwareProfile.LocalGB,
-		},
-	)
+	opts := optionsData{
+		"root_device": devicehints.MakeHintMap(data.RootDeviceHints),
 
-	// boot_mode
-	op, value := buildCapabilitiesValue(ironicNode, data.BootMode)
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    op,
-			Path:  "/properties/capabilities",
-			Value: value,
-		},
-	)
+		// FIXME(dhellmann): This should come from inspecting the host.
+		"cpu_arch": data.HardwareProfile.CPUArch,
+
+		"local_gb": data.HardwareProfile.LocalGB,
+
+		"capabilities": buildCapabilitiesValue(ironicNode, data.BootMode),
+	}
+	updater.SetPropertiesOpts(opts, ironicNode)
 
-	return updates, nil
+	return updater
 }
 
 // We can't just replace the capabilities because we need to keep the
@@ -1074,23 +843,19 @@ func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node, data pr
 // look at the existing value and modify it. This function
 // encapsulates the logic for building the value and knowing which
 // update operation to use with the results.
-func buildCapabilitiesValue(ironicNode *nodes.Node, bootMode metal3v1alpha1.BootMode) (op nodes.UpdateOp, value string) {
+func buildCapabilitiesValue(ironicNode *nodes.Node, bootMode metal3v1alpha1.BootMode) string {
 
 	capabilities, ok := ironicNode.Properties["capabilities"]
 	if !ok {
 		// There is no existing capabilities value
-		return nodes.AddOp, bootModeCapabilities[bootMode]
+		return bootModeCapabilities[bootMode]
 	}
 	existingCapabilities := capabilities.(string)
 
-	// The capabilities value is set, so we will want to replace it.
-	op = nodes.ReplaceOp
-
 	if existingCapabilities == "" {
 		// The existing value is empty so we can replace the whole
 		// thing.
-		value = bootModeCapabilities[bootMode]
-		return
+		return bootModeCapabilities[bootMode]
 	}
 
 	var filteredCapabilities []string
@@ -1101,26 +866,17 @@ func buildCapabilitiesValue(ironicNode *nodes.Node, bootMode metal3v1alpha1.Boot
 	}
 	filteredCapabilities = append(filteredCapabilities, bootModeCapabilities[bootMode])
 
-	value = strings.Join(filteredCapabilities, ",")
-	return
+	return strings.Join(filteredCapabilities, ",")
 }
 
 func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, data provisioner.ProvisionData) (result provisioner.Result, err error) {
 
 	p.log.Info("starting provisioning", "node properties", ironicNode.Properties)
 
-	updates, err := p.getUpdateOptsForNode(ironicNode, data)
-	if err != nil {
-		return transientError(errors.Wrap(err, "failed to update opts for node"))
-	}
-	_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
-	switch err.(type) {
-	case nil:
-	case gophercloud.ErrDefault409:
-		p.log.Info("could not update host settings in ironic, busy")
-		return retryAfterDelay(provisionRequeueDelay)
-	default:
-		return transientError(errors.Wrap(err, "failed to update host settings in ironic"))
+	success, result, err := p.tryUpdateNode(ironicNode,
+		p.getUpdateOptsForNode(ironicNode, data))
+	if !success {
+		return
 	}
 
 	p.log.Info("validating host settings")
@@ -1488,24 +1244,13 @@ func (p *ironicProvisioner) Provision(data provisioner.ProvisionData) (result pr
 }
 
 func (p *ironicProvisioner) setMaintenanceFlag(ironicNode *nodes.Node, value bool) (result provisioner.Result, err error) {
-	_, err = nodes.Update(
-		p.client,
-		ironicNode.UUID,
-		nodes.UpdateOpts{
-			nodes.UpdateOperation{
-				Op:    nodes.ReplaceOp,
-				Path:  "/maintenance",
-				Value: value,
-			},
-		},
-	).Extract()
-	switch err.(type) {
-	case nil:
-	case gophercloud.ErrDefault409:
-		p.log.Info("could not set host maintenance flag, busy")
-		return retryAfterDelay(provisionRequeueDelay)
-	default:
-		return transientError(errors.Wrap(err, "failed to set host maintenance flag"))
+	success, result, err := p.tryUpdateNode(ironicNode,
+		updateOptsBuilder(p.log).SetTopLevelOpt("maintenance", value, nil))
+	if err != nil {
+		err = fmt.Errorf("failed to set host maintenance flag to %v (%w)", value, err)
+	}
+	if !success {
+		return
 	}
 	return operationContinuing(0)
 }
diff --git a/pkg/provisioner/ironic/update_opts.go b/pkg/provisioner/ironic/update_opts.go
new file mode 100644
index 00000000..e60c40f7
--- /dev/null
+++ b/pkg/provisioner/ironic/update_opts.go
@@ -0,0 +1,162 @@
+package ironic
+
+import (
+	"fmt"
+	"reflect"
+
+	"github.com/go-logr/logr"
+
+	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
+)
+
+type optionsData map[string]interface{}
+
+func optionValueEqual(current, value interface{}) bool {
+	if reflect.DeepEqual(current, value) {
+		return true
+	}
+	switch curVal := current.(type) {
+	case []interface{}:
+		// newType could reasonably be either []interface{} or e.g. []string,
+		// so we must use reflection.
+		newType := reflect.TypeOf(value)
+		switch newType.Kind() {
+		case reflect.Slice, reflect.Array:
+		default:
+			return false
+		}
+		newList := reflect.ValueOf(value)
+		if newList.Len() != len(curVal) {
+			return false
+		}
+		for i, v := range curVal {
+			if !optionValueEqual(newList.Index(i).Interface(), v) {
+				return false
+			}
+		}
+		return true
+	case map[string]interface{}:
+		// newType could reasonably be either map[string]interface{} or
+		// e.g. map[string]string, so we must use reflection.
+		newType := reflect.TypeOf(value)
+		if newType.Kind() != reflect.Map ||
+			newType.Key().Kind() != reflect.String {
+			return false
+		}
+		newMap := reflect.ValueOf(value)
+		if newMap.Len() != len(curVal) {
+			return false
+		}
+		for k, v := range curVal {
+			newV := newMap.MapIndex(reflect.ValueOf(k))
+			if !(newV.IsValid() && optionValueEqual(newV.Interface(), v)) {
+				return false
+			}
+		}
+		return true
+	}
+	return false
+}
+
+func deref(v interface{}) interface{} {
+	if v == nil {
+		return nil
+	}
+	if reflect.TypeOf(v).Kind() != reflect.Ptr {
+		return v
+	}
+	if ptrVal := reflect.ValueOf(v); ptrVal.IsNil() {
+		return nil
+	} else {
+		return ptrVal.Elem().Interface()
+	}
+}
+
+func getUpdateOperation(name string, currentData map[string]interface{}, desiredValue interface{}, path string, log logr.Logger) *nodes.UpdateOperation {
+	current, present := currentData[name]
+
+	desiredValue = deref(desiredValue)
+	if desiredValue != nil {
+		if !(present && optionValueEqual(deref(current), desiredValue)) {
+			if log != nil {
+				if present {
+					log.Info("updating option data",
+						"value", desiredValue, "old_value", current)
+				} else {
+					log.Info("adding option data",
+						"value", desiredValue)
+				}
+			}
+			return &nodes.UpdateOperation{
+				Op:    nodes.AddOp, // Add also does replace
+				Path:  path,
+				Value: desiredValue,
+			}
+		}
+	} else {
+		if present {
+			if log != nil {
+				log.Info("removing option data")
+			}
+			return &nodes.UpdateOperation{
+				Op:   nodes.RemoveOp,
+				Path: path,
+			}
+		}
+	}
+	return nil
+}
+
+type nodeUpdater struct {
+	Updates nodes.UpdateOpts
+	log     logr.Logger
+}
+
+func updateOptsBuilder(logger logr.Logger) *nodeUpdater {
+	return &nodeUpdater{
+		log: logger,
+	}
+}
+
+func (nu *nodeUpdater) logger(basepath, option string) logr.Logger {
+	if nu.log == nil {
+		return nil
+	}
+	log := nu.log.WithValues("option", option)
+	if basepath != "" {
+		log = log.WithValues("section", basepath[1:])
+	}
+	return log
+}
+
+func (nu *nodeUpdater) path(basepath, option string) string {
+	return fmt.Sprintf("%s/%s", basepath, option)
+}
+
+func (nu *nodeUpdater) setSectionUpdateOpts(currentData map[string]interface{}, settings optionsData, basepath string) {
+	for name, desiredValue := range settings {
+		updateOp := getUpdateOperation(name, currentData, desiredValue,
+			nu.path(basepath, name), nu.logger(basepath, name))
+		if updateOp != nil {
+			nu.Updates = append(nu.Updates, *updateOp)
+		}
+	}
+}
+
+func (nu *nodeUpdater) SetTopLevelOpt(name string, desiredValue, currentValue interface{}) *nodeUpdater {
+	currentData := map[string]interface{}{name: currentValue}
+	desiredData := optionsData{name: desiredValue}
+
+	nu.setSectionUpdateOpts(currentData, desiredData, "")
+	return nu
+}
+
+func (nu *nodeUpdater) SetPropertiesOpts(settings optionsData, node *nodes.Node) *nodeUpdater {
+	nu.setSectionUpdateOpts(node.Properties, settings, "/properties")
+	return nu
+}
+
+func (nu *nodeUpdater) SetInstanceInfoOpts(settings optionsData, node *nodes.Node) *nodeUpdater {
+	nu.setSectionUpdateOpts(node.InstanceInfo, settings, "/instance_info")
+	return nu
+}


Test patch:
diff --git a/pkg/provisioner/ironic/capabilities_test.go b/pkg/provisioner/ironic/capabilities_test.go
index 2a772a61..79df9727 100644
--- a/pkg/provisioner/ironic/capabilities_test.go
+++ b/pkg/provisioner/ironic/capabilities_test.go
@@ -15,14 +15,12 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 		Node          nodes.Node
 		Mode          metal3v1alpha1.BootMode
 		ExpectedValue string
-		ExpectedOp    nodes.UpdateOp
 	}{
 		{
 			Scenario:      "unset",
 			Node:          nodes.Node{},
 			Mode:          metal3v1alpha1.UEFI,
 			ExpectedValue: "boot_mode:uefi",
-			ExpectedOp:    nodes.AddOp,
 		},
 		{
 			Scenario: "empty",
@@ -33,7 +31,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFI,
 			ExpectedValue: "boot_mode:uefi",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "not-there",
@@ -44,7 +41,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFI,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "add-secure-boot",
@@ -55,7 +51,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFISecureBoot,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi,secure_boot:true",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "uefi-to-uefi",
@@ -66,7 +61,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFI,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "bios-to-bios",
@@ -77,7 +71,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.Legacy,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:bios",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "bios-to-uefi",
@@ -88,7 +81,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFI,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "uefi-to-bios",
@@ -99,7 +91,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.Legacy,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:bios",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "uefi-to-secure",
@@ -110,7 +101,6 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFISecureBoot,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi,secure_boot:true",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 		{
 			Scenario: "secure-to-uefi",
@@ -121,14 +111,12 @@ func TestBuildCapabilitiesValue(t *testing.T) {
 			},
 			Mode:          metal3v1alpha1.UEFI,
 			ExpectedValue: "cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true,boot_mode:uefi",
-			ExpectedOp:    nodes.ReplaceOp,
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.Scenario, func(t *testing.T) {
-			actualOp, actualVal := buildCapabilitiesValue(&tc.Node, tc.Mode)
-			assert.Equal(t, tc.ExpectedOp, actualOp)
+			actualVal := buildCapabilitiesValue(&tc.Node, tc.Mode)
 			assert.Equal(t, tc.ExpectedValue, actualVal)
 		})
 	}
diff --git a/pkg/provisioner/ironic/delete_test.go b/pkg/provisioner/ironic/delete_test.go
index 0b212efa..87e3e201 100644
--- a/pkg/provisioner/ironic/delete_test.go
+++ b/pkg/provisioner/ironic/delete_test.go
@@ -149,7 +149,7 @@ func deleteTest(t *testing.T, detach bool) {
 			expectedDirty:        true,
 			expectedRequestAfter: 0,
 			expectedUpdate: &nodes.UpdateOperation{
-				Op:    "replace",
+				Op:    nodes.AddOp,
 				Path:  "/maintenance",
 				Value: true,
 			},
diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go
index 57c119bd..e16b1b8b 100644
--- a/pkg/provisioner/ironic/updateopts_test.go
+++ b/pkg/provisioner/ironic/updateopts_test.go
@@ -17,6 +17,437 @@ import (
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 )
 
+func TestOptionValueEqual(t *testing.T) {
+	cases := []struct {
+		Name   string
+		Before interface{}
+		After  interface{}
+		Equal  bool
+	}{
+		{
+			Name:   "nil interface",
+			Before: nil,
+			After:  "foo",
+			Equal:  false,
+		},
+		{
+			Name:   "equal string",
+			Before: "foo",
+			After:  "foo",
+			Equal:  true,
+		},
+		{
+			Name:   "unequal string",
+			Before: "foo",
+			After:  "bar",
+			Equal:  false,
+		},
+		{
+			Name:   "equal true",
+			Before: true,
+			After:  true,
+			Equal:  true,
+		},
+		{
+			Name:   "equal false",
+			Before: false,
+			After:  false,
+			Equal:  true,
+		},
+		{
+			Name:   "unequal true",
+			Before: true,
+			After:  false,
+			Equal:  false,
+		},
+		{
+			Name:   "unequal false",
+			Before: false,
+			After:  true,
+			Equal:  false,
+		},
+		{
+			Name:   "equal int",
+			Before: 42,
+			After:  42,
+			Equal:  true,
+		},
+		{
+			Name:   "unequal int",
+			Before: 27,
+			After:  42,
+			Equal:  false,
+		},
+		{
+			Name:   "string int",
+			Before: "42",
+			After:  42,
+			Equal:  false,
+		},
+		{
+			Name:   "int string",
+			Before: 42,
+			After:  "42",
+			Equal:  false,
+		},
+		{
+			Name:   "bool int",
+			Before: false,
+			After:  0,
+			Equal:  false,
+		},
+		{
+			Name:   "int bool",
+			Before: 1,
+			After:  true,
+			Equal:  false,
+		},
+		{
+			Name:   "string map",
+			Before: "foo",
+			After:  map[string]string{"foo": "foo"},
+			Equal:  false,
+		},
+		{
+			Name:   "map string",
+			Before: map[string]interface{}{"foo": "foo"},
+			After:  "foo",
+			Equal:  false,
+		},
+		{
+			Name:   "string list",
+			Before: "foo",
+			After:  []string{"foo"},
+			Equal:  false,
+		},
+		{
+			Name:   "list string",
+			Before: []string{"foo"},
+			After:  "foo",
+			Equal:  false,
+		},
+		{
+			Name:   "map list",
+			Before: map[string]interface{}{"foo": "foo"},
+			After:  []string{"foo"},
+			Equal:  false,
+		},
+		{
+			Name:   "list map",
+			Before: []string{"foo"},
+			After:  map[string]string{"foo": "foo"},
+			Equal:  false,
+		},
+		{
+			Name:   "equal map string-typed",
+			Before: map[string]interface{}{"foo": "bar"},
+			After:  map[string]string{"foo": "bar"},
+			Equal:  true,
+		},
+		{
+			Name:   "unequal map string-typed",
+			Before: map[string]interface{}{"foo": "bar"},
+			After:  map[string]string{"foo": "baz"},
+			Equal:  false,
+		},
+		{
+			Name:   "equal map int-typed",
+			Before: map[string]interface{}{"foo": 42},
+			After:  map[string]int{"foo": 42},
+			Equal:  true,
+		},
+		{
+			Name:   "unequal map int-typed",
+			Before: map[string]interface{}{"foo": "bar"},
+			After:  map[string]int{"foo": 42},
+			Equal:  false,
+		},
+		{
+			Name:   "equal map",
+			Before: map[string]interface{}{"foo": "bar", "42": 42},
+			After:  map[string]interface{}{"foo": "bar", "42": 42},
+			Equal:  true,
+		},
+		{
+			Name:   "unequal map",
+			Before: map[string]interface{}{"foo": "bar", "42": 42},
+			After:  map[string]interface{}{"foo": "bar", "42": 27},
+			Equal:  false,
+		},
+		{
+			Name:   "equal map empty string",
+			Before: map[string]interface{}{"foo": ""},
+			After:  map[string]interface{}{"foo": ""},
+			Equal:  true,
+		},
+		{
+			Name:   "unequal map replace empty string",
+			Before: map[string]interface{}{"foo": ""},
+			After:  map[string]interface{}{"foo": "bar"},
+			Equal:  false,
+		},
+		{
+			Name:   "unequal map replace with empty string",
+			Before: map[string]interface{}{"foo": "bar"},
+			After:  map[string]interface{}{"foo": ""},
+			Equal:  false,
+		},
+		{
+			Name:   "shorter map",
+			Before: map[string]interface{}{"foo": "bar", "42": 42},
+			After:  map[string]interface{}{"foo": "bar"},
+			Equal:  false,
+		},
+		{
+			Name:   "longer map",
+			Before: map[string]interface{}{"foo": "bar"},
+			After:  map[string]interface{}{"foo": "bar", "42": 42},
+			Equal:  false,
+		},
+		{
+			Name:   "different map",
+			Before: map[string]interface{}{"foo": "bar"},
+			After:  map[string]interface{}{"baz": "bar"},
+			Equal:  false,
+		},
+		{
+			Name:   "equal list string-typed",
+			Before: []interface{}{"foo", "bar"},
+			After:  []string{"foo", "bar"},
+			Equal:  true,
+		},
+		{
+			Name:   "unequal list string-typed",
+			Before: []interface{}{"foo", "bar"},
+			After:  []string{"foo", "baz"},
+			Equal:  false,
+		},
+		{
+			Name:   "equal list",
+			Before: []interface{}{"foo", 42},
+			After:  []interface{}{"foo", 42},
+			Equal:  true,
+		},
+		{
+			Name:   "unequal list",
+			Before: []interface{}{"foo", 42},
+			After:  []interface{}{"foo", 27},
+			Equal:  false,
+		},
+		{
+			Name:   "shorter list",
+			Before: []interface{}{"foo", 42},
+			After:  []interface{}{"foo"},
+			Equal:  false,
+		},
+		{
+			Name:   "longer list",
+			Before: []interface{}{"foo"},
+			After:  []interface{}{"foo", 42},
+			Equal:  false,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.Name, func(t *testing.T) {
+			var ne = "="
+			if c.Equal {
+				ne = "!"
+			}
+			assert.Equal(t, c.Equal, optionValueEqual(c.Before, c.After),
+				fmt.Sprintf("%v %s= %v", c.Before, ne, c.After))
+		})
+	}
+}
+
+func TestGetUpdateOperation(t *testing.T) {
+	var nilp *string
+	bar_exist := "bar"
+	bar := "bar"
+	baz := "baz"
+	existingData := map[string]interface{}{
+		"foo":  "bar",
+		"foop": &bar_exist,
+		"nil":  nilp,
+	}
+	cases := []struct {
+		Name          string
+		Field         string
+		NewValue      interface{}
+		ExpectedOp    nodes.UpdateOp
+		ExpectedValue string
+	}{
+		{
+			Name:       "add value",
+			Field:      "baz",
+			NewValue:   "quux",
+			ExpectedOp: nodes.AddOp,
+		},
+		{
+			Name:          "add value pointer",
+			Field:         "baz",
+			NewValue:      &bar,
+			ExpectedValue: bar,
+			ExpectedOp:    nodes.AddOp,
+		},
+		{
+			Name:          "add pointer value pointer",
+			Field:         "nil",
+			NewValue:      &bar,
+			ExpectedValue: bar,
+			ExpectedOp:    nodes.AddOp,
+		},
+		{
+			Name:     "keep value",
+			Field:    "foo",
+			NewValue: "bar",
+		},
+		{
+			Name:     "keep pointer value",
+			Field:    "foop",
+			NewValue: "bar",
+		},
+		{
+			Name:     "keep value pointer",
+			Field:    "foo",
+			NewValue: &bar,
+		},
+		{
+			Name:     "keep pointer value pointer",
+			Field:    "foop",
+			NewValue: &bar,
+		},
+		{
+			Name:       "change value",
+			Field:      "foo",
+			NewValue:   "baz",
+			ExpectedOp: nodes.AddOp,
+		},
+		{
+			Name:       "change pointer value",
+			Field:      "foop",
+			NewValue:   "baz",
+			ExpectedOp: nodes.AddOp,
+		},
+		{
+			Name:          "change value pointer",
+			Field:         "foo",
+			NewValue:      &baz,
+			ExpectedValue: baz,
+			ExpectedOp:    nodes.AddOp,
+		},
+		{
+			Name:          "change pointer value pointer",
+			Field:         "foop",
+			NewValue:      &baz,
+			ExpectedValue: baz,
+			ExpectedOp:    nodes.AddOp,
+		},
+		{
+			Name:       "delete value",
+			Field:      "foo",
+			NewValue:   nil,
+			ExpectedOp: nodes.RemoveOp,
+		},
+		{
+			Name:       "delete value pointer",
+			Field:      "foo",
+			NewValue:   nilp,
+			ExpectedOp: nodes.RemoveOp,
+		},
+		{
+			Name:     "nonexistent value",
+			Field:    "bar",
+			NewValue: nil,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.Name, func(t *testing.T) {
+			path := fmt.Sprintf("test/%s", c.Field)
+			updateOp := getUpdateOperation(
+				c.Field, existingData,
+				c.NewValue,
+				path, nil)
+
+			switch c.ExpectedOp {
+			case nodes.AddOp, nodes.ReplaceOp:
+				assert.NotNil(t, updateOp)
+				ev := c.ExpectedValue
+				if ev == "" {
+					ev = c.NewValue.(string)
+				}
+				assert.Equal(t, c.ExpectedOp, updateOp.Op)
+				assert.Equal(t, ev, updateOp.Value)
+				assert.Equal(t, path, updateOp.Path)
+			case nodes.RemoveOp:
+				assert.NotNil(t, updateOp)
+				assert.Equal(t, c.ExpectedOp, updateOp.Op)
+				assert.Equal(t, path, updateOp.Path)
+			default:
+				assert.Nil(t, updateOp)
+			}
+		})
+	}
+}
+
+func TestTopLevelUpdateOpt(t *testing.T) {
+	u := updateOptsBuilder(log)
+	u.SetTopLevelOpt("foo", "baz", "bar")
+	ops := u.Updates
+	assert.Len(t, ops, 1)
+	op := ops[0].(nodes.UpdateOperation)
+	assert.Equal(t, nodes.AddOp, op.Op)
+	assert.Equal(t, "baz", op.Value)
+	assert.Equal(t, "/foo", op.Path)
+
+	u = updateOptsBuilder(log)
+	u.SetTopLevelOpt("foo", "bar", "bar")
+	assert.Len(t, u.Updates, 0)
+}
+
+func TestPropertiesUpdateOpts(t *testing.T) {
+	newValues := optionsData{
+		"foo": "bar",
+		"baz": "quux",
+	}
+	node := nodes.Node{
+		Properties: map[string]interface{}{
+			"foo": "bar",
+		},
+	}
+
+	u := updateOptsBuilder(log)
+	u.SetPropertiesOpts(newValues, &node)
+	ops := u.Updates
+	assert.Len(t, ops, 1)
+	op := ops[0].(nodes.UpdateOperation)
+	assert.Equal(t, nodes.AddOp, op.Op)
+	assert.Equal(t, "quux", op.Value)
+	assert.Equal(t, "/properties/baz", op.Path)
+}
+
+func TestInstanceInfoUpdateOpts(t *testing.T) {
+	newValues := optionsData{
+		"foo": "bar",
+		"baz": "quux",
+	}
+	node := nodes.Node{
+		InstanceInfo: map[string]interface{}{
+			"foo": "bar",
+		},
+	}
+
+	u := updateOptsBuilder(log)
+	u.SetInstanceInfoOpts(newValues, &node)
+	ops := u.Updates
+	assert.Len(t, ops, 1)
+	op := ops[0].(nodes.UpdateOperation)
+	assert.Equal(t, nodes.AddOp, op.Op)
+	assert.Equal(t, "quux", op.Value)
+	assert.Equal(t, "/instance_info/baz", op.Path)
+}
+
 func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) {
 
 	eventPublisher := func(reason, message string) {}
@@ -36,10 +467,7 @@ func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) {
 		BootMode:        metal3v1alpha1.DefaultBootMode,
 		RootDeviceHints: host.Status.Provisioning.RootDeviceHints,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -135,10 +563,7 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 		BootMode:        metal3v1alpha1.DefaultBootMode,
 		HardwareProfile: hwProf,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -245,10 +670,7 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 		BootMode:        metal3v1alpha1.DefaultBootMode,
 		HardwareProfile: hwProf,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -320,10 +742,7 @@ func TestGetUpdateOptsForNodeLiveIso(t *testing.T) {
 		Image:    *host.Spec.Image,
 		BootMode: metal3v1alpha1.DefaultBootMode,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -345,7 +764,7 @@ func TestGetUpdateOptsForNodeLiveIso(t *testing.T) {
 		{
 			Path:  "/deploy_interface",
 			Value: "ramdisk",
-			Op:    nodes.ReplaceOp,
+			Op:    nodes.AddOp,
 		},
 	}
 
@@ -392,10 +811,7 @@ func TestGetUpdateOptsForNodeImageToLiveIso(t *testing.T) {
 		Image:    *host.Spec.Image,
 		BootMode: metal3v1alpha1.DefaultBootMode,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -413,7 +829,7 @@ func TestGetUpdateOptsForNodeImageToLiveIso(t *testing.T) {
 		{
 			Path:  "/deploy_interface",
 			Value: "ramdisk",
-			Op:    nodes.ReplaceOp,
+			Op:    nodes.AddOp,
 		},
 		{
 			Path: "/instance_info/image_source",
@@ -474,10 +890,7 @@ func TestGetUpdateOptsForNodeLiveIsoToImage(t *testing.T) {
 		Image:    *host.Spec.Image,
 		BootMode: metal3v1alpha1.DefaultBootMode,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -494,7 +907,7 @@ func TestGetUpdateOptsForNodeLiveIsoToImage(t *testing.T) {
 		{
 			Path:  "/deploy_interface",
 			Value: "direct",
-			Op:    nodes.ReplaceOp,
+			Op:    nodes.AddOp,
 		},
 		{
 			Path:  "/instance_info/image_source",
@@ -579,10 +992,7 @@ func TestGetUpdateOptsForNodeSecureBoot(t *testing.T) {
 		BootMode:        metal3v1alpha1.UEFISecureBoot,
 		HardwareProfile: hwProf,
 	}
-	patches, err := prov.getUpdateOptsForNode(ironicNode, provData)
-	if err != nil {
-		t.Fatal(err)
-	}
+	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index b9242d73..b037f376 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -287,6 +287,7 @@ func TestValidateManagementAccessExistingNodeContinue(t *testing.T) {
 		nodes.Rescuing,
 		nodes.UnrescueFail,
 	}
+	clean := true
 
 	for _, status := range statuses {
 		t.Run(string(status), func(t *testing.T) {
@@ -304,6 +305,7 @@ func TestValidateManagementAccessExistingNodeContinue(t *testing.T) {
 				Name:           host.Namespace + nameSeparator + host.Name,
 				UUID:           "uuid", // to match status in host
 				ProvisionState: string(status),
+				AutomatedClean: &clean,
 			}).NodeUpdate(nodes.Node{
 				UUID: "uuid",
 			})
@@ -324,6 +326,93 @@ func TestValidateManagementAccessExistingNodeContinue(t *testing.T) {
 			}
 			assert.Equal(t, "", result.ErrorMessage)
 			assert.Equal(t, false, result.Dirty)
+			assert.Len(t, ironic.GetLastNodeUpdateRequestFor("uuid"), 0)
+		})
+	}
+}
+
+func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
+	liveFormat := "live-iso"
+	imageTypes := []struct {
+		DeployInterface string
+		Image           *metal3v1alpha1.Image
+		InstanceInfo    map[string]interface{}
+	}{
+		{
+			DeployInterface: "",
+			InstanceInfo: map[string]interface{}{
+				"capabilities": map[string]interface{}{},
+			},
+		},
+		{
+			DeployInterface: "direct",
+			Image: &metal3v1alpha1.Image{
+				URL:      "theimage",
+				Checksum: "thechecksum",
+			},
+			InstanceInfo: map[string]interface{}{
+				"image_source":        "theimage",
+				"image_os_hash_algo":  "md5",
+				"image_os_hash_value": "thechecksum",
+				"image_checksum":      "thechecksum",
+				"capabilities":        map[string]interface{}{},
+			},
+		},
+		{
+			DeployInterface: "ramdisk",
+			Image: &metal3v1alpha1.Image{
+				URL:        "theimage",
+				DiskFormat: &liveFormat,
+			},
+			InstanceInfo: map[string]interface{}{
+				"boot_iso":     "theimage",
+				"capabilities": map[string]interface{}{},
+			},
+		},
+	}
+	clean := true
+
+	for _, imageType := range imageTypes {
+		t.Run(imageType.DeployInterface, func(t *testing.T) {
+			// Create a host without a bootMACAddress and with a BMC that
+			// does not require one.
+			host := makeHost()
+			host.Spec.BootMACAddress = ""
+			host.Status.Provisioning.ID = "" // so we don't lookup by uuid
+
+			createCallback := func(node nodes.Node) {
+				t.Fatal("create callback should not be invoked for existing node")
+			}
+
+			ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(nodes.Node{
+				Name:            host.Namespace + nameSeparator + host.Name,
+				UUID:            "uuid", // to match status in host
+				ProvisionState:  string(nodes.Manageable),
+				AutomatedClean:  &clean,
+				InstanceUUID:    string(host.UID),
+				DeployInterface: imageType.DeployInterface,
+				InstanceInfo:    imageType.InstanceInfo,
+			}).NodeUpdate(nodes.Node{
+				UUID: "uuid",
+			})
+			ironic.Start()
+			defer ironic.Stop()
+
+			auth := clients.AuthConfig{Type: clients.NoAuth}
+			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher,
+				ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
+			)
+			if err != nil {
+				t.Fatalf("could not create provisioner: %s", err)
+			}
+
+			result, _, err := prov.ValidateManagementAccess(provisioner.ManagementAccessData{CurrentImage: imageType.Image}, false, false)
+			if err != nil {
+				t.Fatalf("error from ValidateManagementAccess: %s", err)
+			}
+			assert.Equal(t, "", result.ErrorMessage)
+			assert.Equal(t, false, result.Dirty)
+			assert.Len(t, ironic.GetLastNodeUpdateRequestFor("uuid"), 0)
 		})
 	}
 }
@@ -371,6 +460,11 @@ func TestValidateManagementAccessExistingNodeWaiting(t *testing.T) {
 			}
 			assert.Equal(t, "", result.ErrorMessage)
 			assert.Equal(t, true, result.Dirty)
+
+			updates := ironic.GetLastNodeUpdateRequestFor("uuid")
+			assert.Len(t, updates, 1)
+			assert.Equal(t, "/automated_clean", updates[0].Path)
+			assert.Equal(t, true, updates[0].Value)
 		})
 	}
 }
@@ -415,6 +509,7 @@ func TestValidateManagementAccessNewCredentials(t *testing.T) {
 	assert.Equal(t, "uuid", provID)
 
 	updates := ironic.GetLastNodeUpdateRequestFor("uuid")
+	assert.Equal(t, "/driver_info", updates[0].Path)
 	newValues := updates[0].Value.(map[string]interface{})
 	assert.Equal(t, "test.bmc", newValues["test_address"])
 }