Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-08-08
17:44:50 jaypipes dansmith: this patch is mostly in response to mriedem and alex_xu's comments on https://review.openstack.org/#/c/488510/ ps 27.
17:45:12 jaypipes dansmith: i.e. this from mriedem:
17:45:12 jaypipes Related to #4, there is bug https://bugs.launchpad.net/nova/+bug/1679750 where we don't delete allocations from Placement in nova-api when doing a "local delete", i.e. when the compute host is down, or the instance does not have a host associated (e.g. shelved_offloaded state).
17:45:13 openstack Launchpad bug 1679750 in OpenStack Compute (nova) "Allocations are not cleaned up in placement for instance 'local delete' case" [Medium,In progress]
17:45:43 jaypipes dansmith: but I admit I've probably f'd this series up :(
17:45:54 jaypipes trying to fix these corner cases
17:46:06 dansmith jaypipes: the shelved offloaded thing in that bug is conjecture right?
17:46:29 jaypipes dansmith: yes, but listed by mriedem as something to handle
17:46:52 dansmith jaypipes: yeah, but maybe he didn't dig to see how we'd actually get here :)
17:47:08 jaypipes perhaps. this code is fugly, as you know.
17:47:35 mriedem 3 more times
17:47:39 dansmith jaypipes: that bug as written does't actually say anything about shelved, although I thought mriedem had said something about it
17:47:43 openstackgerrit Merged openstack/nova master: Handle ironicclient failures in Ironic driver https://review.openstack.org/487925
17:47:45 mriedem i forgot my kid at camp and they called
17:47:53 mriedem so i'll be paying for that in counseling sessions later
17:48:07 jaypipes mriedem: Dad of the Year.
17:48:11 mriedem tbc, i dropped that link in the change as an fyi
17:48:18 mriedem and listed the move operations
17:48:36 mriedem i was not meaning that the bug had to be fixed in that change
17:48:59 dansmith mriedem: well, we're not sure there is a bug
17:49:02 dansmith or, I'm not
17:49:06 jaypipes mriedem: sure. though the changes I put in there were also to highlight code paths that alex_xu had pointed out around evacuate and the local delete problem where the compute host was down.
17:49:16 dansmith if there is, I'm skeptical that offloaded and host==self is a case we can have :)
17:49:22 jaypipes in there == the new bottom patch on the series.
17:49:35 dansmith jaypipes: the local delete path should be deleting the allocation itself right?
17:49:42 dansmith so that there's nothing to clean up here, ideally
17:49:49 mriedem if the instance is offloaded, host is None
17:50:05 dansmith like, we shouldn't actually delete the instance unless we were able to nuke the allocation
17:50:08 mriedem the point of the bug was that yes we should delete allocations in the local delete case, if we care
17:50:13 dansmith right
17:50:19 dansmith so not in RT at all, IMHO
17:50:30 jaypipes dansmith: the local delete path never gets to the compute host, thus the need for the allocation to be deleted when the compute host starts back up and sees allocations for instances that no longer exist.
17:50:40 mriedem melwitt wrote a test that shows that even though we don't delete the allocations in the api in local delete cases, when the compute host comes back up and the instance is gone, the allocation is deleted by the compute
17:50:44 dansmith jaypipes: no dude, delete it from the api :)
17:50:47 mriedem now ^ might be impacted by whatever you guys are doing
17:50:58 dansmith jaypipes: and don't mark the instance as deleted until you succeeded, or there isn't an allocation
17:51:08 mriedem this https://review.openstack.org/#/c/470578/
17:51:09 dansmith jaypipes: then there's less complexity for the compute node to handle
17:51:33 jaypipes dansmith: oh, you mean delete the allocation from placement during the nova-api's "local delete" operation?
17:52:02 jaypipes dansmith: we'd still need to deal with ocata apis that didn't do that though ;) yay.
17:52:03 dansmith jaypipes: yeah, we delete anything we can from there without the compute node,
17:52:04 dansmith which is kinda the point of it
17:52:10 dansmith since we can totes nuke the allocation we're good
17:52:16 dansmith jaypipes: why?
17:52:28 dansmith jaypipes: if we nuke it from api, we have to be pike, an ocata compute isn't going to care, right?
17:52:36 jaypipes dansmith: are you saying we *currently* delete the allocation from the API?
17:52:51 dansmith no I'm saying we should do that, and that's what the bug is about
17:53:04 dansmith mriedem: right?
17:54:00 jaypipes dansmith: if an ocata api local-deleted an instance, then is upgraded to pike, the compute host is upgraded to pike as well, wouldn't there be an allocation left over for the compute host that would need deleting?
17:54:36 mriedem melwitt: question in https://review.openstack.org/#/c/470578/
17:54:52 dansmith jaypipes: sure, but you're doing that right?
17:54:58 melwitt mriedem: I'll add console proxy stuff to the cells docs. I was thinking to mention that "in the future" we're planning to change the location, so people have a heads up
17:55:10 mriedem dansmith: correct
17:55:30 melwitt mriedem: also thanks for rebasing those backport series. didn't even get a chance to ask you yet :)
17:55:30 mriedem during local delete in the API, we delete shit from external services like cinder/neutron because the compute is down or the instance doesn't have a host (it's offloaded)
17:55:37 mriedem placement would be included in "external shit"
17:55:42 jaypipes dansmith: heh, yes, I am. sorry, I thought you were saying there'd be no need for that code.
17:56:22 dansmith jaypipes: no, I don't think there's a need for the shelve offload part I commented on, but for local delete we should hope to never hit this code if we deleted from the api as expected
17:56:30 jaypipes ahhh, sorry.
17:57:08 dansmith jaypipes: if there is an allocation for us that refers to an instance that is marked as deleted, then we should delete the allocation
17:57:19 mriedem when we shelve offload an instance, the allocations for that compute node should be cleaned up by the RT
17:57:24 dansmith jaypipes: note my comment about notfound for later though
17:57:26 melwitt mriedem: to your question, yeah it seems like it would be racy. not sure what else to do though.
17:57:43 dansmith mriedem: we clean the allocations before we offload it, so I don't think we need to handle cleanup there
17:57:58 mriedem dansmith: via RT yeah?
17:58:03 dansmith mriedem: otherwise we couldn't have been offloaded
17:58:24 mriedem melwitt: you could call the update_available_resource method directly
17:58:26 dansmith mriedem: yeah we call direct to self._update or wahtever
17:58:32 mriedem self.service.manager.update_available_resource or whatever
17:58:41 melwitt oh, okay
17:58:44 dansmith mriedem: yeah, I assumed you mean the periodic osrry
17:58:57 mriedem https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4345
17:59:12 mriedem right it calls rt.update_usages
17:59:14 mriedem *usage
17:59:22 mriedem and the offloaded state is in the ALLOW_REMOVAL_STATES or whatever
17:59:45 mriedem so the only case for local delete of allocatoins in the api is when the instance isn't shelved offloaded but the compute service is down
18:10:23 mriedem jaypipes: tbc, i dont know that we need to handle the local delete in the api thing right now
18:10:52 mriedem unless the computes won't cleanup those allocations when they come back up
18:11:55 cdent I’d like it if we could merge some of the pending code as we have several in flight bug fixes, all vaguely interrelated, and it is hard to find yet more bugs while those are still in the way (and not all in the same stack). We may just need to be willing to have some bugs in master (we already do) so we can find and fix more of them.
18:13:37 dansmith mriedem: jaypipes: agreed
18:15:46 jaypipes dansmith, mriedem: so, bottom line, the changes in https://review.openstack.org/#/c/491850/ are fundamentally OK aside from some of the typos dansmith noted?
18:18:18 mriedem i'll have to go through it,
18:18:24 mriedem i've been waiting for dansmith to be ok with things
18:22:03 mriedem i need to push up something quick and then i'll take a look
18:25:40 dansmith jaypipes: not just typos right?
18:25:52 dansmith or are you going to leave the shelve_offloaded thing?
18:26:09 jaypipes dansmith: I can remove that, sure.
18:26:34 dansmith I think we should remove it if we can't explain it and deal with it later if we determine there's a gap there
18:26:42 dansmith it'll be a shelve gap, of which there are many
18:26:59 dansmith jaypipes: I say clean it up and let mriedem take a look fresh
18:27:09 dansmith not like we're going to get check results any time soon anyway
18:27:18 jaypipes kk
18:27:36 melwitt this is interesting. I'm running the test_boot_from_volume func test after rebasing and it used to allow an in-place resize (1 vcpu to 1 vcpu on a host with only 1 vcpu) but now it's saying it needs 2 vcpus to do the resize
18:28:33 mriedem melwitt: yup :)
18:28:41 mriedem we double the allocations in the scheduler
18:28:50 mriedem well, sum rather than take the max
18:29:11 mriedem https://review.openstack.org/#/c/490085/
18:29:38 melwitt yeah, I remember seeing some mention of that in the channel chats. I guess I'm surprised at the behavior change? I didn't think we'd want to change how that works because users will hit this
18:29:59 melwitt well, I guess maybe resize on same host isn't too common so maybe they won't hit it
18:30:58 mriedem dansmith and i talked about why sum vs max() in https://review.openstack.org/#/c/490085/ but now i can't really remember the reasoning

Earlier   Later