Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-09-26
19:30:19 mriedem i wrote those tests
19:30:29 mriedem so please explain how i'm wrong that they are now made redundant in that change
19:30:36 jaypipes mriedem: that test raising TestingException was not useful. Because TestingException isn't what is ever raised by any code.
19:30:54 mriedem it's simulating the virt driver raising the error during rebuild
19:31:00 mriedem AFTER the successful claim
19:31:10 mriedem it could be ProcessExecutionError
19:31:12 mriedem from driver.spawn()
19:31:14 mriedem if you like
19:31:44 mriedem these 2 tests are testing very specific failures
19:31:45 dansmith mriedem: right but we don't run the claim teardown code in that case
19:32:00 mriedem dansmith: correct, which is why we run the allocation cleanup manually
19:32:03 mriedem and that's what that is testing
19:32:32 mriedem the test you changed isn't meant to test drop_move_claim
19:32:35 mriedem the docstring explains that
19:32:44 jaypipes mriedem: if the point of the test (as is in that docstring) is to ensure allocations are cleaned up after a failed rebuild, then the test should raise the exception that would be raised *after* a claim has been made for the new resources.
19:32:55 dansmith jaypipes: he's saying another one does that
19:33:14 mriedem jaypipes: you realize the virt drivers can raise any kinds of crazy shit right?
19:33:15 dansmith mriedem: so in this case you want the test to validate that the allocations _don't_ get cleaned up is that right?
19:33:31 jaypipes mriedem: Matt, I'm trying to be civil.
19:34:38 mriedem https://review.openstack.org/#/c/499877/
19:35:22 dansmith this is what it's testing: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2800-L2827
19:35:23 dansmith the except exception case of that
19:35:24 mriedem so ^ is testing that drop_move_claim removes the allocation when the claim was successful but the virt driver raised some exception
19:35:37 jaypipes mriedem: OK, I see that now.
19:36:18 mriedem https://review.openstack.org/#/c/499874/ added the other test
19:36:54 mriedem that was a recreate test for a bug
19:36:55 mriedem fixed in https://review.openstack.org/#/c/499878/
19:37:38 dansmith mriedem: we get it
19:37:51 dansmith mriedem: can you answer my question above about what you want it to do?
19:39:32 mriedem the test should go back to whatever it was testing
19:39:43 mriedem which is the case that the claim passes, but the virt driver raises
19:39:51 mriedem so we'd remove the allocation via drop_move_claim before
19:40:02 dansmith right, but you assert some behavior that happens inside drop_move_claim
19:40:07 dansmith which no longer happens
19:40:35 mriedem then that drop_move_claim behavior has to be replayed elsewhere i guess
19:40:37 jaypipes what if the it's a same-host rebuild? :(
19:40:45 mriedem there is no claim for a same host rebuild
19:40:49 jaypipes k
19:40:54 mriedem so you wouldn't hit ComputeResourcesUnavailable
19:41:06 dansmith exactly, but you could hit other exceptions
19:41:21 jaypipes mriedem: but you *would* hit the TestingException "crazy shit"
19:41:40 jaypipes mriedem: and you're asserting that we'd delete the allocation against the instance in that case, right?
19:41:43 mriedem that doesn't have anything to do with dropping an allocation though
19:41:46 mriedem no
19:42:19 jaypipes mriedem: oh, sorry, you're asserting that the *update_available_resource()* call would clean up allocations for a failed build?
19:42:22 jaypipes rebuild.
19:42:26 mriedem no
19:42:29 dansmith no
19:42:33 jaypipes guh
19:42:40 dansmith the test _is_ asserting that the dest host's allocation was cleaned up by drop_move_claim
19:42:43 mriedem we don't ever want to remove allocations for a *rebuild*
19:42:55 mriedem the tests are specifically for evacuate
19:43:01 mriedem where the scheduler creates allocations on the dest host
19:43:13 mriedem we fail the evacuate on the dest host, so we need to remove those allocatoins created by the scheduler
19:44:04 dansmith there's a specific reason why I made this change,
19:44:11 dansmith and I talked it through with jaypipes which is why I made this
19:44:11 mriedem i'm sorry for being grouchy about this,
19:44:23 mriedem but i've spent the better part of the last 6 weeks fixing these allocation bugs,
19:44:26 dansmith so I'll have to go re-load all my context on this before I can really think about it
19:44:29 mriedem so being told i don't understand the test pisses me off
19:44:36 jaypipes mriedem: understood. and you're saying that you want the drop_move_claim() to remove those resources when ComputeResourcesUnvailable is raised but you want update_available_resource() to delete the allocations when a virt driver exception is raised?
19:44:47 dansmith jaypipes: no
19:45:48 dansmith mriedem: without offending your test sensibilities, you see this right? https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2812
19:45:59 dansmith that's what *should* have been deleting the target's allocation
19:46:05 dansmith only in the unavailable case
19:46:16 dansmith but in reality, we were always doing it for the other cases as well
19:47:14 mriedem how?
19:47:30 dansmith how? because we always ran drop_move_claim
19:48:16 dansmith reset the test to where it was and run it with the oddball exception and we'll assert that the dest host claim is zero, but it no longer is after this change
19:48:40 dansmith this: https://pastebin.com/fMUmmgMC
19:49:12 mriedem i don't know if we're talking about the same thing,
19:49:23 mriedem i wrote https://review.openstack.org/#/c/499877/ to show that we didn't need to manually remove allocations on the dest when the driver failed
19:49:27 mriedem because of drop_move_claim
19:49:42 mriedem the other test was to show that we needed to add https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2812
19:49:45 mriedem when the claim itself fails
19:49:51 mriedem raising ComputeResourcesUnavailable
19:49:51 dansmith right, but that doesn't make sense right?
19:50:08 dansmith if we fail for some driver reason, we're now "on" that dest host and should be able to run a same-host rebuild on it
19:50:11 dansmith which won't re-claim for us
19:50:26 mriedem no, we're not on that host
19:50:32 mriedem if driver.spawn fails, we're not on that host
19:50:43 mriedem the instance is only on the dest host if we get here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2836
19:50:48 mriedem which doesn't happen if the driver fails
19:50:58 dansmith why is this here? https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2812
19:51:15 mriedem see https://review.openstack.org/#/c/499878/
19:51:40 mriedem plus the comment above it
19:54:32 mriedem so i don't know what else is going on in this patch, i didn't get that far, i saw the commit message and change to the test and wanted to bring that up since it's approved
19:54:57 dansmith mriedem: okay yeah I really thought that the rt claim would set host and node
19:55:07 dansmith mriedem: you better -2 that or something so it doesn't merge
19:55:18 mriedem i don't think that works
19:55:31 dansmith mriedem: -2 will make it not merge I think, it just won't kick it
19:55:39 openstackgerrit Matt Riedemann proposed openstack/nova master: Move allocation manipulation out of drop_move_claim() https://review.openstack.org/498947
19:55:45 openstackgerrit Dan Smith proposed openstack/nova master: BUMPMove allocation manipulation out of drop_move_claim() https://review.openstack.org/498947
19:55:47 mriedem i know a new commit will do it
19:55:49 mriedem heh
19:55:57 mikal I have a cold, hold me
19:56:05 mriedem gdi mikal
19:56:11 mriedem you've stepped into the wrong room at the wrong time
19:56:17 mriedem way out of line donny
19:56:36 jaypipes you're outta your element, mikal

Earlier   Later