| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 17:09:49 | dansmith | that's the part I don't like | |
| 17:10:03 | artom | I agree - it's also current/latent | |
| 17:10:30 | artom | IIRC we just blow up if we try to migrate an instance with PCI alias-based passthrough | |
| 17:11:00 | sean-k-mooney | we check and fail the migration here https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L216-L221 | |
| 17:11:01 | dansmith | artom: it's not latent because it's the code you're adding | |
| 17:11:20 | sean-k-mooney | if the instance has a pcirequest that is not a neutron port we never call artoms code | |
| 17:11:51 | dansmith | sean-k-mooney: currently. | |
| 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 | |