| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-14 | |||
| 18:55:31 | dansmith | yeah | |
| 18:55:40 | dansmith | I still think we should have two traits though | |
| 18:55:45 | dansmith | "can do this" and "is doing this currently" | |
| 18:56:01 | dansmith | well | |
| 18:56:36 | dansmith | er, yeah.. we want the first for smart scheduling on live migrations, and the second for normal flavor-implies-slower-cheaper-hardware | |
| 18:58:57 | mriedem | i figured "Have the destination look at the service version of the sending node and, if it's older and we're configured for file-backed, reject during pre-check" was for "can do this" | |
| 18:59:24 | dansmith | that is the safety net, if the scheduler is ignorant, or if the operator does a force | |
| 18:59:26 | dansmith | we have to have that | |
| 18:59:34 | mriedem | and the file-backed trait was for "can do it and is configured to do it" | |
| 18:59:36 | dansmith | if we have a can-do-this trait, then we can make the scheduler less ignorant | |
| 18:59:51 | mriedem | yeah i know...but trait explosion | |
| 18:59:52 | dansmith | actually | |
| 19:00:02 | dansmith | I guess after rocky, we don't need the can-do-this trait | |
| 19:00:11 | mriedem | right | |
| 19:00:14 | dansmith | so maybe better to just reno it, put the safety check in because we need it | |
| 19:00:15 | dansmith | yeah, okay | |
| 19:00:16 | dansmith | sorry | |
| 19:00:19 | mriedem | so i think you just need a single trait and the safety valve | |
| 19:00:27 | dansmith | yeah | |
| 19:00:34 | dansmith | zcorneli: you following this? | |
| 19:00:51 | zcorneli | dansmith: Just getting back from lunch, reading through all of it now. | |
| 19:01:06 | dansmith | zcorneli: okay, let me summarize: | |
| 19:01:32 | dansmith | zcorneli: every node has a service record, with a version on it, from a globally monotonic version (objects.service.SERVICE_VERSION) | |
| 19:01:54 | dansmith | zcorneli: the destination node can look up the service record for the sending node, see if it's new enough to be educated about file-backed memory | |
| 19:02:09 | dansmith | zcorneli: if it's not, and the destination is configured for file-backed, then abort the migration | |
| 19:02:36 | dansmith | zcorneli: the rest of the chatter is mostly for later stuff after the initial implementation | |
| 19:05:26 | zcorneli | dansmith: That seems reasonable to me. Is that lookup something that would be reasonable to use from the pre_live_migration method? | |
| 19:05:40 | dansmith | zcorneli: it's a little unconventional, but reasonable yeah | |
| 19:05:58 | dansmith | zcorneli: and you and artom both need to do it for your respective things, so ... it'll be conventional shortly ;) | |
| 19:06:55 | dansmith | zcorneli: https://github.com/openstack/nova/blob/master/nova/objects/service.py#L325-L325 | |
| 19:07:23 | dansmith | zcorneli: srv = objects.Service.get_by_compute_host(ctxt, instance.host); assert(srv.version >= 31) | |
| 19:07:31 | openstackgerrit | Eric Fried proposed openstack/nova master: __str__ methods for RequestGroup, ResourceRequest https://review.openstack.org/568353 | |
| 19:07:37 | dansmith | zcorneli: at the top of that file, bump SERVICE_VERSION and add an entry to the log | |
| 19:08:31 | mriedem | zcorneli: not pre_live_migration, | |
| 19:08:42 | mriedem | check_can_live_migrate_destination | |
| 19:08:45 | dansmith | er, yeah | |
| 19:08:45 | mriedem | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6444 | |
| 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 | mriedem | the nova.objects.fields import in nova.conf.scheduler is killing the test discovery | |
| 19:16:09 | mnaser | import nova.objects.fields inside conf stuff | |
| 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 | dansmith | or virt/hardware | |
| 19:17:18 | mnaser | yeah that's what i was worried about | |
| 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 | 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:25:07 | bauzas | good question ^ | |
| 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? | |