| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 14:04:31 | efried | dtroyer: idk, does refs/changes/$gerritnum work without /$psnum ? | |
| 14:04:32 | sean-k-mooney | it would fail until its merged but it would work after that | |
| 14:04:36 | mriedem | artom: ok i guess i can buy that second one | |
| 14:04:41 | dtroyer | yeah, the filter needs cleaning up, its pretty open now | |
| 14:04:41 | mriedem | artom: so might as well leave them | |
| 14:05:10 | sean-k-mooney | dtroyer: i dont know if the ci is using the git driver or github dirver | |
| 14:05:13 | efried | dtroyer: I always use git review -d, never git fetch | |
| 14:05:28 | sean-k-mooney | dtroyer: if its using the github dirver then https://review.opendev.org/#/c/681474/2 should work | |
| 14:05:43 | sean-k-mooney | efried: same | |
| 14:06:03 | efried | sean-k-mooney: yeah, unless the CI env doesn't have git-review installed :P | |
| 14:06:16 | dtroyer | mee too… I approved that PR, we can iterate from there | |
| 14:06:18 | sean-k-mooney | efried: no it will work | |
| 14:06:41 | sean-k-mooney | this shoudl all be done via the zuul cloner | |
| 14:06:59 | sean-k-mooney | e.g. zuul should be preparing all the repos before the job starts | |
| 14:07:12 | sean-k-mooney | and then the jobs should use those prepreapred repos | |
| 14:07:27 | sean-k-mooney | so we shoudl not use git fetach or git review in the scripts | |
| 14:08:14 | sean-k-mooney | dtroyer: oh i actully didnt mean for it to be aproved since i think there was an internal one coming but i guess thats fine too | |
| 14:08:19 | luyao | sean-k-mooney: http://paste.openstack.org/show/775129/ | |
| 14:09:01 | dansmith | mriedem: just got off a call so I'll look at that deprecation patch in a minute | |
| 14:09:26 | dtroyer | sean-k-mooney: I think Rui is looking at this in the internal env, yes, I'm not sure where he is on that... | |
| 14:09:33 | dansmith | mriedem: considering how long things are taking to get through the gate, are you comfortable going ahead and letting the bottom three patches there go in or do you want to wait? | |
| 14:10:36 | alex_xu | dtroyer: the mroning, Rui and Dolpher talk with me we can remove the hardcode patch checkout and add a filter. Dolpher will do that when he wakeup I think. Dolpher in the US now | |
| 14:11:03 | stephenfin | luyao, sean-k-mooney: so that's triggering this https://github.com/libvirt/libvirt/blob/master/src/qemu/qemu_domain.c#L11604-L11615 | |
| 14:11:08 | dtroyer | alex_xu: ok, thanks, I'll check with him | |
| 14:11:15 | alex_xu | dtroyer: thanks | |
| 14:12:43 | luyao | sean-k-mooney: that's it! | |
| 14:13:17 | alex_xu | yea...that's it | |
| 14:13:19 | sean-k-mooney | this really isnt a hotplug but i guess they build pmem on that backend | |
| 14:13:43 | luyao | stephenfin: thanks | |
| 14:13:49 | stephenfin | so it looks like we need a guest NUMA topology, meaning we're back to "do we have to treat VPMEM different from hugepages/other things that imply a NUMA topology in nova" | |
| 14:14:04 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Follow up for Ib50b6b02208f5bd2972de8a6f8f685c19745514c https://review.opendev.org/681490 | |
| 14:14:12 | mriedem | dansmith: i've already started +2ing | |
| 14:14:24 | mriedem | just cross referencing logs with the latest passing CI job run | |
| 14:15:04 | stephenfin | dansmith: Morning. Can I get you to weigh in on https://review.opendev.org/#/c/681383 before I merge it back? | |
| 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 | |