Repository: metal3-io/baremetal-operator
Pull request #1446 (2023-11-16 10:08:15)

✨ Detect which inspection implementation to use for new nodes

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

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [dtantsur](https://github.com/dtantsur). For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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/main/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"]} -->
metal3-io-bot: @dtantsur: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gomod | 5a87308f391b89ef204b7c3143e6e6f0082265eb | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1446/gomod/1725093428783484928) | true | `/test gomod`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1446). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:dtantsur).

<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 -->
dtantsur: /test-centos-e2e-integration-main 
/test-ubuntu-integration-main 
elfosardo: /lgtm
elfosardo: /test-centos-e2e-integration-main
/test-ubuntu-integration-main
elfosardo: /lgtm

Patch:
diff --git a/go.mod b/go.mod
index 69f0b396..104858c6 100644
--- a/go.mod
+++ b/go.mod
@@ -14,6 +14,7 @@ require (
 	github.com/stretchr/testify v1.8.4
 	go.etcd.io/etcd/client/pkg/v3 v3.5.10
 	go.uber.org/zap v1.26.0
+	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1
 	k8s.io/api v0.27.7
 	k8s.io/apimachinery v0.27.7
 	k8s.io/client-go v0.27.7
@@ -59,7 +60,6 @@ require (
 	github.com/spf13/cobra v1.6.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	go.uber.org/multierr v1.10.0 // indirect
-	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect
 	golang.org/x/net v0.17.0 // indirect
 	golang.org/x/oauth2 v0.8.0 // indirect
 	golang.org/x/sys v0.14.0 // indirect
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 58eadceb..88ed19e0 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -8,8 +8,10 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
+	"golang.org/x/exp/slices"
 
 	"github.com/gophercloud/gophercloud"
+	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/drivers"
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/ports"
 
@@ -315,6 +317,19 @@ func (p *ironicProvisioner) createPXEEnabledNodePort(uuid, macAddress string) er
 	return nil
 }
 
+func (p *ironicProvisioner) getInspectInterface(bmcAccess bmc.AccessDetails) (string, error) {
+	driver, err := drivers.GetDriverDetails(p.client, bmcAccess.Driver()).Extract()
+	if err != nil {
+		return "", fmt.Errorf("cannot load information about driver %s: %w", bmcAccess.Driver(), err)
+	}
+
+	if slices.Contains(driver.EnabledInspectInterfaces, "agent") {
+		return "agent", nil
+	}
+
+	return "inspector", nil // backward compatibility
+}
+
 // ValidateManagementAccess registers the host with the provisioning
 // system and tests the connection information for the host to verify
 // that the location and credentials work.
@@ -367,6 +382,12 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 			return
 		}
 
+		inspectInterface, driverErr := p.getInspectInterface(bmcAccess)
+		if driverErr != nil {
+			result, err = transientError(driverErr)
+			return
+		}
+
 		nodeCreateOpts := nodes.CreateOpts{
 			Driver:              bmcAccess.Driver(),
 			BIOSInterface:       bmcAccess.BIOSInterface(),
@@ -374,7 +395,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 			Name:                ironicNodeName(p.objectMeta),
 			DriverInfo:          driverInfo,
 			DeployInterface:     p.deployInterface(data),
-			InspectInterface:    "inspector",
+			InspectInterface:    inspectInterface,
 			ManagementInterface: bmcAccess.ManagementInterface(),
 			PowerInterface:      bmcAccess.PowerInterface(),
 			RAIDInterface:       bmcAccess.RAIDInterface(),


Test patch:
diff --git a/pkg/provisioner/ironic/testserver/ironic.go b/pkg/provisioner/ironic/testserver/ironic.go
index 85a6dcb3..8cb9b09a 100644
--- a/pkg/provisioner/ironic/testserver/ironic.go
+++ b/pkg/provisioner/ironic/testserver/ironic.go
@@ -51,6 +51,7 @@ func (m *IronicMock) WithDefaultResponses() *IronicMock {
 	m.AddDefaultResponse("/v1/nodes/{id}/states/power", "", http.StatusAccepted, "{}")
 	m.AddDefaultResponse("/v1/nodes/{id}/states/raid", "", http.StatusNoContent, "{}")
 	m.AddDefaultResponse("/v1/nodes/{id}/validate", "", http.StatusOK, validateResult)
+	m.AddDefaultResponse("/v1/drivers/{driver}", "", http.StatusOK, "{}")
 
 	return m
 }
@@ -79,18 +80,24 @@ func (m *IronicMock) WithDrivers() *IronicMock {
 			],
 			"links": [
 			  {
-				"href": "http://[fd00:1101::3]:6385/v1/drivers/fake-hardware",
+				"href": "http://[fd00:1101::3]:6385/v1/drivers/test",
 				"rel": "self"
 			  },
 			  {
-				"href": "http://[fd00:1101::3]:6385/drivers/fake-hardware",
+				"href": "http://[fd00:1101::3]:6385/drivers/test",
 				"rel": "bookmark"
 			  }
 			],
-			"name": "fake-hardware"
+			"name": "test"
 		}]
 	}
 	`, http.StatusOK)
+	m.ResponseWithCode("/v1/drivers/test", `
+	{
+	    "enabled_deploy_interfaces": ["direct", "ramdisk", "custom-agent"],
+	    "enabled_inspect_interfaces": ["agent", "inspector", "no-inspect"]
+	}
+	`, http.StatusOK)
 	return m
 }
 
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 5a73c0c3..eb1f99e6 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -50,6 +50,7 @@ func TestValidateManagementAccessMACOptional(t *testing.T) {
 
 	// Set up ironic server to return the node
 	ironic := testserver.NewIronic(t).
+		WithDrivers().
 		Node(nodes.Node{
 			Name: host.Namespace + nameSeparator + host.Name,
 			UUID: host.Status.Provisioning.ID,
@@ -86,7 +87,7 @@ func TestValidateManagementAccessCreateNodeNoImage(t *testing.T) {
 		createdNode = &node
 	}
 
-	ironic := testserver.NewIronic(t).CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
+	ironic := testserver.NewIronic(t).WithDrivers().CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
 	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
 	ironic.Start()
 	defer ironic.Stop()
@@ -105,6 +106,45 @@ func TestValidateManagementAccessCreateNodeNoImage(t *testing.T) {
 	assert.NotEqual(t, "", createdNode.UUID)
 	assert.Equal(t, createdNode.UUID, provID)
 	assert.Equal(t, createdNode.DeployInterface, "")
+	assert.Equal(t, "agent", createdNode.InspectInterface)
+}
+
+func TestValidateManagementAccessCreateNodeOldInspection(t *testing.T) {
+	// Create a host without a bootMACAddress and with a BMC that
+	// does not require one.
+	host := makeHost()
+	host.Spec.BootMACAddress = ""
+	host.Spec.Image = nil
+	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
+
+	var createdNode *nodes.Node
+
+	createCallback := func(node nodes.Node) {
+		createdNode = &node
+	}
+
+	ironic := testserver.NewIronic(t).CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
+	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
+	ironic.AddDefaultResponse("/v1/drivers/test", "GET", 200, `
+	    {"enabled_inspect_interfaces": ["inspector", "no-inspect"]}
+	`)
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher, ironic.Endpoint(), auth)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	result, provID, err := prov.ValidateManagementAccess(provisioner.ManagementAccessData{}, false, false)
+	if err != nil {
+		t.Fatalf("error from ValidateManagementAccess: %s", err)
+	}
+	assert.Equal(t, "", result.ErrorMessage)
+	assert.NotEqual(t, "", createdNode.UUID)
+	assert.Equal(t, createdNode.UUID, provID)
+	assert.Equal(t, "inspector", createdNode.InspectInterface)
 }
 
 func TestValidateManagementAccessCreateWithImage(t *testing.T) {
@@ -120,7 +160,7 @@ func TestValidateManagementAccessCreateWithImage(t *testing.T) {
 		createdNode = &node
 	}
 
-	ironic := testserver.NewIronic(t).CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
+	ironic := testserver.NewIronic(t).WithDrivers().CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
 	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
 	ironic.Start()
 	defer ironic.Stop()
@@ -156,7 +196,7 @@ func TestValidateManagementAccessCreateWithLiveIso(t *testing.T) {
 		createdNode = &node
 	}
 
-	ironic := testserver.NewIronic(t).CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
+	ironic := testserver.NewIronic(t).WithDrivers().CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
 	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
 	ironic.Start()
 	defer ironic.Stop()