Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-05
20:10:10 mriedem if you delete while in VERIFY_RESIZE status
20:10:56 openstackgerrit Eric Fried proposed openstack/nova master: Remove LazyLoad of Scheduler Clients https://review.openstack.org/615641
20:11:19 efried bauzas, johnthetubaguy, cdent: I couldn't find a circular import, but I guess CI will tell ^
20:12:23 mnaser I’m curious if deleting an instance in ERROR state after failed live migrate or resize doesn’t delete it too
20:12:35 mriedem as far as i can tell, for a resize, we only cleanup the allocations held by the migration record when confirming the server (we delete the source node allocations held by the migration) or on revert we swap the allocations held by the migratoin record on the source node with the instance consumer and drop the allocations held by the instance on the target node
20:13:11 mriedem mnaser: we should always cleanup allocations held by at least the instance, even if error state, either on the compute or in the api (if the compute is down)
20:13:20 mriedem i'm more worried that we're leaking allocations held by the migration record
20:14:12 melwitt yeah, we should be ok on the delete in error state case (the local delete path) will take care of the instance allocation
20:14:30 melwitt but I agree, I'm not seeing where we take care of the migration related allocations
20:14:42 mriedem i do not see where _rollback_live_migration cleans up allocations held by the migration record
20:15:14 mnaser I’ve definitely seen it log the rollback message
20:15:37 mnaser in terms of removing destination allocation
20:19:19 mriedem oh there it is for rollback
20:20:01 mriedem https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6796
20:20:25 mriedem where i'd expect migration allocations to get cleaned up is here https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L761
20:20:32 mriedem on server delete i mean
20:23:44 melwitt yeah. guess VERIFY_RESIZE is the only case where we'd have the situation of having outstanding migration allocations? but you said you are trying to recreate the bug and not seeing outstanding migration allocations when the instance is in VERIFY_RESIZE :\
20:24:13 mriedem correct
20:24:16 mriedem i can push the test up
20:24:21 mriedem maybe my test is busted
20:26:23 melwitt I'm wondering if there's maybe something different about how migrations are handled in a functional test environment (like is anything being faked in a way that covers it up?). but I thought we've been able to demonstrate allocation cleanup bugs with func tests before
20:27:27 cdent mriedem: you could do a pure api driven integration test as a child of https://review.openstack.org/#/c/613386/ pretty easy/quick
20:27:31 openstackgerrit Lance Bragstad proposed openstack/nova master: WIP: experiment with oslo.limit interface https://review.openstack.org/615602
20:27:53 mriedem does that have 2 nodes?
20:28:13 mriedem $.hypervisors.`len`: 1
20:28:14 mriedem nope
20:29:00 cdent oh yeah, that
20:29:15 mriedem resize to same host will also create migration-based allocations (that's a separate bug)
20:29:34 mriedem so it's still probably doable with your thing, but what i've got (in functional tests with python) is easier/faster for me
20:29:55 cdent this was in response to melwitt suggesting that there was some change that functional might be doing something odd
20:30:25 mriedem the functional tests assert that the source node contains the migration allocations after the resize,
20:30:27 mriedem so i think they are ok,
20:30:38 mriedem they just aren't asserting the migration allocations are removed after the server is deleted
20:30:47 lbragstad melwitt jaypipes johnthetubaguy https://review.openstack.org/#/c/615643/ is a quick stab at the oslo.limit changes we talked about (sans requiring a context manager in the initial implementation)
20:30:48 mriedem nor do i see tests that delete the server while it's in VERIFY_RESIZE state
20:31:04 lbragstad https://review.openstack.org/#/c/615602/ is a nova patch based on johnthetubaguy's that tries to use the new changes
20:34:07 melwitt cool lbragstad
20:35:06 openstackgerrit Matt Riedemann proposed openstack/nova master: Add functional test to delete a server while in VERIFY_RESIZE https://review.openstack.org/615644
20:35:08 jaypipes lbragstad: I'm not really following the oslo.limits part, frankly...
20:35:55 jaypipes lbragstad: one of the parts that is incredibly confusing and frustrating about the current nova quota engine is its coupling of limits with usage into the same objects (what it calls a "Quota")
20:36:06 openstackgerrit Eric Fried proposed openstack/nova master: SIGHUP n-cpu to refresh provider tree cache https://review.openstack.org/615646
20:36:14 jaypipes lbragstad: I was thinking that oslo.limits would stick to the limits stuff and stay out of the usage bits.
20:36:56 jaypipes lbragstad: in short, I don't think oslo.limits needs an Enforcer thing.
20:38:07 melwitt jaypipes: I had the same frustration and it didn't occur to me to rename things as you have :P
20:38:45 melwitt I've been pedantic on any new code comments or discussions we've had on the difference between limits and usage
20:41:01 melwitt but, I think with oslo.limit the idea was to try and abstract away as much as possible and let it handle the hierarchical enforcement, and all you do is provide it a callback
20:46:37 lbragstad oh - sure...
20:46:45 lbragstad i can see where that can get muddy
20:47:18 lbragstad i guess the only reason we have usage in the oslo.limits stuff is because we need to calculate enforcement, which checks what users are asking for against what they already have
20:50:49 lbragstad and to melwitt's point, the callback is what oslo.limit is relying on for the usage of X in a given project
20:54:35 melwitt I think what jaypipes is saying is that it's not technically required for oslo.limit to take care of enforcement, since enforcement could be done in projects in whatever way they want. but I thought oslo.limit was also trying to provide a common and simple interface for every project to use and let the callbacks be the only pieces each project would have to write separately. and otherwise use the common oslo.limit interface
20:54:53 mriedem i think i found what cleans up the migration-based allocation during server delete of a VERIFY_RESIZE server
20:55:21 mriedem https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2096
20:56:10 melwitt mriedem: a-ha! nice find. what a sneaky code
20:56:34 melwitt never knew resizes were confirmed right before deleting
20:56:53 melwitt makes sense though
20:58:18 jaypipes lbragstad: I still don't see why oslo.limits needs to "calculate enforcement".
20:58:56 melwitt I don't think it "needs to" but if it does, it makes it so projects don't have to implement the same thing separately. they just have to write callbacks
20:59:39 melwitt maybe that commonized code is minimal though, which I guess is your point
20:59:55 lbragstad yeah - i think we were operating under the assumption that service would give oslo.limit a project_id + resource type and oslo.limit would return a yes or no based on the hierarchy and usage
21:00:35 lbragstad because we didn't want to make service re-implement logic to understand this complex project hierarchy
21:00:41 lbragstad services*
21:01:16 lbragstad but - putting that aside... how do you see nova's interaction with oslo.limit jaypipes?
21:01:25 openstackgerrit Matt Riedemann proposed openstack/nova master: Add functional test to delete a server while in VERIFY_RESIZE https://review.openstack.org/615644
21:03:32 jaypipes lbragstad: I see oslo.limit as basically the client for Keystone's GET /limits API.
21:03:58 jaypipes lbragstad: and maybe the definition of some common objects. that's about it.
21:04:06 lbragstad ok
21:04:31 lbragstad so - to be clear, you except oslo.limit to still handle the tree of projects from keystone and their respective limits?
21:04:42 lbragstad expect*
21:21:09 jaypipes lbragstad: I don't necessarily think so, no...
21:21:38 jaypipes lbragstad: all I need is a flattened dict of project limit amounts for a set of resource types
21:22:14 jaypipes lbragstad: I figured Keystone would do the needful when it came to flattening the returned dict of resource type to limit amount.
21:22:44 jaypipes lbragstad: still trying to finish up this code. I hope things will be clearer with the last patch in this series.
21:22:55 lbragstad ack
21:23:05 lbragstad we kinda do that already - http://specs.openstack.org/openstack/keystone-specs/specs/keystone/rocky/strict-two-level-enforcement-model.html#fetching-project-hierarchy
21:23:11 lbragstad which was required for the limit work
21:43:16 openstackgerrit Matt Riedemann proposed openstack/nova-specs master: Add support for emulated virtual TPM https://review.openstack.org/571111
21:56:54 cfriesen mriedem: thanks for that tweak ^
22:10:12 openstackgerrit Eric Fried proposed openstack/nova master: WIP: update_from_provider_tree: fast fail, clear cache https://review.openstack.org/615677
22:11:49 mriedem efried: cdent: jaypipes: have we found a compromise on https://review.openstack.org/#/c/570847/10/nova/rc_fields.py@45 ?
22:12:01 mriedem NET_BW_EGR_KILOBIT_PER_SEC and NET_BW_IGR_KILOBIT_PER_SEC ?
22:14:00 cdent mriedem: ooph, I had managed to forget about that
22:14:17 mriedem i'm rebasing his series so if we are cool with that naming then i'll make the change
22:17:44 cdent did we consider NET_BW_EGRESS_KBPS or is that too ambiguous for bits and bytes? (although historicall *bps has always been bits?)
22:18:09 cdent mriedem: I don't have a huge opinion on that actual name, just that the symbol and the string in the canonical location be the same thing
22:18:31 cdent given the canonical location is going to change soonish *shrug*
22:19:47 mriedem gibi mentioned the bits vs bytes thing
22:20:10 mriedem "I needed to include the unit of the resource as it is not trivial and as the name needs to be all upper case the bit and byte difference (the capital B in KB means byte officially) in the unit cannot be expressed if abbreviated."
22:21:58 mriedem i'll just change to NET_BW_EGR_KILOBIT_PER_SEC and NET_BW_IGR_KILOBIT_PER_SEC so we can shit and get off the pot
22:22:34 cdent wfm
22:30:13 efried since I think the discussion is all my fault, I might as well weigh back in.
22:30:13 efried I don't see why the const has to be the same as the string.
22:30:13 efried But jaypipes has a point that you can always alias it if you need to.
22:30:13 efried So effit, we can just go back to the original names and suck up literally half the line width Python allows us.
22:32:24 mriedem no backsies now
22:32:26 mriedem i've already changed it
22:39:37 efried idgas
22:47:47 openstackgerrit Matt Riedemann proposed openstack/nova master: Add request_spec.RequestGroup versioned object https://review.openstack.org/568840
22:47:48 openstackgerrit Matt Riedemann proposed openstack/nova master: Add requested_resources field to RequestSpec https://review.openstack.org/567267
22:47:48 openstackgerrit Matt Riedemann proposed openstack/nova master: Add bandwidth related standard resource classes https://review.openstack.org/570847

Earlier   Later