Repository: metal3-io/baremetal-operator
Pull request #495 (2020-04-29 20:25:31)

root device hints
This supercedes #442

Add a new field spec.rootDeviceHints to the API with sub-fields for different types of hints based on what Ironic supports today.

Add a new field status.rootDeviceHints to record the values used when provisioning the host.

Change the Provisioner API to return the new public struct with the more detailed device hints so that the values in the hard-coded profiles can be stored directly in the new status field.

If the user does not provide explicit hints, use the values from the profile.

Implements https://github.com/metal3-io/metal3-docs/blob/master/design/user-defined-root-device-hints.md

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/495#" title="Author self-approved">dhellmann</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)~~ [dhellmann]

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":[]} -->
mikkosest: Looks good, I guess unit test updates in the same PR?
dhellmann: > Looks good, I guess unit test updates in the same PR?

Yes, we'll want that.

Given some of the other work I did last week on #434 I think we're going to want to add these new features to the next API version (v1alpha2 or v1beta1) after we have the CI fixes like #504 in place.
maelk: This should be backward compatible, no ? we should be able to add it without waiting for the next version. I am not sure I get the reason for postponing it.
dhellmann: > This should be backward compatible, no ? we should be able to add it without waiting for the next version. I am not sure I get the reason for postponing it.

The validation for updating the CRD is broken. I think we should prioritize fixing that over extending the API further so that when we do work on the API changes we can trust that we're making all of the necessary changes. That doesn't necessarily assume waiting for the next API version, but since I plan to do that right away as part of fixing the current issue (see #434 for the todo list) I don't think the wait will be very long.
maelk: ok, that's good! thank you
derekhiggins: I posted this in the other PR but not sure if anybody saw it

> I've pushed up a related PR for masters to the installer repository
> openshift/installer#3348
> 
> It suggests a simpler roothint pass through approach, would it be worth considering and allowing ironic to do the validation?

I'm worried this functionality will need to be duplicated(in the installer and in ironic itself) for provisioning master nodes is that the case? and if so could we just passthrough of the json to ironic?
dhellmann: > I posted this in the other PR but not sure if anybody saw it
> 
> > I've pushed up a related PR for masters to the installer repository
> > [openshift/installer#3348](https://github.com/openshift/installer/pull/3348)
> > It suggests a simpler roothint pass through approach, would it be worth considering and allowing ironic to do the validation?
> 
> I'm worried this functionality will need to be duplicated(in the installer and in ironic itself) for provisioning master nodes is that the case? and if so could we just passthrough of the json to ironic?

Yes, the OpenShift installer is going to need to do its own thing. We should talk about that over on that PR, since it's not really relevant to metal3 upstream.
metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | ede242b6268a6f1173278d4aa815a3644b6aa705 | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/495/golint/1262414816425807874) | `/test golint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/deploy/crds/metal3.io_baremetalhosts_crd.yaml b/deploy/crds/metal3.io_baremetalhosts_crd.yaml
index adef06cc..d40431fa 100644
--- a/deploy/crds/metal3.io_baremetalhosts_crd.yaml
+++ b/deploy/crds/metal3.io_baremetalhosts_crd.yaml
@@ -203,6 +203,50 @@ spec:
             online:
               description: Should the server be online?
               type: boolean
+            rootDeviceHints:
+              description: Provide guidance about how to choose the device for the
+                image being provisioned.
+              properties:
+                deviceName:
+                  description: A Linux device name like "/dev/vda". The hint must
+                    match the actual value exactly.
+                  type: string
+                hctl:
+                  description: A SCSI bus address like 0:0:0:0. The hint must match
+                    the actual value exactly.
+                  type: string
+                minSizeGigabytes:
+                  description: The minimum size of the device in Gigabytes.
+                  type: integer
+                model:
+                  description: A vendor-specific device identifier. The hint can be
+                    a substring of the actual value.
+                  type: string
+                rotational:
+                  description: True if the device should use spinning media, false
+                    otherwise.
+                  type: boolean
+                serialNumber:
+                  description: Device serial number. The hint must match the actual
+                    value exactly.
+                  type: string
+                vendor:
+                  description: The name of the vendor or manufacturer of the device.
+                    The hint can be a substring of the actual value.
+                  type: string
+                wwn:
+                  description: Unique storage identifier. The hint must match the
+                    actual value exactly.
+                  type: string
+                wwnVendorExtension:
+                  description: Unique vendor storage identifier. The hint must match
+                    the actual value exactly.
+                  type: string
+                wwnWithExtension:
+                  description: Unique storage identifier with the vendor extension
+                    appended. The hint must match the actual value exactly.
+                  type: string
+              type: object
             taints:
               description: Taints is the full, authoritative list of taints to apply
                 to the corresponding Machine. This list will overwrite any modifications
@@ -572,6 +616,49 @@ spec:
                   - checksum
                   - url
                   type: object
+                rootDeviceHints:
+                  description: The RootDevicehints set by the user
+                  properties:
+                    deviceName:
+                      description: A Linux device name like "/dev/vda". The hint must
+                        match the actual value exactly.
+                      type: string
+                    hctl:
+                      description: A SCSI bus address like 0:0:0:0. The hint must
+                        match the actual value exactly.
+                      type: string
+                    minSizeGigabytes:
+                      description: The minimum size of the device in Gigabytes.
+                      type: integer
+                    model:
+                      description: A vendor-specific device identifier. The hint can
+                        be a substring of the actual value.
+                      type: string
+                    rotational:
+                      description: True if the device should use spinning media, false
+                        otherwise.
+                      type: boolean
+                    serialNumber:
+                      description: Device serial number. The hint must match the actual
+                        value exactly.
+                      type: string
+                    vendor:
+                      description: The name of the vendor or manufacturer of the device.
+                        The hint can be a substring of the actual value.
+                      type: string
+                    wwn:
+                      description: Unique storage identifier. The hint must match
+                        the actual value exactly.
+                      type: string
+                    wwnVendorExtension:
+                      description: Unique vendor storage identifier. The hint must
+                        match the actual value exactly.
+                      type: string
+                    wwnWithExtension:
+                      description: Unique storage identifier with the vendor extension
+                        appended. The hint must match the actual value exactly.
+                      type: string
+                  type: object
                 state:
                   description: An indiciator for what the provisioner is doing with
                     the host.
diff --git a/docs/api.md b/docs/api.md
index c6663995..64e1ba03 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -90,6 +90,50 @@ mainly, but not only, provisioning details.
 
 * *description* -- A human-provided string to help identify the host.
 
+* *hardwareProfile* -- **This field is deprecated. See rootDeviceHints instead.**
+  The name of the hardware profile to use. The following are the current
+  supported `hardwareProfile` settings and their corresponding root devices.
+
+  | **hardwareProfile** | **Root Device** |
+  |---------------------|-----------------|
+  | `unknown`           | /dev/sda        |
+  | `libvirt`           | /dev/vda        |
+  | `dell`              | HCTL: 0:0:0:0   |
+  | `dell-raid`         | HCTL: 0:2:0:0   |
+  | `openstack`         | /dev/vdb        |
+
+  **NOTE:** These are subject to change.
+
+* *rootDeviceHints* -- Guidance for how to choose the device to
+  receive the image being provisioned. The storage devices are
+  examined in the order they are discovered during inspection and the
+  hint values are compared to the inspected values. The first
+  discovered device that matches is used. Hints can be combined, and
+  if multiple hints are provided then a device must match all hints in
+  order to be selected.
+  * *deviceName* -- A string containing a Linux device name like
+    `/dev/vda`. The hint must match the actual value exactly.
+  * *hctl* -- A string containing a SCSI bus address like
+    `0:0:0:0`. The hint must match the actual value exactly.
+  * *model* -- A string containing a vendor-specific device
+    identifier. The hint can be a substring of the actual value.
+  * *vendor* -- A string containing the name of the vendor or
+    manufacturer of the device. The hint can be a substring of the
+    actual value.
+  * *serialNumber* -- A string contianing the device serial
+    number. The hint must match the actual value exactly.
+  * *minSizeGigabytes* -- An integer representing the minimum size of the
+    device in Gigabytes.
+  * *wwn* -- A string containing the unique storage identifier. The
+    hint must match the actual value exactly.
+  * *wwnWithExtension* -- A string containing the unique storage
+    identifier with the vendor extension appended. The hint must match
+    the actual value exactly.
+  * *wwnVendorExtension* -- A string containing the unique vendor
+    storage indentifier. The hint must match the actual value exactly.
+  * *rotational* -- A boolean indicating whether the device should be
+    a rotating disk (`true`) or not (`false`).
+
 ### BareMetalHost status
 
 Moving onto the next block, the *BareMetalHost's* *status* which represents
@@ -153,8 +197,10 @@ details, etc.
   * *systemVendor* -- Contains information about the host's *manufacturer*,
     the *productName* and *serialNumber*.
   * *ramMebibytes* -- The host's amount of memory in Mebibytes.
-* *hardwareProfile* -- The name of the hardware profile that matches the
-  hardware discovered on the host. These details are saved
+
+* *hardwareProfile* -- **This field is deprecated. See rootDeviceHints instead.**
+  The name of the hardware profile that matches the
+  hardware discovered on the host based on the details saved
   to the *Hardware* section. If the hardware does not match any
   known profile, the value `unknown` will be set on this field
   and is used by default. In practice, this only affects which device
@@ -173,6 +219,7 @@ details, etc.
 
 * *poweredOn* -- Boolean indicating whether the host is powered on.
   See *online* on the *BareMetalHost's* *Spec*.
+
 * *provisioning* -- Settings related to deploying an image to the host.
   * *state* -- The current state of any ongoing provisioning operation.
     The following are the currently supported ones:
@@ -193,6 +240,11 @@ details, etc.
       by an agent.
     * *power management error* -- An error was found while trying to power
       the host either on or off.
+  * *id* -- The unique identifier for the service in the underlying
+    provisioning tool.
+  * *image* -- The image most recently provisioned to the host.
+  * *rootDeviceHints* -- The root device selection instructions used
+    for the most recent provisioning operation.
 
 ### BareMetalHost Example
 
diff --git a/go.mod b/go.mod
index f5dea03e..d404ca1a 100644
--- a/go.mod
+++ b/go.mod
@@ -16,6 +16,7 @@ require (
 	github.com/prometheus/client_golang v1.5.1
 	github.com/securego/gosec v0.0.0-20200401082031-e946c8c39989 // indirect
 	github.com/spf13/cobra v0.0.6 // indirect
+	github.com/stretchr/testify v1.4.0
 	golang.org/x/crypto v0.0.0-20200302210943-78000ba7a073 // indirect
 	golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect
 	golang.org/x/net v0.0.0-20200301022130-244492dfa37a // indirect
@@ -28,7 +29,6 @@ require (
 	k8s.io/utils v0.0.0-20200229041039-0a110f9eb7ab
 	sigs.k8s.io/controller-runtime v0.5.2
 	sigs.k8s.io/yaml v1.2.0
-
 )
 
 replace (
diff --git a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
index a4ffc717..2b7b9cc6 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@@ -30,6 +30,48 @@ const (
 	StatusAnnotation = "baremetalhost.metal3.io/status"
 )
 
+// RootDeviceHints holds the hints for specifying the storage location
+// for the root filesystem for the image.
+type RootDeviceHints struct {
+	// A Linux device name like "/dev/vda". The hint must match the
+	// actual value exactly.
+	DeviceName string `json:"deviceName,omitempty"`
+
+	// A SCSI bus address like 0:0:0:0. The hint must match the actual
+	// value exactly.
+	HCTL string `json:"hctl,omitempty"`
+
+	// A vendor-specific device identifier. The hint can be a
+	// substring of the actual value.
+	Model string `json:"model,omitempty"`
+
+	// The name of the vendor or manufacturer of the device. The hint
+	// can be a substring of the actual value.
+	Vendor string `json:"vendor,omitempty"`
+
+	// Device serial number. The hint must match the actual value
+	// exactly.
+	SerialNumber string `json:"serialNumber,omitempty"`
+
+	// The minimum size of the device in Gigabytes.
+	MinSizeGigabytes int `json:"minSizeGigabytes,omitempty"`
+
+	// Unique storage identifier. The hint must match the actual value
+	// exactly.
+	WWN string `json:"wwn,omitempty"`
+
+	// Unique storage identifier with the vendor extension
+	// appended. The hint must match the actual value exactly.
+	WWNWithExtension string `json:"wwnWithExtension,omitempty"`
+
+	// Unique vendor storage identifier. The hint must match the
+	// actual value exactly.
+	WWNVendorExtension string `json:"wwnVendorExtension,omitempty"`
+
+	// True if the device should use spinning media, false otherwise.
+	Rotational *bool `json:"rotational,omitempty"`
+}
+
 // OperationalStatus represents the state of the host
 type OperationalStatus string
 
@@ -165,6 +207,10 @@ type BareMetalHostSpec struct {
 	// automatically determine the profile.
 	HardwareProfile string `json:"hardwareProfile,omitempty"`
 
+	// Provide guidance about how to choose the device for the image
+	// being provisioned.
+	RootDeviceHints *RootDeviceHints `json:"rootDeviceHints,omitempty"`
+
 	// Which MAC address will PXE boot? This is optional for some
 	// types, but required for libvirt VMs driven by vbmc.
 	// +kubebuilder:validation:Pattern=`[0-9a-fA-F]{2}(:[0-9a-fA-F]{2}){5}`
@@ -491,6 +537,9 @@ type ProvisionStatus struct {
 	// Image holds the details of the last image successfully
 	// provisioned to the host.
 	Image Image `json:"image,omitempty"`
+
+	// The RootDevicehints set by the user
+	RootDeviceHints *RootDeviceHints `json:"rootDeviceHints,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
diff --git a/pkg/apis/metal3/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/metal3/v1alpha1/zz_generated.deepcopy.go
index dcd9c3ab..4c57aff7 100644
--- a/pkg/apis/metal3/v1alpha1/zz_generated.deepcopy.go
+++ b/pkg/apis/metal3/v1alpha1/zz_generated.deepcopy.go
@@ -113,6 +113,11 @@ func (in *BareMetalHostSpec) DeepCopyInto(out *BareMetalHostSpec) {
 		}
 	}
 	out.BMC = in.BMC
+	if in.RootDeviceHints != nil {
+		in, out := &in.RootDeviceHints, &out.RootDeviceHints
+		*out = new(RootDeviceHints)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.ConsumerRef != nil {
 		in, out := &in.ConsumerRef, &out.ConsumerRef
 		*out = new(v1.ObjectReference)
@@ -370,6 +375,11 @@ func (in *OperationMetric) DeepCopy() *OperationMetric {
 func (in *ProvisionStatus) DeepCopyInto(out *ProvisionStatus) {
 	*out = *in
 	in.Image.DeepCopyInto(&out.Image)
+	if in.RootDeviceHints != nil {
+		in, out := &in.RootDeviceHints, &out.RootDeviceHints
+		*out = new(RootDeviceHints)
+		(*in).DeepCopyInto(*out)
+	}
 	return
 }
 
@@ -383,6 +393,27 @@ func (in *ProvisionStatus) DeepCopy() *ProvisionStatus {
 	return out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *RootDeviceHints) DeepCopyInto(out *RootDeviceHints) {
+	*out = *in
+	if in.Rotational != nil {
+		in, out := &in.Rotational, &out.Rotational
+		*out = new(bool)
+		**out = **in
+	}
+	return
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RootDeviceHints.
+func (in *RootDeviceHints) DeepCopy() *RootDeviceHints {
+	if in == nil {
+		return nil
+	}
+	out := new(RootDeviceHints)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *Storage) DeepCopyInto(out *Storage) {
 	*out = *in
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller.go b/pkg/controller/baremetalhost/baremetalhost_controller.go
index 15aa32ef..382bdae9 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller.go
@@ -566,6 +566,7 @@ func (r *ReconcileBareMetalHost) actionMatchProfile(prov provisioner.Provisioner
 		info.log.Info("updating hardware profile", "profile", hardwareProfile)
 		info.publishEvent("ProfileSet", fmt.Sprintf("Hardware profile set: %s", hardwareProfile))
 	}
+
 	info.host.ClearError()
 	return actionComplete{}
 }
@@ -615,6 +616,13 @@ func (r *ReconcileBareMetalHost) actionProvisioning(prov provisioner.Provisioner
 	return actionComplete{}
 }
 
+// clearHostProvisioningSettings removes the values related to
+// provisioning that do not trigger re-provisioning from the status
+// fields of a host.
+func clearHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) {
+	host.Status.Provisioning.RootDeviceHints = nil
+}
+
 func (r *ReconcileBareMetalHost) actionDeprovisioning(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	info.log.Info("deprovisioning")
 
@@ -639,9 +647,10 @@ func (r *ReconcileBareMetalHost) actionDeprovisioning(prov provisioner.Provision
 		return actionContinueNoWrite{}
 	}
 
-	// After the provisioner is done, clear the image settings so we
-	// transition to the next state.
+	// After the provisioner is done, clear the provisioning settings
+	// so we transition to the next state.
 	info.host.Status.Provisioning.Image = metal3v1alpha1.Image{}
+	clearHostProvisioningSettings(info.host)
 
 	return actionComplete{}
 }
@@ -778,12 +787,44 @@ func (r *ReconcileBareMetalHost) actionManageReady(prov provisioner.Provisioner,
 	}
 
 	if info.host.NeedsProvisioning() {
+		// Ensure the root device hints we're going to use are stored.
+		dirty, err := saveHostProvisioningSettings(info.host)
+		if err != nil {
+			return actionError{errors.Wrap(err, "Could not save the host provisioning settings")}
+		}
+		if dirty {
+			info.log.Info("updating host provisioning settings")
+		}
 		info.host.ClearError()
 		return actionComplete{}
 	}
 	return r.manageHostPower(prov, info)
 }
 
+// saveHostProvisioningSettings copies the values related to
+// provisioning that do not trigger re-provisioning into the status
+// fields of the host.
+func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty bool, err error) {
+
+	// Ensure the root device hints we're going to use are stored.
+	//
+	// If the user has provided explicit root device hints, they take
+	// precedence. Otherwise use the values from the hardware profile.
+	hintSource := host.Spec.RootDeviceHints
+	if hintSource == nil {
+		hwProf, err := hardware.GetProfile(host.HardwareProfile())
+		if err != nil {
+			return false, errors.Wrap(err, "Could not update root device hints")
+		}
+		hintSource = &hwProf.RootDeviceHints
+	}
+	if (hintSource != nil && host.Status.Provisioning.RootDeviceHints == nil) || *hintSource != *(host.Status.Provisioning.RootDeviceHints) {
+		host.Status.Provisioning.RootDeviceHints = hintSource
+		dirty = true
+	}
+	return
+}
+
 func (r *ReconcileBareMetalHost) saveHostStatus(host *metal3v1alpha1.BareMetalHost) error {
 	t := metav1.Now()
 	host.Status.LastUpdated = &t
diff --git a/pkg/hardware/profile.go b/pkg/hardware/profile.go
index 908a7747..73c357ba 100644
--- a/pkg/hardware/profile.go
+++ b/pkg/hardware/profile.go
@@ -2,6 +2,8 @@ package hardware
 
 import (
 	"fmt"
+
+	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"
 )
 
 const (
@@ -17,7 +19,7 @@ type Profile struct {
 
 	// RootDeviceHints holds the suggestions for placing the storage
 	// for the root filesystem.
-	RootDeviceHints RootDeviceHints
+	RootDeviceHints metal3v1alpha1.RootDeviceHints
 
 	// RootGB is the size of the root volume in GB
 	RootGB int
@@ -29,26 +31,12 @@ type Profile struct {
 	CPUArch string
 }
 
-// RootDeviceHints holds the hints for specifying the storage location
-// for the root filesystem for the image.
-//
-// NOTE(dhellmann): Valid ironic hints are: "vendor,
-// wwn_vendor_extension, wwn_with_extension, by_path, serial, wwn,
-// size, rotational, name, hctl, model"
-type RootDeviceHints struct {
-	// A device name like "/dev/vda"
-	DeviceName string
-
-	// A SCSI bus address like 0:0:0:0
-	HCTL string
-}
-
 var profiles = make(map[string]Profile)
 
 func init() {
 	profiles[DefaultProfileName] = Profile{
 		Name: DefaultProfileName,
-		RootDeviceHints: RootDeviceHints{
+		RootDeviceHints: metal3v1alpha1.RootDeviceHints{
 			DeviceName: "/dev/sda",
 		},
 		RootGB:  10,
@@ -58,7 +46,7 @@ func init() {
 
 	profiles["libvirt"] = Profile{
 		Name: "libvirt",
-		RootDeviceHints: RootDeviceHints{
+		RootDeviceHints: metal3v1alpha1.RootDeviceHints{
 			DeviceName: "/dev/vda",
 		},
 		RootGB:  10,
@@ -68,7 +56,7 @@ func init() {
 
 	profiles["dell"] = Profile{
 		Name: "dell",
-		RootDeviceHints: RootDeviceHints{
+		RootDeviceHints: metal3v1alpha1.RootDeviceHints{
 			HCTL: "0:0:0:0",
 		},
 		RootGB:  10,
@@ -78,7 +66,7 @@ func init() {
 
 	profiles["dell-raid"] = Profile{
 		Name: "dell-raid",
-		RootDeviceHints: RootDeviceHints{
+		RootDeviceHints: metal3v1alpha1.RootDeviceHints{
 			HCTL: "0:2:0:0",
 		},
 		RootGB:  10,
@@ -88,7 +76,7 @@ func init() {
 
 	profiles["openstack"] = Profile{
 		Name: "openstack",
-		RootDeviceHints: RootDeviceHints{
+		RootDeviceHints: metal3v1alpha1.RootDeviceHints{
 			DeviceName: "/dev/vdb",
 		},
 		RootGB:  10,
diff --git a/pkg/provisioner/ironic/devicehints/devicehints.go b/pkg/provisioner/ironic/devicehints/devicehints.go
new file mode 100644
index 00000000..f4f0cbd3
--- /dev/null
+++ b/pkg/provisioner/ironic/devicehints/devicehints.go
@@ -0,0 +1,54 @@
+package devicehints
+
+import (
+	"fmt"
+
+	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"
+)
+
+// MakeHintMap converts a RootDeviceHints instance into a string map
+// suitable to pass to ironic.
+func MakeHintMap(source *metal3v1alpha1.RootDeviceHints) map[string]string {
+	hints := map[string]string{}
+
+	if source == nil {
+		return hints
+	}
+
+	if source.DeviceName != "" {
+		hints["name"] = fmt.Sprintf("s== %s", source.DeviceName)
+	}
+	if source.HCTL != "" {
+		hints["hctl"] = fmt.Sprintf("s== %s", source.HCTL)
+	}
+	if source.Model != "" {
+		hints["model"] = fmt.Sprintf("<in> %s", source.Model)
+	}
+	if source.Vendor != "" {
+		hints["vendor"] = fmt.Sprintf("<in> %s", source.Vendor)
+	}
+	if source.SerialNumber != "" {
+		hints["serial"] = fmt.Sprintf("s== %s", source.SerialNumber)
+	}
+	if source.MinSizeGigabytes != 0 {
+		hints["size"] = fmt.Sprintf(">= %d", source.MinSizeGigabytes)
+	}
+	if source.WWN != "" {
+		hints["wwn"] = fmt.Sprintf("s== %s", source.WWN)
+	}
+	if source.WWNWithExtension != "" {
+		hints["wwn_with_extension"] = fmt.Sprintf("s== %s", source.WWNWithExtension)
+	}
+	if source.WWNVendorExtension != "" {
+		hints["wwn_vendor_extension"] = fmt.Sprintf("s== %s", source.WWNVendorExtension)
+	}
+	switch {
+	case source.Rotational == nil:
+	case *source.Rotational == true:
+		hints["rotational"] = "true"
+	case *source.Rotational == false:
+		hints["rotational"] = "false"
+	}
+
+	return hints
+}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 372600f7..fab9139a 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -24,6 +24,7 @@ import (
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/hardware"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/devicehints"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/hardwaredetails"
 )
 
@@ -560,6 +561,7 @@ func (p *ironicProvisioner) UpdateHardwareState() (result provisioner.Result, er
 func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (updates nodes.UpdateOpts, err error) {
 
 	hwProf, err := hardware.GetProfile(p.host.HardwareProfile())
+
 	if err != nil {
 		return updates, errors.Wrap(err,
 			fmt.Sprintf("Could not start provisioning with bad hardware profile %s",
@@ -673,13 +675,12 @@ func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (update
 		op = nodes.ReplaceOp
 		p.log.Info("updating root_device")
 	}
-	hints := map[string]string{}
-	switch {
-	case hwProf.RootDeviceHints.DeviceName != "":
-		hints["name"] = hwProf.RootDeviceHints.DeviceName
-	case hwProf.RootDeviceHints.HCTL != "":
-		hints["hctl"] = hwProf.RootDeviceHints.HCTL
-	}
+
+	// hints
+	//
+	// If the user has provided explicit root device hints, they take
+	// precedence. Otherwise use the values from the hardware profile.
+	hints := devicehints.MakeHintMap(p.host.Status.Provisioning.RootDeviceHints)
 	p.log.Info("using root device", "hints", hints)
 	updates = append(
 		updates,


Test patch:
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller_test.go b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
index 02211998..8fd7b8db 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller_test.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
@@ -7,6 +7,8 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	corev1 "k8s.io/api/core/v1"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1037,3 +1039,111 @@ func TestDeleteHost(t *testing.T) {
 		})
 	}
 }
+
+// TestUpdateRootDeviceHints verifies that we apply the correct
+// precedence rules to the root device hints settings for a host.
+func TestUpdateRootDeviceHints(t *testing.T) {
+	rotational := true
+
+	testCases := []struct {
+		Scenario string
+		Host     metal3v1alpha1.BareMetalHost
+		Dirty    bool
+		Expected *metal3v1alpha1.RootDeviceHints
+	}{
+		{
+			Scenario: "override profile with explicit hints",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					HardwareProfile: "libvirt",
+					RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+						DeviceName:         "userd_devicename",
+						HCTL:               "1:2:3:4",
+						Model:              "userd_model",
+						Vendor:             "userd_vendor",
+						SerialNumber:       "userd_serial",
+						MinSizeGigabytes:   40,
+						WWN:                "userd_wwn",
+						WWNWithExtension:   "userd_with_extension",
+						WWNVendorExtension: "userd_vendor_extension",
+						Rotational:         &rotational,
+					},
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "libvirt",
+				},
+			},
+			Dirty: true,
+			Expected: &metal3v1alpha1.RootDeviceHints{
+				DeviceName:         "userd_devicename",
+				HCTL:               "1:2:3:4",
+				Model:              "userd_model",
+				Vendor:             "userd_vendor",
+				SerialNumber:       "userd_serial",
+				MinSizeGigabytes:   40,
+				WWN:                "userd_wwn",
+				WWNWithExtension:   "userd_with_extension",
+				WWNVendorExtension: "userd_vendor_extension",
+				Rotational:         &rotational,
+			},
+		},
+
+		{
+			Scenario: "use profile hints",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					HardwareProfile: "libvirt",
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "libvirt",
+				},
+			},
+			Dirty: true,
+			Expected: &metal3v1alpha1.RootDeviceHints{
+				DeviceName: "/dev/vda",
+			},
+		},
+
+		{
+			Scenario: "default profile hints",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					HardwareProfile: "unknown",
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "unknown",
+				},
+			},
+			Dirty: true,
+			Expected: &metal3v1alpha1.RootDeviceHints{
+				DeviceName: "/dev/sda",
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Scenario, func(t *testing.T) {
+			dirty, err := saveHostProvisioningSettings(&tc.Host)
+			if err != nil {
+				t.Fatal(err)
+			}
+			assert.Equal(t, tc.Dirty, dirty, "dirty flag did not match")
+			assert.Equal(t, tc.Expected, tc.Host.Status.Provisioning.RootDeviceHints)
+		})
+	}
+}
diff --git a/pkg/provisioner/ironic/devicehints/devicehints_test.go b/pkg/provisioner/ironic/devicehints/devicehints_test.go
new file mode 100644
index 00000000..609f1344
--- /dev/null
+++ b/pkg/provisioner/ironic/devicehints/devicehints_test.go
@@ -0,0 +1,157 @@
+package devicehints
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"
+)
+
+func TestMakeHintMap(t *testing.T) {
+	addressableTrue := true
+	addressableFalse := false
+
+	for _, tc := range []struct {
+		Scenario string
+		Hints    metal3v1alpha1.RootDeviceHints
+		Expected map[string]string
+	}{
+		{
+			Scenario: "device-name",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				DeviceName: "userd_devicename",
+			},
+			Expected: map[string]string{
+				"name": "s== userd_devicename",
+			},
+		},
+		{
+			Scenario: "hctl",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				HCTL: "1:2:3:4",
+			},
+			Expected: map[string]string{
+				"hctl": "s== 1:2:3:4",
+			},
+		},
+		{
+			Scenario: "model",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				Model: "userd_model",
+			},
+			Expected: map[string]string{
+				"model": "<in> userd_model",
+			},
+		},
+		{
+			Scenario: "vendor",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				Vendor: "userd_vendor",
+			},
+			Expected: map[string]string{
+				"vendor": "<in> userd_vendor",
+			},
+		},
+		{
+			Scenario: "serial-number",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				SerialNumber: "userd_serial",
+			},
+			Expected: map[string]string{
+				"serial": "s== userd_serial",
+			},
+		},
+		{
+			Scenario: "min-size",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				MinSizeGigabytes: 40,
+			},
+			Expected: map[string]string{
+				"size": ">= 40",
+			},
+		},
+		{
+			Scenario: "wwn",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				WWN: "userd_wwn",
+			},
+			Expected: map[string]string{
+				"wwn": "s== userd_wwn",
+			},
+		},
+		{
+			Scenario: "wwn-with-extension",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				WWNWithExtension: "userd_with_extension",
+			},
+			Expected: map[string]string{
+				"wwn_with_extension": "s== userd_with_extension",
+			},
+		},
+		{
+			Scenario: "wwn-extension",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				WWNVendorExtension: "userd_vendor_extension",
+			},
+			Expected: map[string]string{
+				"wwn_vendor_extension": "s== userd_vendor_extension",
+			},
+		},
+		{
+			Scenario: "rotational-true",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				Rotational: &addressableTrue,
+			},
+			Expected: map[string]string{
+				"rotational": "true",
+			},
+		},
+		{
+			Scenario: "rotational-false",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				Rotational: &addressableFalse,
+			},
+			Expected: map[string]string{
+				"rotational": "false",
+			},
+		},
+		{
+			Scenario: "everything-bagel",
+			Hints: metal3v1alpha1.RootDeviceHints{
+				DeviceName:         "userd_devicename",
+				HCTL:               "1:2:3:4",
+				Model:              "userd_model",
+				Vendor:             "userd_vendor",
+				SerialNumber:       "userd_serial",
+				MinSizeGigabytes:   40,
+				WWN:                "userd_wwn",
+				WWNWithExtension:   "userd_with_extension",
+				WWNVendorExtension: "userd_vendor_extension",
+				Rotational:         &addressableTrue,
+			},
+			Expected: map[string]string{
+				"name":                 "s== userd_devicename",
+				"hctl":                 "s== 1:2:3:4",
+				"model":                "<in> userd_model",
+				"vendor":               "<in> userd_vendor",
+				"serial":               "s== userd_serial",
+				"size":                 ">= 40",
+				"wwn":                  "s== userd_wwn",
+				"wwn_with_extension":   "s== userd_with_extension",
+				"wwn_vendor_extension": "s== userd_vendor_extension",
+				"rotational":           "true",
+			},
+		},
+		{
+			Scenario: "empty",
+			Hints:    metal3v1alpha1.RootDeviceHints{},
+			Expected: map[string]string{},
+		},
+	} {
+		t.Run(tc.Scenario, func(t *testing.T) {
+			actual := MakeHintMap(&tc.Hints)
+			assert.Equal(t, tc.Expected, actual, "hint map does not match")
+		})
+	}
+}
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index d172f9a2..e26ac1ad 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -1,8 +1,11 @@
 package ironic
 
 import (
+	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 
@@ -17,6 +20,114 @@ func init() {
 	logf.SetLogger(logf.ZapLogger(true))
 }
 
+func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) {
+	rotational := true
+	host := &metal3v1alpha1.BareMetalHost{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "myhost",
+			Namespace: "myns",
+			UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+		},
+		Spec: metal3v1alpha1.BareMetalHostSpec{
+			Image: &metal3v1alpha1.Image{
+				URL: "not-empty",
+			},
+			Online:          true,
+			HardwareProfile: "libvirt",
+			RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+				DeviceName:         "userd_devicename",
+				HCTL:               "1:2:3:4",
+				Model:              "userd_model",
+				Vendor:             "userd_vendor",
+				SerialNumber:       "userd_serial",
+				MinSizeGigabytes:   40,
+				WWN:                "userd_wwn",
+				WWNWithExtension:   "userd_with_extension",
+				WWNVendorExtension: "userd_vendor_extension",
+				Rotational:         &rotational,
+			},
+		},
+		Status: metal3v1alpha1.BareMetalHostStatus{
+			Provisioning: metal3v1alpha1.ProvisionStatus{
+				ID: "provisioning-id",
+				// Place the hints in the status field to pretend the
+				// controller has already reconciled partially.
+				RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+					DeviceName:         "userd_devicename",
+					HCTL:               "1:2:3:4",
+					Model:              "userd_model",
+					Vendor:             "userd_vendor",
+					SerialNumber:       "userd_serial",
+					MinSizeGigabytes:   40,
+					WWN:                "userd_wwn",
+					WWNWithExtension:   "userd_with_extension",
+					WWNVendorExtension: "userd_vendor_extension",
+					Rotational:         &rotational,
+				},
+			},
+			HardwareProfile: "libvirt",
+		},
+	}
+
+	eventPublisher := func(reason, message string) {}
+
+	prov, err := newProvisioner(host, bmc.Credentials{}, eventPublisher)
+	ironicNode := &nodes.Node{}
+
+	patches, err := prov.getUpdateOptsForNode(ironicNode)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Logf("patches: %v", patches)
+
+	expected := []struct {
+		Path  string            // the node property path
+		Map   map[string]string // Expected roothdevicehint map
+		Value interface{}       // the value being passed to ironic (or value associated with the key)
+	}{
+		{
+			Path:  "/properties/root_device",
+			Value: "userdefined_devicename",
+			Map: map[string]string{
+				"name":                 "s== userd_devicename",
+				"hctl":                 "s== 1:2:3:4",
+				"model":                "<in> userd_model",
+				"vendor":               "<in> userd_vendor",
+				"serial":               "s== userd_serial",
+				"size":                 ">= 40",
+				"wwn":                  "s== userd_wwn",
+				"wwn_with_extension":   "s== userd_with_extension",
+				"wwn_vendor_extension": "s== userd_vendor_extension",
+				"rotational":           "true",
+			},
+		},
+	}
+
+	for _, e := range expected {
+		t.Run(e.Path, func(t *testing.T) {
+			t.Logf("expected: %v", e)
+			var update nodes.UpdateOperation
+			for _, patch := range patches {
+				update = patch.(nodes.UpdateOperation)
+				if update.Path == e.Path {
+					break
+				}
+			}
+			if update.Path != e.Path {
+				t.Errorf("did not find %q in updates", e.Path)
+				return
+			}
+			t.Logf("update: %v", update)
+			if e.Map != nil {
+				assert.Equal(t, e.Map, update.Value, fmt.Sprintf("%s does not match", e.Path))
+			} else {
+				assert.Equal(t, e.Value, update.Value, fmt.Sprintf("%s does not match", e.Path))
+			}
+		})
+	}
+}
+
 func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 	host := &metal3v1alpha1.BareMetalHost{
 		ObjectMeta: metav1.ObjectMeta{
@@ -53,8 +164,6 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 
 	t.Logf("patches: %v", patches)
 
-	var update nodes.UpdateOperation
-
 	expected := []struct {
 		Path  string      // the node property path
 		Key   string      // if value is a map, the key we care about
@@ -84,11 +193,6 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 			Path:  "/instance_info/root_gb",
 			Value: 10,
 		},
-		{
-			Path:  "/instance_info/root_device",
-			Value: "/dev/vda",
-			Key:   "name",
-		},
 		{
 			Path:  "/properties/cpu_arch",
 			Value: "x86_64",
@@ -99,18 +203,23 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 		},
 	}
 
-	for i, e := range expected {
-		update = patches[i].(nodes.UpdateOperation)
-		if e.Key != "" {
-			m := update.Value.(map[string]string)
-			if m[e.Key] != e.Value {
-				t.Errorf("expected %s=%q got %s=%q", e.Path, e.Value, update.Path, update.Value)
+	for _, e := range expected {
+		t.Run(e.Path, func(t *testing.T) {
+			t.Logf("expected: %v", e)
+			var update nodes.UpdateOperation
+			for _, patch := range patches {
+				update = patch.(nodes.UpdateOperation)
+				if update.Path == e.Path {
+					break
+				}
 			}
-		} else {
-			if update.Value != e.Value {
-				t.Errorf("expected %s=%q got %s=%q", e.Path, e.Value, update.Path, update.Value)
+			if update.Path != e.Path {
+				t.Errorf("did not find %q in updates", e.Path)
+				return
 			}
-		}
+			t.Logf("update: %v", update)
+			assert.Equal(t, e.Value, update.Value, fmt.Sprintf("%s does not match", e.Path))
+		})
 	}
 }
 
@@ -150,8 +259,6 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 
 	t.Logf("patches: %v", patches)
 
-	var update nodes.UpdateOperation
-
 	expected := []struct {
 		Path  string      // the node property path
 		Key   string      // if value is a map, the key we care about
@@ -177,11 +284,6 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 			Path:  "/instance_info/root_gb",
 			Value: 10,
 		},
-		{
-			Path:  "/instance_info/root_device",
-			Value: "0:0:0:0",
-			Key:   "hctl",
-		},
 		{
 			Path:  "/properties/cpu_arch",
 			Value: "x86_64",
@@ -192,17 +294,22 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 		},
 	}
 
-	for i, e := range expected {
-		update = patches[i].(nodes.UpdateOperation)
-		if e.Key != "" {
-			m := update.Value.(map[string]string)
-			if m[e.Key] != e.Value {
-				t.Errorf("expected %s=%q got %s=%q", e.Path, e.Value, update.Path, update.Value)
+	for _, e := range expected {
+		t.Run(e.Path, func(t *testing.T) {
+			t.Logf("expected: %v", e)
+			var update nodes.UpdateOperation
+			for _, patch := range patches {
+				update = patch.(nodes.UpdateOperation)
+				if update.Path == e.Path {
+					break
+				}
 			}
-		} else {
-			if update.Value != e.Value {
-				t.Errorf("expected %s=%q got %s=%q", e.Path, e.Value, update.Path, update.Value)
+			if update.Path != e.Path {
+				t.Errorf("did not find %q in updates", e.Path)
+				return
 			}
-		}
+			t.Logf("update: %v", update)
+			assert.Equal(t, e.Value, update.Value, fmt.Sprintf("%s does not match", e.Path))
+		})
 	}
 }