| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-05 | |||
| 18:08:02 | bauzas | example https://stackoverflow.com/questions/15331726/how-does-the-functools-partial-work-in-python | |
| 18:08:44 | efried | return getattr(self.instance, name) | |
| 18:08:44 | efried | self.instance = self.klass(*self.args, **self.kwargs) | |
| 18:08:44 | efried | if self.instance is None: | |
| 18:08:44 | efried | def __getattr__(self, name): | |
| 18:08:44 | efried | bauzas: butbutbut, why wouldn't it work to just do this: | |
| 18:10:47 | efried | (FWIW, it seems to do what I need) | |
| 18:12:07 | bauzas | yup, but then that's not a lazy loader ;) | |
| 18:12:23 | efried | bauzas: Say wha? | |
| 18:13:23 | efried | bauzas: The only difference in laziness is if a caller pulled a method but didn't call it. | |
| 18:13:32 | bauzas | efried: calling __getattr__ in your case will run getattr() of the instance, right? | |
| 18:13:47 | efried | yes | |
| 18:14:07 | bauzas | so, it's executing synchronously | |
| 18:14:28 | bauzas | the problem is when importing the module | |
| 18:14:46 | bauzas | if you don't lazy load it, then you're loopîng | |
| 18:15:09 | bauzas | see why I lazy-load it below | |
| 18:15:14 | efried | You mean getattr looping on itself? | |
| 18:15:25 | bauzas | nope | |
| 18:15:52 | bauzas | we will only import the modules when we call select_destinations() | |
| 18:16:14 | bauzas | *not when we import nova.scheduler.client* :) | |
| 18:16:40 | efried | that... still happens? | |
| 18:17:15 | efried | Never mind about why it matters that we wait to import the report client... | |
| 18:17:19 | bauzas | efried: see this blogpost https://snarky.ca/lazy-importing-in-python-3-7/ | |
| 18:19:08 | bauzas | efried: anyway, test it | |
| 18:19:22 | bauzas | efried: when I wrote this, we were having an import loop | |
| 18:19:29 | bauzas | hence the lazyload | |
| 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 | |