Repository: metal3-io/baremetal-operator
Pull request #611 (2020-07-31 17:23:16)

ensure boot mode is set before introspection begins
Reverse part of the work from
https://github.com/metal3-io/baremetal-operator/pull/602 based on new
feedback from the ironic team and the realization that the deploy boot
mode only controls the mode used during deploying and not
introspection.

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/611#" 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":[]} -->
dhellmann: /cc @zaneb @dtantsur 

dhellmann: /test-integration
metal3-io-bot: @dhellmann: The following test **failed**, say `/retest` to rerun them all:

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



<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/pkg/apis/metal3/v1alpha1/baremetalhost_types.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
index 1ad1b84f..5bf1124c 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@@ -587,6 +587,15 @@ type BareMetalHost struct {
 	Status BareMetalHostStatus `json:"status,omitempty"`
 }
 
+// BootMode returns the boot method to use for the host.
+func (host *BareMetalHost) BootMode() BootMode {
+	mode := host.Spec.BootMode
+	if mode == "" {
+		return DefaultBootMode
+	}
+	return mode
+}
+
 // Available returns true if the host is available to be provisioned.
 func (host *BareMetalHost) Available() bool {
 	if host.Spec.ConsumerRef != nil {
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller.go b/pkg/controller/baremetalhost/baremetalhost_controller.go
index 4b9630b9..8eaa2c8e 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller.go
@@ -487,6 +487,14 @@ func (r *ReconcileBareMetalHost) actionRegistering(prov provisioner.Provisioner,
 		info.postSaveCallbacks = append(info.postSaveCallbacks, updatedCredentials.Inc)
 	}
 
+	// Make sure we have saved the current boot mode value.
+	bootMode := info.host.BootMode()
+	if bootMode != info.host.Status.Provisioning.BootMode {
+		info.log.Info("saving boot mode")
+		info.host.Status.Provisioning.BootMode = bootMode
+		return actionContinue{}
+	}
+
 	provResult, err := prov.ValidateManagementAccess(credsChanged)
 	if err != nil {
 		noManagementAccess.Inc()
@@ -845,17 +853,6 @@ func saveHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) (dirty boo
 		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 a15e38ae..6ecc5190 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -48,8 +48,8 @@ const (
 )
 
 var bootModeCapabilities = map[metal3v1alpha1.BootMode]string{
-	metal3v1alpha1.UEFI:   "uefi",
-	metal3v1alpha1.Legacy: "bios",
+	metal3v1alpha1.UEFI:   "boot_mode:uefi",
+	metal3v1alpha1.Legacy: "boot_mode:bios",
 }
 
 func init() {
@@ -243,6 +243,8 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 	if ironicNode == nil {
 		p.log.Info("registering host in ironic")
 
+		bootMode := p.host.BootMode()
+
 		ironicNode, err = nodes.Create(
 			p.client,
 			nodes.CreateOpts{
@@ -255,6 +257,9 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 				PowerInterface:      p.bmcAccess.PowerInterface(),
 				RAIDInterface:       p.bmcAccess.RAIDInterface(),
 				VendorInterface:     p.bmcAccess.VendorInterface(),
+				Properties: map[string]interface{}{
+					"capabilities": bootModeCapabilities[bootMode],
+				},
 			}).Extract()
 		// FIXME(dhellmann): Handle 409 and 503? errors here.
 		if err != nil {
@@ -790,27 +795,6 @@ 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
 }
 


Test patch:
diff --git a/pkg/apis/metal3/v1alpha1/baremetalhost_types_test.go b/pkg/apis/metal3/v1alpha1/baremetalhost_types_test.go
index fd7b4b65..75023826 100644
--- a/pkg/apis/metal3/v1alpha1/baremetalhost_types_test.go
+++ b/pkg/apis/metal3/v1alpha1/baremetalhost_types_test.go
@@ -4,6 +4,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -665,3 +667,41 @@ func TestGetImageChecksum(t *testing.T) {
 		})
 	}
 }
+
+func TestBootMode(t *testing.T) {
+	for _, tc := range []struct {
+		Scenario  string
+		HostValue BootMode
+		Expected  BootMode
+	}{
+		{
+			Scenario:  "default",
+			HostValue: "",
+			Expected:  UEFI,
+		},
+		{
+			Scenario:  "UEFI",
+			HostValue: UEFI,
+			Expected:  UEFI,
+		},
+		{
+			Scenario:  "legcy",
+			HostValue: Legacy,
+			Expected:  Legacy,
+		},
+	} {
+		t.Run(tc.Scenario, func(t *testing.T) {
+			host := &BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+				},
+				Spec: BareMetalHostSpec{
+					BootMode: tc.HostValue,
+				},
+			}
+			actual := host.BootMode()
+			assert.Equal(t, tc.Expected, actual)
+		})
+	}
+}
diff --git a/pkg/controller/baremetalhost/baremetalhost_controller_test.go b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
index 8caa62a1..e37bbe88 100644
--- a/pkg/controller/baremetalhost/baremetalhost_controller_test.go
+++ b/pkg/controller/baremetalhost/baremetalhost_controller_test.go
@@ -1165,87 +1165,6 @@ 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)
-		})
-	}
-}
-
 func TestProvisionerIsReady(t *testing.T) {
 	host := newDefaultHost(t)
 
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index 19e19ccd..edd5c2db 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -376,125 +376,6 @@ 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),
-			)
-		})
-	}
-}
-
 func makeHost() *metal3v1alpha1.BareMetalHost {
 	rotational := true