| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 17:11:54 | artom | sean-k-mooney, aha, thanks for that - that confirms my idea that we should do claims *after* your MigrationPreCheck | |
| 17:12:10 | artom | So I'll have to change that | |
| 17:12:34 | sean-k-mooney | artom: https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L317-L326 | |
| 17:12:55 | sean-k-mooney | we do the pci check before calling check_can_live_migrate_destiation | |
| 17:13:11 | sean-k-mooney | so we should do the claims in check_can_live_migrate_destination | |
| 17:13:20 | artom | Ah, sorry, missed the wider context in your link. OK, then we're good. | |
| 17:13:22 | dansmith | artom: so my concern is that you say "we should never get here" and throw up an empty PCIRequests object, which I think goes into the migration context, and eventually gets applied to the instance | |
| 17:13:46 | artom | dansmith, ah, indeed | |
| 17:13:48 | sean-k-mooney | dansmith: we get there for cold migration | |
| 17:13:57 | dansmith | artom: so if the conductor stuff changes in the future and we *do* get here, I worry that we could dump the PCIRequests from the instance when we apply | |
| 17:14:23 | dansmith | sean-k-mooney: I know, but cold migration is still handled.. that's not my problem.. my problem is that live migration is *begging* to become a silent data corruption problem in the future I think | |
| 17:14:47 | artom | dansmith, so for live migration _test_pci in the claim would have to be skipped entirely | |
| 17:14:58 | sean-k-mooney | we put the check in specifcally to prevent issue related to pci devices | |
| 17:15:12 | sean-k-mooney | we would only remove it if we added support for it | |
| 17:15:20 | artom | ... while still keeping pci_requests? | |
| 17:15:23 | dansmith | artom: it looks to me like for cold, we get the requests and filter them based on their handled-by-neutron-ness or something | |
| 17:15:29 | sean-k-mooney | which i dont think we ever will as we cant safely hot unplug generic pci devices | |
| 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 | artom | dansmith, so move the check that sean-k-mooney linked into the claim? | |
| 17:17:53 | sean-k-mooney | # allow only VIF related PCI requests in live migration. | |
| 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 | |