Repository: metal3-io/baremetal-operator
Pull request #953 (2021-08-13 13:05:47)

Honor DefaultDeployInterface value from Ironic config
Honor DefaultDeployInterface value from Ironic config.

Signed-off-by: s3rj1k <evasive.gyron@gmail.com>

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 assign **stbenjam** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @stbenjam` in a comment when ready.

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":["stbenjam"]} -->
s3rj1k: /approve
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/953#issuecomment-898444587" title="Approved">s3rj1k</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **stbenjam** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @stbenjam` in a comment when ready.

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":["stbenjam"]} -->
s3rj1k: /test-integration
dtantsur: I think it can be simplified. Instead of providing any value, we can use the JSON patch "delete" on `/driver_interface`. This will reset the field to its default value, exactly what you want.
s3rj1k: @dtantsur Care to provide code example? With PATCH approach change is needed outside of `deployInterface()` as it seems ?
dtantsur: This is how we do it in Python client, not sure if it helps: https://opendev.org/openstack/python-ironicclient/src/branch/master/ironicclient/osc/v1/baremetal_node.py#L1354-L1356
s3rj1k: > This is how we do it in Python client, not sure if it helps: https://opendev.org/openstack/python-ironicclient/src/branch/master/ironicclient/osc/v1/baremetal_node.py#L1354-L1356

But does this happen before node is created or after?

in baremetal-operator `deployInterface` return value is used for node creation https://github.com/metal3-io/baremetal-operator/blob/d08856d6dc597e6aadaea24fdfc15594a414938c/pkg/provisioner/ironic/ironic.go#L374

so if I understand correct, `PATCH` approach would be applicable only after node is created, somewhere near here 
https://github.com/metal3-io/baremetal-operator/blob/d08856d6dc597e6aadaea24fdfc15594a414938c/pkg/provisioner/ironic/ironic.go#L454

but this is exactly what I want to avoid, by creating node with needed  `deployInterface` value so there would not be a need to change that value for existing nodes.
dtantsur: Then we can just provide an empty value and let Ironic decide?
s3rj1k: @dtantsur We probably could, @zaneb what do you think?
dtantsur: /test-integration
/lgtm
metal3-io-bot: New changes are detected. LGTM label has been removed.
s3rj1k: /test-integration
s3rj1k: /retest
s3rj1k: /re-test
s3rj1k: /test all
s3rj1k: /test-integration
s3rj1k: @dtantsur Do you have any ideas why test-integration fails ?

Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 13075b7a..783e59b1 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -372,7 +372,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 				BootInterface:       bmcAccess.BootInterface(),
 				Name:                p.objectMeta.Name,
 				DriverInfo:          driverInfo,
-				DeployInterface:     p.deployInterface(data.CurrentImage),
+				DeployInterface:     p.deployInterface(data),
 				InspectInterface:    "inspector",
 				ManagementInterface: bmcAccess.ManagementInterface(),
 				PowerInterface:      bmcAccess.PowerInterface(),
@@ -739,8 +739,11 @@ func (p *ironicProvisioner) setDirectDeployUpdateOptsForNode(ironicNode *nodes.N
 		"image_disk_format":   imageData.DiskFormat,
 	}
 	updater.
-		SetInstanceInfoOpts(optValues, ironicNode).
-		SetTopLevelOpt("deploy_interface", "direct", ironicNode.DeployInterface)
+		SetInstanceInfoOpts(optValues, ironicNode)
+
+	if ironicNode.DeployInterface == "ramdisk" || ironicNode.DeployInterface == "custom-agent" {
+		updater.SetTopLevelOpt("deploy_interface", nil, ironicNode.DeployInterface)
+	}
 
 	driverOptValues := optionsData{
 		"force_persistent_boot_device": "Default",
@@ -910,11 +913,13 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, data pr
 	return
 }
 
-func (p *ironicProvisioner) deployInterface(image *metal3v1alpha1.Image) (result string) {
-	result = "direct"
-	if image.IsLiveISO() {
+func (p *ironicProvisioner) deployInterface(data provisioner.ManagementAccessData) (result string) {
+	if data.CurrentImage.IsLiveISO() {
 		result = "ramdisk"
 	}
+	if data.HasCustomDeploy {
+		result = "custom-agent"
+	}
 	return result
 }
 


Test patch:
diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go
index 44044962..32503970 100644
--- a/pkg/provisioner/ironic/updateopts_test.go
+++ b/pkg/provisioner/ironic/updateopts_test.go
@@ -885,6 +885,7 @@ func TestGetUpdateOptsForNodeLiveIsoToImage(t *testing.T) {
 		InstanceInfo: map[string]interface{}{
 			"boot_iso": "oldimage",
 		},
+		DeployInterface: "ramdisk",
 	}
 
 	provData := provisioner.ProvisionData{
@@ -906,9 +907,8 @@ func TestGetUpdateOptsForNodeLiveIsoToImage(t *testing.T) {
 			Op:   nodes.RemoveOp,
 		},
 		{
-			Path:  "/deploy_interface",
-			Value: "direct",
-			Op:    nodes.AddOp,
+			Path: "/deploy_interface",
+			Op:   nodes.RemoveOp,
 		},
 		{
 			Path:  "/instance_info/image_source",
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index bfcc5b7e..f9b26840 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -108,7 +108,7 @@ func TestValidateManagementAccessCreateNodeNoImage(t *testing.T) {
 	assert.Equal(t, "", result.ErrorMessage)
 	assert.NotEqual(t, "", createdNode.UUID)
 	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, createdNode.DeployInterface, "direct")
+	assert.Equal(t, createdNode.DeployInterface, "")
 }
 
 func TestValidateManagementAccessCreateWithImage(t *testing.T) {
@@ -143,7 +143,7 @@ func TestValidateManagementAccessCreateWithImage(t *testing.T) {
 	}
 	assert.Equal(t, "", result.ErrorMessage)
 	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, createdNode.DeployInterface, "direct")
+	assert.Equal(t, createdNode.DeployInterface, "")
 	updates, _ := ironic.GetLastRequestFor("/v1/nodes/node-0", http.MethodPatch)
 	assert.Contains(t, updates, "/instance_info/image_source")
 	assert.Contains(t, updates, host.Spec.Image.URL)
@@ -347,7 +347,6 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 			DriverInfo: map[string]interface{}{},
 		},
 		{
-			DeployInterface: "direct",
 			Image: &metal3v1alpha1.Image{
 				URL:      "theimage",
 				Checksum: "thechecksum",