| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-31 | |||
| 15:08:23 | mriedem | and can't delete the existing one because the existing one has allocations already? | |
| 15:08:48 | belmoreira | mriedem: yes | |
| 15:08:54 | belmoreira | the cleanest way to recover would be to delete everything related to ironic resources providers and recreate again | |
| 15:09:06 | jaypipes | sahid, stephenfin: at least in so much as the NUMA topology stuff wasn't really germane to the "request different providers supply some X amount of dedicated or shared CPU resources for the guest" | |
| 15:09:25 | belmoreira | unfortunately I have ~2000 nodes in this state | |
| 15:09:28 | mriedem | belmoreira: yeah i think what i'd probably do is pick one resource provider / allocation combo since they should be 1:1 with ironic, | |
| 15:09:40 | mriedem | clean it up using the osc-placement CLI | |
| 15:09:57 | mriedem | and then run heal_allocations but probably want to put a cap on the --max-count | |
| 15:09:59 | stephenfin | jaypipes: ack. In a meeting/working on NUMA-aware vswitches, but I'll take a look before EOD | |
| 15:10:12 | mriedem | belmoreira: this is why i had a note in the code about allowing passing in a single instance id to heal | |
| 15:10:17 | mriedem | that would be ideal in this case as a test | |
| 15:10:38 | jaypipes | stephenfin: no worries, you already reviewed again (and I will update to your comments) | |
| 15:10:46 | belmoreira | mriedem I will first test all on copies of the DBs | |
| 15:11:04 | mriedem | belmoreira: ok yeah definitely doing it in a test env would be good for both of us :) | |
| 15:12:26 | belmoreira | other use-case for this code is updating ironic allocations that were created before a resource_class was defined | |
| 15:13:38 | mriedem | so the existing allocations for the ironic instance are against vcpu/memory_mb/disk_gb and you want to change then to be against custom_baremetal_belmiro, yes? | |
| 15:13:46 | mriedem | *them | |
| 15:14:31 | belmoreira | mriedem: but the recreation of the compute_node in nova is something that needs to be reviewed; has been creating few issues. Don't know yet why this happened... | |
| 15:14:35 | mriedem | that's probably where the --force option mentioned in the comments would come in, since currently the command will skip any instances that already have allocations, not attempt to overwrite them | |
| 15:15:27 | mriedem | belmoreira: so i thought that sounded very much like https://review.openstack.org/#/c/508555/ | |
| 15:15:27 | belmoreira | mriedem:yes, that's way I was pointing this as another use-case | |
| 15:16:53 | belmoreira | mriedem: this was not rebalance... we had an operator working in ironic/nova-compute and know is very hard to track why this happened | |
| 15:18:07 | belmoreira | mriedem: but anyway it looks a very dangerous operation to do when having placement. Before placement this was OK (recreating a new compute_node) | |
| 15:18:13 | mriedem | was the related nova-compute service deleted by any chance? | |
| 15:18:37 | belmoreira | mriedem: no, I don't yet | |
| 15:18:40 | mriedem | ok | |
| 15:18:53 | mriedem | for ironic the compute node record should have a predictable uuid | |
| 15:19:21 | mriedem | unless... | |
| 15:19:33 | mriedem | we don't use the ironic node uuid when creating the compute node | |
| 15:20:37 | stephenfin | mriedem, dansmith: We've been talking about extending the limits dict but I just noticed this https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1968-L1991 | |
| 15:20:39 | mriedem | looks like we don't https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L580 | |
| 15:21:10 | stephenfin | mriedem, dansmith: Given that we've essentially deprecated 'limits' and have 'requested_networks' available to us, couldn't I just use that as is? | |
| 15:21:14 | jaypipes | stephenfin: who's been talking about extending the limits dict? | |
| 15:21:16 | belmoreira | mriedem: that's the problem. When the compute_node is recreated we get a new uuid | |
| 15:21:18 | mriedem | belmoreira: so i think in that code, resources['hypervisor_hostname'] for ironic is always the node uuid, | |
| 15:21:26 | stephenfin | jaypipes: not me | |
| 15:21:27 | mriedem | we should use that when creating the compute node record if we think it's new | |
| 15:21:35 | mriedem | belmoreira: yup, seems like a trivial fix | |
| 15:21:49 | mriedem | belmoreira: not sure if you have enough words to open a bug? | |
| 15:22:39 | mriedem | stephenfin: my guess is requested_networks aren't passed back to the cell conductor during a reschedule | |
| 15:22:55 | mriedem | oh it looks like they are https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1890 | |
| 15:22:56 | belmoreira | This is related with https://review.openstack.org/#/c/508555/ | |
| 15:23:16 | belmoreira | sorry not this one... | |
| 15:23:34 | stephenfin | mriedem: Yeah, I checked - the info is there | |
| 15:24:16 | stephenfin | For migrate and unshelve, I would need to check 'instance.info_cache.network_info' but that's a-ok | |
| 15:25:03 | kashyap | stephenfin: Thanks for the review here, https://review.openstack.org/#/c/567258/. Yep, that could go... | |
| 15:25:32 | kashyap | I'll respin. | |
| 15:26:37 | mriedem | belmoreira: hmm, this should discover if there is already a compute node in the db with the same hypervisor_hostname, which is the ironic node uuid https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L518 | |
| 15:26:44 | belmoreira | mriedem: https://bugs.launchpad.net/nova/+bug/1771806 (the main issue here is the new UUID that's generated. Don't think is required to have a different bug) | |
| 15:26:45 | openstack | Launchpad bug 1771806 in OpenStack Compute (nova) "Ironic nova-compute failover creates new resource provider removing the resource_provider_aggregates link " [Medium,Confirmed] - Assigned to Surya Seetharaman (tssurya) | |
| 15:26:50 | mriedem | so it might not be a rebalance, but that code should have found it | |
| 15:26:50 | openstackgerrit | Merged openstack/nova stable/ocata: Fix shelving a paused instance https://review.openstack.org/569655 | |
| 15:27:29 | mriedem | unless you're hitting https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L531 | |
| 15:27:50 | mriedem | belmoreira: yeah, and in https://bugs.launchpad.net/nova/+bug/1771806 i asked why https://review.openstack.org/#/c/508555/ doesn't fix the issue, and i'm still kind of wondering that | |
| 15:27:51 | openstack | Launchpad bug 1771806 in OpenStack Compute (nova) "Ironic nova-compute failover creates new resource provider removing the resource_provider_aggregates link " [Medium,Confirmed] - Assigned to Surya Seetharaman (tssurya) | |
| 15:28:02 | mriedem | are you seeing the error from this condition? https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L531 | |
| 15:28:44 | mriedem | jroll: oath is cells v2 now right? | |
| 15:28:47 | mriedem | + placement | |
| 15:28:56 | mriedem | has to be if ocata | |
| 15:29:13 | jroll | mriedem: yeah | |
| 15:29:15 | jroll | single cell | |
| 15:29:25 | mriedem | is that cell VMs or BMs? | |
| 15:29:38 | mriedem | just wondering if you've seen similar issues to what belmiro is describing above | |
| 15:29:38 | jroll | VM | |
| 15:29:41 | mriedem | oh | |
| 15:29:44 | jroll | still working on the baremetal upgrade | |
| 15:29:47 | mriedem | ok | |
| 15:30:00 | jroll | about to jump in a meeting but will read back shortly | |
| 15:31:01 | belmoreira | mriedem: I didn't hit that because in that case doesn't rebalance | |
| 15:31:40 | mriedem | belmoreira: sure but you don't need to rebalance to hit this check | |
| 15:31:52 | mriedem | the code that creates the new compute node record with the new uuid is here https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L580 | |
| 15:32:02 | mriedem | and right before that, we check the rebalance thing https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L574 | |
| 15:32:25 | mriedem | which will query the db for existing compute node recoreds by nodename (ironic node uuid) https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L518 | |
| 15:32:34 | jroll | mriedem: belmoreira: at a skim, this looks like https://bugs.launchpad.net/nova/+bug/1750450 | |
| 15:32:35 | mriedem | so unless the node uuid changed, we should find the existing compute node reord | |
| 15:32:35 | openstack | Launchpad bug 1750450 in OpenStack Compute (nova) "ironic: n-cpu fails to recover after losing connection to ironic-api and placement-api" [Low,Fix released] - Assigned to Jim Rollenhagen (jim-rollenhagen) | |
| 15:33:13 | jroll | or similar | |
| 15:33:30 | mriedem | similar, but that says, | |
| 15:33:31 | mriedem | "When ironic-api and placement-api return, nova will see nodes, create compute_node records for them, and try to create new resource providers (as they are new compute_node records). This will fail with a name conflict, and the nodes will be unusable." | |
| 15:33:42 | mriedem | which, unless the node uuids change, should still find the existing compute node records in the db | |
| 15:33:49 | mriedem | here https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L518 | |
| 15:34:49 | tssurya | mriedem: could be we return false here : https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L511 ? | |
| 15:35:00 | mriedem | tssurya: not if it's the ironic driver | |
| 15:35:12 | mriedem | that's the only one that has rebalances_nodes = True | |
| 15:35:22 | mriedem | for the libvirt driver, yes | |
| 15:35:26 | tssurya | oh okay | |
| 15:35:47 | mriedem | but for the libvirt driver, you should only be able to get into this weird scenario if the physical hostname changes | |
| 15:35:51 | mriedem | for the compute host | |
| 15:36:12 | tssurya | yea understood | |
| 15:37:29 | mriedem | we probably want/need https://review.openstack.org/#/c/545479/ in stable branches | |
| 15:37:35 | belmoreira | wouldn't the logic be simpler if compute_nodes aren't recreated and only have the "host" updated | |
| 15:38:04 | mriedem | belmoreira: that's what that _check_for_nodes_rebalance method tries to do | |
| 15:38:27 | mriedem | if it finds a compute node with the same nodename (ironic node uuid), update the host on the existing compute node record | |
| 15:39:00 | mriedem | we only create a compute node record if we can't find an existing one with the same nodename (or if we found >1) | |
| 15:39:08 | mriedem | which is why i was asking if you saw the >1 error in the logs | |
| 15:39:48 | belmoreira | give me few minutes... | |
| 15:40:13 | mriedem | i think probably regardless of this, we should probably have the virt drivers report a uuid up to the RT so it can use that to set ComputeNode.uuid if the virt driver has something it wants to use (like the ironic node uuid), so we don't just generate a random uuid | |
| 15:40:31 | mriedem | that would likely make debugging ironic stuff easier in the future | |
| 15:40:34 | belmoreira | +1 | |
| 15:42:22 | mriedem | stephenfin: regarding your question about using the requested_networks rather than the limits, that could technically work, but what i don't really like about that is then we have this special edge case of getting limits from 2 places | |