fix: restrict get_or_create to get_or_create_on_node - #462
AloizioMacedo wants to merge 5 commits into
Conversation
The function was doing some duplicated work with get_or_create_on_node precisely for the case of a mac address being on the wrong node, which is very likely related to LP:2147706
|
Check where you would like a Mattermost message to be sent to when CI completes and this PR is merged
|
|
I put this in draft since I was not able to reproduce the issue yet. Will try a couple times, but the changes seem to be meaningful regardless. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[maas-code-reviewer review]
LLM-generated review from https://github.com/canonical/maas-code-reviewer.
Intended to assist a human reviewer, not replace one — suggestions may be
incorrect, please verify before acting.
Overall, this is a clean approach to fix the bug where multiple objects are returned by get_or_create. Shifting the creation and moving logic to get_or_create_on_node removes duplication and keeps the interface logic central. However, I noticed a critical issue regarding how updates are saved to the database. Because of the removal of the explicit update_fields logic for name and node_config in update_physical_interface, modifications made to existing interfaces inside get_or_create_on_node are no longer saved to the database. Additionally, please ensure that any tests calling or mocking get_or_create_on_node are updated to expect a tuple.
The same interface is moved into the new node, instead of being deleted and recreated.
| node.is_commissioning() or (node.enable_hw_sync and port is not None) | ||
| ) | ||
|
|
||
| interface, created = PhysicalInterface.objects.get_or_create_on_node( |
There was a problem hiding this comment.
maybe add a regression test which sets up that duplicate state and runs the commissioning network update.
3 participants
LP:2147706 reports that
get_or_createis erroring with multiple entries after commissioning an initial set of servers. The only way that this seems possible is if there are multiple interfaces with the same mac address, likely on different nodes since the issue happens after creating a first set of nodes and not on a clean environment.The code in
update_physical_interfacedoes not account for this, but accounts for the possibility of the interface being in the incorrect node and does some cleanup related to this. This cleanup has duplicated logic inget_or_create_on_node, so we just use that instead (which should avoid the multiple entries issues due to the restriction on the node).I took the liberty of also returning the information of whether the interface was created from
get_or_create_on_node, to mirror whatget_or_createdoes instead of having to do extra queries to check if something was added.Resolves: LP:2147706