| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-05 | |||
| 18:19:32 | efried | bauzas: Test it how? Is there test code somewhere that verifies that the importing is being done lazily? | |
| 18:19:43 | bauzas | but now we changed a lot of the client, so maybe it's not needed | |
| 18:20:01 | bauzas | efried: you can pdb it, right? | |
| 18:20:11 | efried | I would think so. | |
| 18:20:19 | bauzas | or you can just remove the lazyloader and test whether we still have the import loop | |
| 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 | 'nova.scheduler.client.report.SchedulerReportClient')) | |
| 18:20:54 | efried | self.reportclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | 'nova.scheduler.client.query.SchedulerQueryClient')) | |
| 18:20:54 | efried | self.queryclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | def __init__(self): | |
| 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 | And IMO that doesn't really save you anything over just instantiating the class right away in the SchedulerClient init. | |
| 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:40 | cdent | right | |
| 18:32:02 | cdent | It's not clear why you'd want to wait | |
| 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: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: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() stubs in test_serversV21 https://review.openstack.org/615342 | |
| 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:03 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Update compute API.get() stubs for test_*security_groups https://review.openstack.org/615344 | |
| 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: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: Remove "API Service Version" upgrade check https://review.openstack.org/615348 | |
| 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: Update compute API.get() stubs in test_access_ips https://review.openstack.org/615346 | |
| 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 | |