| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 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 | artom | No, but they do make things less racy and simplify getting the new instance numa topology | |
| 17:42:19 | sean-k-mooney | artom was trying to reuse the existing cold migraiton code to reduce the code change | |
| 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 | |
| 17:49:20 | sean-k-mooney | im going to grab dinner but ill kick of a devstack run before i go and setup the test enviromint | |
| 17:49:57 | artom | dansmith, seems likely, yeah | |
| 17:51:11 | sean-k-mooney | we dont use the pci request spec object from the instance sriov migration by the way | |
| 17:51:27 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6428-L8993 | |
| 17:51:46 | dansmith | sean-k-mooney: because why? we store details in neutron about it? | |
| 17:52:13 | artom | Hrmm | |
| 17:52:15 | artom | So actually | |
| 17:52:16 | sean-k-mooney | the only info we need to pass back as the pci address of the new device | |
| 17:52:18 | artom | AFAICT | |
| 17:52:31 | artom | The PCI stuff in the claim doesn't actually *claim* any resources | |
| 17:52:34 | sean-k-mooney | so we store that in the vif port profile which is wher ewe pass it to neutron | |
| 17:52:36 | artom | Just tests that it's supported | |
| 17:53:00 | dansmith | sean-k-mooney: okay, but nova's own structure would still have them in the pci_requests right? | |
| 17:53:07 | sean-k-mooney | yes | |
| 17:53:07 | dansmith | so what about like rebuild or something? | |
| 17:53:35 | artom | Ah, no, it does the _decrease_pool_count() thing, which "claims" the resource | |
| 17:54:20 | sean-k-mooney | how does it interact with this? | |
| 17:54:33 | sean-k-mooney | the claims have 3 states | |
| 17:54:41 | sean-k-mooney | free,cliamed and allocated | |
| 17:54:50 | sean-k-mooney | something like that | |
| 17:55:07 | sean-k-mooney | one of them meens its reserved for an instace the the otehr is its in use | |
| 17:55:10 | dansmith | I'm saying I would expect we use pci_requests if we're doing a rebuild on the instance | |
| 17:55:23 | dansmith | or other operations | |