| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-08-06 | |||
| 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 | |
| 15:17:07 | dansmith | we should be able to clean all this up at this point anyway I think | |
| 15:17:16 | dansmith | perhaps we should make a point of ripping this all out for stein? | |
| 15:17:29 | dansmith | anyone with an unconfirmed migration between pike and stein probably has other problems :) | |
| 15:19:13 | gibi | dansmith: to avoid hitting the legacy and later rip it out I need to change https://github.com/openstack/nova/blob/8688b25ca7379391cba28fab30b5a628957e673e/nova/compute/manager.py#L3952-L3955 to query the migration allocation from the source host. Does that make sense to you? | |
| 15:19:45 | dansmith | well.. | |
| 15:20:08 | dansmith | the point of that is to make the destination node not do anything at all (i.e. not run the legacy path) if the new-style allocations were used | |
| 15:21:04 | dansmith | so, I guess, but you'll have to do a lookup of the source uuid I think in order to use that method | |
| 15:21:16 | gibi | dansmith: yes that is the goal. if that code sees the allocation held by the migration on the source host it returns and let the finish_revert_resize do the work | |
| 15:21:18 | dansmith | which is a little bit icky | |
| 15:22:14 | gibi | dansmith: ohh. Then we just assume there is no legacy migration any more and unconditioanlly not doing anython on the dest host | |
| 15:22:18 | dansmith | gibi: ah, just change that call to get_allocations_by_consumer() and if the migration has any allocations, then we must be doing new-style | |
| 15:22:32 | gibi | dansmith: good point, we can do that | |
| 15:23:14 | gibi | then I will file a bug and this small change as a bugfix. then later when Stein is open I can remove the whole legacy path in a separate patch | |
| 15:23:29 | dansmith | yar | |
| 15:23:48 | gibi | dansmith: thanks for the brainpower | |
| 15:24:29 | dansmith | np :) | |
| 15:30:28 | mdbooth | Incidentally, my pbr test problem was that I wasn't explicitly installing python-testrepository. This resulted in <unfathomable pbr-related weirdness> ultimately resulting in the tests attempting to run functional rather than just unit. | |
| 15:35:06 | efried | jaypipes, mriedem, dansmith: What is it going to take to have sufficient confidence in any of the various attempts to reduce the number of redundant placement calls per periodic to actually merge code? | |
| 15:36:36 | efried | e.g. these two are quite similar | |
| 15:36:36 | efried | https://review.openstack.org/#/c/588091/ (removes the "first" _update) | |
| 15:36:36 | efried | https://review.openstack.org/#/c/587050/ (removes the "last" _update) | |
| 15:36:36 | efried | and are both showing green across the board test-wise. | |
| 15:37:46 | jaypipes | efried: which one do you prefer? | |
| 15:38:41 | efried | jaypipes: It makes little difference to me. The first one removes more lines of code. At the moment it's sitting on top of the reshaper series, but could be extracted easily. | |
| 15:42:11 | mriedem | idk | |
| 15:42:27 | mriedem | stuff like that likely won't show a side effect until we've merged it and run it awhile and found some weird failures, | |
| 15:42:33 | mriedem | which will likely be non-trivial to debug | |
| 15:42:38 | efried | just so. | |
| 15:42:43 | mriedem | so definitely not rocky | |
| 15:43:15 | mriedem | and i'd expect what you'd see are weird scheduling failures | |
| 15:43:20 | mriedem | due to some timing issue | |
| 15:43:34 | dansmith | and maybe not at all at gate-level scale | |
| 15:44:39 | mriedem | would be nice to have some company with a stress test lab kick around either of those to see what falls out | |
| 15:44:47 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Delete instance_group_member records from API DB during archive https://review.openstack.org/588943 | |
| 15:45:07 | mriedem | but i'm pretty sure at this point in my time working on openstack, no company has a stress test lab that shares results publicly :) | |
| 15:45:32 | mriedem | if we don't see it in the gate, | |
| 15:45:44 | mriedem | we'll see it 18-24 months from now when someone actually upgrades to use it and finds problems | |
| 15:45:50 | efried | right | |
| 15:46:02 | jaypipes | mriedem: 36-64 months from now. | |
| 15:46:11 | mriedem | well, cern would be the first to hit it probably | |
| 15:46:13 | efried | But even that is a big "maybe". | |
| 15:46:56 | mriedem | i also haven't had the time to read cdent's write up on this either yet | |
| 15:47:09 | efried | mriedem: TL;DR we're calling _update twice every periodic. | |
| 15:47:16 | mriedem | yeah i knew that much :) | |
| 15:47:24 | efried | which calls all the placement things to refresh the cache etc. | |
| 15:47:33 | efried | that's really all there is to it. | |
| 15:47:42 | cdent | mriedem: there's not much more than that what efried says, must more words to indicate the code path | |