| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 17:16:00 | dansmith | artom: blindly without checking | |
| 17:16:13 | sean-k-mooney | ok so why don t we jsut do the same check again | |
| 17:16:23 | dansmith | you guys are saying it's fine to make dangerous assumptions because "nobody will ever ask for X" but that's *always* a bad assumption | |
| 17:16:47 | dansmith | sean-k-mooney: that's my point, assert that the thing is the way you think it is, instead of just assuming you can ignore all of it because something upstream of you did the check | |
| 17:16:51 | artom | dansmith, so if we keep pci_requests, but skip _test_pci... | |
| 17:16:57 | dansmith | because in the future when that check is gone, different, mixed-versions, etc... | |
| 17:17:25 | sean-k-mooney | dansmith: no im not im saying we specificaly thought about this edgecase in the spec and code and added a check to prevet the dangours edge case for the sriov mirgation | |
| 17:17:26 | dansmith | artom: would it make sense to just make _test_pci check the neutron-ness (or whatever) and not freak out over those things? | |
| 17:17:39 | sean-k-mooney | but im totally fine with adding another chacke for numa path | |
| 17:17:47 | sean-k-mooney | just copy past | |
| 17:17:49 | sean-k-mooney | for pci_request in self.instance.pci_requests.requests: | |
| 17:17:51 | sean-k-mooney | if pci_request.source != objects.InstancePCIRequest.NEUTRON_PORT: | |
| 17:17:53 | sean-k-mooney | # allow only VIF related PCI requests in live migration. | |
| 17:17:53 | artom | dansmith, so move the check that sean-k-mooney linked into the claim? | |
| 17:17:55 | sean-k-mooney | raise exception.MigrationPreCheckError( | |
| 17:17:57 | sean-k-mooney | reason= "non-VIF related PCI requests for instance " | |
| 17:17:59 | sean-k-mooney | "are not allowed for live migration.") | |
| 17:18:02 | sean-k-mooney | no | |
| 17:18:11 | sean-k-mooney | we dont want to fail late | |
| 17:18:41 | sean-k-mooney | bu just check again in the compute node | |
| 17:18:44 | dansmith | can we not do the same check in _test_pci, and ignore any that are vif-related, and fail if we find non-vif-related ones/ | |
| 17:19:01 | artom | dansmith, conversely, make apply_migration_context() smarter, and only set the new_ fields on the instance if they're set on the migration_context? | |
| 17:19:09 | dansmith | artom: no | |
| 17:19:29 | dansmith | artom: because that is deeply-buried silent ignoring of data you're trying to save, just because you only think that will happen from the migration path | |
| 17:19:56 | dansmith | this is all about making claims work for pci devices, so make it work, don't just "fix" the problem with lots of side effects everywhere else | |
| 17:20:22 | artom | dansmith, claims *already* work for PCI devices | |
| 17:20:32 | dansmith | sorry, making claims work during live migration | |
| 17:20:34 | dansmith | mind-o there | |
| 17:20:35 | artom | They're used when booting and cold migrating | |
| 17:20:39 | dansmith | yes yes I know | |
| 17:20:52 | artom | This is about having PCI live migration in different places | |
| 17:21:03 | artom | *PCI live migration logic in different | |
| 17:21:30 | artom | dansmith, so correct me if I'm wrong, but looking at https://github.com/openstack/nova/blob/master/nova/objects/instance.py#L1004-L1007 | |
| 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 | |