Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-10
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: report VPMEM resources by provider tree https://review.opendev.org/678454
14:25:24 openstackgerrit Luyao Zhong proposed openstack/nova master: libvirt: Enable driver discovering PMEM namespaces https://review.opendev.org/678453
14:25:25 openstackgerrit Luyao Zhong proposed openstack/nova master: Parse vpmem related flavor extra spec https://review.opendev.org/678456
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:26 openstackgerrit Luyao Zhong proposed openstack/nova master: Add functional tests for virtual persistent memory https://review.opendev.org/678470
14:25:26 openstackgerrit Luyao Zhong proposed openstack/nova master: libvirt: Enable driver configuring PMEM namespaces https://review.opendev.org/679640
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: Add functional tests for virtual persistent memory https://review.opendev.org/678470
14:36:36 openstackgerrit Luyao Zhong proposed openstack/nova master: libvirt: Enable driver configuring PMEM namespaces https://review.opendev.org/679640
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,

Earlier   Later