| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 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 | alex_xu | stephenfin: yes, we said no NUMA affinity in the first step | |
| 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: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 | |
| 14:34:25 | alex_xu | sean-k-mooney: no, the numa topology filter won't stop that, since the instance has no numa_toplogy obj | |
| 14:34:40 | sean-k-mooney | righ with the special case code it wont | |
| 14:34:42 | alex_xu | sean-k-mooney: we only implicit to create one in libvirt driver | |
| 14:34:46 | sean-k-mooney | so that is why it works | |
| 14:34:51 | mloza | Is there an option in nova for instance timeout to ERROR state when a compute node fails? | |
| 14:35:11 | sean-k-mooney | but was awsering your question about what does hw:numa_nodes=1 do | |
| 14:35:36 | sean-k-mooney | all other implcit numa toploges act exactly like hw:numa_nodes=1 | |
| 14:35:44 | alex_xu | yes | |
| 14:35:50 | sean-k-mooney | hw:pmem is not the same as hw:numa_nodes=1 | |
| 14:36:11 | stephenfin | okay, I hate the idea of the implicit NUMA affinity when VPMEM is specified working differently from implicit VPMEM for other things (hugepages, CPU pinning), but I agree it doesn't make sense to work that way if we can't provide VPMEM NUMA affinity | |
| 14:36:18 | sean-k-mooney | and we should not imply it is in any docs, comment or release notes | |
| 14:36:35 | stephenfin | I'm going to have a look to see how hard tacking on proper NUMA affinity is | |
| 14:37:02 | sean-k-mooney | stephenfin: its not hard it has been blocked because its not plamcent native. | |
| 14:37:04 | stephenfin | I don't buy the argument that we shouldn't add more things to the NUMATopology object. NUMA in placement isn't ready so we've to use what we got | |
| 14:37:14 | stephenfin | put politely, that's nonsense :) | |
| 14:38:03 | sean-k-mooney | its been nonsense for 3 years but that is not going to change. so we should enbrace doing it the plamcnet way. in U to give mriedem a nickel | |
| 14:38:10 | stephenfin | gimme a half hour to check out how tough this would be to do. If it's terrible, I'll hold my tongue | |
| 14:39:00 | alex_xu | stephenfin: yes, it will be very easy to support numa affinity once we have numa in placement | |
| 14:39:09 | sean-k-mooney | its 1 field in the host numa toplogy blob and a tweek to the numa toplogy filter to match on it in the numa_fit_instace_to_host funciton | |
| 14:39:12 | alex_xu | stephenfin: just like vgpu, the first step no numa affinity | |
| 14:39:33 | sean-k-mooney | alex_xu: its very easy to support it without placmeent but not polically viable | |
| 14:39:45 | stephenfin | vGPU didn't imply an implicit guest NUMA topology though, that's the key difference here | |
| 14:40:13 | bauzas | looks like we side-tracked the discussion | |
| 14:40:36 | bauzas | if that's about providing a NUMA support in placement, that's something we'll do in Unicorn | |
| 14:40:54 | bauzas | because we understand the concerns | |
| 14:41:09 | alex_xu | stephenfin: the imply an implicit guest NUMA and NUMA affinity are different thing | |
| 14:41:11 | bauzas | my PMs can pay for a nickel if that's the problem | |
| 14:41:28 | sean-k-mooney | alex_xu: not currently | |
| 14:41:34 | sean-k-mooney | form a nova point of view | |
| 14:41:38 | stephenfin | alex_xu: only for VPMEM though | |
| 14:41:43 | sean-k-mooney | but they woudl be with vpmem | |
| 14:42:04 | sean-k-mooney | so that break operators understand of what we men when we say there is an implcit numa toplogy | |
| 14:42:26 | sean-k-mooney | alex_xu: we can crate a numa toplogy as a impmeenation detail | |