| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-09 | |||
| 14:02:01 | stephenfin | weeird | |
| 14:02:11 | stephenfin | there must be something else going on here so | |
| 14:02:18 | artom | stephenfin, so, dunno if this is relevant, but while working on the the NUMA LM func tests, I noticed there was an issue in how you started computes with different fakelibvirt connections for each | |
| 14:02:26 | stephenfin | shoot | |
| 14:02:47 | artom | stephenfin, https://review.opendev.org/#/c/672595/45/nova/tests/functional/libvirt/base.py | |
| 14:03:14 | artom | Again, no idea if that's what's causing your thing now, but could be relevant | |
| 14:04:01 | stephenfin | oh, fun | |
| 14:12:55 | stephenfin | artom: The other approach is this one https://review.opendev.org/#/c/678470/24/nova/tests/unit/virt/libvirt/fakelibvirt.py | |
| 14:13:11 | stephenfin | which I think accomplishes much the same thing | |
| 14:13:35 | stephenfin | there are quite a few in-flight changes with those files atm :) | |
| 14:15:09 | openstackgerrit | Brin Zhang proposed openstack/nova master: Follow-up: Add delete_on_termination to volume-attach API https://review.opendev.org/680655 | |
| 14:15:28 | artom | stephenfin, and then you stub that connection into each compute manager I guess? | |
| 14:15:53 | artom | Oh, that's not your patch | |
| 14:15:54 | artom | Hrmm | |
| 14:17:02 | kashyap | aspiers: Any reason you know this method is in virt/hardware.py -- _get_flavor_image_meta()? It can be used by "non-hardware"-related features, too | |
| 14:17:15 | aspiers | kashyap: git blame? | |
| 14:17:17 | kashyap | Maybe a question for stephenfin, as he wrote that :-) | |
| 14:17:37 | kashyap | aspiers: Already did; 48e0c000fefe561ec5cfa1364ef2cac2f6ce9244 | |
| 14:18:37 | kashyap | aspiers: stephenfin: Actually, disregard...it makes sense, because of the 'hw' key :D | |
| 14:35:19 | dansmith | mriedem: assume you'll take another trip through the numa patches today? I'm about to start into the forbidden garden of aggregates | |
| 14:35:53 | bhagyashris | mriedem: Hi | |
| 14:36:30 | mriedem | dansmith: i was going over them over the weekend but stopped short of the big one | |
| 14:36:38 | mriedem | waiting on ci results in the special job | |
| 14:36:42 | dansmith | ah cool | |
| 14:37:00 | mriedem | https://review.opendev.org/#/c/635229/ has new wizardry that is going to require stephenfin and/or sean-k-mooney | |
| 14:42:33 | sean-k-mooney | mriedem: is that the patch you had question relating to the xml generation on | |
| 14:44:36 | sean-k-mooney | mriedem: if there is a specific thing you wanted me to look at i can do it quickly otherwise im going to finish with stephenfin series then start on artoms | |
| 14:45:46 | artom | sean-k-mooney, the new allocation_cpu_tuning, to cover the case when an instance with no NUMA topology is on a host with vcpu_pin_set | |
| 14:45:59 | sean-k-mooney | ah right | |
| 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 | |