| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-14 | |||
| 19:08:31 | mriedem | zcorneli: not pre_live_migration, | |
| 19:08:42 | mriedem | check_can_live_migrate_destination | |
| 19:08:45 | mriedem | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6444 | |
| 19:08:45 | dansmith | er, yeah | |
| 19:09:05 | mriedem | src_compute_info is a ComputeNode object (i think) that will have the host and node name to lookup the service versoin | |
| 19:09:34 | dansmith | yeah, either way | |
| 19:10:01 | mriedem | oh right instance.host/node would also have that, right | |
| 19:10:20 | dansmith | and, compute host == service, we don't need to care about the node | |
| 19:10:35 | mriedem | sure for libvirt we don't | |
| 19:10:57 | dansmith | we wouldn't for ironic either since the service version is on the compute host, but anyway | |
| 19:11:04 | mriedem | true | |
| 19:11:19 | mriedem | zcorneli: also, this is why supporting rolling upgrades is hard, in case you were wondering | |
| 19:11:20 | mriedem | :) | |
| 19:11:32 | dansmith | heh | |
| 19:11:53 | zcorneli | mriedem: Yea, can definitely see that. | |
| 19:14:31 | zcorneli | dansmith: mriedem: If I'm following this right, I'll increment the service version as part of the patch, and check within check_can_live_migrate_destination that if file_backed_memory is enabled, and the source service version is < the new service version, fail. | |
| 19:14:47 | dansmith | yup | |
| 19:15:03 | zcorneli | Sounds pretty reasonable. Seems like a good way to handle things. | |
| 19:15:12 | mriedem | mnaser: your patch is bombing out on unit tests hard | |
| 19:15:26 | mnaser | mriedem: oh thats weird i didnt really touch that | |
| 19:15:28 | mriedem | http://logs.openstack.org/25/566425/3/check/openstack-tox-py27/f1bc5f0/job-output.txt.gz#_2018-05-14_18_53_55_549849 | |
| 19:15:52 | dansmith | mnaser: not just failing, bombing.. not just bombing, but bombing *hard*. | |
| 19:15:55 | mnaser | i had a feeling | |
| 19:15:58 | mnaser | that was gonna happen | |
| 19:16:09 | mnaser | import nova.objects.fields inside conf stuff | |
| 19:16:09 | mriedem | the nova.objects.fields import in nova.conf.scheduler is killing the test discovery | |
| 19:16:27 | mriedem | circular import somewhere probably | |
| 19:16:50 | mnaser | yeah.. | |
| 19:16:57 | dansmith | since config is so early, | |
| 19:17:07 | dansmith | you might have to move those arch definitions somewhere neutral | |
| 19:17:14 | dansmith | like Switzerland | |
| 19:17:18 | mnaser | yeah that's what i was worried about | |
| 19:17:18 | dansmith | or virt/hardware | |
| 19:17:21 | mnaser | lol | |
| 19:17:26 | mriedem | nazi arch list? | |
| 19:17:31 | dansmith | mnaser: still, very worthwhile to get the choices= coverage I think | |
| 19:17:43 | mnaser | who's okay with approving a change doing that in the same patch | |
| 19:18:11 | dansmith | I would do it separately but I don't care that much | |
| 19:18:23 | mnaser | nova/virt/hardware imports conf too | |
| 19:18:42 | dansmith | virt/arch.py then | |
| 19:18:58 | mnaser | it should probably be its own patch because i'll probably have to change all the references | |
| 19:19:24 | dansmith | mnaser: you get paid by the patch so two patches is more better right? | |
| 19:20:23 | dansmith | mnaser: If I was my own boss, I'd make up all kinds of silly metrics upon which to base my compensation | |
| 19:20:25 | mnaser | dansmith, mriedem: ideally i'd like to have this to be backportable to queens at least... because it prevents us from multiarch.. how about i do a patch without choices= and then follow up that moves stuff to virt/arch then adding the choices in a third patch, allowing us to have 1 backportable patch that doesnt break the world? | |
| 19:20:37 | mnaser | dansmith: but i have to be in the top 10 in stackalytics to get my bonus | |
| 19:20:58 | dansmith | mnaser: I'd rather it be one patch than go in (and especially be backported) without choices= | |
| 19:21:25 | mnaser | (i honestly had a candidate that said they couldn't change their email because "their marketing department at old company" needed to show off cores upstream) | |
| 19:21:29 | dansmith | if you have a config you backport that has no restrictions and then introduce a restriction, you broke compat | |
| 19:21:55 | mnaser | as you can imagine that was eye opening.. | |
| 19:22:29 | mnaser | dansmith: ok i see, i guess i'll throw the dice and hope for mercy by the stable maint gods | |
| 19:23:01 | dansmith | mnaser: mriedem is a stable maint god | |
| 19:23:09 | dansmith | and I'm a stable maint peasant | |
| 19:23:23 | dansmith | so do whatever he says, but I think he'll be on the same page :) | |
| 19:24:13 | bauzas | jaypipes: around ? | |
| 19:25:02 | bauzas | jaypipes: about https://review.openstack.org/#/c/557065/ | |
| 19:25:07 | bauzas | good question ^ | |
| 19:25:07 | mriedem | mnaser: agree with dan, single change with the config option added with choices is best, since it defaults to None it should be ok | |
| 19:27:30 | jaypipes | bauzas: yes? | |
| 19:27:49 | artom | I think with NUMA it's a bit different - in the sense that an instance can switch from file-backed to non-file backed, and the user might never now (latencies notwithstanding) | |
| 19:27:59 | bauzas | jaypipes: so, about your point, maybe we should have some aliases for PCI devices ? | |
| 19:28:20 | bauzas | like we did for PCI passthrought ? | |
| 19:29:01 | artom | For NUMA... an instance *has* a NUMA topology, and we can't get rid of it, so if both source and dest can't handle live migrating the instance NUMA topology, so to speak, it's fair to bail out | |
| 19:29:11 | bauzas | jaypipes: and then the compute would pass custom traits using those aliases ? | |
| 19:29:53 | bauzas | like CUSTOM_GPU_M608Q | |
| 19:32:06 | bauzas | mriedem: heh, btw. I just paid for a Huawei P20 Pro. Hope you'll get some money thanks to me :p | |
| 19:34:59 | zcorneli | dansmith: mriedem: Looking at check_can_live_migrate_destination, there doesn't look like anything in there actually fails out a migration, just collects data for the source to decide on. | |
| 19:35:34 | jaypipes | bauzas: why? | |
| 19:36:12 | jaypipes | bauzas: I mean, why would we create such a custom trait? | |
| 19:36:23 | jaypipes | bauzas: it doesn't mean anything. | |
| 19:36:30 | jaypipes | bauzas: it's just a vendor ID. | |
| 19:36:45 | mriedem | bauzas: i won't hold my breath | |
| 19:36:58 | jaypipes | bauzas: we should be putting the standard GPU traits in the flavor | |
| 19:37:48 | bauzas | jaypipes: maybe, I'm not specifically torn on that | |
| 19:38:09 | mriedem | zcorneli: you have to raise MigrationPreCheckError | |
| 19:38:16 | bauzas | jaypipes: tbc, the spec is for passing multiple vGPU types, but not on *asking* for a specific type | |
| 19:38:22 | artom | Unrelated logging question - if a method isn't passed a context, will LOG.debug still print the request ID? | |
| 19:38:35 | mriedem | zcorneli: this is an rpc call to the dest compute https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L222 | |
| 19:38:40 | bauzas | jaypipes: for that, I'd love to see Xen folks | |
| 19:38:50 | artom | I thought it did some black magic with the context, so if you're not passing it to a method, any LOG calls from within that method won't have the request ID | |
| 19:39:03 | mriedem | zcorneli: if we fail with MigrationPreCheckError we'll handle it here https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L337 and continue looking for another valid destination host | |
| 19:39:15 | bauzas | jaypipes: my point is that, for the moment, operators wanting to provide flavors for a specific type could just use placement-client and use custom traits, right? | |
| 19:39:19 | mriedem | artom: yes | |
| 19:39:31 | mriedem | artom: the context is stored in memory | |
| 19:39:42 | mriedem | which causes all sorts of confusion wrt the request id | |
| 19:39:42 | bauzas | jaypipes: so, the question you have is more about how nova could use standard traits, then | |
| 19:39:47 | jaypipes | bauzas: why would they do that, though? | |
| 19:40:00 | bauzas | jaypipes: why what ? using custom traits ? | |
| 19:40:00 | mriedem | artom: https://github.com/openstack/oslo.context/blob/master/oslo_context/context.py#L40 | |
| 19:40:15 | bauzas | jaypipes: because custom traits already exist | |
| 19:40:25 | jaypipes | bauzas: the user wants a vGPU that has certain capabilities (resolution, max display heads, support for OpenCL, etc) | |
| 19:40:39 | bauzas | jaypipes: sure, I understand your point | |
| 19:40:45 | jaypipes | bauzas: the user doesn't want a specific M60-8Q Nvidia vGPU "type". | |
| 19:40:56 | artom | mriedem, so there's a global context per thread? | |
| 19:40:56 | bauzas | jaypipes: I don't disagree with that :) | |
| 19:41:06 | bauzas | jaypipes: tbh, I'm fine with that | |
| 19:41:36 | jaypipes | bauzas: when we discussed this with the Xen folks on the original VGPU spec, jianghuaw_ and I specifically agreed that vgpu "types" (which are nothing more than vendor jargon) would *not* be used as traits. | |
| 19:41:43 | bauzas | jaypipes: my plan was about passing aliases, but for sure, we could have other conf opts for saying : "this trait = that PCI id" | |
| 19:42:20 | bauzas | jaypipes: but like I said in my gerrit reply, I don't think it's related to the spec I wrote, maybe in a separate spec ? | |
| 19:42:37 | zcorneli | mriedem: Ah, ok, needed to look up one more layer. Thanks! | |
| 19:42:40 | bauzas | or we could opt something like "this PCI id = those traits" | |