| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 19:55:15 | dansmith | lol | |
| 19:55:16 | dansmith | riiiight | |
| 19:55:36 | mriedem | i want to say making count_usage_from_placement=True by default was dependent on consumer types to smooth out some of the inconsistencies with the legacy counting method today in edge cases | |
| 19:55:40 | sean-k-mooney | the lol being ever getting to unifed limits | |
| 19:55:40 | mriedem | like during a resize | |
| 19:56:06 | melwitt | unified limits is going to go through the same bake time that counting usage from placement is. it will begin defaulted to False | |
| 19:56:09 | dansmith | mriedem: I didn't think that's why we didn't make it default, because we'd still do the other thing for the non-placement-able resources | |
| 19:56:28 | dansmith | sean-k-mooney: I'm saying don't count your eggs before they hatch | |
| 19:56:34 | sean-k-mooney | ok | |
| 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 | melwitt | mriedem pointed that out earlier | |
| 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: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 | melwitt | I dunno where that saying comes from actually | |
| 20:03:51 | mriedem | sean-k-mooney: whatever, that's better than nothign | |
| 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 | |