feat: adding nautobot-sync post-inspection hook #1406
Conversation
haseebsyed12
force-pushed
the
nautobot-device-hook
branch
3 times, most recently
from
20b7bdd to
10aa4bb
Compare
haseebsyed12
force-pushed
the
nautobot-device-hook
branch
5 times, most recently
from
87c0cfb to
2ec6720
Compare
haseebsyed12
force-pushed
the
nautobot-device-hook
branch
from
2ec6720 to
57dd66c
Compare
stevekeay
left a comment
stevekeay
left a comment
There was a problem hiding this comment.
My concern here is that we are doing this work inside Ironic. This raises several questions:
- A transient failure means the data won't get synced - there is no good way to retry this operation other than re-inspecting the node
- If a port is created or updated by some means other than inspection, it won't get synced unless/until an inspection happens
- The node has to exist in nautobot before this will work. Of course inspection is slow enough that in the normal case there is plenty of time to create the node, but because creating a node uses a different mechanism, it's possible for it to be delayed somehow, and we try to create the ports before the node.
- This implementation is tied to the "internal" ironic model / api. If we were using the REST API to fetch the data from Ironic then (in theory at least) the code will be less susceptible to breakage as new versions of openstack are released
- Do we want nautobot as a dependency for our ironic build? (Code we run there has uncontrolled write access to the database. OpenStack is so huge that this hardly matters, but the guiding principle is to keep these surfaces small)
- If this operation is slow (e.g. timeout connecting to nautobot) it can hold up ironic's workers. I honestly don't know if this is a concern or not
None of the above is a problem and I don't mind either way, but if it is easy to use the asynchronous event streaming mechanism then I feel like that is a more sound architecture. The producer-consumer pattern gracefully handles retries and ordering of events, de-couples ironic from nautobot, both in terms of code and operationally, and integrates with the two systems via stable, published, documented APIs.
| "S105", # allow hardcoded passwords in tests | ||
| "S106", # allow hardcoded passwords in function arguments in tests |
There was a problem hiding this comment.
I would prefer to add these in-line, because if the offending code is changed it is very difficult to remember to come back and clean up. Also it targets a specific line, keeping the guard rails active for the rest of the file in case someone accidentally adds a real password later.
test_creds = "testpass" # noqa: S105
| interface.update( | ||
| mac_address=mac_address, | ||
| status="Active", | ||
| type="25gbase-x-sfp28", # Default type, could be made configurable |
There was a problem hiding this comment.
We don't know what the interface type is. Should we be setting this? Perhaps we should set it to some special value like "unknown", or something that is obviously incorrect like "10baseT".
| status="Active", | ||
| type="25gbase-x-sfp28", |
There was a problem hiding this comment.
repeated above, can we simplify?
| ) | ||
| # Update interface attributes | ||
| interface.update( | ||
| mac_address=mac_address, |
There was a problem hiding this comment.
Would we not update the interface's name too?
| LOG.debug("Missing switch connection data for interface") | ||
| return | ||
|
|
||
| # Find the switch device by chassis MAC address |
There was a problem hiding this comment.
It may be preferable to look up the switch by its hostname instead.
When a switch is replaced, the MAC address will change.
| return cable | ||
|
|
||
| # Create new cable | ||
| cable = nautobot.dcim.cables.create( |
There was a problem hiding this comment.
I think this will fail if one of the ports is already connected. We might want to cuckoo this case (delete the competing cable) because otherwise I think that would require manual intervention to straighten things out.
| "name": task.node.name, | ||
| "properties": task.node.properties, | ||
| "driver_info": task.node.driver_info, |
There was a problem hiding this comment.
I don't see these being used anywhere. What am I missing?
No description provided.