Earlier  
Later
New Search
Posted Nick Remark
#openstack-sdks - 2019-06-05
20:40:02 efried For (2) I think we should let the exception raise.
20:40:27 mordred yes - but we should be sure to provide good errors when we do
20:40:43 efried Today, for both paths, we allow the Connection with default adapter settings.
20:40:49 efried That seems... not good.
20:41:36 efried I would be able to manage this, but hamstrung by the fact that we don't want to import oslo.config on the prod side.
20:41:55 efried Options:
20:42:01 mordred we can import it behind protection
20:42:04 efried 1) import oslo.config in from_conf
20:42:18 efried because if you're using from_conf presumably you've got it installed somewhere
20:42:36 efried 2) string scrape exception class.__name__ or similar (ew)
20:42:44 mordred yup! and it should be fine to import it there - or even just a try import block at top
20:43:13 mordred nah - I think it's totally fine to import oslo.config for the purposes of from_conf - we just dont' want people who are using sdk for ansible playbooks to need to install oslo.config
20:43:28 efried okay, I'll go with it. Thanks
20:43:45 efried um, one more thing
20:43:51 efried How do I prevent Connection?
20:43:55 efried for a given service?
20:44:29 mordred sure thing! also - we should maybe consider triggering an attempted auth or something in from_conf - since we otherwise only create the adapters on-demand (for things like OSC) - but I imagine in nova getting the error earlier rather than later would be better
20:45:12 mordred efried: that's a great question ... we have a has_{service} structure so that config can assert that we don't have a service - but it's not plumbed in to the Connection/adapter code
20:45:17 mordred now that you mention it - it should be :)
20:45:26 mordred I can make a patch for that
20:48:43 efried mordred: My dumb approach would be to make ConnectionMeta.__new__ return None if not any(key.startswith(service_type + '_') for key in self.config)
20:48:58 efried ...but only if we came from_conf
20:49:34 mordred efried: so - in Connection, we have has_service which does: if not self.config.config.get('has_%s' % service_key, True): - which we use in other places
20:50:51 mordred I'm thinking perhaps codify that a little bit, add a has_service on CloudRegion, then in from_conf we can add an else condition on if project_name not in conf: which sets has_{service_type} to false for that service type
20:50:52 efried o, so dumb second iteration, ConnectionMeta.__new__ returns None if not has_service(service_type) ?
20:51:02 mordred then in __new__ ... yeah, does that ^^
20:51:57 mordred that way it's a broadly applicable flag people can use to fully disable something if they need to for whatever reason
20:52:17 mordred (although as a followup we could get fancy and return an object that throws an exception in its __getattr__)
20:52:29 efried yeah, was just thinking that
20:52:45 mordred because "None object has no attribute servers" is always a crappy error message
20:52:51 efried yeah
20:53:16 efried fancier, make the exception message describe why we disabled the service
20:53:22 mordred yeah!
20:53:30 efried I'm getting all woozy just thinking about it.
20:53:33 mordred even fancier - make the exception install the service for you
20:53:47 efried wait, that's what we have today
20:53:56 efried more or less
20:53:57 mordred "you tried to use barbican, which was not enabled, so I installed barbican on some vms and registered them with your keystone catalog for you"
20:54:06 efried heh
20:54:16 mordred wcpgw?
20:54:45 efried So I think what I want is a CloudRegion._disable_service(service_type, reason=None)
20:54:55 mordred yeah
20:55:06 mordred totally
20:55:36 efried or maybe that's in Connection.
20:55:51 efried and I guess in the from_conf branch of Connection, I parse the config and call _disable_service for any service that I didn't register
20:56:41 efried or are you saying adding has_${service_type}=False in the config will give me all of that?
20:57:16 efried cause yeah, sure would prefer to do the work from from_conf
20:59:39 efried except ExceptionsFromOsloConfig as e:
20:59:39 efried log.warning("Disabling service %s because %s", st, e)
20:59:39 efried opt_dict['has_' + st] = False
20:59:39 efried opt_dict[st + '_disabled_reason'] = "Encountered an exception trying to process ksa configs: %s" % e
21:00:10 efried mordred: can you make it so I can do that ^ and everything else will magically come together?
21:00:22 mordred efried: yes. well - I'm saying adding has_{service_type}=False SHOULD give you all of that
21:00:29 mordred and yes - I'll work on that patch
21:00:38 efried noyce.
21:00:57 efried I was going through dtantsur|afk's comments, thinking I could submit a small and simple fup patch
21:01:03 efried and here we are.
21:01:15 efried I'll try to go through the *rest* of the comments and see what else shakes out.
21:05:35 mordred efried: unfortunately (or fortunately, depending on optimism or pessimism) dtantsur|afk is very good at leaving important comments
21:06:02 efried mordred: Well, I knew we had a hole there; he just reminded me by suggesting we log something in that space.
21:06:02 openstackgerrit Monty Taylor proposed openstack/openstacksdk master: Swap default of use_direct_get https://review.opendev.org/663429
21:23:05 openstackgerrit Monty Taylor proposed openstack/openstacksdk master: WIP Plumb service disabling into Connection adapters https://review.opendev.org/663435
21:23:10 mordred efried: something like that ^^
21:23:29 mordred efried: work still needed, but I think that's mostly what would be needed for what we need in from_conf
21:23:48 efried mordred: I'll have the test cases up in a sec...
21:23:56 mordred sweet
21:24:17 mordred efried: if we don't watch out - this whole this is going to become viable
21:24:22 mordred then what will we be able to complain about?
21:24:28 efried oh, I have faith
21:30:58 openstackgerrit Eric Fried proposed openstack/openstacksdk master: (Broken) from_conf test paths for oslo.config exceptions https://review.opendev.org/663439
21:31:05 efried mordred: see how that grabs ya ^
21:31:40 mordred sweet
21:33:19 efried I'm going to put those two in series and try to put a fix on top.
21:34:09 mordred that's exciting
21:35:05 efried it's scary, but somewhat liberating [1], to be throwing all these patches around without tracking bugs/stories against them
21:35:05 efried [1] like I imagine skydiving to be
21:38:20 efried mordred: If instead of raising right away, we just disable service for both cases (1) and (2) listed above, I can get away without importing oslo.config...
21:40:25 mordred efried: oh neat. then we could just do the "raise a useful error instead of None" patch - and further improvements can be around providing better/earlier errors
21:40:39 efried yuh
22:03:45 openstackgerrit Eric Fried proposed openstack/openstacksdk master: WIP Plumb service disabling into Connection adapters https://review.opendev.org/663435
22:03:45 openstackgerrit Eric Fried proposed openstack/openstacksdk master: Disable service on exception in from_conf https://review.opendev.org/663449
22:03:50 efried mordred: thar she blows. Works like a charm.
22:04:18 efried mordred: I did a little refactoring of your pieces in there (which could probably be done in your patch instead of mine, but whatever)
22:21:20 mordred efried: \o/
22:21:31 efried mordred: I'm working on the "better exception" thing.
22:21:57 efried mordred: Not sure if you'll want different/additional testing from what I'm doing.
22:22:10 mordred I'm sure what you're doing is perfect
22:22:29 efried oh, I'm sure.
22:25:50 openstackgerrit Eric Fried proposed openstack/openstacksdk master: Show a pretty reason when service is disabled https://review.opendev.org/663453
22:26:08 efried mordred: ^
22:27:46 mordred efried: dude, that's so cool
22:27:52 efried :)
22:33:56 efried mordred: Hm, if you're feeling generous, it's possible we could consider the testing in the top two patches as sufficient for your otherwise-WIP.
22:34:42 efried so like, squash or reorganize and we're done?
22:35:59 mordred efried: yah - honestly, I think that's probably right. you have a sec to do that squash? (maybe pick the change-id from one of your changes, since those were the "point" of the work)
22:36:25 efried yeah, you want me to just squash them all into one?
22:37:18 mordred yeah - why not - I think it might be easier to understand as one big one
22:38:50 efried on it
22:41:24 efried gosh, I thought I was going to have a really long commit message, but from the pov of a single patch, it's fairly simple :)
22:46:10 openstackgerrit Eric Fried proposed openstack/openstacksdk master: Handle oslo.config exceptions in from_conf https://review.opendev.org/663439

Earlier   Later