| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-21 | |||
| 19:49:54 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add ConfirmResizeTask https://review.opendev.org/637070 | |
| 19:49:54 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add confirm_snapshot_based_resize conductor RPC method https://review.opendev.org/637075 | |
| 19:49:55 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Confirm cross-cell resize from the API https://review.opendev.org/637316 | |
| 19:49:55 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add revert_snapshot_based_resize_at_dest compute method https://review.opendev.org/637630 | |
| 19:49:56 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Deal with cross-cell resize in _remove_deleted_instances_allocations https://review.opendev.org/639453 | |
| 19:49:56 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add finish_revert_snapshot_based_resize_at_source compute method https://review.opendev.org/637647 | |
| 19:49:57 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add RevertResizeTask https://review.opendev.org/638046 | |
| 19:49:57 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add revert_snapshot_based_resize conductor RPC method https://review.opendev.org/638047 | |
| 19:49:58 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Revert cross-cell resize from the API https://review.opendev.org/638048 | |
| 19:50:06 | efried | (It'll work fine now since the repro... doesn't.) | |
| 19:52:10 | mriedem | efried: ok i think i see the problem, | |
| 19:52:19 | mriedem | RT._claim_existing_migration is what sets Migration.dest_compute, not conductor, | |
| 19:52:36 | mriedem | so you need to mock that, run the original and then run periodics before returning from _claim_existing_migration | |
| 19:53:14 | mriedem | the live migration task in conductor sets Migration.dest_compute so that's where i got confused | |
| 19:53:54 | mriedem | i guess we do that for the live migration task because we used to never do claims in the compute for live migration | |
| 19:54:08 | mriedem | so we'd never call RT._claim_existing_migration for live migration before train | |
| 19:54:20 | mriedem | well, and we still don't if the instance doesn't have a numa topology | |
| 19:54:56 | efried | mriedem: I tried doing as you suggest and ran into the deadlock. | |
| 19:55:04 | efried | but | |
| 19:55:23 | efried | why does Migration.dest_compute make the difference? | |
| 19:55:42 | mriedem | because RT._update_available_resource calls MigrationList.get_in_progress_by_host_and_node | |
| 19:56:00 | efried | okay | |
| 19:56:05 | mriedem | without having Migration.dest_compute set the migration won't be returning for that dest host during the periodic | |
| 19:56:07 | efried | so I was barking up the right tree | |
| 19:56:22 | efried | to get around the deadlock, should I ... stub out the lock? | |
| 19:56:30 | efried | that seems pretty dangerous. | |
| 19:57:17 | mriedem | hmm | |
| 19:59:37 | efried | Perhaps I can stub out the lock just from within my stub | |
| 20:00:13 | mriedem | well, if we have to monkey with locks we're likely going about this the wrong way | |
| 20:00:30 | mriedem | so maybe you can't reproduce with a cold migration resize_claim and instead need to do a live migration in the test, | |
| 20:00:37 | mriedem | because in that case Migration.dest_compute is set in the conductor | |
| 20:00:55 | efried | okay. | |
| 20:01:00 | efried | still _prep_resize tho? | |
| 20:01:11 | mriedem | no, that's not called for live migration | |
| 20:01:16 | mriedem | you could probably stub pre_live_migration | |
| 20:01:21 | mriedem | that runs on the dest | |
| 20:01:52 | mriedem | unless the instance has a numa topology i don't think it'll ever have a migration context so it doesn't matter too much | |
| 20:04:12 | efried | woot, got repro | |
| 20:05:16 | mriedem | yass | |
| 20:05:21 | mriedem | now i have to go pick up some contacts | |
| 20:08:57 | openstackgerrit | Eric Fried proposed openstack/nova master: Func: bug 1849165: mig race with _populate_assigned_resources https://review.opendev.org/689866 | |
| 20:08:57 | openstack | bug 1849165 in OpenStack Compute (nova) "_populate_assigned_resources raises "TypeError: argument of type 'NoneType' is not iterable" during active migration" [High,In progress] https://launchpad.net/bugs/1849165 - Assigned to Eric Fried (efried) | |
| 20:08:57 | openstackgerrit | Eric Fried proposed openstack/nova master: Don't populate resources for not-yet-migrated inst https://review.opendev.org/689842 | |
| 20:09:05 | efried | mriedem_afk, luyao: ^ | |
| 20:13:28 | openstackgerrit | Eric Fried proposed openstack/nova master: Func: bug 1849165: mig race with _populate_assigned_resources https://review.opendev.org/689866 | |
| 20:13:28 | openstack | bug 1849165 in OpenStack Compute (nova) "_populate_assigned_resources raises "TypeError: argument of type 'NoneType' is not iterable" during active migration" [High,In progress] https://launchpad.net/bugs/1849165 - Assigned to Eric Fried (efried) | |
| 20:13:28 | openstackgerrit | Eric Fried proposed openstack/nova master: Don't populate resources for not-yet-migrated inst https://review.opendev.org/689842 | |
| 20:13:32 | efried | now with pep8 fixed! | |
| 20:56:49 | openstackgerrit | Florian Haas proposed openstack/nova stable/queens: Explain nested guest support https://review.opendev.org/609790 | |
| 20:58:02 | mriedem | efried: +2 on both | |
| 20:58:26 | efried | thanks mriedem. I think gibi is to blame for the formatting -- I copy/pasted test_bug_1845291.py | |
| 20:58:36 | mriedem | tarnations | |
| 20:59:29 | efried | We should get someone like wangfaxin to propose a patch to reformat it. | |
| 20:59:41 | efried | https://review.opendev.org/#/q/owner:wangfaxin%2540inspur.com+status:open | |
| 21:12:47 | mriedem | whatever helps the new contributor | |
| 21:13:04 | mriedem | efried: i replied in https://review.opendev.org/#/c/689049/ - i'm not sure how much we're on the same page, but i think i'm thinking more we need to do your "eventually" case | |
| 21:13:59 | mriedem | you said, "TL;DR the only thing that should change right now is: don't retry on consumer 409." but that's not what we do today anyway in move_allocations | |
| 21:14:00 | efried | oh | |
| 21:14:04 | efried | yeah | |
| 21:14:04 | mriedem | unless i don't understand how that code works | |
| 21:14:15 | efried | no, you're right, I forgot we were doing the text scraping thing | |
| 21:14:25 | efried | in that case, we shouldn't "fix" this at all. | |
| 21:14:25 | mriedem | ok | |
| 21:16:47 | efried | I'll need to swap it back in to be more thoroughly convinced of that ^ but at least last week when I was thinking this through, I convinced myself that making these narrow/local changes was not going to help anything (given that we're not retrying on consumer alloc 409) | |
| 21:17:24 | mriedem | i agree that we don't have the context for figuring out what to do within move_allocations, | |
| 21:17:37 | mriedem | which is why i think we need a separate method that wraps move_allocations and has the necessary logic | |
| 21:17:45 | mriedem | which only gets used in the revert case | |
| 21:18:51 | mriedem | i've got about 45 min to kill so maybe i can hack something together for what i'm thinking | |
| 21:19:19 | lifeless | mriedem: constraints was parallel to solver work FWIW | |
| 21:19:33 | lifeless | mriedem: even with a solver we want stability and precise controls :) | |
| 21:20:16 | lifeless | mriedem: I did have a solver branch, but yeah, job changes at the wrong time stalled that work then someone else popped up so ... its in a branch on github somewhere but the other volunteers thing will eventuate eventually I hope | |
| 21:35:31 | mordred | lifeless: you're a solver branch | |
| 22:02:05 | mriedem | lifeless: ack | |
| 22:02:33 | mriedem | efried: ok i've got a poc which passes functional tests, i'll push that up for you and gibi to ponder before trying to cover new unit tests | |
| 22:03:02 | efried | ight | |
| 22:04:17 | mriedem | i think you're right in that there is still a race with the instance being deleted no matter how tight i make things unless we assert the existence of the instance both before and after calling move_allocations, which kind of sucks | |
| 22:15:15 | lifeless | mordred: weak sauce | |
| 22:20:48 | efried | mriedem: do you agree the proper thing would be to pull the instance allocation at the very beginning of the operation? | |
| 22:22:18 | mriedem | like before we move the instance allocations to the migration record before calling the scheduler? | |
| 22:22:37 | mriedem | or just before calling move_allocations on revert? | |
| 22:22:59 | efried | the former | |
| 22:23:43 | efried | or, does that operation return a payload? in which case keep hold of the generation from there. | |
| 22:24:37 | mriedem | does what operation return a payload? scheduling? | |
| 22:25:38 | efried | Moving allocs to the mig record. | |
| 22:26:02 | mriedem | POST /allocations returns a 204 | |
| 22:26:06 | mriedem | so no payload | |
| 22:26:08 | efried | but | |
| 22:26:14 | efried | we must have pushed a gen to do it | |
| 22:26:19 | efried | so, save that gen. | |
| 22:27:01 | mriedem | the target consumer generatoin is optional in move_allocations, which is the problem in my bug; the source consumer generation is required | |
| 22:27:04 | mriedem | but i see what you're saying | |
| 22:27:45 | efried | In English: "Make sure the instance didn't change between when we started the migration and... the bug place." | |
| 22:28:49 | mriedem | yeah idk what could go wonky with that, seems we'd only use that in an edge case, e.g. the target consumer doesn't have allocations for us to revert with a gen | |
| 22:29:20 | mriedem | for now i've just got a big comment so i can push this up | |
| 23:05:12 | melwitt | argh how do I get kibana to not show me the same change a million times for a build_status:FAILURE | |
| 23:10:40 | mriedem | you need one line from the console | |
| 23:11:42 | melwitt | ok | |
| 23:12:28 | mriedem | "ERROR: all: commands failed"? | |
| 23:12:32 | mriedem | though that's only if tempest failed | |
| 23:12:47 | mriedem | "failed: 1" would be if any play failed | |