| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-14 | |||
| 17:43:32 | dansmith | heh | |
| 17:43:43 | mriedem | we finish each other's | |
| 17:44:01 | dansmith | sentences. | |
| 17:44:25 | efried | you guys are gross | |
| 17:44:32 | dansmith | hah | |
| 17:44:56 | dansmith | we're both planning to get pre-summit haircuts on wednesday | |
| 17:45:18 | mriedem | mine is tomorrow | |
| 17:45:22 | dansmith | aww | |
| 17:54:04 | mnaser | i cant be the only one who constantly types 'reno add', get an error then 'reno new' | |
| 17:58:42 | mriedem | tox -e venv -- reno new <slug> | |
| 18:01:44 | mriedem | dansmith: https://review.openstack.org/#/c/567899/ fixes a regression that was backported through to ocata so we should probably get that fix in and backported, | |
| 18:01:55 | mriedem | i had a recreate with our live migration job and the patch on top shows this fixes it | |
| 18:04:16 | zcorneli | dansmith: For file backed memory, any new thoughts on how to handle the old->new migration issue? | |
| 18:07:11 | openstackgerrit | Merged openstack/nova master: Suppress UUID warning in map_instance unit tests https://review.openstack.org/568263 | |
| 18:07:58 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Added ability to configure default architecture for ImagePropertiesFilter https://review.openstack.org/566425 | |
| 18:08:23 | dansmith | mriedem: ack | |
| 18:08:48 | dansmith | zcorneli: remind me, the new xml is calculated on the sending node, to be used by the destination node right? | |
| 18:09:12 | zcorneli | dansmith: That's what I've seen, and how the code seems to expect it. | |
| 18:09:31 | zcorneli | dansmith: Haven't seen any XML modification on the destination yet. | |
| 18:10:06 | dansmith | zcorneli: yeah, so, if that's the case, then an older node migrating to a newer one may break things by sending it a non-file-backed xml, when the node is configured to use file-backed | |
| 18:10:16 | dansmith | because it's ignorant | |
| 18:10:55 | dansmith | zcorneli: so, if there's a way for the new node to reject the migration gracefully if it detects that situation, that'd be ideal | |
| 18:12:04 | dansmith | zcorneli: so on the destination, some way to check that the source node is new enough to be honoring this.. maybe with the flag in the migratedata object, if that gets passed to the destination from the source at that point? | |
| 18:12:18 | zcorneli | dansmith: Artom proposed rejecting the migration (He's working on something similar with NUMA?), but you mentioned that's not necessarily nice on the scheduler. | |
| 18:12:41 | zcorneli | I can check with him to see how he's checking for it, and use something similar (No point to reinventing the wheel) | |
| 18:12:50 | dansmith | zcorneli: we need to avoid banning cross-version migrations altogether | |
| 18:13:40 | dansmith | zcorneli: it'd be nice if the scheduler could know about it, but I think it's probably too much for the first go-round | |
| 18:13:49 | zcorneli | dansmith: I think we can just ban it when it would break things. Just need to do some investigation on how the migration_data is passed around, etc. | |
| 18:14:40 | dansmith | zcorneli: yeah, so I think maybe it's okay to just reject the migration on the destination if that happens, and then make a big stink in the release note that you probably shouldn't turn that on for any new nodes until everything is upgraded | |
| 18:16:45 | zcorneli | dansmith: Works for me. Maybe a note in the config entry too. | |
| 18:17:05 | dansmith | zcorneli: yeah, also put this wrinkle in the spec | |
| 18:37:39 | mriedem | dansmith: the source node modifies the xml to send to the dest | |
| 18:37:48 | mriedem | based on the migrate_data sent back from the dest during pre_live_migration | |
| 18:37:49 | dansmith | yeah | |
| 18:38:04 | dansmith | and the migratedata is created for the first time on the dest right? | |
| 18:38:09 | mriedem | yes | |
| 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 | dansmith | ah, okay | |
| 18:42:11 | mriedem | https://review.openstack.org/#/c/522537/ is my conductor change btw | |
| 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? | |