| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 16:39:28 | openstackgerrit | sean mooney proposed openstack/nova master: DNM: Run LM integration tests with NUMA flavor https://review.opendev.org/678887 | |
| 16:39:34 | sean-k-mooney | artom: mriedem dansmith i think ^ will fix it | |
| 16:45:46 | efried | stephenfin: you still around? | |
| 16:46:04 | efried | I've been in meetings for 2h wanting to talk to you about this, but afraid I may have missed my window :( | |
| 16:50:14 | artom | sean-k-mooney, ack, much thanks! | |
| 16:50:49 | sean-k-mooney | we should have 3 jobs trying to test numa with the gate as we speak | |
| 16:51:04 | sean-k-mooney | the singel and multinode jobs i wrote and running and yours | |
| 16:51:30 | sean-k-mooney | i assume i should be able to go test the latest version of your code again manually? | |
| 16:56:15 | sean-k-mooney | oh i just rediscovered somthing quite useful | |
| 16:57:11 | sean-k-mooney | the experimenatl queue is way fater the check. which i kind of knew other pipleine are faster but i forgot | |
| 16:57:34 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Specify availability_zone to unshelve https://review.opendev.org/663851 | |
| 17:03:07 | mriedem | alex_xu: for your tomorrow, please take another pass on ^ | |
| 17:03:14 | mriedem | so we can get 2.77 in and move on | |
| 17:06:06 | dansmith | artom: it might be easier to discuss https://review.opendev.org/#/c/635669/39/nova/compute/resource_tracker.py here | |
| 17:06:36 | artom | dansmith, shoot :) | |
| 17:07:04 | dansmith | artom: my point is that you're assuming that if we're live migrating an instance, you can pass an empty PCIRequests object into the migration claim, because a live-migrating instance couldn't possibly have pci devices, right? | |
| 17:07:49 | artom | dansmith, no, because we don't want the claim to think it has any PCI devices | |
| 17:08:02 | dansmith | um | |
| 17:08:10 | dansmith | artom: why is that? | |
| 17:08:37 | artom | Because for a subset of PCI devices (SRIOV Neutron ports), that's handled buy Sean's code | |
| 17:09:00 | artom | So we don't want to refuse based something that should otherwise work | |
| 17:09:15 | artom | The missing piece here is that all other PCI devices *should* get refused | |
| 17:09:28 | dansmith | should get refused somewhere earlier in the stack right? | |
| 17:09:32 | artom | Yeah | |
| 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 | |