| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 19:29:02 | sean-k-mooney | yes | |
| 19:29:03 | efried | that was in there too. | |
| 19:29:49 | dansmith | well, I don't see how we can be landing any of this without the quota bit in place at the very least | |
| 19:29:50 | mriedem | which is because of this right? https://github.com/openstack/nova/blob/stable/stein/nova/virt/libvirt/driver.py#L852 | |
| 19:30:16 | mriedem | is hw:emulator_threads_policy and that +1 thing for pcpu restricted to the libvirt driver or does that logic creep into the api? | |
| 19:30:59 | sean-k-mooney | am well that is hard to answer | |
| 19:31:04 | mriedem | GREAT | |
| 19:31:09 | sean-k-mooney | only the libvirt driver supprots pinning | |
| 19:31:14 | sean-k-mooney | and this only works with pinning | |
| 19:31:24 | sean-k-mooney | so this only works with the libvirt dirver | |
| 19:31:32 | dansmith | that is not the right answer | |
| 19:31:50 | dansmith | letting stuff like that creep into the API because only one driver supports it is how we have a ton of xen-specific warts on the api | |
| 19:31:56 | sean-k-mooney | is the right answer we hope to remove that in U | |
| 19:32:23 | sean-k-mooney | because we hope to remvoe it in U | |
| 19:32:36 | mriedem | delicious agent builds | |
| 19:32:39 | sean-k-mooney | now that we have support for share | |
| 19:33:14 | sean-k-mooney | wehich mapps the emulator threads to the same cpu pool as the floating vms we dont think we need isolate anymore | |
| 19:33:33 | mriedem | sean-k-mooney: so by remove "that" you mean hw:emulator_threads_policy ? | |
| 19:33:41 | sean-k-mooney | yes | |
| 19:34:44 | sean-k-mooney | what stephen and i woudl like to propose in U is this. if you have cpu_shared_set defiend the emultor thread run there. if not they run on the same cores as your pinned vm | |
| 19:34:59 | sean-k-mooney | that is what you get if you do hw:emulator_threads_policy=share today | |
| 19:35:26 | sean-k-mooney | and if we only support 1 value for the option we can remove it entirly and just do that by default | |
| 19:36:02 | mriedem | efried: so you might want to not drop the -2 on https://review.opendev.org/#/c/671793/23 until the quota issue is sorted out | |
| 19:36:18 | mriedem | this quota issue https://review.opendev.org/#/c/674895/32/nova/virt/libvirt/driver.py@7457 | |
| 19:36:30 | efried | done | |
| 19:36:37 | sean-k-mooney | do we have quota issue wiht this? | |
| 19:36:48 | sean-k-mooney | i though quota was being counted on flavor.vcpu | |
| 19:36:58 | dansmith | sean-k-mooney: did you read the comments? | |
| 19:36:59 | sean-k-mooney | not on the resouce class in train | |
| 19:37:06 | sean-k-mooney | dansmith: no not yet | |
| 19:37:10 | dansmith | sean-k-mooney: maybe do that :) | |
| 19:37:51 | efried | good, now everyone can focus on vpmem | |
| 19:37:58 | sean-k-mooney | ok but since we only support vms with all pinned or all shared cpus with this seriese it does not change things as far as i was aware in train | |
| 19:38:50 | dansmith | before this, we'd limit you to at least your vcpu quopta | |
| 19:38:53 | dansmith | after this, no limits AFAICT | |
| 19:39:18 | dansmith | honestly, I'm not sure how you're going to fix it so it works the same with placement quotas and nova quotas | |
| 19:39:18 | sean-k-mooney | we should still be limiting on flavor.vcpu | |
| 19:39:29 | dansmith | not with placement | |
| 19:39:32 | dansmith | did you read the comments? | |
| 19:39:47 | sean-k-mooney | im trying to find which patch you put it on | |
| 19:39:57 | mriedem | (2:36:21 PM) mriedem: this quota issue https://review.opendev.org/#/c/674895/32/nova/virt/libvirt/driver.py@7457 | |
| 19:40:00 | sean-k-mooney | oh but i think i know what your saying | |
| 19:40:17 | sean-k-mooney | we have the option to track cpu quota in placmenet already | |
| 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 | |