| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 19:51:15 | mriedem | and here https://github.com/openstack/nova/blob/f4ca3e70852c0a7ed7904a9f2d7177c9118d3d1c/nova/scheduler/client/report.py#L2356 | |
| 19:51:19 | dansmith | melwitt: ack | |
| 19:51:28 | sean-k-mooney | dansmith: not direclty that is a sideffect | |
| 19:51:31 | dansmith | mriedem: if you combine them yes | |
| 19:52:08 | dansmith | mriedem: that's a hack to make it work, not the right solution | |
| 19:52:12 | mriedem | i know we dont want to combine them externally, but the non-placement counting method is going to be based on instances.vcpus which comes from the embedded instance.flavor.vcpus | |
| 19:52:17 | sean-k-mooney | dansmith: the follow up spec that wants to have a vm with some pinned cores and some floating cores need them to be different thigns | |
| 19:52:35 | dansmith | mriedem: right but we should be moving to placement quotas anyway | |
| 19:53:11 | dansmith | sean-k-mooney: AFAIK, this work is aimed at making pcpus not just a per-host special case of pretending that they're like vcpus | |
| 19:53:23 | dansmith | sean-k-mooney: so, yeah, obviously separate quotas is not the goal of this, | |
| 19:53:32 | dansmith | but all of this is important to get right | |
| 19:53:38 | sean-k-mooney | yes | |
| 19:53:45 | sean-k-mooney | im not disputing that at all | |
| 19:53:50 | dansmith | otherwise people really need to segregate these hosts still, which means the rest of it doesn't get us anything | |
| 19:54:16 | sean-k-mooney | im not sure i agree on the last point but im not going to rathole on it either | |
| 19:54:35 | sean-k-mooney | i think this will change in U again with unified limits | |
| 19:54:53 | sean-k-mooney | so i would prefer to groub them in Train to keep the pretrain behavior | |
| 19:54:59 | sean-k-mooney | and then only change the behavior once | |
| 19:55:10 | sean-k-mooney | in U when everything goes to unifed limits | |
| 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 | mriedem | like during a resize | |
| 19:55:40 | sean-k-mooney | the lol being ever getting to unifed limits | |
| 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 | 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 | |