| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 14:06:53 | alex_xu | also the vpmem CI is pretty close https://review.opendev.org/#/c/679640/14 \o/ | |
| 14:06:54 | kashyap | efried: vPMEM? | |
| 14:07:22 | efried | kashyap: Yeah, that patch is just "do shit efried doesn't understand to the libvirt xml" | |
| 14:07:32 | efried | so I don't think you need to understand vpmem | |
| 14:07:45 | sean-k-mooney | efried: the pmem seriese. i can take a look if kashyap cant | |
| 14:07:48 | kashyap | efried: It's been forever on my to-read list, but I was pulled into urgent downstream stuff last/this week | |
| 14:07:59 | bauzas | efried: once I'm done with gibi's series and stephenfin's cpu-resources, I could give some look at https://review.opendev.org/#/q/topic:bp/virtual-persistent-memory | |
| 14:08:44 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fixing broken links https://review.opendev.org/681206 | |
| 14:09:09 | efried | Thanks folks. bauzas kashyap sean-k-mooney in the interest of focusing resources, if it's possible to start with https://review.opendev.org/#/c/678455/ rather than going through the whole series, that's the one where we have a known reviewer gap. | |
| 14:09:33 | alex_xu | bauzas: kashyap sean-k-mooney thanks also | |
| 14:09:53 | bauzas | efried: honestly, reviewing the series needs me looking from the start, but I can surely just look at the patches without really commenting them | |
| 14:10:05 | sean-k-mooney | yes if that is the one that need to be reviewed i can start there in about 5 mins | |
| 14:10:08 | kashyap | efried: Nod; that's what I clicked on | |
| 14:10:55 | gibi | stephenfin: a question about the cpu series. The vcpu_pins_set can be and empty string and the nova will use every host cpu for the vcpus. Does this behaviour preserved for the cpu_shared_set and cpu_dedicated_set ? | |
| 14:11:52 | efried | gibi: iiuc no, you'll get an error if you try that. | |
| 14:12:04 | bauzas | gibi: good question, we haven't agreed on that in the spec | |
| 14:13:20 | gibi | efried: so I can assume that when the vcpu_pin_set is removed then there will be no way to implicitly offer every host cpu as vcpu? But I have to explicitly list them in one of the new configs | |
| 14:13:50 | stephenfin | gibi: It's preserved, yes | |
| 14:14:10 | bauzas | stephenfin: only if you don't use the new options, right? | |
| 14:14:29 | bauzas | stephenfin: because if you start messing with your options, then something will come up :) | |
| 14:14:39 | stephenfin | any(vcpu_pin_set, compute.cpu_shared_set, compute.cpu_dedicated_set) == False --> allocate all host CPUs for PCPU inventory | |
| 14:14:43 | stephenfin | bauzas: correct | |
| 14:14:56 | sean-k-mooney | whats the status on the SEV series by the way. stephen did you get a chance to look at the bottom patch? i think that was all that was left to be +w and the rest were ready to go? | |
| 14:15:00 | efried | Right, I guess we don't have to make the decision about what gibi is asking until a future release. | |
| 14:15:06 | efried | sean-k-mooney: it's gateward | |
| 14:15:08 | stephenfin | sean-k-mooney: It's gone through | |
| 14:15:19 | stephenfin | going, rather | |
| 14:15:21 | sean-k-mooney | ok so i can unload the SEV context form my brain | |
| 14:15:32 | sean-k-mooney | at least for a while thats good | |
| 14:15:35 | stephenfin | burn it. burn it all | |
| 14:15:51 | stephenfin | at least until people start using it in two years or so | |
| 14:15:52 | gibi | efried: sure, the part of what happens when we remove the vcpu_pin_set is for the future not for Train | |
| 14:16:00 | gibi | stephenfin: thanks | |
| 14:16:23 | stephenfin | gibi, efried: fwiw, when we remove 'vcpu_pin_set' I'd like to preserve the same behavior | |
| 14:16:30 | stephenfin | though sean-k-mooney disagrees | |
| 14:16:45 | stephenfin | in U or V or whatever, the above simply becomes | |
| 14:16:53 | stephenfin | any(compute.cpu_shared_set, compute.cpu_dedicated_set) == False --> allocate all host CPUs for PCPU inventory | |
| 14:17:00 | gibi | stephenfin: I have conflicting requirements internally what empty vcpu_pin_set "should" mean | |
| 14:17:05 | stephenfin | damn, sorry, VCPU | |
| 14:17:07 | stephenfin | in both cases | |
| 14:17:20 | gibi | stephenfin: yeah, VCPU. even beter | |
| 14:17:36 | efried | alex_xu: even if not yet +2able, do you see https://review.opendev.org/#/c/671800/ being generally sane, enough to unblock the bottom and start merging cpu-resources patches? | |
| 14:17:39 | stephenfin | yeah, my mistake | |
| 14:18:18 | alex_xu | efried: yes, I think it is good. the case I tested passed | |
| 14:18:32 | efried | stephenfin: any reservations about that ^ ? | |
| 14:19:30 | stephenfin | efried: Nope. I need to fix a thing with quotas and discuss whether we need the scheduler option with dansmith ([1]) but neither of those should affect anything [1] https://review.opendev.org/#/c/671801 | |
| 14:19:42 | stephenfin | *anything lower in the series | |
| 14:20:16 | stephenfin | sean-k-mooney: Want to skim through https://review.opendev.org/#/c/671793/ again now that the fixes for your issues are merged directly in? | |
| 14:20:49 | sean-k-mooney | am ill add it to the queue after the vpmem patch but sure | |
| 14:20:56 | efried | bauzas: would you please re+2 and +W the bottom cpu-resources https://review.opendev.org/#/c/671793/ once sean-k-mooney has done that ^ ? | |
| 14:25:23 | openstackgerrit | Luyao Zhong proposed openstack/nova master: Claim resources in resource tracker https://review.opendev.org/678452 | |
| 14:25:24 | openstackgerrit | Luyao Zhong proposed openstack/nova master: libvirt: Enable driver discovering PMEM namespaces https://review.opendev.org/678453 | |
| 14:25:24 | openstackgerrit | Luyao Zhong proposed openstack/nova master: libvirt: report VPMEM resources by provider tree https://review.opendev.org/678454 | |
| 14:25:25 | openstackgerrit | Luyao Zhong proposed openstack/nova master: libvirt: Support VM creation with vpmems and vpmems cleanup https://review.opendev.org/678455 | |
| 14:25:25 | openstackgerrit | Luyao Zhong proposed openstack/nova master: Parse vpmem related flavor extra spec https://review.opendev.org/678456 | |
| 14:25:26 | openstackgerrit | Luyao Zhong proposed openstack/nova master: libvirt: Enable driver configuring PMEM namespaces https://review.opendev.org/679640 | |
| 14:25:26 | openstackgerrit | Luyao Zhong proposed openstack/nova master: Add functional tests for virtual persistent memory https://review.opendev.org/678470 | |
| 14:27:09 | artom | sean-k-mooney, thanks for testing, I saw your comments while waiting at the doctor's office. I'm going to try and fix the func tests, and then get a better handle on why instance.refresh() makes it work | |
| 14:27:25 | sean-k-mooney | artom: well it does not | |
| 14:27:36 | artom | sean-k-mooney, hah, so it never worked? | |
| 14:27:37 | sean-k-mooney | at least adding it before apply does not work | |
| 14:27:51 | sean-k-mooney | im gong to try adding after drop migration context | |
| 14:27:55 | artom | sean-k-mooney, no, it was on the source, before calling driver.cleanup() | |
| 14:28:11 | sean-k-mooney | oh i thought it was in post live migrate | |
| 14:28:15 | artom | The theory was that driver.cleanup(), at least in libvirt, calls instance.save() and clobbers that the dest saved | |
| 14:28:31 | sean-k-mooney | oh ok | |
| 14:28:36 | kashyap | efried: Ah, nice - just noticed that Luyao also wrote the upstream libvirt-proper part of the 'pmem' / related NVDIMM support bits :-). (/me needs more time, it's a massive context-switch for my slow brain.) | |
| 14:28:47 | sean-k-mooney | ill look at the old code and try and figure out where to add it | |
| 14:28:55 | sean-k-mooney | if you know then i can test it | |
| 14:29:05 | sean-k-mooney | or feel free to updated it on the host i gave you acess too | |
| 14:29:07 | efried | kashyap: that sounds impressive | |
| 14:29:10 | sean-k-mooney | whichever works | |
| 14:29:15 | artom | mriedem, ^^ ... would we be OK using instance.refresh() without a clear handle on why it's needed? I'm assuming not | |
| 14:31:03 | sean-k-mooney | efried: https://review.opendev.org/#/c/678455/24/nova/virt/libvirt/config.py is the primary file you want our input on yes | |
| 14:32:04 | efried | sean-k-mooney: yeah, I can at least somewhat understand the others. | |
| 14:32:33 | sean-k-mooney | i have looked at the other and assming this file looks sane those do too | |
| 14:32:53 | sean-k-mooney | im almost done but sofar nothing jumps out as me as obviously wrong | |
| 14:33:24 | efried | luyao: https://review.opendev.org/#/c/678456/ quick fix here please | |
| 14:34:02 | luyao | efried : ok | |
| 14:35:09 | mriedem | artom: i thought sean-k-mooney said it was needed | |
| 14:35:42 | sean-k-mooney | mriedem: i said it does not work without it. i have not tested readding instance.refresh in the correct place | |
| 14:35:58 | mriedem | artom: like i said in review, my guess is the copy of the instance on the source has a dirty migration_context field, the applies it's copy and drops it, and then the source does an instance.save and saves the dirty copy | |
| 14:36:26 | sean-k-mooney | the current code is setting the correct numa toplogy in the migration context but its not makeing it into the numa_toplogy field int eh instance_extra table | |
| 14:36:35 | openstackgerrit | Luyao Zhong proposed openstack/nova master: Parse vpmem related flavor extra spec https://review.opendev.org/678456 | |
| 14:36:36 | openstackgerrit | Luyao Zhong proposed openstack/nova master: libvirt: Enable driver configuring PMEM namespaces https://review.opendev.org/679640 | |
| 14:36:36 | openstackgerrit | Luyao Zhong proposed openstack/nova master: Add functional tests for virtual persistent memory https://review.opendev.org/678470 | |
| 14:36:52 | sean-k-mooney | luyao: efried alex_xu where are ye subtracting the alingment form the requested size | |
| 14:37:02 | sean-k-mooney | self.target_size = kwargs.get("size_kb", 0) - self.align_size | |
| 14:37:03 | artom | mriedem, it *is* needed, sean-k-mooney confirmed this morning | |
| 14:37:21 | artom | But I don't have a good understanding of why | |
| 14:37:22 | sean-k-mooney | luyao: efried alex_xu https://review.opendev.org/#/c/678455/24/nova/virt/libvirt/config.py@3180 | |
| 14:37:30 | artom | Err, brb, my client's all wonky | |
| 14:39:40 | luyao | efried, sean-k-mooney: because the label will occupy some space, and the the size must be aligned | |
| 14:40:08 | sean-k-mooney | luyao: sure but jsut substracting the alignment is not correct | |
| 14:40:25 | sean-k-mooney | it will result in a smaller allcotion then you asked for.well | |
| 14:40:35 | sean-k-mooney | there are two ways to handel it | |
| 14:40:39 | sean-k-mooney | if we know the lable size | |
| 14:40:41 | artom | mriedem, btw, I removed all the allocation-style pinning/non-NUMA stuff that we talked about last night. You and sean-k-mooney convinced me it was 1. tangential (long-standing issue not related to intance NUMA topology, fixed with a hard reboot) 2. scope creep that's too risky at this point. So I pushed a PS last night with out it | |
| 14:40:51 | artom | mriedem, so, in my mind, all that's left is the instance.refresh() thing | |
| 14:40:58 | sean-k-mooney | we can add that then round up to the next size or we had do what your doing | |