| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 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 | |
| 14:42:53 | sean-k-mooney | but we should never telll people we are because they will assume it works the same way as the other cases we do that | |
| 14:44:40 | sean-k-mooney | alex_xu: just to clarify we use teh terms implcit and explcit numa topoyg in downstream docs and in bug reports and customer expect an implict numa toplogy to be the same as and expclti one. that is the only reason i bring up docs/release notes | |
| 14:45:23 | mriedem | here is an idea, defer vpmems since you're still arguing about designs 1 day before FF and just sort it out in Ussuri :) | |