| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-21 | |||
| 16:06:04 | gibi | so the pci_claim at the second migration will see consitent data | |
| 16:07:36 | gibi | mriedem: now I think not rolling back this is not causing problems. but I agree that it is not nice to have wrong data in the PCI request after rollback | |
| 16:08:55 | mriedem | so could we have a case like, | |
| 16:09:10 | mriedem | 1. create server using port request and it gets parent_ifname foo, | |
| 16:09:25 | mriedem | 2. try to migrate the server and we update the instance pci_request to point at parent_ifname bar, | |
| 16:09:39 | mriedem | 3. migrate fails and we don't rollback the instance pci_request | |
| 16:09:54 | mriedem | 4. another server is created - it can't use parent_ifname bar even though nothing is using it right? | |
| 16:10:14 | mriedem | iow it's "claimed" by the failed first server | |
| 16:10:25 | gibi | this is not part of the claim | |
| 16:10:41 | gibi | it drivers the claim to select the good PF | |
| 16:10:52 | gibi | but the whole PF never claimed | |
| 16:11:07 | mriedem | ok, then how about if we hard reboot the failed server, will it try to use parent_ifname bar and fail if another guest on the same host is already using that? | |
| 16:11:42 | gibi | mriedem: if we hard reboot then nobody will check the parent_ifname. It is only the VF pci address that matters | |
| 16:12:14 | mriedem | ok i don't know how any of this stuff is actually used in the driver | |
| 16:12:21 | mriedem | just thinking about fallout scenarios | |
| 16:12:24 | gibi | parent_finame is not a consumable, it is like a trait | |
| 16:12:58 | gibi | it helps selecting the proper pool of VFs during the pci_claom | |
| 16:13:45 | mriedem | efried: alex_xu: looks like we have a fun TypeError for the resources stuff added late in Train https://zuul.opendev.org/t/openstack/build/2b10b4a240b84245bcee3366db93951d/log/logs/screen-n-cpu.txt.gz?severity=4#2675 | |
| 16:15:01 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Fix policy doc for host_status and extended servers attribute https://review.opendev.org/689833 | |
| 16:15:07 | gibi | mriedem: made a TODO to look into how hard to roll back the parent_ifname during rollback or revert | |
| 16:17:25 | mriedem | gibi: ack, not a huge deal | |
| 16:17:35 | mriedem | i know you like to pull these types of threads in your spare time :) | |
| 16:17:44 | gibi | :) | |
| 16:18:09 | openstack | Launchpad bug 1849165 in OpenStack Compute (nova) "_populate_assigned_resources raises TypeError: argument of type 'NoneType' is not iterable" [High,New] | |
| 16:18:09 | mriedem | efried: alex_xu: https://bugs.launchpad.net/nova/+bug/1849165 seems there is a race in that resources code in the RT during migrations | |
| 16:22:08 | mriedem | this is where i say even things that touch the RT which aren't used still have side effects... | |
| 16:30:19 | efried | mriedem: is that happening because mig.instance.migration_context isn't set at that point? | |
| 16:33:15 | mriedem | yeah | |
| 16:33:33 | mriedem | i haven't traced everything here, but in this case i think the RT is running the periodic on the dest host before the instance gets there | |
| 16:33:44 | mriedem | so the Migration record exists pointing at the source and dest host, but the instance hasn't moved yet | |
| 16:34:14 | mriedem | and given the migration record is usually created in the control plane but the migration context doesn't exist until we do a claim in the compute, there is a window | |
| 16:34:56 | mriedem | dansmith: remember https://review.opendev.org/#/c/274870/ ? i'm seeing where we hit the KeyError in _pair_instances_to_migrations and end up still lazy-loading the migration.instance.migration_context/flavor - what do you think about changing Migration.instance to load the Instance with migration_context and flavor fields always? | |
| 16:35:28 | openstackgerrit | Eric Fried proposed openstack/nova master: Always trait the compute node RP with COMPUTE_NODE https://review.opendev.org/688979 | |
| 16:35:34 | efried | bauzas: added reno ^ | |
| 16:37:13 | efried | mriedem: so `if not mig_ctx: continue` ? | |
| 16:39:35 | mriedem | efried: yeah i think so | |
| 16:39:46 | efried | mriedem: I'll throw that out | |
| 16:40:19 | mriedem | _update_usage_from_migration is what populates self.tracked_migrations and you can see the "elif incoming and not tracked:" logic | |
| 16:40:23 | mriedem | that's the case we're hitting here i think | |
| 16:40:58 | mriedem | [None req-dd5ddbad-4234-4288-bbab-2c3d20b7f4ad None None] [instance: cd4148a2-4550-4e83-b6f7-c91752eaf779] Starting to track incoming migration 407fd025-e8ba-4012-ade7-d0255d2a1837 | |
| 16:41:03 | mriedem | [None req-dd5ddbad-4234-4288-bbab-2c3d20b7f4ad None None] Error updating resources for node ubuntu-bionic-rax-iad-0012404623. | |
| 16:43:32 | efried | mriedem: but it looks like we should be under COMPUTE_RESOURCE_SEMAPHORE in all those code paths | |
| 16:43:40 | gmann | mriedem: melwitt https://review.opendev.org/#/c/689833/ | |
| 16:43:41 | mriedem | doesn't matter | |
| 16:43:55 | mriedem | efried: that's per-worker, not global | |
| 16:44:02 | mriedem | efried: so: | |
| 16:44:14 | mriedem | 1. conductor creates migration record with source and dest host set, | |
| 16:44:24 | mriedem | 2. update_available_resource runs on dest host to start tracking incoming migration | |
| 16:44:29 | mriedem | kaboom | |
| 16:44:52 | mriedem | 3. claim happens on dest host to create migration_context (on a different request thread) | |
| 16:45:50 | dansmith | mriedem: yeah, makes sense.. if we're looking for an instance involved with a migration, it's likely we care about the migration context | |
| 16:47:04 | mriedem | dansmith: ok will push something | |
| 16:47:38 | efried | mriedem: are you working up a func test? | |
| 16:47:43 | mriedem | nope | |
| 16:48:11 | efried | is a func test possible? | |
| 16:48:41 | mriedem | yeah probably | |
| 16:48:52 | mriedem | i mean, i *could* write one up | |
| 16:49:10 | mriedem | if $$$properly$$$ motivated | |
| 16:52:07 | openstackgerrit | Dan Smith proposed openstack/nova master: Add image precaching docs for aggregates https://review.opendev.org/687348 | |
| 16:52:24 | efried | I just can't seem to stop seeing that ^ as "preaching" | |
| 16:52:29 | dansmith | mriedem: ^ includes some generic image caching doc stuff in addition to precaching | |
| 16:52:56 | dansmith | as we discussed | |
| 16:53:56 | openstack | bug 1847302 in OpenStack Compute (nova) "doc: need admin guide for the image cache" [Undecided,New] https://launchpad.net/bugs/1847302 | |
| 16:53:56 | mriedem | is it worth marking "Partial-Bug: #1847302" or at least related? | |
| 16:54:09 | dansmith | if you want | |
| 16:54:21 | mriedem | i think that woudl be good (Related-Bug) | |
| 16:54:28 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Don't populate resources for not-yet-migrated instance https://review.opendev.org/689842 | |
| 16:54:31 | efried | mriedem: ^ | |
| 16:54:33 | openstackgerrit | Dan Smith proposed openstack/nova master: Add image precaching docs for aggregates https://review.opendev.org/687348 | |
| 16:54:54 | dansmith | mriedem: already did partial-, I can change to related- if you want, as I'm sure this will need some respins | |
| 16:55:10 | mriedem | up to you, only difference is with partial the bug is assigned to you | |
| 16:55:21 | mriedem | which might imply signing up for more work than you want | |
| 16:56:36 | dansmith | drat! :) | |
| 17:01:22 | efried | So we need update_available_resource on the dest compute to run after x and before z, yah? | |
| 17:01:22 | efried | - z claim on destination (_move_claim) -- this is the thing that sets the instance's migration_context | |
| 17:01:22 | efried | - y schedule | |
| 17:01:22 | efried | - x create migration context -- this creates the migration record and populates the instance in it, but the instance doesn't have a migration_context yet | |
| 17:01:22 | efried | - migrate instance, which has the following steps: | |
| 17:01:22 | efried | - create instance | |
| 17:01:22 | efried | still trying to understand the race. | |
| 17:03:52 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Join migration_context and flavor in Migration.instance https://review.opendev.org/689846 | |
| 17:03:53 | mriedem | your steps are wrong | |
| 17:04:32 | mriedem | you don't control when update_available_resource runs, it's on a timer in a thread pool | |
| 17:04:58 | efried | well, we can trigger it manually in a test | |
| 17:05:34 | mriedem | 1. create instance, | |
| 17:06:02 | mriedem | 2. wrap _prep_resize on the dest compute service to trigger the periodic on the dest when calling _prep_resize | |
| 17:06:08 | mriedem | 3. initiate a migration | |
| 17:06:11 | mriedem | check logs for TypeError | |
| 17:06:54 | dansmith | this is happening in CI yeah? | |
| 17:07:04 | dansmith | so more soak time and we would have found it I guess | |
| 17:07:07 | mriedem | yeah, multinode migration tests | |
| 17:09:42 | mriedem | efried: i can whip up a func recreate test quick if you want | |
| 17:09:43 | efried | can we please merge https://review.opendev.org/#/c/686207/ which will be useful for this? | |
| 17:09:54 | mriedem | no | |
| 17:09:55 | efried | mriedem: I'm working on it, wouldn't mind the experience. | |
| 17:10:01 | mriedem | because we don't want to backport that spy stuff | |
| 17:10:05 | mriedem | and this has to go to train | |
| 17:10:12 | efried | ight | |
| 17:10:34 | mriedem | i mean, i'd rather not backport the spy stuff | |
| 17:10:44 | mriedem | if enough stuff builds on it i guess it will eventually happen | |