| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-10 | |||
| 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 | |
| 15:54:05 | sean-k-mooney | artom: i jsut wanted to circle back and see if you had anything form me to test or if i should start looking into where to fix the issue | |
| 15:54:22 | artom | sean-k-mooney, I added instance.refresh() back it, so that's settled | |
| 15:54:31 | alex_xu | sean-k-mooney: but I think this is wrong https://review.opendev.org/#/c/678455/25/nova/virt/libvirt/driver.py@5458 | |
| 15:54:42 | sean-k-mooney | artom: ok did you push that? | |
| 15:54:51 | artom | The func tests weren't hitting it because driver.cleanup() is called conditionally, and the func test env isn't meeting those conditions | |
| 15:54:53 | artom | sean-k-mooney, I did push | |
| 15:55:03 | alex_xu | sean-k-mooney: I checked the nova show, I saw there is "hw:numa_nodes" being added, so I guess that is persistented in the db | |
| 15:55:06 | sean-k-mooney | ok then ill check it locally unless you have already | |
| 15:55:09 | artom | If I just change the code to always call it, I can reproduce | |
| 15:55:19 | sean-k-mooney | artom: that is not correct | |
| 15:55:20 | artom | And instance.refresh() does indeed fix it | |
| 15:55:27 | artom | sean-k-mooney, I know :) It was just to test | |
| 15:55:33 | mriedem | stephenfin: did you ever re-post your PCPU upgrade ML thread with [nova] tagged on it to actually get operator visibility? | |
| 15:55:42 | artom | Next step is to trigger driver.cleanup in the "real" way in func tests | |
| 15:55:48 | sean-k-mooney | we should not see hw:numa_nodes in nova show | |
| 15:55:59 | alex_xu | sean-k-mooney: yes, we shouldn't | |
| 15:56:01 | sean-k-mooney | artom: sorry that was for allex | |
| 15:56:12 | sean-k-mooney | artom: i have not looked at your change | |
| 15:56:19 | stephenfin | mriedem: No, it didn't seem necessary since we'd solved the upgrade issue in a way that didn't require anything special from the operator | |
| 15:56:28 | stephenfin | outside of bog standard config options | |
| 15:56:34 | sean-k-mooney | alex_xu: we should not see hw:numa_nodes=1 if its not in the flavor | |
| 15:56:51 | sean-k-mooney | alex_xu: we do not see that when you get implcit numa toplogyies in other cases | |
| 15:57:02 | sean-k-mooney | alex_xu: so if you are seeing it then the code is incorrect | |
| 15:57:08 | artom | sean-k-mooney, we're good, don't worry :) | |
| 15:57:17 | mriedem | stephenfin: isn't dansmith's comment all about a nasty upgrade problem? | |
| 15:57:26 | alex_xu | sean-k-mooney: I think the problem is the patch is change instance.flavor direclty, after driver.spawn, the nova-comptue update the instance object, then persistent it into the db. | |
| 15:57:28 | mriedem | to which operators, like mnaser, might want to weigh in? | |
| 15:57:32 | sean-k-mooney | alex_xu: if we were to cold migrate the instance the behavior sould chagne if we save it to the db | |
| 15:57:47 | mnaser | hm | |
| 15:57:53 | dansmith | mriedem: yeah, the "plan" doesn't seem super great to me as currently laid out :/ | |
| 15:57:59 | sean-k-mooney | alex_xu: ya we shoudl not be changeing tehe flavor at all | |
| 15:58:13 | stephenfin | mriedem: an intractable one though. Even if operators don't like the little dance we're doing, I fail to see how there's an alternative | |
| 15:58:14 | mriedem | mnaser: you'd need someone to tl;dr it (i would also) | |
| 15:58:15 | sean-k-mooney | alex_xu: the other case dont chagne the flaovr they just create a numa toplogy | |
| 15:58:21 | mriedem | since there are 5 conversations going on at once in here right now | |
| 15:58:23 | mnaser | sounds like an ML post? | |
| 15:58:28 | mnaser | that i can read | |
| 15:58:37 | mriedem | mnaser: there was one which no operators read :) | |
| 15:58:43 | mriedem | b/c it was'nt tagged for [nova] or [ops] | |
| 15:58:43 | sean-k-mooney | so the pmem code is tacking a shortcut by updating the flavor. on a hard reboot that instance would be pinned | |
| 15:59:07 | stephenfin | sean-k-mooney: I'm looking at that at the moment. I don't like it. | |
| 15:59:09 | stephenfin | Not at all | |
| 15:59:31 | stephenfin | Assuming you're referring to https://review.opendev.org/#/c/678455/25/nova/virt/libvirt/driver.py@5458 | |
| 15:59:33 | mriedem | stephenfin: for the sake of everyone's clarity, could you post a new ML thread with the proposed upgrade path for PCPU and tag with [nova] and [ops]? | |
| 15:59:36 | sean-k-mooney | stephenfin: if we need to create a numa toplogy we should move it to where we do it for hugepages right? | |
| 15:59:56 | sean-k-mooney | stephenfin: yes | |
| 16:00:03 | sean-k-mooney | stephenfin: that is the hack that i dont like | |
| 16:00:05 | stephenfin | sean-k-mooney: exactly what I'm writing in a comment as we speak | |
| 16:00:19 | stephenfin | what is it with people trying to hack flavors :D | |
| 16:00:22 | stephenfin | mriedem: sure | |
| 16:00:35 | stephenfin | though I really don't see the point | |
| 16:00:42 | dansmith | stephenfin: "intractable" is a bit of a silly characterization :) | |
| 16:00:43 | alex_xu | sean-k-mooney: yea, agree with you | |
| 16:00:50 | stephenfin | because the only people that can solve this are in this channel/on the review already | |
| 16:01:42 | sean-k-mooney | stephenfin: the issue is that we want a numa topology in the xml. but not in the numa toplogy filter | |
| 16:01:55 | stephenfin | dansmith: Possibly :) I have been thinking about this for quite some time though and we've gone through a lot of options, so it starts looking like that to me, heh | |
| 16:03:16 | sean-k-mooney | alex_xu: since stephenfin is looking at it im gong to review his cpu code then ill come back to this after i test artoms code | |
| 16:03:19 | mriedem | stephenfin: saying "because the only people that can solve this are in this channel/on the review already" is not true imo - if you've got a hard upgrade thing coming for operators, you likely should get some feedback from them before pushing forward | |
| 16:03:28 | alex_xu | sean-k-mooney: thanks a lot | |
| 16:04:07 | sean-k-mooney | mriedem: the upgrade will be signifcantly harder if we also have to deal with numa in placment in the same release | |
| 16:04:26 | mriedem | sean-k-mooney: i don't know what that has to do with this at all | |
| 16:04:34 | sean-k-mooney | mriedem: if we defer pcpus in placment to U we will have to deal with both in one go | |
| 16:04:55 | mriedem | i didn't say anything about deferring | |
| 16:05:33 | mriedem | i said, does anyone outside of the 3 people reviewing this that will actually have to deal with the upgrade know what the plan is | |
| 16:05:36 | mriedem | and are they ok with it | |
| 16:05:39 | sean-k-mooney | right but the current upgrade approch is the best we could come up with and we went to the MLs and asked if the toggel was ok | |
| 16:05:54 | mriedem | and no operators even saw that thread, | |
| 16:06:06 | mriedem | which is why i asked (again) if it could be posed with a [nova][ops] tag | |
| 16:06:08 | mriedem | to get visibility | |
| 16:06:18 | mriedem | lack of feedback from operators is not agreement | |