| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2019-06-05 | |||
| 17:11:40 | Shrews | it would be nice behavior, i agree | |
| 17:12:54 | Shrews | mordred: +2'd the nodepool change | |
| 17:13:26 | mordred | \o/ | |
| 18:54:56 | openstackgerrit | Brian Haley proposed openstack/python-openstackclient master: Support IPv6 addresses better https://review.opendev.org/524420 | |
| 20:13:23 | efried | mordred: Do we not defer message format interpolation in logging in openstacksdk? | |
| 20:13:47 | mordred | efried: well - I mean, that's the intent - that doesn't mean we always do it | |
| 20:14:08 | efried | okay, I'll keep looking for examples. The one I happend to land on first, doesn't. | |
| 20:14:21 | efried | self.log.debug( | |
| 20:14:21 | efried | "Turning off SSL warnings for {full_name}" | |
| 20:14:21 | efried | " since verify=False".format(full_name=self.full_name)) | |
| 20:14:42 | mordred | yeah - it's entirely possible there are more places like that | |
| 20:37:53 | efried | mordred: I am unsatisfied with the current handling of "You've got a section called [foo] but when I try to load Adapter opts from it, I get an exception." | |
| 20:39:09 | efried | For example, I am able to trigger this code path in the following two ways: | |
| 20:39:09 | efried | (1) Register the [foo] group, but don't register ksa adapter opts in it | |
| 20:39:09 | efried | (2) Register [foo] and register ksa adapter opts in it, but f up the settings | |
| 20:39:38 | efried | For (1) I think we should make the Connection refuse to talk to foo.service_type | |
| 20:39:51 | efried | ...but otherwise that shouldn't be an exception. | |
| 20:40:00 | mordred | agree | |
| 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. | |