| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 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~ | |
| 15:25:14 | sean-k-mooney | dansmith: you prefered to add pmem suport without requireing numa in the inital version corect? | |
| 15:25:29 | sean-k-mooney | dansmith: i could be miss rememebring if so my applogies | |
| 15:26:13 | sean-k-mooney | dansmith: i just rememebr you and i were the most active redhatters in that discuession in the ptg. | |
| 15:26:43 | alex_xu | sean-k-mooney: here is one http://52.27.155.124/95/674895/35/check/pmem-tempest-plugin-filtered/af5fdef/controller/logs/screen-n-cpu.txt.gz | |
| 15:27:06 | alex_xu | sean-k-mooney: search the 'pmem', you will see the xml output by nova-compute log | |
| 15:27:34 | dansmith | sean-k-mooney: I seriously doubt I said that | |
| 15:29:22 | sean-k-mooney | dansmith: in that case my applogies for invoking your name :) | |
| 15:29:34 | sean-k-mooney | alex_xu: so this is the generated domain | |
| 15:29:36 | sean-k-mooney | http://paste.openstack.org/show/774840/ | |
| 15:30:06 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Fix the race in confirm resize func test https://review.opendev.org/681238 | |
| 15:30:13 | gibi | mriedem, bauzas: ^^ | |
| 15:30:58 | sean-k-mooney | alex_xu: it is both pinning ram of the and the core of the guest to float over host numa node 0 | |
| 15:31:23 | alex_xu | sean-k-mooney: yes, but I think the is based on old patchset, since that ci job always pull a old version of luyao's patch | |
| 15:31:34 | alex_xu | in new version, it shouldn't have the pinning | |
| 15:31:35 | sean-k-mooney | alex_xu: but it is not specificy a constrait on pmem device as far as i can see | |
| 15:33:05 | alex_xu | sean-k-mooney: yes, that is the output for patchset13, sorry | |
| 15:33:24 | sean-k-mooney | alex_xu: im not sure that is a good thing. but ok. do you have an updated run | |
| 15:33:29 | sean-k-mooney | ill check the ci logs i guess | |
| 15:33:48 | alex_xu | sean-k-mooney: luyao is trying to get one for you | |
| 15:34:04 | sean-k-mooney | http://52.27.155.124/93/671793/23/check/pmem-tempest-plugin-filtered/23b32fa/ shoudl have them right | |
| 15:34:28 | sean-k-mooney | that from ps 23 | |
| 15:34:49 | sean-k-mooney | i found one | |
| 15:35:03 | alex_xu | sean-k-mooney: no, it won't, the CI always try to apply the PS13, Rui is trying to remove that. That is why you can see other patch also can pass the pmem ci test | |
| 15:37:04 | sean-k-mooney | in the new code will both the numatune and cputune element be removed if only a pmem device is requested in the flavor | |
| 15:39:19 | kashyap | sean-k-mooney: To my eyes this whole NUMA and PMEM interaction looks sutble enough that some more functional testing is required... | |
| 15:41:48 | sean-k-mooney | kashyap: well there are functional tests in https://review.opendev.org/#/c/678470/27 | |
| 15:41:53 | sean-k-mooney | but i have not looked at them yet | |
| 15:42:50 | kashyap | sean-k-mooney: Ah, missed it; thx | |
| 15:43:59 | sean-k-mooney | ... its locking up my firefox windwo trying to open it for some reason | |
| 15:44:16 | bauzas | gibi: https://review.opendev.org/#/c/681238/3 | |
| 15:44:22 | bauzas | gibi: can I update the commit msg ? | |
| 15:44:45 | sean-k-mooney | kashyap: the functional test will not assert anythin about the xml generation | |
| 15:44:46 | gibi | bauzas: sure go ahead | |
| 15:45:55 | kashyap | sean-k-mooney: Sure, I see those are already in the main change. | |
| 15:46:41 | sean-k-mooney | there are no assertion made about numa element in the main change as far as i can tell | |
| 15:46:54 | sean-k-mooney | rather numatune and cputune | |
| 15:48:07 | bauzas | gibi: cool, will do and +2 | |
| 15:48:12 | gibi | bauzas: thanks a lot | |
| 15:48:41 | mriedem | gibi: nits in https://review.opendev.org/#/c/676140/19 for a follow up | |
| 15:48:52 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Fix the race in confirm resize func test https://review.opendev.org/681238 | |
| 15:49:25 | sean-k-mooney | efried_afk: alex_xu ill try to come back to the pmem stuff in an hour or so. i need to review stephens patch then sync with artom | |
| 15:49:52 | gibi | mriedem: ack. thanks | |
| 15:50:07 | sean-k-mooney | stephenfin: you wanted me to review https://review.opendev.org/#/c/671793/ specifcally right. should i also review the rest | |
| 15:50:27 | stephenfin | sean-k-mooney: would be helpful, aye | |
| 15:50:35 | alex_xu | sean-k-mooney: i got one http://paste.openstack.org/show/774842/ | |
| 15:50:37 | stephenfin | I'm marching through the pmem stuff | |
| 15:51:05 | bauzas | but I'll look at your series tomorrow morning | |
| 15:51:13 | sean-k-mooney | alex_xu: looking | |
| 15:51:16 | artom | sean-k-mooney, for once, I think I'm good, and do not require your excellent services :) | |
| 15:52:30 | sean-k-mooney | alex_xu: i think that that is correct. it is creatin a virtual numa toplogy but it is not tiying it to the host in any way | |
| 15:53:06 | sean-k-mooney | artom: the pmem device is also assocaitated with the virtual guest numa node 0 | |
| 15:53:27 | sean-k-mooney | artom: are you working on fixing the persitence issue | |