Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-05-31
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
15:48:45 mriedem yup https://github.com/openstack/nova/blob/master/nova/objects/selection.py#L53
15:49:36 mriedem and then those are passed to compute here https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L1265
15:49:58 mriedem so unless dansmith feels otherwise, i'd prefer to just continue using the limits dict for now for limits specific things
15:50:06 mriedem rather than add another wrinkle to where we get limits in the RT
15:50:15 stephenfin mriedem: So you're thinking I shouldn't be doing this https://review.openstack.org/#/c/564448/3/nova/virt/hardware.py@1609 right?
15:50:24 stephenfin i.e. no extra parameter
15:50:51 stephenfin instead, do limits['network_requests'] or the likes inside that function
15:51:45 mriedem i can't hardly read that code so i'm not sure what it's doing
15:51:50 dansmith mriedem: I'm on a call right now, but I asserted in our talk that limits is really just a temporary mechanism for communicating decisions from the scheduler filters to the lower layers,
15:51:56 dansmith which this is I think, so I think it fits
15:52:04 dansmith are you agreeing with that or asserting something else?
15:52:14 mriedem dansmith: you and i are in agreement,
15:52:36 mriedem stephenfin found that we pass requested_networks down to compute also, which will have the same type of informatoin in them so we could use that, and i'm asserting we shouldn't b/c it's confusing
15:52:48 dansmith agree
15:53:06 mriedem stephenfin: numa_fit_instance_to_host doesn't modify the limits dict today does it?
15:54:32 stephenfin mriedem: Narp. _numa_fit_instance_cell consumes any NUMATopologyLimits objects contained therein but doesn't modify it
15:54:38 mriedem stephenfin: i don't think that numa_fit_instance_to_host should modify the limits dict, the scheduler filter should do that
15:54:58 mriedem i would basically like to keep all of this as decoupled as reasonably possible
15:55:04 mriedem because right now it's a tightly coupled mess
15:55:33 mriedem and it's like the last part of nova i'm not really familiar with, and don't really ever want to be if i can help it :)
15:56:07 stephenfin By scheduler filter you mean the NUMATopologyFilter?
15:56:10 mriedem yes
15:56:17 mriedem the thing that already puts the numa limits in the limits dict today
15:56:26 mriedem the scheduler filters are the only things that modify the limits dict
15:56:32 mriedem we should maintain that pattern
15:56:56 openstackgerrit Merged openstack/nova stable/ocata: Add ssbd and virt-ssbd flags to cpu_model_extra_flags whitelist https://review.openstack.org/570514
15:57:02 stephenfin We're still going to need some way to get that info into the filter though
15:57:17 stephenfin So we'd still be modifying RequestSpec
15:57:20 mriedem the request spec will have the requested_networks right?
15:57:26 mriedem yes i know and agreed with that already
15:58:22 mriedem api -> modify reqspec.requested_networks like pci requests -> scheduler -> numa filter + modify limits -> conductor -> compute -> RT (claim from limits) -> allocate_for_instance -> update instance info cache with network physnet and tunneled info -> driver.spawn
15:58:24 mriedem done!
15:59:07 stephenfin Oh, so I still have the RequestSpec changes which means for the scheduler I'll just be undoing this change https://review.openstack.org/#/c/564448/3/nova/virt/hardware.py@1609
15:59:13 stephenfin (where I add 'network_requests'
15:59:28 stephenfin ...and instead consuming 'network_requests' from limits
15:59:53 belmoreira mriedem: I don't have the logs for https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L531
16:00:01 stephenfin Then I can use that for the call to 'instance_claim'
16:00:02 stephenfin Gotcha
16:00:06 mriedem stephenfin: i don't really know what that code is doing, so hard for me to say
16:00:17 stephenfin mriedem: I'm just referring to the function signature

Earlier   Later