| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 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 | |
| 14:41:00 | luyao | sean-k-mooney : Yes it is, it will be smaller | |
| 14:41:42 | mriedem | artom: ok. the instance.refresh() certainly doesn't hurt so i have no issues with leaving that in | |
| 14:41:52 | dansmith | same, | |
| 14:42:00 | sean-k-mooney | luyao: i normally would have assumed we would handel this by adding the overhaed size and round up to then next aliment boundry like we do for block device but i guess we cant do that as we need to fit in the size in the placemnt allocation | |
| 14:42:02 | dansmith | I had said I wasn't totally sure why, but wasn't surprised it was needed | |
| 14:42:22 | sean-k-mooney | luyao: so you chose to subtract the alignment | |
| 14:42:27 | luyao | sean-k-mooney: the size must be aligned by alignsize | |
| 14:42:31 | mriedem | i think it would be pretty simple to debug after the fact if we have a working job - just dump the source instance.migration_context to logs, call post at dest, then refresh and dump it again to the logs | |
| 14:42:35 | sean-k-mooney | luyao: yes i know | |
| 14:42:43 | sean-k-mooney | im not arguing that point | |
| 14:42:49 | dansmith | mriedem: yeah | |
| 14:42:53 | sean-k-mooney | but that code does not actully guarentee that | |
| 14:43:12 | artom | mriedem, dansmith, ack, so first thing I'll do is put it back in, and while you guys check whether I've addressed the rest of the feedback satisfactorily, I can fix the func tests for not picking that up, and bet a better handle on why it's needed in the first place | |
| 14:43:40 | luyao | sean-k-mooney: let me check it again | |
| 14:43:48 | dansmith | artom: ack | |
| 14:44:02 | sean-k-mooney | luyao: im refering to https://review.opendev.org/#/c/678455/24/nova/virt/libvirt/config.py@3180 by the way | |
| 14:45:12 | sean-k-mooney | luyao: oh you are relying on the plamcent step size to prevent non aligned allocations | |
| 14:46:02 | luyao | sean-k-mooney: sorry, I don't understand | |