| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 14:15:08 | dansmith | mriedem: but the bottom one? /me looks | |
| 14:15:27 | stephenfin | (this is the "try for PCPUs, and if that fails try for VCPUs" change) | |
| 14:15:33 | dansmith | mriedem: oh I see okay | |
| 14:15:42 | dansmith | stephenfin: okay, in a bit | |
| 14:15:45 | luyao | stephenfin: yes, we create one guest numa if no numa specified in Nova, if numa is set, just put vpmems on numa 0 | |
| 14:15:48 | stephenfin | ta | |
| 14:16:12 | stephenfin | luyao: Okay, but why don't we do the pinning to a host NUMA node like we do normally? | |
| 14:16:27 | sean-k-mooney | stephenfin: it did in a previous version | |
| 14:16:42 | sean-k-mooney | it was removed to allow vms that are bigger then a singel numa node to work | |
| 14:17:06 | stephenfin | can we not do that? | |
| 14:17:09 | alex_xu | stephenfin: just because in PTG someone said we should support the floating VM with vpmem | |
| 14:17:13 | stephenfin | and fix it later, for huge pages too | |
| 14:17:42 | alex_xu | then that use-case write into the spec, then that is why we support that | |
| 14:17:58 | sean-k-mooney | alex_xu: i can pretty much guranette that this wont be supported with floating instance downstream. | |
| 14:18:27 | sean-k-mooney | it might but we have no customer askign for that as far as im aware | |
| 14:18:37 | stephenfin | I don't think that's a valid request, and I don't think we should be catering to it this cycle | |
| 14:18:53 | stephenfin | If we do want to cater to it, we should do the same for hugepages too | |
| 14:19:00 | sean-k-mooney | and cpu pinning | |
| 14:19:05 | stephenfin | and CPU pinning, yeah | |
| 14:19:24 | sean-k-mooney | there was a propal for hyperv to allow this more or less years ago | |
| 14:19:34 | alex_xu | sean-k-mooney: yea, I have to say the initiial proposal doesn't support that also, it added due to the ptg discussion. | |
| 14:19:47 | alex_xu | emm.... | |
| 14:19:56 | alex_xu | I think how much effort we can remove it | |
| 14:19:59 | stephenfin | (apologies, btw, for brining this up so late in the day /o\ This week was the first time I made my way that far up the patch list) | |
| 14:20:06 | mriedem | artom: a couple more nits in https://review.opendev.org/#/c/634606/84 if you're respinning or add them in a follow up | |
| 14:20:09 | alex_xu | do we need to change hw:pmem parse too much | |
| 14:20:22 | artom | http://giphygifs.s3.amazonaws.com/media/rl0FOxdz7CcxO/giphy.gif | |
| 14:20:27 | bauzas | stephenfin: do you want to continue reviewing your series ? | |
| 14:20:35 | sean-k-mooney | alex_xu: i think we dont need to chagne that at all | |
| 14:20:36 | artom | mriedem, yeah, respinning at this point | |
| 14:20:41 | stephenfin | bauzas: Yup, go for it. There's only two changes to be aware of | |
| 14:20:42 | bauzas | or maybe I should look at gibi_fly's one | |
| 14:21:02 | alex_xu | sean-k-mooney: stephenfin how about just like cpu_policy, when the cpu_policy specific, then we create numa_topology for the instance in the api layer | |
| 14:21:15 | sean-k-mooney | alex_xu: the only thing that would change is we create a numa toplogy in the schduler and we ingore pmem devices in the numa toplogy filter | |
| 14:21:40 | sean-k-mooney | alex_xu: that is what we want to do | |
| 14:21:59 | alex_xu | luyao: ^ sorry, is that easy for your | |
| 14:22:02 | sean-k-mooney | we want to add anothger condtional to get_instance_numa_constiratis ? | |
| 14:22:15 | stephenfin | bauzas: Your call :) I would ask that you hit the bottom three though since you hit them already and all I've done if merge back the follow-ups | |
| 14:22:17 | stephenfin | They start here https://review.opendev.org/#/c/671793/ | |
| 14:22:26 | sean-k-mooney | stephenfin hand the sepcific funtion that need 1 more if | |
| 14:22:33 | bauzas | stephenfin: cool | |
| 14:22:47 | stephenfin | sean-k-mooney: um, what? :) | |
| 14:22:57 | alex_xu | sean-k-mooney: stephenfin luyao I guess we just fall into this branch https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L1740 | |
| 14:23:02 | stephenfin | oh, as if that needed another if statement | |
| 14:23:09 | sean-k-mooney | stephenfin: what was the name of the function in hardware.py that we need to add the if tooo | |
| 14:23:16 | stephenfin | ohhh | |
| 14:23:18 | mriedem | artom: ack, you'll have to rebase your functional test patch as well | |
| 14:23:21 | stephenfin | numa_get_constraints, iirdc | |
| 14:23:23 | stephenfin | *iirc | |
| 14:23:37 | mriedem | and https://review.opendev.org/#/c/680739/2 i guess? | |
| 14:23:39 | artom | mriedem, yep, I intentionally left it in merge conflict to facilitate checking changes on the lower patches | |
| 14:23:47 | stephenfin | alex_xu: yup, that's what I think | |
| 14:23:55 | alex_xu | stephenfin: cool | |
| 14:24:28 | alex_xu | luyao: so sorry | |
| 14:24:40 | alex_xu | luyao: are you still at office, I can take over that | |
| 14:24:47 | sean-k-mooney | basically we need to update this right https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L1684-L1708 | |
| 14:24:56 | artom | mriedem, and I think at this point that's how it'll stay until the things merge? I don't want to have you wade through a rebased PS just to see that I addressed the test/wording feedback | |
| 14:25:16 | sean-k-mooney | so that if the flavor has pmen we go into that if and call _get_numa_topology_auto | |
| 14:25:27 | sean-k-mooney | which creates the numa toplogy of 1 | |
| 14:25:40 | stephenfin | exactly | |
| 14:25:51 | luyao | alex_xu: sean-k-mooney stephenfin : I prefer current solution first, when we support numa affinity for vpmem, then generate numa in scheduler | |
| 14:26:02 | sean-k-mooney | actully on just make it hit https://github.com/openstack/nova/blob/master/nova/virt/hardware.py#L1741 | |
| 14:26:07 | sean-k-mooney | either will work | |
| 14:26:09 | mriedem | artom: ? you can address changes on https://review.opendev.org/#/c/634606/84 and https://review.opendev.org/#/c/640021/50 without touching the already approved patches | |
| 14:26:25 | mriedem | just start at https://review.opendev.org/#/c/640021/50, git rebase -i HEAD~2, | |
| 14:26:29 | mriedem | and then git review -R -y | |
| 14:26:41 | artom | mriedem, right, I was saying I'd have to rebase those as well if I want to get the func test out of merge conflict | |
| 14:26:49 | mriedem | oh yeah i care less about that right now | |
| 14:26:59 | artom | So instead of seeing simple one-line changes, you'd see the rebase stuff as well | |
| 14:27:01 | sean-k-mooney | luyao: yes but the current solution is techdebt you know that right and it does nto work the same as other case that create a implcit numa toplogy | |
| 14:27:17 | mriedem | yeah i hate expecting a simple diff from the previous patchset and finding the person rebased on master | |
| 14:27:42 | alex_xu | sean-k-mooney: is there any case that doesn't work | |
| 14:27:59 | alex_xu | I didn't know the previous discussion | |
| 14:28:09 | sean-k-mooney | alex_xu: yes one. you request more cpus or ram then fit on 1 numa node | |
| 14:28:22 | mriedem | nothing like last minute redesigns for vpmems | |
| 14:28:31 | sean-k-mooney | if we leave the code as it is | |
| 14:28:33 | dansmith | artom: mriedem: I replied on the deprecation patch.. I'll take another scan through the last functional patch and try to give my go/no-go which I think is what mriedem is looking for | |
| 14:28:34 | mriedem | i should get a nickel for every time sean-k-mooney says "we'll do x in U" | |
| 14:29:02 | sean-k-mooney | please remvoe any reference to creating an implcit numa toplogy in the docs because it does not act the same | |
| 14:29:10 | alex_xu | sean-k-mooney: I don't get that. there is any limit on the number of cpus and ram in 1 numa node? | |
| 14:29:42 | stephenfin | luyao: Why can't we do NUMA affinity for VPMEM? Just return the needed info in the NUMATopology object and let the NUMATopologyFilter do the work for you? | |
| 14:29:50 | stephenfin | Just like we do for hugepages | |
| 14:29:58 | sean-k-mooney | alex_xu: we do not allow guest to oversubsribe against it self | |
| 14:30:22 | sean-k-mooney | if you have a numa toplogy of 1 then you cant have more cpus or ram then is availabel on that numa node | |
| 14:30:34 | sean-k-mooney | this is check both in the driver and the numa toplogy filter | |
| 14:30:38 | luyao | stephenfin: we can, but the first design not support that, alex_xu had discuss on PTG before | |
| 14:30:56 | alex_xu | sean-k-mooney: the floating VM can | |
| 14:31:10 | sean-k-mooney | only within the host numa cell | |
| 14:31:11 | sean-k-mooney | but yes | |
| 14:31:13 | efried | dansmith: if you can ack the approach for the quota (even if minor tweaks are needed) I can unblock cpu-resources so we have time for the zillion rechecks that will surely be needed. | |
| 14:31:30 | alex_xu | sean-k-mooney: so it still works :) | |
| 14:32:21 | sean-k-mooney | if the host has 16 core and 64GB of ram in the numa cell you cant have a vm wiith 20 core or 96GB of ram | |
| 14:32:21 | alex_xu | stephenfin: yes, we said no NUMA affinity in the first step | |
| 14:32:39 | sean-k-mooney | i think in the sake fo time we jsut have to follow what the spec says | |
| 14:33:03 | alex_xu | stephenfin: I guess the reason the people doesn't want us put anything more in NUMATopology object anymore | |
| 14:33:09 | sean-k-mooney | its sucks we have to special case and we shoudl document this does not work like other implict numa toplogies | |
| 14:33:38 | sean-k-mooney | alex_xu: ya that was why we did not do numa this cycle | |
| 14:33:42 | alex_xu | sean-k-mooney: qemu will refuse to startup that VM? | |
| 14:33:58 | sean-k-mooney | alex_xu: no the numa toplogy filter will prevent it | |
| 14:34:02 | sean-k-mooney | nova does not allow that | |