Repository: metal3-io/baremetal-operator
Pull request #1450 (2023-11-17 16:52:13)

✨ Support both Ironic and Inspector inventory formats
They provide the same information in different places.

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 | 5ad44b376d43759f05ff1410afbddaa57ff6826d | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1450/gomod/1725557480227868672) | true | `/test gomod`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1450). [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

Patch:
diff --git a/go.mod b/go.mod
index c4b8dfba..69f0b396 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ go 1.20
 require (
 	github.com/go-logr/logr v1.3.0
 	github.com/google/safetext v0.0.0-20230106111101-7156a760e523
-	github.com/gophercloud/gophercloud v1.5.1-0.20231106162611-af1813efe0d1
+	github.com/gophercloud/gophercloud v1.5.1-0.20231117122435-08456f7fe42e
 	github.com/metal3-io/baremetal-operator/apis v0.4.0
 	github.com/metal3-io/baremetal-operator/pkg/hardwareutils v0.4.0
 	github.com/onsi/gomega v1.30.0
@@ -62,7 +62,7 @@ require (
 	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.13.0 // indirect
+	golang.org/x/sys v0.14.0 // indirect
 	golang.org/x/term v0.13.0 // indirect
 	golang.org/x/text v0.13.0 // indirect
 	golang.org/x/time v0.3.0 // indirect
diff --git a/go.sum b/go.sum
index 6571f6ed..c470351e 100644
--- a/go.sum
+++ b/go.sum
@@ -73,8 +73,8 @@ github.com/google/safetext v0.0.0-20230106111101-7156a760e523 h1:i4NsbmB9pD5+Ggp
 github.com/google/safetext v0.0.0-20230106111101-7156a760e523/go.mod h1:mJNEy0r5YPHC7ChQffpOszlGB4L1iqjXWpIEKcFpr9s=
 github.com/google/uuid v1.4.0 h1:MtMxsa51/r9yyhkyLsVeVt0B+BGQZzpQiTQ4eHZ8bc4=
 github.com/google/uuid v1.4.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
-github.com/gophercloud/gophercloud v1.5.1-0.20231106162611-af1813efe0d1 h1:4crYiUFYzhxv60/bsLnyc7esUsOqQl3DgErAGNsOgL8=
-github.com/gophercloud/gophercloud v1.5.1-0.20231106162611-af1813efe0d1/go.mod h1:99P1GdytPKaSR7/ugQJo1dX6th5FlHdXXvKqvRJQfO4=
+github.com/gophercloud/gophercloud v1.5.1-0.20231117122435-08456f7fe42e h1:acHzJUXVfS+GImmi+yyM2+AOiaO94Z4KaTTFVU6Ov9U=
+github.com/gophercloud/gophercloud v1.5.1-0.20231117122435-08456f7fe42e/go.mod h1:ecnaHbFxBunc2UeAONDQbkQacv6WP1syeZ02TgAB7kA=
 github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU=
 github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
 github.com/inconshreveable/mousetrap v1.0.1 h1:U3uMjPSQEBMNp1lFxmllqCPM6P5u/Xq7Pgzkat/bFNc=
@@ -197,8 +197,8 @@ golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20210330210617-4fbd30eecc44/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.13.0 h1:Af8nKPmuFypiUBjVoU9V20FiaFXOcuZI21p0ycVYYGE=
-golang.org/x/sys v0.13.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.14.0 h1:Vz7Qs629MkJkGyHxUlRHizWJRG2j8fbQKjELVSNhy7Q=
+golang.org/x/sys v0.14.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.13.0 h1:bb+I9cTfFazGW51MZqBVmZy7+JEJMouUHTUSKVQLBek=
 golang.org/x/term v0.13.0/go.mod h1:LTmsnFJwVN6bCy1rVCoS+qHT1HhALEFxKncY3WNNh4U=
diff --git a/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go b/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go
index 6b480167..c1821977 100644
--- a/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go
+++ b/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go
@@ -16,7 +16,7 @@ import (
 
 // GetHardwareDetails converts Ironic introspection data into BareMetalHost HardwareDetails.
 func GetHardwareDetails(data *nodes.InventoryData, logger logr.Logger) *metal3api.HardwareDetails {
-	inspectorData, err := data.PluginData.AsInspectorData()
+	ironicData, inspectorData, err := data.PluginData.GuessFormat()
 	if err != nil {
 		logger.Error(err, "cannot get plugin data from inventory, some fields will not be available")
 	}
@@ -25,18 +25,18 @@ func GetHardwareDetails(data *nodes.InventoryData, logger logr.Logger) *metal3ap
 	details.Firmware = getFirmwareDetails(data.Inventory.SystemVendor.Firmware)
 	details.SystemVendor = getSystemVendorDetails(data.Inventory.SystemVendor)
 	details.RAMMebibytes = data.Inventory.Memory.PhysicalMb
-	details.NIC = getNICDetails(data.Inventory.Interfaces, inspectorData.AllInterfaces)
+	details.NIC = getNICDetails(data.Inventory.Interfaces, ironicData, inspectorData)
 	details.Storage = getStorageDetails(data.Inventory.Disks)
 	details.CPU = getCPUDetails(&data.Inventory.CPU)
 	details.Hostname = data.Inventory.Hostname
 	return details
 }
 
-func getVLANs(intf introspection.BaseInterfaceType) (vlans []metal3api.VLAN, vlanid metal3api.VLANID) {
-	if intf.LLDPProcessed == nil {
+func getVLANs(lldp map[string]interface{}) (vlans []metal3api.VLAN, vlanid metal3api.VLANID) {
+	if lldp == nil {
 		return
 	}
-	if spvs, ok := intf.LLDPProcessed["switch_port_vlans"]; ok {
+	if spvs, ok := lldp["switch_port_vlans"]; ok {
 		if data, ok := spvs.([]map[string]interface{}); ok {
 			vlans = make([]metal3api.VLAN, len(data))
 			for i, vlan := range data {
@@ -49,18 +49,28 @@ func getVLANs(intf introspection.BaseInterfaceType) (vlans []metal3api.VLAN, vla
 			}
 		}
 	}
-	if vid, ok := intf.LLDPProcessed["switch_port_untagged_vlan_id"].(int); ok {
+	if vid, ok := lldp["switch_port_untagged_vlan_id"].(int); ok {
 		vlanid = metal3api.VLANID(vid)
 	}
 	return
 }
 
 func getNICDetails(ifdata []inventory.InterfaceType,
-	basedata map[string]introspection.BaseInterfaceType) []metal3api.NIC {
+	ironicData *inventory.StandardPluginData,
+	inspectorData *introspection.Data) []metal3api.NIC {
 	var nics []metal3api.NIC
 	for _, intf := range ifdata {
-		baseIntf := basedata[intf.Name]
-		vlans, vlanid := getVLANs(baseIntf)
+		var lldp map[string]interface{}
+		var pxeEnabled bool
+		if ironicData != nil {
+			pxeEnabled = ironicData.AllInterfaces[intf.Name].PXEEnabled
+			lldp = ironicData.ParsedLLDP[intf.Name]
+		} else {
+			pxeEnabled = inspectorData.AllInterfaces[intf.Name].PXE
+			lldp = inspectorData.AllInterfaces[intf.Name].LLDPProcessed
+		}
+
+		vlans, vlanid := getVLANs(lldp)
 		// We still store one nic even if both ips are unset
 		// if both are set, we store two nics with each ip
 		if intf.IPV4Address != "" || intf.IPV6Address == "" {
@@ -73,7 +83,7 @@ func getNICDetails(ifdata []inventory.InterfaceType,
 				VLANs:     vlans,
 				VLANID:    vlanid,
 				SpeedGbps: intf.SpeedMbps / 1000,
-				PXE:       baseIntf.PXE,
+				PXE:       pxeEnabled,
 			})
 		}
 		if intf.IPV6Address != "" {
@@ -86,7 +96,7 @@ func getNICDetails(ifdata []inventory.InterfaceType,
 				VLANs:     vlans,
 				VLANID:    vlanid,
 				SpeedGbps: intf.SpeedMbps / 1000,
-				PXE:       baseIntf.PXE,
+				PXE:       pxeEnabled,
 			})
 		}
 	}


Test patch:
diff --git a/pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go b/pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go
index a3c68b64..c77eb035 100644
--- a/pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go
+++ b/pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go
@@ -6,25 +6,24 @@ import (
 
 	"github.com/gophercloud/gophercloud/openstack/baremetal/inventory"
 	"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
+	"github.com/stretchr/testify/assert"
 
 	metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func TestGetVLANs(t *testing.T) {
-	vlans, vid := getVLANs(introspection.BaseInterfaceType{
-		LLDPProcessed: map[string]interface{}{
-			"switch_port_vlans": []map[string]interface{}{
-				{
-					"id":   1,
-					"name": "vlan1",
-				},
-				{
-					"id":   4094,
-					"name": "vlan4094",
-				},
+	vlans, vid := getVLANs(map[string]interface{}{
+		"switch_port_vlans": []map[string]interface{}{
+			{
+				"id":   1,
+				"name": "vlan1",
+			},
+			{
+				"id":   4094,
+				"name": "vlan4094",
 			},
-			"switch_port_untagged_vlan_id": 1,
 		},
+		"switch_port_untagged_vlan_id": 1,
 	})
 	if vid != 1 {
 		t.Errorf("Unexpected untagged VLAN ID %d", vid)
@@ -41,31 +40,29 @@ func TestGetVLANs(t *testing.T) {
 }
 
 func TestGetVLANsMalformed(t *testing.T) {
-	vlans, vid := getVLANs(introspection.BaseInterfaceType{
-		LLDPProcessed: map[string]interface{}{
-			"switch_port_vlans": []map[string]interface{}{
-				{
-					"foo":  "bar",
-					"name": "vlan1",
-				},
-				{
-					"foo": "bar",
-					"id":  1,
-				},
-				{
-					"name": "vlan2",
-					"id":   "2",
-				},
-				{
-					"name": 3,
-					"id":   3,
-				},
-				{
-					"foo": "bar",
-				},
+	vlans, vid := getVLANs(map[string]interface{}{
+		"switch_port_vlans": []map[string]interface{}{
+			{
+				"foo":  "bar",
+				"name": "vlan1",
+			},
+			{
+				"foo": "bar",
+				"id":  1,
+			},
+			{
+				"name": "vlan2",
+				"id":   "2",
+			},
+			{
+				"name": 3,
+				"id":   3,
+			},
+			{
+				"foo": "bar",
 			},
-			"switch_port_untagged_vlan_id": "1",
 		},
+		"switch_port_untagged_vlan_id": "1",
 	})
 	if vid != 0 {
 		t.Errorf("Unexpected untagged VLAN ID %d", vid)
@@ -89,27 +86,11 @@ func TestGetVLANsMalformed(t *testing.T) {
 		t.Errorf("Unexpected VLAN %d %s", vlans[0].ID, vlans[0].Name)
 	}
 
-	vlans, vid = getVLANs(introspection.BaseInterfaceType{
-		LLDPProcessed: map[string]interface{}{
-			"switch_port_vlans": map[string]interface{}{
-				"foo": "bar",
-			},
-			"switch_port_untagged_vlan_id": "1",
-		},
-	})
-	if vid != 0 {
-		t.Errorf("Unexpected untagged VLAN ID %d", vid)
-	}
-	if len(vlans) != 0 {
-		t.Errorf("Expected 0 VLANs, got %d", len(vlans))
-	}
-
-	vlans, vid = getVLANs(introspection.BaseInterfaceType{
-		LLDPProcessed: map[string]interface{}{
-			"switch_port_vlans": []interface{}{
-				"foo",
-			},
+	vlans, vid = getVLANs(map[string]interface{}{
+		"switch_port_vlans": map[string]interface{}{
+			"foo": "bar",
 		},
+		"switch_port_untagged_vlan_id": "1",
 	})
 	if vid != 0 {
 		t.Errorf("Unexpected untagged VLAN ID %d", vid)
@@ -118,9 +99,9 @@ func TestGetVLANsMalformed(t *testing.T) {
 		t.Errorf("Expected 0 VLANs, got %d", len(vlans))
 	}
 
-	vlans, vid = getVLANs(introspection.BaseInterfaceType{
-		LLDPProcessed: map[string]interface{}{
-			"switch_port_vlans": "foo",
+	vlans, vid = getVLANs(map[string]interface{}{
+		"switch_port_vlans": []interface{}{
+			"foo",
 		},
 	})
 	if vid != 0 {
@@ -130,8 +111,8 @@ func TestGetVLANsMalformed(t *testing.T) {
 		t.Errorf("Expected 0 VLANs, got %d", len(vlans))
 	}
 
-	vlans, vid = getVLANs(introspection.BaseInterfaceType{
-		LLDPProcessed: map[string]interface{}{},
+	vlans, vid = getVLANs(map[string]interface{}{
+		"switch_port_vlans": "foo",
 	})
 	if vid != 0 {
 		t.Errorf("Unexpected untagged VLAN ID %d", vid)
@@ -140,7 +121,7 @@ func TestGetVLANsMalformed(t *testing.T) {
 		t.Errorf("Expected 0 VLANs, got %d", len(vlans))
 	}
 
-	vlans, vid = getVLANs(introspection.BaseInterfaceType{})
+	vlans, vid = getVLANs(map[string]interface{}{})
 	if vid != 0 {
 		t.Errorf("Unexpected untagged VLAN ID %d", vid)
 	}
@@ -149,28 +130,26 @@ func TestGetVLANsMalformed(t *testing.T) {
 	}
 }
 
-func TestGetNICDetails(t *testing.T) {
-	nics := getNICDetails(
-		[]inventory.InterfaceType{
-			{
-				Name:        "eth0",
-				IPV4Address: "192.0.2.1",
-				MACAddress:  "00:11:22:33:44:55"},
-			{
-				Name:        "eth1",
-				IPV6Address: "2001:db8::1",
-				MACAddress:  "66:77:88:99:aa:bb",
-				SpeedMbps:   1000},
-			{
-				Name:        "eth46",
-				IPV6Address: "2001:db8::2",
-				IPV4Address: "192.0.2.2",
-				MACAddress:  "00:11:22:33:44:66"},
-			{
-				Name:       "ethNoIp",
-				MACAddress: "00:11:22:33:44:77"},
+func TestGetNICDetailsInspector(t *testing.T) {
+	ironicData := inventory.StandardPluginData{
+		AllInterfaces: map[string]inventory.ProcessedInterfaceType{
+			"eth0": {
+				PXEEnabled: true,
+			},
 		},
-		map[string]introspection.BaseInterfaceType{
+		ParsedLLDP: map[string]inventory.ParsedLLDP{
+			"eth0": {
+				"switch_port_vlans": []map[string]interface{}{
+					{
+						"id": 1,
+					},
+				},
+				"switch_port_untagged_vlan_id": 1,
+			},
+		},
+	}
+	inspectorData := introspection.Data{
+		AllInterfaces: map[string]introspection.BaseInterfaceType{
 			"eth0": {
 				PXE: true,
 				LLDPProcessed: map[string]interface{}{
@@ -182,51 +161,91 @@ func TestGetNICDetails(t *testing.T) {
 					"switch_port_untagged_vlan_id": 1,
 				},
 			},
-		})
+		},
+	}
+	interfaces := []inventory.InterfaceType{
+		{
+			Name:        "eth0",
+			IPV4Address: "192.0.2.1",
+			MACAddress:  "00:11:22:33:44:55"},
+		{
+			Name:        "eth1",
+			IPV6Address: "2001:db8::1",
+			MACAddress:  "66:77:88:99:aa:bb",
+			SpeedMbps:   1000},
+		{
+			Name:        "eth46",
+			IPV6Address: "2001:db8::2",
+			IPV4Address: "192.0.2.2",
+			MACAddress:  "00:11:22:33:44:66"},
+		{
+			Name:       "ethNoIp",
+			MACAddress: "00:11:22:33:44:77"},
+	}
 
-	// 5 expected because eth46 results in two items
-	if len(nics) != 5 {
-		t.Errorf("Expected 5 NICs, got %d", len(nics))
-	}
-	if (!reflect.DeepEqual(nics[0], metal3api.NIC{
-		Name: "eth0",
-		MAC:  "00:11:22:33:44:55",
-		IP:   "192.0.2.1",
-		PXE:  true,
-		VLANs: []metal3api.VLAN{
-			{ID: 1},
+	cases := []struct {
+		name          string
+		ironicData    *inventory.StandardPluginData
+		inspectorData *introspection.Data
+	}{
+		{
+			name:          "with-ironic",
+			ironicData:    &ironicData,
+			inspectorData: nil,
+		},
+		{
+			name:          "with-inspector",
+			ironicData:    nil,
+			inspectorData: &inspectorData,
 		},
-		VLANID: 1,
-	})) {
-		t.Errorf("Unexpected NIC data")
-	}
-	if (!reflect.DeepEqual(nics[1], metal3api.NIC{
-		Name:      "eth1",
-		MAC:       "66:77:88:99:aa:bb",
-		IP:        "2001:db8::1",
-		SpeedGbps: 1,
-	})) {
-		t.Errorf("Unexpected NIC data")
-	}
-	if (!reflect.DeepEqual(nics[2], metal3api.NIC{
-		Name: "eth46",
-		MAC:  "00:11:22:33:44:66",
-		IP:   "192.0.2.2",
-	})) {
-		t.Errorf("Unexpected NIC data")
-	}
-	if (!reflect.DeepEqual(nics[3], metal3api.NIC{
-		Name: "eth46",
-		MAC:  "00:11:22:33:44:66",
-		IP:   "2001:db8::2",
-	})) {
-		t.Errorf("Unexpected NIC data")
-	}
-	if (!reflect.DeepEqual(nics[4], metal3api.NIC{
-		Name: "ethNoIp",
-		MAC:  "00:11:22:33:44:77",
-	})) {
-		t.Errorf("Unexpected NIC data")
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			nics := getNICDetails(interfaces, tc.ironicData, tc.inspectorData)
+
+			// 5 expected because eth46 results in two items
+			assert.Len(t, nics, 5)
+			if (!reflect.DeepEqual(nics[0], metal3api.NIC{
+				Name: "eth0",
+				MAC:  "00:11:22:33:44:55",
+				IP:   "192.0.2.1",
+				PXE:  true,
+				VLANs: []metal3api.VLAN{
+					{ID: 1},
+				},
+				VLANID: 1,
+			})) {
+				t.Errorf("Unexpected NIC data")
+			}
+			if (!reflect.DeepEqual(nics[1], metal3api.NIC{
+				Name:      "eth1",
+				MAC:       "66:77:88:99:aa:bb",
+				IP:        "2001:db8::1",
+				SpeedGbps: 1,
+			})) {
+				t.Errorf("Unexpected NIC data")
+			}
+			if (!reflect.DeepEqual(nics[2], metal3api.NIC{
+				Name: "eth46",
+				MAC:  "00:11:22:33:44:66",
+				IP:   "192.0.2.2",
+			})) {
+				t.Errorf("Unexpected NIC data")
+			}
+			if (!reflect.DeepEqual(nics[3], metal3api.NIC{
+				Name: "eth46",
+				MAC:  "00:11:22:33:44:66",
+				IP:   "2001:db8::2",
+			})) {
+				t.Errorf("Unexpected NIC data")
+			}
+			if (!reflect.DeepEqual(nics[4], metal3api.NIC{
+				Name: "ethNoIp",
+				MAC:  "00:11:22:33:44:77",
+			})) {
+				t.Errorf("Unexpected NIC data")
+			}
+		})
 	}
 }