| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-14 | |||
| 18:47:39 | dansmith | right, but that's not what we need for live migration | |
| 18:48:03 | dansmith | you can migrate to/from a host with file-backed even if you don't, but only if you are new enough to *know* about it and tweak the new xml properly | |
| 18:48:09 | mriedem | not sure i'm following; if the instance is on a source host with the file-backed-memory trait, we need to find a dest host with that same trait set right? | |
| 18:48:15 | dansmith | no | |
| 18:48:26 | dansmith | you can migrate from file-backed to non-file-backed, and vice versa | |
| 18:48:38 | dansmith | but you can't migrate from an older node that doesn't know anything about this, | |
| 18:48:56 | dansmith | because it will generate non-filed-backed libvirt xml and send it to the dest, forcing it to be memory-backed even if that host is configured for file-backed | |
| 18:49:17 | dansmith | it's really kindof a dumb thing about how we do live migrations | |
| 18:49:31 | dansmith | we foist our desired xml on the destination even if it violates something about how it is configured | |
| 18:50:26 | dansmith | I can explain this quickly in a hangout if it'd be easier | |
| 18:50:32 | mriedem | if the instance is on an old source compute that doesn't report the file-backed trait, i'm not sure why we wouldn't just say filter out all dest hosts that also don't have that trait; i understand you can go back and forth if the computes are new enough to know if they have this or not, so maybe we need a compute rpc version check in here too? | |
| 18:50:59 | jaypipes | bauzas: k, reviewed the vgpu type spec. no vote. just asked a question. | |
| 18:51:14 | jaypipes | bauzas: the answer to that question is very important for me to know. | |
| 18:51:32 | dansmith | mriedem: but that's the can-do-it flag, if we filter out hosts without that trait | |
| 18:52:06 | dansmith | mriedem: we could do a service version for this if you want I guss | |
| 18:52:16 | dansmith | feels a little strange for a virt-specific thing, | |
| 18:52:30 | dansmith | and rpc version bump is less useful if we're not actually adding a parameter for something | |
| 18:53:08 | mriedem | yes it does, and i'm also not sure if/how it would make sense here | |
| 18:53:25 | dansmith | we could do one of two things: | |
| 18:53:36 | mriedem | i guess we can use a service version check to know if the source compute is too old to make a reliable guess about the trait | |
| 18:53:41 | dansmith | 1. Not let you enable file-backed if the minimum service version says you have older computes (which sucks) | |
| 18:54:02 | dansmith | 2. 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 | |
| 18:55:04 | mriedem | with (2) we still have to build in a trait placement req filter right? | |
| 18:55:08 | dansmith | no | |
| 18:55:22 | dansmith | it would just mean we force the scheduler to burn a retry on it | |
| 18:55:27 | mriedem | i guess we dont' have to, it would make scheduling more efficient | |
| 18:55:28 | dansmith | so we could do it, or do without | |
| 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 | 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 | |