ports API allows node IDs in addition to node UUIDs

1:07 PM <jroll> BadCub: lol, I picked that up at the end of kilo and never finished it, I know Haomeng was working on the same thing but maybe fined a different bug?
1:09 PM <BadCub> Haomeng: are you working on something similar to/or dup of ^^^
1:10 PM <BadCub> thanks jroll :-) I will wait to hear from Haomeng on that one

I'll take a look at this if HaoMeng and jroll haven't patched yet.

That comment looks like it's on the wrong bug, that was in the context of an i18n thing.

I'm looking at one test case in-depth to see how the tests rely on this. Multiple test cases are failing with the same error message when I change Port._set_node_uuid to only allow the passed value to be a node UUID (changing objects.Node.get to objects.Node.get_by_uuid within the function).

The test case I'm examining is test_get_one in tests/api/v1/test_ports.

The part of the test that's failing with this change is the GET request to retrieve the port. It looks like the port retrieved via RPC in get_one in the ports API controller doesn't have a _node_uuid attached but does have a node ID attached.

Like your stack trace shows, having the node ID in the RPC (or anything that passes a node id to the Port constructor in the ports API controller) causes a NodeNotFound error. convert_with_links creates a new Port from the RPC port with the node ID still attached, causing the error.

I think the best next step is to examine what the RPC call to retrieve a port returns in a successful invocation of get_one with the small change described above. This will show us what the RPC port *should* look like and isolate what the tests are doing differently that causes them to fail. If there are no possible successful invocations of get_one with the change described above, I might have to reconsider some assumptions about what _set_node_uuid expects and guarantees.

It looks like the actual API may rely on this, too - not just the tests. Changing this to Node.get_by_uuid makes devstack fail to boot correctly: https://github.com/openstack/ironic/blob/master/ironic/api/controllers/v1/port.py#L60

I'm going to add some logging / use pdb and see what happens when I use the original code to perform port API operations with the node ID. I'll also try seeing if the same thing is happening as in the tests when switching to get_by_uuid.

The logging didn't offer that much insight. The only consistent thing that I was able to glean was that on devstack startup the Port constructor is called and tries to set the node UUID to a real node UUID first, then it gets called again down the stack with the internal node ID. I'm not sure why there are two calls being made.

I attached the logs created by a test run with the logs defined in this commit: https://github.com/supermari0/ironic/blob/2b2bd8999daac64b476fa4c6f54f5a0d727e9026/ironic/api/controllers/v1/port.py

I eventually figured out that when any function needs to call the convert_with_links function on a port, the node UUID isn't present in the RPC node, since the node UUID isn't retained in the port from the DB. This is what prevents _set_node_uuid from calling objects.Node.get_by_uuid instead of objects.Node.get since the constructor attempts to set node_uuid to node_id. The setter eventually figures out what the correct node UUID should be after retrieving the node, and that's why calling the setter with the node ID doesn't cause failures.

I tried to retrieve the Node by its ID in convert_with_links and set the node UUID on the port before it's passed to the constructor, but tests were still failing when replacing objects.Node.get with objects.Node.get_by_uuid. The commit with that change is here: https://github.com/supermari0/ironic/commit/499e20e5270430a5d5765297a85f5709cc9f788b

I'm going to take a break from this bug to work on other things, but feel free to reassign if someone else wants to take it.

Fix proposed to branch: master
Review: https://review.openstack.org/403910

Given a patch has already been proposed that is just in merge conflict, it seems like someone could pick this up and move it forward as a low hanging fruit item. The contributor would just need to rebase and continue change 403910.