Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-11-05
18:20:30 bauzas if you have concerns by this class
18:20:39 bauzas anyway, 7:20pm here
18:20:40 bauzas ++
18:20:54 efried def __init__(self):
18:20:54 efried self.queryclient = LazyLoader(importutils.import_class(
18:20:54 efried 'nova.scheduler.client.query.SchedulerQueryClient'))
18:20:54 efried self.reportclient = LazyLoader(importutils.import_class(
18:20:54 efried 'nova.scheduler.client.report.SchedulerReportClient'))
18:21:07 efried Pretty sure that -^ is actually doing the import right away.
18:21:47 efried You're calling LazyLoader with the *result* of running importutils.import_class(), which is the imported class.
18:24:06 lbragstad melwitt that's a good question, verify is going to need the current usage and limit information, which means it might need to use etcd
18:24:10 lbragstad to get that information
18:24:18 cdent efried: sure looks that way to me too
18:24:27 cdent the reason it avod the import loop is simply because it is called "later"
18:24:34 cdent but it isn't lazy
18:24:43 efried yuh. I guess I need to pull this part out into a separate patch, sigh.
18:26:11 melwitt lbragstad: I was wondering, is verify() even useful though if we are distributed locking on enforce + create? I dunno, just unhelpfully thinking aloud :P
18:26:56 lbragstad oh... i missed the distributed lock part
18:27:17 lbragstad using etcd as a distributed lock would prevent the need for verify(), then?
18:30:15 melwitt that's what I was thinking. this came up at... was it the forum in vancouver? someone asked why do the recheck thing to avoid races, why not use distributed locking instead, and people had some ideas about using etcd as part of distributed locking. and then we were thinking that could be just one of many potential approaches that oslo.limit could provide underneath
18:30:30 melwitt *to handle races
18:31:08 lbragstad oh - sure
18:31:19 lbragstad i haven't thought about that specifically, yet
18:31:22 melwitt and I was just thinking whether that affects the API we're thinking. maybe it would just make verify() optional in the case of that backend
18:31:26 efried cdent, bauzas: oic now. The lazy loader is deferring the *instantiation* of the class until some method is called on it. The import of the module is still happening as soon as you instantiate SchedulerClient. But the arg being passed to the LazyLoader is the class object, not an instance of that class.
18:31:26 efried And IMO that doesn't really save you anything over just instantiating the class right away in the SchedulerClient init.
18:31:40 cdent right
18:32:02 cdent It's not clear why you'd want to wait
18:32:08 melwitt lbragstad: yeah. too early to think too much about it but at the same time, throwing it out there in case it would cause a big problem with what we're thinking about for now
18:32:08 lbragstad in that same session i want to say we were talking about using etcd as a way to communicate events between nova (oslo.limit) and keystone
18:32:20 melwitt ah, gotcha
18:32:55 lbragstad because - once you go past the 2nd layer of projects, it becomes harder to make assumptions about the resources in the tree, i think
18:33:29 dansmith and if you take the locking approach,
18:33:51 dansmith you have to figure out how high to lock
18:34:13 lbragstad http://specs.openstack.org/openstack/keystone-specs/specs/keystone/rocky/strict-two-level-enforcement-model.html#limit-and-usage-awareness-across-endpoints
18:34:25 lbragstad dansmith yeah - that's another important bit
18:35:45 melwitt lbragstad: thanks for the link, I had missed that
18:36:32 lbragstad yeah, turns out that was an important bit to include as justification for why we decided to keep things at 2 levels
19:30:24 openstackgerrit Jay Pipes proposed openstack/nova master: quota: rename arguments to clarify they are limits https://review.openstack.org/615633
19:31:21 lbragstad does nova allow users to create multiple instances in multiple projects in one request?
19:31:39 melwitt no
19:31:51 lbragstad ok - cool, just wanted to double check
19:33:38 openstackgerrit Merged openstack/nova master: Minimal construct plumbing for nova service-list when a cell is down https://review.openstack.org/584829
19:38:02 openstackgerrit Matt Riedemann proposed openstack/nova master: Update compute API.get() mocks in test_server_metadata https://review.openstack.org/615341
19:38:02 openstackgerrit Matt Riedemann proposed openstack/nova master: Update compute API.get() stubs in test_serversV21 https://review.openstack.org/615342
19:38:03 openstackgerrit Matt Riedemann proposed openstack/nova master: Update compute API.get() stubs in test_server_actions https://review.openstack.org/615343
19:38:03 openstackgerrit Matt Riedemann proposed openstack/nova master: Update compute API.get() stubs for test_*security_groups https://review.openstack.org/615344
19:38:04 openstackgerrit Matt Riedemann proposed openstack/nova master: Update compute API.get() stubs for test_disk_config https://review.openstack.org/615345
19:38:05 openstackgerrit Matt Riedemann proposed openstack/nova master: Update compute API.get() stubs in test_access_ips https://review.openstack.org/615346
19:38:05 openstackgerrit Matt Riedemann proposed openstack/nova master: Drop pre-cellsv2 compat in compute API.get() https://review.openstack.org/615347
19:38:05 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove "API Service Version" upgrade check https://review.openstack.org/615348
20:07:32 mriedem i'm pretty sure we don't cleanup allocations held by a migration record if we delete a server while it's in VERIFY_RESIZE status, but my recreate test isn't failing for that, and i don't see where in the code we'd cleanup allocatoins for migration records when deleting a server
20:09:08 cdent mriedem: I told someone internally to make a bug about that recently
20:09:22 mriedem it came to mind in the ML thread with mnaser about leaked/incorrect allocations
20:09:29 mriedem but can't seem to reproduce it
20:09:32 cdent (upstream bug) but guess they didn't.
20:09:52 cdent I think they may have convinced themselves it wasn't happening. I'll see if I can find a reference
20:10:04 mriedem i can't see how it *can't* be happening
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.

Earlier   Later