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