Repository: metal3-io/baremetal-operator
Pull request #909 (2021-06-09 14:34:22)

Always build a configdrive except for live ISO
We have default meta data that we should pass in any case.

PR based on top of https://github.com/metal3-io/baremetal-operator/pull/900.

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

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **dtantsur** after the PR has been reviewed.

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

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

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

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":["dtantsur"]} -->
dtantsur: /test-integration
hardys: This lgtm - one small nit re the comment but IMO it makes sense to always pass the metal3 meta-data regardless of the user input in the spec.

I suspect that the reason this ended up originally conditional on the userData is that was at first the only field included in the config-drive, then others got added later (@zaneb do you recall if that's accurate perhaps?)
zaneb: > I suspect that the reason this ended up originally conditional on the userData is that was at first the only field included in the config-drive, then others got added later (@zaneb do you recall if that's accurate perhaps?)

Yes, I'm 99% sure it was unintentional for that reason. Metadata was added later in #448 and NetworkData in #348.
metal3-io-bot: @dtantsur: PR needs rebase.

<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.
</details>

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index f9e7b09a..b26668f3 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -438,6 +438,10 @@ type Image struct {
 	DiskFormat *string `json:"format,omitempty"`
 }
 
+func (image *Image) IsLiveISO() bool {
+	return image != nil && image.DiskFormat != nil && *image.DiskFormat == "live-iso"
+}
+
 // Custom deploy is a description of a customized deploy process.
 type CustomDeploy struct {
 	// Custom deploy method name.
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 7d4a3580..6ec4f0bb 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -795,7 +795,7 @@ func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, im
 	if hasCustomDeploy {
 		// Custom deploy process
 		p.setCustomDeployUpdateOptsForNode(ironicNode, imageData, updater)
-	} else if imageData.DiskFormat != nil && *imageData.DiskFormat == "live-iso" {
+	} else if imageData.IsLiveISO() {
 		// Set live-iso format options
 		p.setLiveIsoUpdateOptsForNode(ironicNode, imageData, updater)
 	} else {
@@ -897,7 +897,7 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, data pr
 
 func (p *ironicProvisioner) deployInterface(image *metal3v1alpha1.Image) (result string) {
 	result = "direct"
-	if image != nil && image.DiskFormat != nil && *image.DiskFormat == "live-iso" {
+	if image.IsLiveISO() {
 		result = "ramdisk"
 	}
 	return result
@@ -957,7 +957,7 @@ func (p *ironicProvisioner) Adopt(data provisioner.AdoptData, force bool) (resul
 func (p *ironicProvisioner) ironicHasSameImage(ironicNode *nodes.Node, image metal3v1alpha1.Image) (sameImage bool) {
 	// To make it easier to test if ironic is configured with
 	// the same image we are trying to provision to the host.
-	if image.DiskFormat != nil && *image.DiskFormat == "live-iso" {
+	if image.IsLiveISO() {
 		sameImage = (ironicNode.InstanceInfo["boot_iso"] == image.URL)
 		p.log.Info("checking image settings",
 			"boot_iso", ironicNode.InstanceInfo["boot_iso"],
@@ -1124,6 +1124,15 @@ func (p *ironicProvisioner) Prepare(data provisioner.PrepareData, unprepared boo
 }
 
 func (p *ironicProvisioner) getConfigDrive(data provisioner.ProvisionData) (configDrive nodes.ConfigDrive, err error) {
+	// In theory, Ironic can support configdrive with live ISO by attaching
+	// it to another virtual media slot. However, some hardware does not
+	// support two virtual media devices at the same time, so we shouldn't
+	// try it.
+	if data.Image.IsLiveISO() {
+		p.log.Info("not providing config drive for live ISO")
+		return
+	}
+
 	// Retrieve instance specific user data (cloud-init, ignition, etc).
 	userData, err := data.HostConfig.UserData()
 	if err != nil {
@@ -1147,7 +1156,7 @@ func (p *ironicProvisioner) getConfigDrive(data provisioner.ProvisionData) (conf
 	}
 
 	// Retrieve meta data with fallback to defaults from provisioner.
-	metaData := map[string]interface{}{
+	configDrive.MetaData = map[string]interface{}{
 		"uuid":             string(p.objectMeta.UID),
 		"metal3-namespace": p.objectMeta.Namespace,
 		"metal3-name":      p.objectMeta.Name,
@@ -1160,19 +1169,11 @@ func (p *ironicProvisioner) getConfigDrive(data provisioner.ProvisionData) (conf
 		return configDrive, errors.Wrap(err, "could not retrieve metadata")
 	}
 	if metaDataRaw != "" {
-		if err = yaml.Unmarshal([]byte(metaDataRaw), &metaData); err != nil {
+		if err = yaml.Unmarshal([]byte(metaDataRaw), &configDrive.MetaData); err != nil {
 			return configDrive, errors.Wrap(err, "failed to unmarshal metadata from secret")
 		}
 	}
 
-	// Set metaData if any field is populated by a user.
-	if metaDataRaw != "" || networkDataRaw != "" || userData != "" {
-		configDrive.MetaData = metaData
-		p.log.Info("triggering provisioning with config drive")
-	} else {
-		p.log.Info("triggering provisioning without config drive")
-	}
-
 	return
 }
 


Test patch:
diff --git a/pkg/provisioner/ironic/configdrive_test.go b/pkg/provisioner/ironic/configdrive_test.go
index 3a41c0fc..1a55a7dc 100644
--- a/pkg/provisioner/ironic/configdrive_test.go
+++ b/pkg/provisioner/ironic/configdrive_test.go
@@ -19,14 +19,37 @@ func TestEmpty(t *testing.T) {
 	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
 
 	cases := []struct {
-		name     string
-		hostData provisioner.HostConfigData
-		expected nodes.ConfigDrive
+		name       string
+		hostData   provisioner.HostConfigData
+		diskFormat string
+		expected   nodes.ConfigDrive
 	}{
 		{
-			name:     "empty",
+			name:     "default",
 			hostData: fixture.NewHostConfigData("", "", ""),
-			expected: nodes.ConfigDrive{},
+			expected: nodes.ConfigDrive{
+				MetaData: map[string]interface{}{
+					"local-hostname":   "myhost",
+					"local_hostname":   "myhost",
+					"metal3-name":      "myhost",
+					"metal3-namespace": "myns",
+					"name":             "myhost",
+				},
+			},
+		},
+		{
+			name:       "default with disk format",
+			hostData:   fixture.NewHostConfigData("", "", ""),
+			diskFormat: "qcow2",
+			expected: nodes.ConfigDrive{
+				MetaData: map[string]interface{}{
+					"local-hostname":   "myhost",
+					"local_hostname":   "myhost",
+					"metal3-name":      "myhost",
+					"metal3-namespace": "myns",
+					"name":             "myhost",
+				},
+			},
 		},
 		{
 			name:     "everything",
@@ -90,6 +113,12 @@ func TestEmpty(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:       "live ISO",
+			hostData:   fixture.NewHostConfigData("", "", ""),
+			diskFormat: "live-iso",
+			expected:   nodes.ConfigDrive{},
+		},
 	}
 
 	for _, tc := range cases {
@@ -118,9 +147,18 @@ func TestEmpty(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
+			var diskFormat *string
+			if tc.diskFormat != "" {
+				diskFormat = &tc.diskFormat
+			}
+
 			result, err := prov.getConfigDrive(provisioner.ProvisionData{
 				HostConfig: tc.hostData,
 				BootMode:   v1alpha1.DefaultBootMode,
+				Image: v1alpha1.Image{
+					URL:        "http://image",
+					DiskFormat: diskFormat,
+				},
 			})
 
 			if len(tc.expected.MetaData) > 0 {