Repository: metal3-io/baremetal-operator (go)

Issue:
baremetal-operator doesn't set the Ironic boot capabilities
Problem:

Deploying an environment with bare metal nodes simulated as VMs via redfish interface with sushy-tools. After master and worker nodes have been provisioned the master nodes have UEFI firmware set while the worker nodes have BIOS firmware set.

There seems to be a config drift in the bootstrap and baremetal-operator Ironic. On the bootstrap Ironic we set "capabilities": "boot_mode:uefi" as introduced by https://github.com/openshift/installer/pull/2727/files while in bmo Ironic the nodes do not have capabilities set:

```yaml
(openstack-cli) [kni@provisionhost-0 ~]$ openstack baremetal node show openshift-worker-0 -f yaml
allocation_uuid: null
automated_clean: null
bios_interface: no-bios
boot_interface: ipxe
chassis_uuid: null
clean_step: {}
conductor: master-0.ocp-edge-cluster.qe.lab.redhat.com
conductor_group: ''
console_enabled: false
console_interface: no-console
created_at: '2020-02-10T20:51:10+00:00'
deploy_interface: direct
deploy_step: {}
description: null
driver: redfish
driver_info:
  deploy_kernel: http://172.22.0.3:6180/images/ironic-python-agent.kernel
  deploy_ramdisk: http://172.22.0.3:6180/images/ironic-python-agent.initramfs
  redfish_address: http://192.168.123.1:8000
  redfish_password: '******'
  redfish_system_id: /redfish/v1/Systems/9a7aabf3-0e44-4a25-8dc9-4ff8fcbe445d
  redfish_username: admin
driver_internal_info:
  agent_last_heartbeat: '2020-02-10T20:57:29.519171'
  agent_url: http://172.22.0.95:9999
  agent_version: 5.0.1.dev7
  deploy_boot_mode: bios
  deploy_steps: null
  is_whole_disk_image: true
  last_power_state_change: '2020-02-10T20:58:34.159989'
  root_uuid_or_disk_id: '0x00000000'
extra: {}
fault: null
inspect_interface: inspector
inspection_finished_at: null
inspection_started_at: '2020-02-10T20:51:12+00:00'
instance_info:
  configdrive: '******'
  image_checksum: http://172.22.0.3:6180/images/rhcos-44.81.202001241431.0-openstack.x86_64.qcow2/rhcos-44.81.202001241431.0-compressed.x86_64.qcow2.md5sum
  image_source: http://172.22.0.3:6180/images/rhcos-44.81.202001241431.0-openstack.x86_64.qcow2/rhcos-44.81.202001241431.0-compressed.x86_64.qcow2
  image_type: whole-disk-image
  image_url: '******'
  root_gb: 10
instance_uuid: 78285fd9-739c-4409-87a5-30310529e3bf
last_error: null
maintenance: false
maintenance_reason: null
management_interface: redfish
name: openshift-worker-0
network_interface: noop
owner: null
power_interface: redfish
power_state: power on
properties:
  capabilities: cpu_vt:true,cpu_aes:true,cpu_hugepages:true,cpu_hugepages_1g:true
  cpu_arch: x86_64
  cpus: '8'
  local_gb: 50
  memory_mb: '16384'
  root_device:
    name: /dev/sda
protected: false
protected_reason: null
provision_state: active
provision_updated_at: '2020-02-10T20:58:35+00:00'
raid_config: {}
raid_interface: no-raid
rescue_interface: no-rescue
reservation: null
resource_class: null
storage_interface: noop
target_power_state: null
target_provision_state: null
target_raid_config: {}
traits: []
updated_at: '2020-02-10T20:58:35+00:00'
uuid: 29817852-edcc-4c94-beca-a9d6101374f0
vendor_interface: no-vendor
```

Pull request description:
implement explicit boot mode api field
Default to UEFI everywhere, but allow the user to override that value
by setting Spec.BootMode.

Implements https://github.com/metal3-io/metal3-docs/blob/master/design/baremetal-operator/explicit-boot-mode.md

Fixes #422 

Patch:
diff --git a/deploy/crds/metal3.io_baremetalhosts_crd.yaml b/deploy/crds/metal3.io_baremetalhosts_crd.yaml
index 9d50187d..37620ef4 100644
--- a/deploy/crds/metal3.io_baremetalhosts_crd.yaml
+++ b/deploy/crds/metal3.io_baremetalhosts_crd.yaml
@@ -92,7 +92,7 @@ spec:
                 type: string
               bootMode:
                 description: Select the method of initializing the hardware during
-                  boot to override the value based on the BMC driver.
+                  boot. Defaults to UEFI.
                 enum:
                 - UEFI
                 - legacy
diff --git a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
index cc9e2bca..1ad1b84f 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@@ -79,8 +79,9 @@ type BootMode string
 
 // Allowed boot mode from metal3
 const (
-	UEFI   BootMode = "UEFI"
-	Legacy BootMode = "legacy"
+	UEFI            BootMode = "UEFI"
+	Legacy          BootMode = "legacy"
+	DefaultBootMode BootMode = UEFI
 )
 
 // OperationalStatus represents the state of the host
@@ -226,8 +227,8 @@ type BareMetalHostSpec struct {
 	// being provisioned.
 	RootDeviceHints *RootDeviceHints `json:"rootDeviceHints,omitempty"`
 
-	// Select the method of initializing the hardware during boot to
-	// override the value based on the BMC driver.
+	// Select the method of initializing the hardware during
+	// boot. Defaults to UEFI.
 	// +optional
 	BootMode BootMode `json:"bootMode,omitempty"`
 
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller.go b/pkg/controller/baremetalhost/baremetalhost_controller.go
index 4b23b87f..8433c4b0 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller.go
@@ -798,7 +798,7 @@ func (r *ReconcileBareMetalHost) actionManageReady(prov provisioner.Provisioner,
 	}
 
 	if info.host.NeedsProvisioning() {
-		// Ensure the root device hints we're going to use are stored.
+		// Ensure the provisioning settings 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")}
@@ -833,6 +833,18 @@ func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty boo
 		host.Status.Provisioning.RootDeviceHints = hintSource
 		dirty = true
 	}
+
+	// Make sure we have saved the current boot mode value, using a
+	// default if the spec does not include a value.
+	bootMode := host.Spec.BootMode
+	if bootMode == "" {
+		bootMode = metal3v1alpha1.DefaultBootMode
+	}
+	if bootMode != host.Status.Provisioning.BootMode {
+		host.Status.Provisioning.BootMode = bootMode
+		dirty = true
+	}
+
 	return
 }
 
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 6bd5b6a3..f678891e 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -47,6 +47,11 @@ const (
 	powerNone    = "None"
 )
 
+var bootModeCapabilities = map[metal3v1alpha1.BootMode]string{
+	metal3v1alpha1.UEFI:   "uefi",
+	metal3v1alpha1.Legacy: "bios",
+}
+
 func init() {
 	// NOTE(dhellmann): Use Fprintf() to report errors instead of
 	// logging, because logging is not configured yet in init().
@@ -785,12 +790,33 @@ func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (update
 		},
 	)
 
+	// boot_mode
+	_, ok := ironicNode.InstanceInfo["deploy_boot_mode"]
+	if !ok {
+		op = nodes.AddOp
+	} else {
+		op = nodes.ReplaceOp
+	}
+	p.log.Info("setting boot_mode",
+		"operation", op,
+		"instance_info", ironicNode.InstanceInfo,
+		"bootMode", p.host.Status.Provisioning.BootMode,
+	)
+	updates = append(
+		updates,
+		nodes.UpdateOperation{
+			Op:    op,
+			Path:  "/instance_info/deploy_boot_mode",
+			Value: bootModeCapabilities[p.host.Status.Provisioning.BootMode],
+		},
+	)
+
 	return updates, nil
 }
 
 func (p *ironicProvisioner) startProvisioning(ironicNode *nodes.Node, hostConf provisioner.HostConfigData) (result provisioner.Result, err error) {
 
-	p.log.Info("starting provisioning")
+	p.log.Info("starting provisioning", "node properties", ironicNode.Properties)
 
 	updates, err := p.getUpdateOptsForNode(ironicNode)
 	_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()


Test patch:
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller_test.go b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
index 9e56227b..e1313560 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller_test.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
@@ -1159,3 +1159,84 @@ func TestUpdateRootDeviceHints(t *testing.T) {
 		})
 	}
 }
+
+// TestUpdateBootMode verifies that we apply the correct boot mode
+func TestUpdateBootMode(t *testing.T) {
+	testCases := []struct {
+		Scenario string
+		Host     metal3v1alpha1.BareMetalHost
+		Dirty    bool
+		Expected metal3v1alpha1.BootMode
+	}{
+		{
+			Scenario: "set default",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "unknown",
+				},
+			},
+			Dirty:    true,
+			Expected: metal3v1alpha1.DefaultBootMode,
+		},
+		{
+			Scenario: "user value",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					BootMode: metal3v1alpha1.Legacy,
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "unknown",
+				},
+			},
+			Dirty:    true,
+			Expected: metal3v1alpha1.Legacy,
+		},
+		{
+			Scenario: "already set",
+			Host: metal3v1alpha1.BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+					UID:       "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
+				},
+				Spec: metal3v1alpha1.BareMetalHostSpec{
+					BootMode:        metal3v1alpha1.Legacy,
+					HardwareProfile: "libvirt",
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "libvirt",
+					Provisioning: metal3v1alpha1.ProvisionStatus{
+						BootMode: metal3v1alpha1.Legacy,
+						RootDeviceHints: &metal3v1alpha1.RootDeviceHints{
+							DeviceName: "/dev/vda",
+						},
+					},
+				},
+			},
+			Dirty:    false,
+			Expected: metal3v1alpha1.Legacy,
+		},
+	}
+
+	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.BootMode)
+		})
+	}
+}
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index e26ac1ad..8cc022ad 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -313,3 +313,122 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 		})
 	}
 }
+
+func TestGetUpdateOptsForNodeBootMode(t *testing.T) {
+
+	testCases := []struct {
+		Scenario string
+		BootMode metal3v1alpha1.BootMode
+		Node     nodes.Node
+		Expected nodes.UpdateOperation
+	}{
+		{
+			Scenario: "add uefi",
+			BootMode: metal3v1alpha1.UEFI,
+			Node:     nodes.Node{},
+			Expected: nodes.UpdateOperation{
+				Op:    nodes.AddOp,
+				Path:  "/instance_info/deploy_boot_mode",
+				Value: "uefi",
+			},
+		},
+		{
+			Scenario: "add uefi",
+			BootMode: metal3v1alpha1.Legacy,
+			Node:     nodes.Node{},
+			Expected: nodes.UpdateOperation{
+				Op:    nodes.AddOp,
+				Path:  "/instance_info/deploy_boot_mode",
+				Value: "bios",
+			},
+		},
+		{
+			Scenario: "replace uefi",
+			BootMode: metal3v1alpha1.UEFI,
+			Node: nodes.Node{
+				InstanceInfo: map[string]interface{}{
+					"deploy_boot_mode": "legacy",
+				},
+			},
+			Expected: nodes.UpdateOperation{
+				Op:    nodes.ReplaceOp,
+				Path:  "/instance_info/deploy_boot_mode",
+				Value: "uefi",
+			},
+		},
+		{
+			Scenario: "replace legacy",
+			BootMode: metal3v1alpha1.Legacy,
+			Node: nodes.Node{
+				InstanceInfo: map[string]interface{}{
+					"deploy_boot_mode": "legacy",
+				},
+			},
+			Expected: nodes.UpdateOperation{
+				Op:    nodes.ReplaceOp,
+				Path:  "/instance_info/deploy_boot_mode",
+				Value: "bios",
+			},
+		},
+	}
+
+	eventPublisher := func(reason, message string) {}
+
+	for _, tc := range testCases {
+		t.Run(tc.Scenario, func(t *testing.T) {
+			t.Logf("expected: %v", tc.Expected)
+
+			// Create a host with the boot mode in the status set to
+			// the test value. getUpdateOptsForNode() doesn't care
+			// where that value came from before it ended up in the
+			// status.
+			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",
+						Checksum:     "checksum",
+						ChecksumType: metal3v1alpha1.MD5,
+						DiskFormat:   pointer.StringPtr("raw"),
+					},
+					Online: true,
+				},
+				Status: metal3v1alpha1.BareMetalHostStatus{
+					HardwareProfile: "libvirt",
+					Provisioning: metal3v1alpha1.ProvisionStatus{
+						BootMode: tc.BootMode,
+					},
+				},
+			}
+
+			prov, err := newProvisioner(&host, bmc.Credentials{}, eventPublisher)
+			patches, err := prov.getUpdateOptsForNode(&tc.Node)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			t.Logf("patches: %v", patches)
+
+			var update nodes.UpdateOperation
+			for _, patch := range patches {
+				update = patch.(nodes.UpdateOperation)
+				if update.Path == tc.Expected.Path {
+					break
+				}
+			}
+			if update.Path != tc.Expected.Path {
+				t.Errorf("did not find %q in updates", tc.Expected.Path)
+				return
+			}
+			t.Logf("update: %v", update)
+			assert.Equal(t,
+				tc.Expected, update,
+				fmt.Sprintf("%s does not match", tc.Expected.Path),
+			)
+		})
+	}
+}