feat: configure ironic inspection notifications for nautobot integration

[haseebsyed12]

Automatic synchronization of baremetal server hardware inventory from OpenStack Ironic to Nautobot when servers complete the inspection phase during enrollment.
By leveraging Ironic's existing hardware inspection process, Oslo event notifications, Argo Events and Argo Workflows, we can automatically populate Nautobot.

[stevekeay]

I left a partial review - I think this has borrowed some parts of the enrol process such as validations that are not really appropriate to perform here, since the node has already enrolled and it's too late to do anything about it.

I think this code should concentrate on doing one thing well which is too keep nautobot in sync as well as it can, and in a timely manner.

Validation and reporting probably belong elsewhere. That said, there is nothing wrong with noticing that there is a problem and logging it, or even updating nautobot to reflect the error state. However we should really have a use-case for doing this and I would prefer to see us use the openstack API to report/audit issues.

[stevekeay]

Ironic has the switch hostname in baremetal port local_link attribute. Should we be using that? It is a real pain to gather the switch MAC address and populate these into nautobot in the first place, and we only ever did that to work around iDRAC's partial LLDP information. If we are using agent-based inspection then we have the full LLDP data available, including the hostname.

[stevekeay]

This is correct today, but I hard a rumor that maybe a server could be connected to switches in a neighboring rack, therefore a server could (in future) legitimately be connected to switches in two different racks.

[stevekeay]

If Ironic was able to Inspect the server, I would say it has gotten past the planning phase. Can we check the reasoning for this Status value?

[stevekeay]

I understand you are just documenting existing behaviour, and you didn't decide on these values here. But these assumptions that are not true for all our existing hardware. If we don't know the exact interface type it might be better to set this to something generic, or if that is not available, then something obviously incorrect like like "10baseT".

[stevekeay]

I thought it was getting this information from Ironic?

[stevekeay]

Who specified this behaviour? Shouldn't nautobot be updated with whatever ironic says? If we want to report on bad cabling then nautobot might be the way that a DC tech "views" the problem.

[stevekeay]

Is this the desired behaviour? Shouldn't nautobot be updated with whatever ironic says?

[stevekeay]

If the IP is already listed in ironic then the duplicate has NOT been prevented.

The system can detect the duplicate but what does it DO with this information?

If bNautobot doesn't allow duplicate IPs, we just log the error and fail to create the IPAddress in nautobot. But then we have not documented this IP, so perhaps it should delete the conflicting IP address assignment and replace it with the new information?

[stevekeay]

Again what does it do if it finds something is invalid?

[stevekeay]

So the behaviour if we have an incomplete topology is to simply not bother updating nautobot? Wouldn't it be better to log the situation and continue, or maybe even set some kind of flag in nautobot like "NODE_WITH_INADEQUATE_NETWORK_TOPOLOGY"

[stevekeay]

Also, this duplicates some of the functionality in #1347 where we do the LLDP parsing.

[cardoe]

+1 from me but I'll defer to @stevekeay to approve since he had the most review feedback.

I'm good with these changes now.

[cardoe]

@haseebsyed12 you'll want to clean up your commit messages.

[cardoe]

You also need to rebase to not fail on the python dependencies change.

[cardoe]

Looks okay to me @stevekeay I'll defer to you to approve since you have a few outstanding review items.

[stevekeay]

Ironic has a built-in post-inspection hook that augments the node inventory with parsed LLDP data.

I was able to enable this in undercloud-deploy/.../helm-configs/ironic.yaml:

    inspector:
      ...
      hooks: "$default_hooks,parse-lldp,resource-class,update-baremetal-port"

I tried it with agent inspection, where it added the following key to the inventory. With redfish inspection we don't get any LLDP data, but Nidhi was going to synthesise some based on what we fetch from redfish, not sure yet how all of this is going to hang together:

  "plugin_data": {
    ...
    "parsed_lldp": {
      "eno3np0": {
        "switch_chassis_id": "c4:7e:e0:e4:55:3f",
        "switch_port_id": "Ethernet1/1",
        "switch_port_description": "Dell-93GSW04_NIC.Integrated.1-1",
        "switch_system_name": "f20-3-1.iad3",
        "switch_port_mtu": 9000,
        "switch_port_vlans": [
          {
            "name": "4010_provisioning",
            "id": 4010
          }
        ],
        "switch_port_link_aggregation_enabled": false,
        "switch_port_link_aggregation_support": true,
        "switch_port_link_aggregation_id": 0
      },

Note that we should only rely on switch_chassis_id, switch_system_name and switch_port_id. Per rackspace security standard, all the other fields ought to be disabled on the switch and are therefore likely to disappear at some point.

[stevekeay]

What am I missing here? It looks like class Inventory does not actually do anything.

Suggested change

	class Inventory:
	"""Data source that provides chassis information from Ironic inspection data."""
	def __init__(self, inspection_data: dict):
	self.inspection_data = inspection_data
	self._chassis_info: ChassisInfo | None = None
	def get_chassis_info(self) -> ChassisInfo:
	if self._chassis_info is None:
	self._chassis_info = chassis_info_from_ironic_data(self.inspection_data)
	return self._chassis_info
	@property
	def ip_address(self) -> str:
	return self.inspection_data["inventory"]["bmc_address"]
	@property
	def hostname(self) -> str:
	return self.inspection_data["inventory"]["hostname"]
	def get_device_info(inspection_data: dict) -> ChassisInfo:
	try:
	data_source = Inventory(inspection_data)
	chassis_info = data_source.get_chassis_info()
	def get_device_info(inspection_data: dict) -> ChassisInfo:
	try:
	chassis_info = chassis_info_from_ironic_data(inspection_data)

[haseebsyed12]

I've removed the Inventory class and simplified the get_device_info function to directly call chassis_info_from_ironic_data()

[stevekeay]

Should this message say enroll? This is not nessecarily happening at enrol time, is it?

[stevekeay]

Suggested change

	device_info: ChassisInfo = get_device_info(node_inventory)
	device_info = get_device_info(node_inventory)

[stevekeay]

These interface names are specific to Dell servers and might be misleading if e.g. a HP server also has an eno8*03 or an eno*p*. I feel like this ought to return linux_interface_name if we're not on a dell server.

[stevekeay]

I'd be tempted to just write this out the dumb way:

INTERFACE_NAME_MAPPING = {
  "eno8303": "NIC.Embedded.1-1-1",
  "eno8403": "NIC.Embedded.2-1-1",
  "ens2f0np0": "NIC.Slot.1-1",
  ...etc...
}

INTERFACE_NAME_MAPPING.get(linux_interface_name, linux_interface_name)

[stevekeay]

It seems like this duplicates some of understack/python/understack-workflows/understack_workflows/oslo_event/ironic_port.py - how should we address that?

[cardoe]

So looking at https://docs.openstack.org/ironic/latest/admin/notifications.html wouldn't we really just want to trigger on:

• baremetal.node.update.end and possibly baremetal.node.create.end though I suspect the node will not be created with enough info in it and we'd either create or update the node inside of Nautobot
• baremetal.node.delete.end we would remove the node from Nautobot
• baremetal.port.update.end and possibly bare metal.port.create.end would create/update the port in Nautobot
• baremetal.port.delete.end would clean up said port
• baremetal.portgroup.update.end and baremetal.portgroup.create.end same way
• baremetal.portgroup.delete.end same