| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-31 | |||
| 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 | openstackgerrit | Merged openstack/nova stable/ocata: Fix shelving a paused instance https://review.openstack.org/569655 | |
| 15:26:50 | mriedem | so it might not be a rebalance, but that code should have found it | |
| 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 | jroll | VM | |
| 15:29:38 | mriedem | just wondering if you've seen similar issues to what belmiro is describing above | |
| 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 | 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:32:35 | mriedem | so unless the node uuid changed, we should find the existing compute node reord | |
| 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 | |
| 15:42:50 | mriedem | stephenfin: limits are more or less deprecated because of the filter scheduler and placement but that *only* covers vcpu/ram/disk, and the FilterScheduler, | |
| 15:43:12 | mriedem | the CachingScheduler and anything related to numa relies on limits from the scheduler still | |
| 15:43:12 | jroll | mriedem | we probably want/need https://review.openstack.org/#/c/545479/ in stable branches <- right, that's the bugfix for the thing I linked | |
| 15:43:22 | mriedem | jroll: yeah i know | |
| 15:43:30 | jroll | ok | |
| 15:43:38 | mriedem | jroll: the patch seemed to have some controversy, but it was merged so might as well be backported | |
| 15:43:57 | mriedem | stephenfin: i would need to double check, but i think the only thing we do claims on in the RT w/o the limits dict is the pci requests | |
| 15:44:05 | mriedem | because those were done differently, for whatever reason | |
| 15:44:12 | mriedem | and the inconsistency is hella confusing | |
| 15:44:30 | stephenfin | mriedem: Quick debugging showed the limits dict was always empty | |
| 15:44:36 | stephenfin | Using filter_scheduler, anyway | |
| 15:44:42 | mriedem | stephenfin: for vcpu/ram/disk yes | |
| 15:44:50 | mriedem | the numa filter was, i thought, the only one that put stuff into it | |
| 15:44:58 | mriedem | if using filter scheduler | |
| 15:45:12 | mriedem | but you made some comment in vancouver about how that might not be true, which dansmith ack'ed and i didn't get | |
| 15:45:38 | mriedem | pci_requests are the snowflake right now https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L211 | |
| 15:46:14 | mriedem | if we passed through network_requests, then we'd have (1) limits (2) pci_requests and (3) network_requests, all for doing similar claims type stuff | |
| 15:46:22 | mriedem | which makes me want to do bad things, physically | |
| 15:46:31 | mriedem | maybe i'm the only one that feels this way though | |
| 15:46:35 | stephenfin | mriedem: You're referring to https://github.com/openstack/nova/blob/master/nova/scheduler/filters/numa_topology_filter.py#L102 ? | |
| 15:46:42 | mriedem | stephenfin: yes | |
| 15:47:41 | stephenfin | That does clear things up slightly. I was looking at the limits arg to 'numa_fit_instance_to_host' which is an object, not a dict. Hella confusing | |
| 15:48:27 | mriedem | i think https://github.com/openstack/nova/blob/master/nova/scheduler/filter_scheduler.py#L354 is where we take the limits from the HostState after the filters run and shove them into the Selection object which gets passed back to conducotr | |
| 15:48:29 | mriedem | *conductor | |