| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 17:21:52 | artom | Seems apply_migration_context is already smart enough to only look at fields in migration_context that are actually set | |
| 17:22:06 | dansmith | what's your point? | |
| 17:22:07 | artom | So what's so bad about removing pci_requests from live migration claims entirely? | |
| 17:22:20 | artom | Well, I'm assuming that'll also remove them from the migration context | |
| 17:23:09 | dansmith | because that's obscure behavior to solve the actual problem, which is that you need to keep pci_requests, but only allow the claim to work if they're of the right type | |
| 17:23:47 | dansmith | why just ignore them entirely and keep them out of the context as a special case when you can keep all the other code the same, and just not proceed if they're not valid for live migration? | |
| 17:23:54 | artom | I'd dispute that - pci_requests is handled outside the claim, so why does the claim need to work with them? | |
| 17:23:58 | sean-k-mooney | why do we not jsut check the migration type here https://review.opendev.org/#/c/635669/39/nova/compute/resource_tracker.py@307 and raise an excepion if any pci device are requested that are not related to neutron | |
| 17:24:11 | sean-k-mooney | does ^ that not solve the issue | |
| 17:24:15 | dansmith | sean-k-mooney: YES | |
| 17:24:18 | dansmith | it's what I'm asking for | |
| 17:24:33 | sean-k-mooney | it should never raise but f it does it means we fucked up and remove the conductor check by mistake | |
| 17:24:41 | dansmith | or something changed | |
| 17:25:01 | sean-k-mooney | ya | |
| 17:25:08 | dansmith | if that were to happen with the current code, we would just blow away pci_requests on the instance | |
| 17:25:12 | sean-k-mooney | but it would catch such a change | |
| 17:25:16 | dansmith | right | |
| 17:25:22 | artom | sean-k-mooney, will the claim pass though? | |
| 17:25:49 | dansmith | we're already examining the requests for other migration types and doing things like this, we should do the same for live migration, but whatever different behavior we need | |
| 17:25:50 | artom | I'm worried about double-claiming | |
| 17:26:05 | sean-k-mooney | we should not try to claim them if it is a live migration | |
| 17:26:09 | sean-k-mooney | if it not we should | |
| 17:26:27 | sean-k-mooney | but we should check instead of just nulling it out | |
| 17:26:36 | artom | sean-k-mooney, exactly, if this is a live migration, the claim cannot have any pci_requests, because your code handles all that | |
| 17:26:54 | sean-k-mooney | yes. | |
| 17:27:08 | sean-k-mooney | we could make the other code use the move claims | |
| 17:27:18 | artom | You said you don't want to fail late | |
| 17:27:23 | sean-k-mooney | but we didnt wnat to do that since that code make so many assumtion about it not being a live migration | |
| 17:27:36 | sean-k-mooney | i dont | |
| 17:27:46 | sean-k-mooney | i still want to keep the check we have in the conductor | |
| 17:27:59 | dansmith | me too | |
| 17:28:17 | sean-k-mooney | im just saying we check twice and dont care that we already checked | |
| 17:28:22 | artom | OK, so say we keep that check, and add a the same kind of check at https://review.opendev.org/#/c/635669/39/nova/compute/resource_tracker.py@307 | |
| 17:28:34 | artom | And we live migrate an instance with NUMA and a Neutron SRIOV port | |
| 17:28:40 | artom | Both of those checks will pass, right? | |
| 17:28:46 | sean-k-mooney | yes | |
| 17:29:17 | sean-k-mooney | and in that case you can set pci_resst=[] safely or we can rewrite the sriov stuff to get teh device form the move claim | |
| 17:30:18 | artom | Well we can't just set pci_requests=[], because as dansmith pointed out, when we apply that claim, we'll clobber the instance's pci requests with the empty one from the claim | |
| 17:30:27 | artom | So we have to keep the current logic | |
| 17:30:28 | artom | Except | |
| 17:31:03 | artom | Your code is claiming PCI devices here: https://review.opendev.org/#/c/634606/58/nova/compute/manager.py@6465 | |
| 17:31:19 | artom | And mine will run the MoveClaim either immediately before or after yours | |
| 17:31:33 | sean-k-mooney | we would only do it in the neutron case and we create new request in that case in my code anyway | |
| 17:31:40 | dansmith | why is it that this needs to be different at all between live and cold migration? | |
| 17:31:55 | artom | Now this may be ignorance on my part, but to me it would mean one of those claims will fail because the previous one will have used up the device | |
| 17:32:04 | artom | Or can fail, at least | |
| 17:32:33 | sean-k-mooney | mainly when we reviewd the sriov spec it was decied that it did nto make sense to make move claims work for live migration but we decided to use the for numa | |
| 17:33:45 | artom | dansmith, I get ya | |
| 17:34:44 | artom | I'm proposing the "workaround" of unsetting pci_requests from live migration claims to avoid any clobbering, and we can consolidate SRIOV and NUMA claims in a follow up? | |
| 17:34:58 | dansmith | -2 on that | |
| 17:35:27 | artom | So... straight up consolidation right away? | |
| 17:35:49 | dansmith | answer my question above about why this needs to be different | |
| 17:35:50 | sean-k-mooney | if we most but i really dont like move claimes | |
| 17:36:01 | dansmith | not sure if sean-k-mooney answered it or not, but I didn't understand the words | |
| 17:36:04 | sean-k-mooney | but it should not be too hard to make it work | |
| 17:36:14 | artom | dansmith, why live migration claims need to be different with respect to PCI devices? | |
| 17:36:24 | dansmith | sean-k-mooney: isn't the whole point of this work to integrate the move claims with this stuff so that it works? | |
| 17:36:42 | dansmith | artom: yes | |
| 17:36:50 | sean-k-mooney | we thought that was too much work to justify doing that for sriov | |
| 17:37:09 | sean-k-mooney | also if i recall jay was not a fan of that approch | |
| 17:37:27 | dansmith | so the plan was just to re-use most of the same code paths but special-case out the live migration? | |
| 17:37:30 | sean-k-mooney | which is why we built on the multiple port binding feature and do not use port bindings | |
| 17:38:00 | artom | dansmith, because for the subset of pci_requests that's supported for live migration, claiming resources on the destination is already being handled in https://review.opendev.org/#/c/634606/58/nova/compute/manager.py@6465 | |
| 17:38:03 | sean-k-mooney | no sriov migration jsut does not use move claims because live migration dose not use them | |
| 17:38:38 | artom | sean-k-mooney, so you left me to do the gruntwork l) | |
| 17:38:40 | artom | ;) | |
| 17:39:02 | sean-k-mooney | artom: honestly i was hoping you would not use move claims either but i kind of gave up on that | |
| 17:39:04 | artom | dansmith, and that code predates using claims for live migration at all | |
| 17:39:38 | dansmith | I honestly don't understand this | |
| 17:39:48 | artom | dansmith, can you expand on that "this" is :) | |
| 17:39:53 | dansmith | you want to use move claims for numa but not pci and have a separate code path just for pci live migration? | |
| 17:40:03 | artom | dansmith, PCI predates NUMA | |
| 17:40:08 | dansmith | and? | |
| 17:40:39 | artom | And they decided not use to claims, presumably because the PCI tracker makes it easier to claim resources without going through actual MoveClaim objects | |
| 17:40:47 | sean-k-mooney | and both spec propsoed different approche to the same thing because different peopel reveies and had differen preferences | |
| 17:40:58 | dansmith | you seem to be saying "I can make the mess bigger because it's already a mess" and "even though we're rounding out live support for a thing that already works cold, I'm going to do it a different way because there are already lots of fragments" | |
| 17:41:06 | dansmith | neither of those really satisfy me | |
| 17:41:19 | artom | For NUMA LM, MoveClaim was a handy way to get both claiming of resources and the new instance NUMA topology in a "single operation" | |
| 17:41:47 | sean-k-mooney | move claims are not stictly requried for numa migration either | |
| 17:42:03 | sean-k-mooney | but it would require a lttile more work | |
| 17:42:19 | sean-k-mooney | artom was trying to reuse the existing cold migraiton code to reduce the code change | |
| 17:42:19 | artom | No, but they do make things less racy and simplify getting the new instance numa topology | |
| 17:42:36 | sean-k-mooney | artom: i dont think they do | |
| 17:42:48 | sean-k-mooney | we both claim in the same place. more or less | |
| 17:44:18 | artom | sean-k-mooney, yeah, maybe there was a way to do it less racy-ly even without claims | |
| 17:44:39 | dansmith | tbh, I don't really care whether this uses claims or not, | |
| 17:44:42 | dansmith | but if you're going to, | |
| 17:44:57 | dansmith | I think that just "if live migration: do different thing" in a bunch of random places is not moving us forward | |
| 17:45:16 | dansmith | especially when it comes to things that may silently break data | |
| 17:45:28 | mriedem | "if pci: something something dragons that no one but sean understands" | |
| 17:45:29 | artom | dansmith, so that would mean folding SRIOV live migration into the claims | |
| 17:45:31 | mriedem | ^ since juno | |
| 17:45:37 | dansmith | artom: btw, you've done all this local testing of this.. have you included sriov migration and not seen it clobber pci_requests? | |
| 17:45:44 | sean-k-mooney | we currently do the claim here for sriov https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6428-L6467 | |
| 17:45:57 | artom | dansmith, SRIOV with this is completely untested :( | |
| 17:46:05 | sean-k-mooney | right after we call check_can_live_migrate_source in check_can_live_migrate_destination | |
| 17:46:17 | dansmith | artom: so it's very likely that you're blowing those away with this yeah? | |
| 17:46:22 | artom | Although sean-k-mooney's saying apparently all recent-ish NICs can do SRIOV, so maybe I *do* have the hardware? | |
| 17:46:52 | sean-k-mooney | artom: i have hardware and i set up port forwading so you can ssh in. | |
| 17:47:03 | sean-k-mooney | but im going to check both again after dinner | |