| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 19:40:33 | mriedem | by default we don't use placement for quota usage counting | |
| 19:40:34 | sean-k-mooney | we enabled it by default stien right | |
| 19:40:40 | mriedem | f naw | |
| 19:40:46 | sean-k-mooney | oh we dont | |
| 19:40:46 | mriedem | where is melwitt | |
| 19:40:53 | dansmith | we should be in train I think right? | |
| 19:40:56 | dansmith | meaning, | |
| 19:41:00 | dansmith | we should be turning that on but have't | |
| 19:41:02 | melwitt | mriedem: hiding | |
| 19:41:21 | sean-k-mooney | dansmith: so if we turn that on then yes we have a proablem | |
| 19:41:26 | sean-k-mooney | if we dont we should not | |
| 19:41:30 | dansmith | no | |
| 19:41:36 | dansmith | we have a problem because people could have that on | |
| 19:41:43 | sean-k-mooney | ture | |
| 19:41:43 | dansmith | you know config knobs can be set to not the default right? :) | |
| 19:41:45 | mriedem | cern uses it | |
| 19:43:24 | sean-k-mooney | ok i see the problem. i was aware that was a thing but ya i had not consiered the impact to this | |
| 19:43:37 | mriedem | quota is generally the last thing anyone thinks about | |
| 19:44:30 | dansmith | if CPUs weren't the most expensive and constrained resource in the cloud, then maybe less of an issue, but.... :D | |
| 19:45:01 | mriedem | [quota]/injected_file_content_bytes is a major concern of mine | |
| 19:45:38 | sean-k-mooney | im not sure if sarcasim or if i shoudl feel realy bad for you | |
| 19:45:46 | mriedem | heh, sarcasm | |
| 19:46:41 | sean-k-mooney | dansmith: i always ran out of ram first but ya. i think the only thing we coudl do woudl be to could both inventories. at least short term | |
| 19:47:03 | sean-k-mooney | wehere does that code live | |
| 19:47:12 | sean-k-mooney | e.g. where we check the quota | |
| 19:47:14 | melwitt | enforcing quota on other resource classes (DISK_GB and more) is part of the proposal in the unified limits spec, as I think sean-k-mooney mentioned earlier | |
| 19:48:17 | dansmith | melwitt: right, but this set is taking dedicated cpus out of the equation for the cpus quota, | |
| 19:48:29 | dansmith | effectively making them unconstrained (and unconstrainable) | |
| 19:48:29 | melwitt | sean-k-mooney: nova/quota.py is the main file | |
| 19:48:35 | dansmith | but they're the most expensive things | |
| 19:48:37 | mriedem | sean-k-mooney: start here https://github.com/openstack/nova/blob/f4ca3e70852c0a7ed7904a9f2d7177c9118d3d1c/nova/quota.py#L1281 | |
| 19:48:38 | sean-k-mooney | melwitt: thanks | |
| 19:49:09 | dansmith | and combining them together as a hack is pretty dumb, because the whole point of this effort is to make dedicated cpus be first class citizens instead of a hack | |
| 19:49:35 | dansmith | the other problem is, | |
| 19:50:01 | dansmith | we wouldn't want the quota behavior to be different depending on whether or not you're using placement for quota | |
| 19:50:02 | melwitt | dansmith: yeah, sorry, was trying to say that if enforcing quota for PCPU resource class is what's needed then that's a big amount of work | |
| 19:50:07 | dansmith | somewhat unrelated, | |
| 19:50:16 | dansmith | melwitt: were't we going to enable quota in placement by default soon? | |
| 19:50:30 | sean-k-mooney | dansmith: well if combined them together ti woudl be maintained previous behavior | |
| 19:50:31 | dansmith | melwitt: yep, and I'm saying that's the right solution | |
| 19:50:49 | dansmith | sean-k-mooney: the point of this work is to make pcpus not work like vcpus right? :) | |
| 19:51:07 | melwitt | dansmith: I don't remember a specific timeline, just that we wanted to let it bake with cern for awhile before making it default | |
| 19:51:09 | mriedem | idk that counting PCPU for cores quota from placement is that hard, you'd just sum VCPU and PCPU here wouldn't you? https://github.com/openstack/nova/blob/f4ca3e70852c0a7ed7904a9f2d7177c9118d3d1c/nova/scheduler/client/report.py#L2344 | |
| 19:51:13 | sean-k-mooney | and the two main goals of this work was co existing of pinned and floating vms on the same host and removeing the race on claiming pinned cpus | |
| 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 | 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 | |