Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-10
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?
15:10:17 gibi mriedem: the current patch adds the instance action event waiting for every confirm resize tests in test_servers
15:10:38 gibi mriedem: not just to the bw related confirm resize tests
15:10:55 mriedem yeah i didn't expect to see all of those other changes in tests that aren't doing exotic allocatoins
15:11:05 mriedem i mean, it's fine, and we could just put the helper in ServerMovingTests
15:11:47 gibi this race does not depend on any exotic allocation.
15:12:15 gibi true we only saw the issue in the bw related tests
15:12:51 mriedem putting it in ServerMovingTests is probably not good enough then, since the new tests don't use that
15:12:54 mriedem in PortResourceRequestBasedSchedulingTestBase
15:13:02 mriedem could go in ProviderUsageBaseTestCase though...
15:13:36 mriedem up to you - i just don't love the copy/paste so we should use a helper at some point, fup or whatever - up to you
15:13:48 gibi mriedem: OK. I will respin the patch
15:14:28 openstackgerrit Artom Lifshitz proposed openstack/nova master: NUMA live migration support https://review.opendev.org/634606
15:14:29 openstackgerrit Artom Lifshitz proposed openstack/nova master: Deprecate CONF.workarounds.enable_numa_live_migration https://review.opendev.org/640021
15:14:29 openstackgerrit Artom Lifshitz proposed openstack/nova master: Functional tests for NUMA live migration https://review.opendev.org/672595
15:14:50 bauzas gibi: could you please give me again the race fix change ?
15:14:58 bauzas gibi: can't easily find it
15:15:03 gibi bauzas: https://review.opendev.org/#/c/681238
15:15:08 bauzas gibi: thanks
15:15:34 bauzas ah, that's because the topic name changed :)
15:16:15 sean-k-mooney alex_xu: luyao have you tested the vpmem code with instace that request hugepage or pinning but dont specify a numa toplogy
15:17:23 artom dansmith, mriedem, ^^
15:18:00 bauzas gibi: so you'll respin https://review.opendev.org/#/c/681238 ?
15:18:06 alex_xu sean-k-mooney: I guess luyao have test on that. But that will be a case like normal instance with numa. Since when we specify pinning or hugepage, we will create one node numa for the instance, right?
15:19:01 sean-k-mooney alex_xu: we should but i need to make sure https://review.opendev.org/#/c/678455/25/nova/virt/libvirt/driver.py@4664 does nto break that
15:19:44 sean-k-mooney e.g. and instance with hw:cpu_policy=dedicated + vpmem needs to be pinned
15:19:59 sean-k-mooney and i need to ensure adding not need_pin wont break that
15:20:19 alex_xu sean-k-mooney: https://review.opendev.org/#/c/678455/25/nova/virt/libvirt/driver.py@5454, for that case, the need_pin should be True, since the instance has numa_topology
15:20:31 sean-k-mooney instance.numa_topology shoudl be set in the calim but i need to triple check
15:20:41 alex_xu yea
15:20:53 gibi bauzas: yeah, I will respin soon.
15:21:09 bauzas cool
15:21:22 alex_xu sean-k-mooney: actually we set instance.numa_topology in the API layer I think
15:21:30 sean-k-mooney alex_xu: i think the code is correct but im not sure im happy with this be in a diffreent location then the other places we create a numa toplogy implcitly
15:22:15 alex_xu sean-k-mooney: yea, that is agreement on the PTG, people said they want the vpmem on no numa instance, but I totally understand your point
15:23:01 alex_xu I can't remember who is asking, but it should be redhat people
15:23:46 sean-k-mooney well as a redhat person i did not but i think maybe dansmith prefered supporting this. i kown we said we woudl do that at the PTG however
15:24:16 alex_xu yea, actually I'm on your side in the beginning
15:24:17 sean-k-mooney alex_xu: do you have an xml i can inspect
15:24:42 dansmith sean-k-mooney: um, what?
15:24:44 sean-k-mooney e.g. an exmaple xml that was generated using this code
15:25:11 alex_xu sean-k-mooney: luyao has, but she is on the way home~

Earlier   Later