Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-10
19:56:37 dansmith unified limits has been a long time coming, so..
19:57:25 sean-k-mooney ya i know. so are you opposed to internally in nova combining them for Train
19:57:31 mriedem dansmith: without digging up old review comments and ML thread conversations, i want to say my recollection was (1) counting usage from placement has at least 3 differences in behavior from legacy counting - which are documenting in the config option help text and (2) the main benefit is for multi-cell deployments, of which there are few,
19:57:39 mriedem and (3) it landed late in stein,
19:57:44 sean-k-mooney when counting using placment
19:57:56 mriedem so to reduce the risk on non-multi-cell deployments with the behavior change, default to legacy counting
19:58:07 mriedem melwitt: ^ is that what you remember?
19:58:22 melwitt it landed early in train, the data migration landed late in stein
19:58:32 dansmith oh, I thought it was stein
19:58:37 mriedem i thought it was stein too...
19:58:38 dansmith in that case, can't default it in train
19:59:02 mriedem https://review.opendev.org/#/c/638073/ it was train
19:59:04 mriedem time flies
19:59:19 sean-k-mooney well if we cant default it in train then we still need to supprot it in train with pcpu in plamcent right
19:59:39 melwitt mriedem: but yeah I think the combo of all those reasons were why we default to false
20:00:40 melwitt big delta from legacy counting and only multi-cell ppl likely to "need" it (for the down cell resilience). so we chose not to impose the big delta in behavior on the majority who don't need down cell resilience
20:01:08 sean-k-mooney so do we just want to add another line here https://github.com/openstack/nova/blob/f4ca3e70852c0a7ed7904a9f2d7177c9118d3d1c/nova/scheduler/client/report.py#L2356 to make it work by combining them?
20:01:22 sean-k-mooney *them being PCPUs
20:01:25 mriedem to be fair though, some of those differences in behavior in counting also changed since i think ocata or pike
20:01:25 melwitt mriedem pointed that out earlier
20:01:41 mriedem when we moved to counting in general and dropped reservations
20:01:51 mriedem and no one apparently noticed until we noticed in stein :)
20:01:52 sean-k-mooney yes im wondering if we should comment tha ton stephens patch as the way forward so he can do it tomorrow
20:02:21 melwitt mriedem: I think there was only one change in pike, doesn't leave room for a revert resize, IIRC (I documented all of it on the review comment)
20:02:24 mriedem sean-k-mooney: i think one could say, "one way forward is ...."
20:02:38 melwitt the new stuff, there's way more deltas than that, a big laundry list
20:03:23 mriedem what is a laundry list anyway? 1. put stuff in washer and wash, 2. put stuff in dryer, 3. fold. wouldn't a more accurate list be a grocery list?
20:03:29 sean-k-mooney mriedem: ok but stephen isnt here and i wanted to provide a summary to him but ill just tell him to read scollback in the morning
20:03:37 melwitt mriedem: hah
20:03:51 mriedem sean-k-mooney: whatever, that's better than nothign
20:03:51 melwitt I dunno where that saying comes from actually
20:04:08 mriedem https://english.stackexchange.com/questions/437507/what-is-the-origin-of-the-phrase-laundry-list
20:04:22 melwitt I still haven't wrapped my head around what "combining" the resource classes means and whether/how it's different from legacy counting
20:04:33 mriedem it wouldn't be different from legacy counting
20:04:35 melwitt I mean, I know it means VCPU + PCPU
20:04:40 melwitt ok
20:04:42 mriedem so i think the summary is,
20:05:05 mriedem 1. with legacy counting, pcpu is counted the same since we use instance.vcpus which comes from the flavor
20:05:16 mriedem 2. with placement counting, we'd have to combine VCPU and PCPU usage to match ^
20:05:26 dansmith 3. neither are actually right
20:05:35 mriedem 3. long-term combining them goes against the goal of separate them as countable trackable resources
20:05:39 mriedem right
20:06:28 mriedem so the immediate question is, is #2 good enough for train with punting sorting out #3 to the future
20:07:01 sean-k-mooney so this is a bit of nova i dont really fully understand. melwitt is there anythin we can do before unified limts to supprot not combining them if that is prefrable
20:07:16 sean-k-mooney or do we just wati for unifed limits to do #3
20:07:59 melwitt I guess if it's not worse than what's possible with legacy counting (inability to separate) then the hack doesn't sound like a huge issue to me. it would obviously need a TODO on it to get rid of it when unified limit support exists and is enabled
20:09:59 melwitt sean-k-mooney: nothing reasonable, really. you'd have to add a new quota config and resource for "pcpu_whatever" and use that to let people set the limit separately. because until unified limits, there would be no way for nova to consume the unified limit for PCPU that a person sets in keystone
20:10:36 sean-k-mooney right and we dont really want to do that because techdebth
20:10:52 efried From where I sit (outside the swirling maelstrom of actual understanding) it sounds like we've had a pretty big imbalance "forever" where we've been counting dedicated and virtual cpus "the same" from a quota perspective. If so, we're not making it worse by continuing to do that, just now we're doing it with a separate resource class for the former.
20:10:53 melwitt yeah, that would be a big tech debt
20:11:21 melwitt whereas the hack of combining VCPU and PCPU doesn't sound like big debt to me, unless I've missed something more complex about it
20:11:30 melwitt "small debt"
20:13:07 sean-k-mooney it should be small to do i think. i assume its just the line mriedem linked but this code is new to me
20:13:19 mriedem plus functional testing
20:13:39 sean-k-mooney we need to haneld this branch too https://github.com/openstack/nova/blob/f4ca3e70852c0a7ed7904a9f2d7177c9118d3d1c/nova/scheduler/client/report.py#L2344
20:13:52 mriedem i'd want to see a functional test that creates a server that allocates PCPU inventory and then assert that cores usage for that tenant is incremented
20:14:29 sean-k-mooney i think stephen has a function test for the first half so the quota check shoudl not be too hard to add
20:17:21 sean-k-mooney ok so he has functional test in the reshap patch at a minium
20:18:20 sean-k-mooney ok yes he add more here https://review.opendev.org/#/c/671801/43/nova/tests/functional/libvirt/test_numa_servers.py
20:18:48 sean-k-mooney so that proably where it makes sense to ad the test
20:41:19 mriedem artom: https://review.opendev.org/#/c/640021/48
20:45:02 mriedem i probably won't get to the functional test patch tonight
20:46:45 sean-k-mooney mriedem: for what its worth i did test teh [upgrade_levels]/compute=stein case prviously
20:47:02 sean-k-mooney i can test it again tomorrow im more or less done for the day
20:47:22 sean-k-mooney btu if either node has [upgrade_levels]/compute=stein we endup with the stien behavior
20:47:40 sean-k-mooney the migration successes as long as the cores used in the souce host exist on the dest
20:47:45 sean-k-mooney but no xml is updated
20:47:57 mriedem so it goes back to the bug behavior
20:47:58 mriedem right?
20:48:03 sean-k-mooney yep
20:48:15 sean-k-mooney it goes back to the current master/stien behavior
20:48:20 mriedem yeah i don't know that we really need to bend over backward to try and detect that from conductor
20:49:05 mriedem if you're computes are fully upgraded and restarted and reporting as train service versions you should have also removed any manual pins
20:49:11 sean-k-mooney if you dont have can_live_migrate_numa set we will block it anyway
20:49:15 mriedem *your
20:49:44 mriedem not with the new checks
20:49:49 mriedem we would totally ignore can_live_migrate_numa
20:49:59 mriedem as long as all computes are reporting train
20:50:04 sean-k-mooney i thik we have a min compute servcie check too before we ignore it
20:50:20 sean-k-mooney oh thats the compute service
20:50:22 sean-k-mooney not the RPC
20:50:25 mriedem if we pass that check we ignore the config
20:50:30 mriedem right
20:51:09 sean-k-mooney well ya i agree we likely dont need to bend over backwards to check this in the conductor
20:52:10 sean-k-mooney anyway my concentration is totally gone so ill call it a day.
20:52:42 sean-k-mooney artom: ill test v 49 tommorow when you have addressed mriedem comments
21:10:33 mriedem melwitt: brinzhang has been asking me to review this api change https://review.opendev.org/#/c/674243/ but i haven't had time with the bw provider migrate and numa lm stuff - maybe you can peruse it?
21:11:09 artom mriedem, ack, thanks!
21:11:22 artom (The func test can wait until after FF if that's what it comes to, right?)
21:11:39 artom I mean, it's not plan A, but...
21:13:21 mriedem my priority is sean's manual testing and the gate job
21:13:50 mriedem brinzhang: a follow up is not the correct answer to all issues https://review.opendev.org/#/c/679413/4
21:19:03 melwitt mriedem: will do
21:25:47 openstackgerrit Dustin Cowles proposed openstack/nova master: Use SDK for add/remove instance info from node https://review.opendev.org/659691
21:25:48 openstackgerrit Dustin Cowles proposed openstack/nova master: Use SDK for getting network metadata from node https://review.opendev.org/670213
21:50:02 openstackgerrit Dustin Cowles proposed openstack/nova master: Use SDK for getting network metadata from node https://review.opendev.org/670213
21:50:02 openstackgerrit Dustin Cowles proposed openstack/nova master: Use SDK for add/remove instance info from node https://review.opendev.org/659691
23:34:51 openstackgerrit Merged openstack/nova master: api-ref: fix server topology "host_numa_node" field param name https://review.opendev.org/680775
#openstack-nova - 2019-09-11
00:20:35 melwitt sean-k-mooney: fyi I've proposed a stable/stein release of os-vif. I didn't find one already proposed https://review.opendev.org/681358
00:31:48 sean-k-mooney release or branch. there is a a stable branch but i dont think we have done a stabel release in a while

Earlier   Later