| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-14 | |||
| 18:38:18 | mriedem | in check_dest_can_live_migrate.... something like that | |
| 18:38:21 | dansmith | yeah, so that'll make it hard for a new dest to know it should fail | |
| 18:38:28 | mriedem | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6444 | |
| 18:39:12 | mriedem | conductor will retry hosts up to some live migration max retry limit (defaults to unlimited), | |
| 18:39:34 | dansmith | yeah, but we have to have a way for the destination to know it needs to fail | |
| 18:40:08 | mriedem | and conductor is probably not an appropriate place for this since it's libvirt-driver specific | |
| 18:40:38 | mriedem | if we expressed file-backed as a driver capability, it could be a trait on the compute node and we could schedule using a placement request filter | |
| 18:41:07 | mriedem | is there a spec for this? | |
| 18:41:23 | dansmith | mriedem: yeah, that was my first thing on the "make the scheduler know" but then I decided it was maybe too much | |
| 18:41:24 | dansmith | mriedem: yeah | |
| 18:41:37 | dansmith | https://review.openstack.org/#/c/563704/ | |
| 18:41:46 | dansmith | mriedem: artom has this problem with the numa stuff too | |
| 18:41:59 | mriedem | yes and i do too for the port binding stuff | |
| 18:42:11 | mriedem | https://review.openstack.org/#/c/522537/ is my conductor change btw | |
| 18:42:11 | dansmith | ah, okay | |
| 18:42:20 | mriedem | which i told artom to base his numa stuff from | |
| 18:42:29 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Added ability to configure default architecture for ImagePropertiesFilter https://review.openstack.org/566425 | |
| 18:42:50 | mnaser | ^ that should be ready to review again if i didn't break something | |
| 18:42:54 | dansmith | mriedem: so, the cap/trait thing is maybe a little harder than you describe, because we would have to look at the instance's existing host, see if it had that trait, and then ask for hosts with that | |
| 18:42:55 | dansmith | you can't tell just from the instance config or anything | |
| 18:43:03 | mnaser | passed pep8 locally | |
| 18:43:19 | dansmith | mriedem: but if we have three such examples already then maybe it makes sense to go ahead with the scheduler-based approach | |
| 18:43:54 | dansmith | mriedem: although we still need to fail in all cases if you force the migration and skip the scheduler, but I guess a very late and painful failure on the destination is reasonable if you turned off all the safeguards anyway | |
| 18:44:15 | mriedem | yes i'm fine with kicking people in the balls if they force the host and bypass the scheduler | |
| 18:44:24 | dansmith | mriedem: er, actually the trait thing is harder | |
| 18:44:33 | dansmith | mriedem: the problem is if you're moving from an older node to a newer one, | |
| 18:44:37 | mriedem | in my port binding case, i'm lucky i guess in that we don't rely on the dest to do stuff pre-live-migration, we handle setting up the dest host port bindings in conductor | |
| 18:44:52 | dansmith | so basically we'd have to know if they're on a host that can't do it, and then ask for other hosts that can't do it | |
| 18:45:09 | mriedem | dansmith: you could use forbidden traits for that | |
| 18:45:34 | dansmith | mriedem: yeah I know, but but it means we have to expose two traits (eventually): can-do-this and is-configured-for-this | |
| 18:45:59 | dansmith | and then schedule for !can-do-this if they're on a host without can-do-this | |
| 18:46:15 | dansmith | because people will want to schedule on is-configured-for-this too, like a cheaper flavor that gets file-backed memory | |
| 18:46:16 | mriedem | can't that just be a single trait? | |
| 18:46:19 | dansmith | no | |
| 18:46:37 | dansmith | because a host is either configured to do this or not, it can't reasonably have instances with both right now | |
| 18:46:49 | dansmith | otherwise we have to expose two pools of memory in placement for a host | |
| 18:47:09 | mriedem | right, that's the file_backed_memory option right? | |
| 18:47:11 | mriedem | it's a boolean | |
| 18:47:21 | dansmith | that's the "is this enabled" flag yeah | |
| 18:47:24 | dansmith | but for migration, | |
| 18:47:27 | mriedem | so if file_backed_memory is True for a given compute service, we report that it has the file-backed-memory trait | |
| 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 | |