Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-10
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
14:47:50 luyao sean-k-mooney: the target size is aligned by alignsize, this is guaruanteed by creating namepsace, then I subtract the alignment, it will be aligned by alignsize also.
14:48:26 luyao sean-k-mooney: what is plamcent step size
14:48:29 sean-k-mooney yes but this code is relying on other code makeing sure that size_kb is a multiple of the aliment size
14:48:40 sean-k-mooney it is nto actully checkign for that here its just assuming it
14:49:08 sean-k-mooney which is why this normally would not guarentee its a still alinged but in this case it will be
14:50:41 sean-k-mooney luyao: ignore the placment step size we decided to model it diffrently
14:51:05 luyao sean-k-mooney: the size is return from ndctl utility, ndctl will guarentee the size is aligned. the alignsize is also from ndctl
14:51:14 sean-k-mooney we had two proposals this is why it would have been better for me to review all of the series rather then jsut this patch
14:51:31 sean-k-mooney ok
14:51:43 efried luyao: FYI: step_size is the granularity with which you're allowed to make an allocation for a resource.
14:51:43 efried So if you have 64 units of something, and your step_size is 8, you're allowed to allocate 8, 16, 24... but not 9 or 3 or 14.
14:52:21 sean-k-mooney efried: ya so in the current propoal we are tracking invenoties of namespace size
14:52:29 sean-k-mooney so i think its 1
14:52:42 sean-k-mooney but we had debated modeling it differently at one point
14:53:13 sean-k-mooney in its current form its not relevent. but i need to remind my self of some of teh other decisions
14:54:34 luyao efried,sean-k-mooney: thanks, I see. it has no step_size, it is not partable
14:54:58 sean-k-mooney luyao: well its set to 1
14:55:13 efried Yes, I remember this now. 1 is appropriate, because they're single namespaces
14:55:18 sean-k-mooney so you get pmem in units of 1 namespace
14:55:24 luyao sean-k-mooney: yes
14:55:24 efried ^
14:55:46 efried and the size/align shouldn't actually leak into any kind of placement, scheduling, etc. code.
14:55:56 efried If it's anywhere at all, it would be deep in the libvirt-isms.
14:56:13 efried because as far as nova+placement are concerned, a namespace is a single unit.
14:56:40 sean-k-mooney efried: at one point we had discussed the idea of dynamically creating the namespces but we regjected that
14:57:06 sean-k-mooney when we were considring it the alinment was one of the posible values for the stepsize i belive
14:57:13 sean-k-mooney anyway that is not relevent
14:57:18 efried right
14:57:41 openstackgerrit Balazs Gibizer proposed openstack/nova master: Fix the race in confirm resize func test https://review.opendev.org/681238
14:57:50 gibi mriedem: ^^
14:59:12 efried dansmith, stephenfin: FYI on the pmem CI, the reason it's passing on patches outside of the pmem series is because currently the job is configured to pull down the top patch (https://review.opendev.org/#/c/678470) before running.
14:59:12 efried I'm still digging into what the tests are actually doing.
14:59:41 gibi mriedem: it seems that the top of the bw series hits the race so I'm wondering what to do. Rebase the series to inclued the fix in the bottom, or wait for the grenade test result first as gate is slow
14:59:57 dansmith efried: are you talking about vpmem or PCPU?
15:00:39 efried dansmith: vpmem
15:01:18 efried bbiab
15:03:02 mriedem gibi: a nit inline on that race test fix
15:03:36 mriedem gibi: i think we'll just get the race fix merged today and then it's the same as you rebasing your series on it
15:04:01 gibi mriedem: OK. will put back the lower() call.
15:05:18 gibi mriedem: ahh good point, the gate will rebase the series for the test anyhow
15:07:44 mriedem gibi: left another comment, don't know if you want to fup again or not,
15:08:06 mriedem but since you changed all of these copy/paste blocks of confirmResize/wait for status/wait for event, those could go into a single helper method
15:09:17 gibi mriedem: OK I will put that in a helper. Do you suggest to only change the new confirm resize tests?
15:09:38 mriedem i'm not sure what you mean by 'new confirm resize tests'
15:10:08 mriedem like the new bw provider migration tests?

Earlier   Later