| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-08-06 | |||
| 13:51:51 | dansmith | also, queried about whether or not the GET is enough. I guess I would expect there are some attachment states that can't be reversed blindly by the boot process? | |
| 13:52:59 | lyarwood | dansmith: ack thanks | |
| 13:55:45 | openstackgerrit | sahid proposed openstack/nova master: hardware: fix memory check usage for small/large pages https://review.openstack.org/532168 | |
| 13:56:00 | openstackgerrit | Lee Yarwood proposed openstack/nova master: DNM/WIP compute: Reduce likelihood of bdm creation race during attach https://review.openstack.org/589164 | |
| 13:59:47 | mriedem | dansmith: lyarwood: that's where i'm leaning - re-create attachments in conductor build_instances if we're rescheduling | |
| 13:59:56 | dansmith | yeah | |
| 14:00:03 | dansmith | t'would make more sense to me to do that | |
| 14:00:48 | lyarwood | mriedem / dansmith ; kk, tbh I've spent very little time looking around within conductor and just assumed it wouldn't be the right place, I'll try to move things over now and respin. | |
| 14:04:59 | mdbooth | OS_TEST_PATH | |
| 14:07:58 | efried | mdbooth: \o/ nice one | |
| 14:08:29 | mdbooth | Although I can't see why rpmbuild isn't using the default | |
| 14:14:58 | mdbooth | mriedem: Wow, brave. | |
| 14:15:42 | mriedem | already mostly done | |
| 14:15:54 | mdbooth | Any good nuggets? | |
| 14:16:46 | mriedem | mdbooth: i'll post a summary with a link to my spreadsheet in the ML once i'm done | |
| 14:16:55 | mriedem | there are definitely some things we can upstream to nova | |
| 14:17:21 | mriedem | given it was a snapshot based on pike, there are also several bug fixes they have backported so not really forks, just cherry picks we don't have in stable/pike | |
| 14:17:40 | mriedem | and other bug fixes that weren't reported upstream | |
| 14:17:56 | mriedem | e.g. https://review.openstack.org/#/c/588689/ | |
| 14:18:05 | mriedem | and https://review.openstack.org/#/c/588657/ | |
| 14:18:26 | mdbooth | Nice | |
| 14:18:32 | mdbooth | Ah, so you do at least have a git repo | |
| 14:18:43 | mdbooth | You're not just staring at a massive unified diff | |
| 14:18:54 | mriedem | i am | |
| 14:18:55 | mriedem | https://github.com/starlingx-staging/stx-nova/commit/71acfeae0d1c59fdc77704527d763bd85a276f9a | |
| 14:19:07 | mriedem | oh yeah i do have the actual git repo cloned | |
| 14:19:11 | mriedem | so i can search things for context | |
| 14:19:14 | mriedem | but i'm mostly starting with the diff | |
| 14:20:04 | mriedem | i'm also starting to glaze over things toward the end here out of sheaer fatigue | |
| 14:20:06 | mriedem | *sheer | |
| 14:26:56 | gibi | dansmith: hi! I'm looking at some migation revert functional test case and it seems we are still hitting the legacy allocation handling code path when reverting the allocation on the destination | |
| 14:27:08 | gibi | dansmith: I think it is a bug in https://github.com/openstack/nova/blob/8688b25ca7379391cba28fab30b5a628957e673e/nova/compute/manager.py#L3952-L3955 | |
| 14:27:40 | gibi | dansmith: cn_uuid points to the destinantion host based but the migration.uuid only supposed to hold allocation on the source host | |
| 14:27:53 | gibi | dansmith: So the if condition never be true | |
| 14:28:41 | gibi | dansmith: Did I missed something or it is really a bug? | |
| 14:30:56 | dansmith | gibi: there's something tricky about this, let me re-load context | |
| 14:37:47 | openstackgerrit | Lee Yarwood proposed openstack/nova master: conductor: Recreate volume attachments during a reschedule https://review.openstack.org/587071 | |
| 14:40:51 | dansmith | gibi: this is the context I was thinking of, although reading that review comment now doesn't seem to make sense: https://review.openstack.org/#/c/498948/9/nova/compute/manager.py@3648 | |
| 14:41:07 | dansmith | gibi: however, I rarely add code like this without some test fail making me do it, | |
| 14:41:11 | dansmith | so I'm not sure | |
| 14:41:32 | dansmith | maybe raise there and run all the test_servers tests to see if something fails? | |
| 14:43:24 | dansmith | it's possible that it was there for some case which has since been removed, but.. worth a shot | |
| 14:43:51 | dansmith | I was thinking that we actually call that on both the source and destination | |
| 14:43:58 | dansmith | because of finish_resize_revert_on_destination(), | |
| 14:44:01 | dansmith | but i don't see it in there | |
| 14:44:07 | gibi | dansmith: I will check resize-same-host sceneario. a simple migrate revert func test now goes to the legacy path | |
| 14:44:08 | dansmith | so I wonder if it was at some point | |
| 14:44:23 | dansmith | gibi: we have those tests in tree though right? | |
| 14:44:30 | dansmith | to verify the migration-holding allocations? | |
| 14:44:32 | gibi | dansmith: we have functional tests | |
| 14:44:47 | gibi | dansmith: for same host resize and migrate too | |
| 14:45:16 | dansmith | gibi: right, which validate the non-legacy path works yeah? | |
| 14:45:51 | gibi | dansmith: which validates that allocations handled properly in non-legacy case. But I think those tests are still hitting the legacy path | |
| 14:46:10 | gibi | let me reproduce both with migrate and same host resize | |
| 14:46:29 | dansmith | hmm | |
| 14:46:41 | dansmith | so the tests aren't noticing that we're leaking an allocation or something? | |
| 14:47:20 | gibi | dansmith: I think we are eventually not leaking as the legacy codepath also handles the alloction properly | |
| 14:48:01 | dansmith | okay I'm not sure how that could be, if it's not handling the migration uuid, but I'll wait for your analysis :) | |
| 14:48:02 | gibi | dansmith: https://github.com/openstack/nova/blob/8688b25ca7379391cba28fab30b5a628957e673e/nova/compute/manager.py#L3982 this will remove the allocation on the destination | |
| 14:48:25 | dansmith | on the dest, yeah, but the source? | |
| 14:48:55 | openstackgerrit | Merged openstack/nova master: Reload oslo_context after calling monkey_patch() https://review.openstack.org/587772 | |
| 14:49:15 | gibi | dansmith: yeah, I'm confused now. I will do the reproduction | |
| 14:50:24 | mriedem | the allocation on the source should get moved by conductor right? or is this for the "old computes" scenario? | |
| 14:51:11 | dansmith | mriedem: he thinks we're taking the old doubled patch during some cleanups | |
| 14:51:25 | dansmith | but I'm not sure how we could, without failing those tests (or having big holes in them | |
| 14:51:59 | dansmith | well, and his point is there is a clause in the delete_after_move function we can't explain | |
| 14:52:04 | dansmith | which he thinks might be related | |
| 14:56:09 | dansmith | gibi: I added an exception to that clause and we _do_ hit it in the functional tests | |
| 14:56:16 | dansmith | so that _is_ true somewhere :) | |
| 14:56:27 | dansmith | during revert resize | |
| 14:57:02 | dansmith | in three tests | |
| 14:57:40 | dansmith | test_migrate_revert, test_resize_revert, and test_resize_revert_reverse in ServerMovingTests | |
| 15:05:54 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Define irrelevant-files for tempest-full-py3 job https://review.openstack.org/589039 | |
| 15:06:31 | gibi | dansmith: did you put the raise in the legacy path? | |
| 15:06:47 | dansmith | gibi: no, in the clause you think can't ever be true | |
| 15:07:05 | dansmith | gibi: this: https://pastebin.com/GC17gjG4 | |
| 15:08:46 | gibi | dansmith: I put it in front of the legacy code and hit the same tests you hit | |
| 15:09:01 | gibi | dansmith: it doesn't make any sense | |
| 15:09:27 | dansmith | hrm. | |
| 15:09:40 | dansmith | oh, well, | |
| 15:09:46 | dansmith | that can happen in the legit case, | |
| 15:10:01 | dansmith | because we're just checking for allocs there. if we find none, we will fall through to the legacy path as expected | |
| 15:10:53 | gibi | dansmith: ahh. When I question the existence of an if condition above it was the if allocs | |
| 15:11:13 | dansmith | oh | |
| 15:11:37 | gibi | as we are querying alloctions held by the migration.uuid on the _dest_ host | |
| 15:11:47 | gibi | but that is always empty | |
| 15:11:55 | gibi | as migration holds allocation on the source host only | |
| 15:12:01 | dansmith | right right | |
| 15:12:19 | dansmith | except for same-host, but that should never hit this because of the source check above | |
| 15:12:30 | dansmith | I'll move the exception and re-run to confirm | |
| 15:12:47 | gibi | I did that and got green results | |
| 15:13:34 | gibi | so my theory is that in case of revert on dest we always see empty allocs and don't return so we hit the legacy path below | |
| 15:14:22 | dansmith | okay | |
| 15:14:53 | dansmith | hmm, which I guess isn't a problem, | |
| 15:15:11 | dansmith | if we're deleting dest allocs and restoring source allocs, | |
| 15:15:22 | dansmith | except for the case of if we miss the migration allocs we need to delete | |
| 15:15:37 | gibi | it seams it doesn't casue a leak, but I have to leak a bit more closer | |
| 15:15:41 | dansmith | gibi: still not sure where this is coming from though -- are you chasing a bug or writing a test or what? | |
| 15:16:44 | gibi | dansmith: trying implement placement 1.28 support in report client (consumer_generation) and writing consumer gen conflict tests and I hit this legacy path that I thought I should not hit any more | |
| 15:16:57 | dansmith | okay | |