Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-09
14:46:16 sean-k-mooney this https://review.opendev.org/#/c/635229/60/nova/virt/libvirt/migration.py@101
14:46:21 artom So actually the field is new in https://review.opendev.org/#/c/634827/
14:46:47 artom sean-k-mooney, well, PS61 is the latest but yeah
14:47:20 dansmith efried: shilpasd: I found something else on the base patch for forbidden aggregates
14:47:45 dansmith I'm pretty sure we don't deepcopy() the default value, which is why I did default=None for the "aggregates" field right above it.
14:48:11 dansmith either way, it should be consistent I think, and I expect =None is more clearly safe than a default of something mutable (like mutable default arguments for methods in python)
14:48:41 sean-k-mooney mriedem: artom so the context for this is that since we added the vcpu_pin_set option, if you have used it we have relied on operators ensuring that all host in the same host aggrate/availablity zone (depending on how you choose to defien the region you can live migrate vm within) has vcpu_pin_set set to the same value
14:49:13 shilpasd dansmith: Thanks for review
14:49:56 dansmith shilpasd: does my comment make sense?
14:50:10 dansmith shilpasd: also, if you're not able to make this change right away I could probably just do it myself
14:50:18 sean-k-mooney mriedem: more specifically if you define vcpu_pin_set we copy the value into <vcpu cpuset=vcpu_pin_set> for all instnace but we never updated that on live migration
14:50:39 shilpasd dansmith: yes, your comment make sense, will have test cases with default config (i.e. no filter remove or append) and with remove and with append
14:50:53 sean-k-mooney so since artom is fixing it for numa instance we asked him to fix non numa instance too
14:51:03 mriedem sean-k-mooney: the config option help for that says nothing about that assumption/requirement https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.vcpu_pin_set
14:51:03 stephenfin TIL 'functional.libvirt.base.ServersTestBase' uses API v2.0 by default. That's two hours I won't get back \o/
14:51:30 dansmith shilpasd: oh you're talking about the later one
14:51:40 dansmith shilpasd: I +2d that already, so don't change it, and/or change it in a followup
14:51:53 dansmith shilpasd: I _just_ left a more complicated comment on the base patch, so go look at that real quick
14:52:00 sean-k-mooney mriedem: that is the entire orginal reason it was intoduced. to define the set of cpu that guest can run on. it was not just for pinned/numa instance
14:52:43 mriedem sean-k-mooney: ok....that doesn't mean much to me since i'm not familiar with when or why that was added,
14:52:50 mriedem i'm just saying, "if you have used it we have relied on operators ensuring that all host in the same host aggrate/availablity zone (depending on how you choose to defien the region you can live migrate vm within) has vcpu_pin_set set to the same value" is not documented
14:53:07 mriedem so undocumented assertions are kind of....hairy
14:53:31 artom mriedem, yep, which why with this new allocation_cpu_tuning stuff, it won't be necessary
14:53:47 artom Previously we hoped both dest and source had it set the same for LM to work
14:53:55 artom Now we handle them being different
14:53:56 mriedem i see there is a todo about it in https://docs.openstack.org/nova/latest/user/flavors.html?highlight=vcpu_pin_set
14:54:08 sean-k-mooney mriedem: ah ok its not documented upstream correct. it used to be documented in intels referece architecure as the way we advised people to partition cpu for the host/guest
14:54:47 openstackgerrit Stephen Finucane proposed openstack/nova master: Add reshaper for PCPU https://review.opendev.org/674895
14:55:24 sean-k-mooney mriedem: the behavior if you did not have them the same was that provide all the cores existed on the destinatinon, the migration would suceeed but the vm would use the wrong cores until it was hard rebooted
14:55:39 sean-k-mooney mriedem: if the core did not exist on the dest host then libvirt would reject the migration
14:55:59 sean-k-mooney so it was a self correcting problem once the guest hard rebooted
14:56:16 mriedem super
14:56:34 stephenfin mriedem: can we easily mutate policy in tests?
14:56:38 mriedem so tl;dr artom's series fixes that now so you don't have to have strict isolation and config in aggregates now?
14:56:41 mriedem stephenfin: yes
14:56:48 sean-k-mooney mriedem: yes
14:56:52 mriedem stephenfin: check functional tests thta use the PolicyFixture
14:56:57 stephenfin sweet. ta
14:57:05 artom mriedem, for that particular option, anyways
14:57:15 artom (vcpu_pin_set)
14:57:17 mriedem stephenfin: e.g. self.policy_fixture.set_rules({
14:57:17 mriedem servers_policies.ZERO_DISK_FLAVOR: base_policies.RULE_ADMIN_API},
14:57:17 mriedem overwrite=False)
14:58:39 shilpasd dansmith: yes cleared, will address for now 'complicated comment on the base patch'
14:59:11 dansmith heh okay
15:14:44 openstackgerrit Merged openstack/python-novaclient master: Microversion 2.78 - show server topology https://review.opendev.org/670790
15:15:41 sean-k-mooney artom: mriedem by the way we will need the <vcpu cpuset> xml update code for stephens pcup series too
15:16:16 sean-k-mooney so this is generally good to have anyway. it would be needed for non pinned guests when cpu_share_set is defiended
15:16:41 mriedem i don't really know anything about that series or how it's going to work but ok
15:16:53 sean-k-mooney *when cpu_shared_set is defined to different value on different hosts
15:17:37 sean-k-mooney mriedem: well we are just using cpu_shared_set instead of vcpu_pinned_set for the same behavior
15:17:52 sean-k-mooney e.g. defineing the set of core floating vms can float over
15:21:28 mriedem so do we expect instances using PCPU resources to be able to live migrate in Train with that series?
15:22:10 sean-k-mooney yes
15:22:57 sean-k-mooney if both seris merge we expect to be able to live migrate with PCPUs or VCPUs and have the xmls updated correctly in both cases
15:23:06 bauzas mriedem: this series will only support PCPU allocations once *all* computes are Train
15:23:22 sean-k-mooney mriedem: we defered instance have both pcpus and vcpus at the same time to U or later
15:23:27 bauzas mriedem: once the operator modifies an option
15:24:12 sean-k-mooney bauzas: correct. all of this is opt in. if you dont modify the configs for the PCPU seriese then you keep the old behavior
15:24:53 sean-k-mooney which is what we need for upgrades. and hopeful in U we can start enableing thing by defualt before removing the legacy options in V
15:25:10 mriedem bauzas: that wasn't what i asked
15:25:21 bauzas hum okay
15:26:03 sean-k-mooney bauzas: mriedem was asing will the numa migration feature wokr with instance that have PCPU allocations
15:26:12 sean-k-mooney *asking
15:26:14 bauzas ok
15:27:04 donnyd sean-k-mooney: just an FYI got an electrician out this weekend to do my standby generator. Which means power had to be shutdown for 8 hours.. which means FN was taken out of the pool on Saturday
15:28:01 artom sean-k-mooney, and it will? To me it seems like black magic that it would, but I guess it makes sense
15:28:11 donnyd So the NUMA job won't schedule until it comes back online. If I am really lucky the propane guy will be here today to finish up that install and then FN will be back online whether I have commercial power or not
15:28:25 artom NUMA LM just consumes _get_guest_numa_config, and then plays with the XML
15:28:43 mriedem sean-k-mooney: sort of, but without numa
15:29:15 sean-k-mooney donnyd: yep i saw the update on openstack infra
15:29:17 mriedem sean-k-mooney: since you said the pcpu stuff would need what artom is adding anyway, i was wondering if we expected instances with pcpu allocations to live migrate in train, not specific to numa though
15:29:41 mriedem b/c i think pcpu in train is already very risky, and trying to say that live migration would work for those as well was doubly risky
15:29:44 sean-k-mooney mriedem: all instance with pcpus have a numa toplogy
15:29:50 mriedem oh
15:30:22 sean-k-mooney hw:cpu_policy=dedicate create a numa toplogy of 1 the same as hw:numa_nodes=1
15:30:33 sean-k-mooney same with hw:mem_page_size set to anything
15:30:35 artom sean-k-mooney, I think he means stephenfin's new PCPU resource class
15:30:40 sean-k-mooney yep
15:30:55 sean-k-mooney the only thing that will use that are things with hw:cpu_policy=dedicated
15:30:59 artom Oh, right, that's how we end up with instances with that resource
15:31:02 mriedem hmm, is that just libvirt specific or actually codified into how the code outside of the driver works?
15:31:05 artom By doing anything NUMA-y
15:31:08 sean-k-mooney so all things with PCPUs will have numa
15:31:26 sean-k-mooney mriedem: technically just libvirt but no other dirver currely support pinning
15:31:58 sean-k-mooney mriedem: the api does not require the guest to have a numa toplogy of 1
15:32:05 sean-k-mooney so people should not rely on it
15:32:13 sean-k-mooney but that is what the only implmentaion does
15:34:32 stephenfin sean-k-mooney: bikeshedding time. I need a name for this variable https://review.opendev.org/#/c/674895/30/nova/virt/libvirt/driver.py@7439
15:35:01 stephenfin I can't use allocations because that conflicts with an existing variable, and instance_allocations doesn't work since we're tracking migrations for both instances and migration records
15:35:17 stephenfin instance_migration_allocations is too ugly
15:36:05 efried bad_allocs?
15:36:34 efried although 'bad' isn't really a good adjective for them, in instances_with_bad_allocations either.
15:37:05 stephenfin yeah, I've renamed the latter to 'allocations_needing_reshape'
15:37:07 efried but instances_with_allocations_that_need_to_be_reshaped is unwieldy, as I'm sure you realized while you were working on this.
15:38:11 efried stephenfin: btw, you've noticed there's a tox failure somewhere down the series yah?
15:38:25 stephenfin Damn. I had not
15:39:11 efried looks like https://review.opendev.org/#/c/680983/1
15:39:31 efried stephenfin: at least ^
15:40:12 artom stephenfin, "legacy_instances"?

Earlier   Later