| opendevreview | Takashi Kajinami proposed openstack/oslo.messaging master: Add delete_rpc_server_queues https://review.opendev.org/c/openstack/oslo.messaging/+/1008831 | 03:18 |
|---|---|---|
| opendevreview | Merged openstack/oslo.log master: Drop version check for eventlet < 0.38.2 https://review.opendev.org/c/openstack/oslo.log/+/1008500 | 04:14 |
| opendevreview | Juan Larriba proposed openstack/oslo.middleware master: Use middleware-owned provider for API tracing https://review.opendev.org/c/openstack/oslo.middleware/+/1008816 | 08:52 |
| opendevreview | Merged openstack/oslo.messaging master: Make SSL protocol constant loading resilient to deprecations https://review.opendev.org/c/openstack/oslo.messaging/+/1007134 | 10:47 |
| amorin | I saw nothing about oslo.x on PTG, am I missing something? | 12:36 |
| cardoe | tkajinam: did you see the thread on the ML about RabbitMQ and oslo.messaging with the weird backtrace? It seems like we have some validation logic error with oslo.config? | 15:14 |
| tkajinam | cardoe, I guess that's the symptom caused by missing [oslo_concurrency] lock_path | 16:03 |
| tkajinam | but that stack trace is quite ugly and we should avoid it | 16:04 |
| tkajinam | or something is registering a OptGroup named "group" | 16:05 |
| tkajinam | in the DEFAULT section | 16:05 |
| tkajinam | cardoe, ok that's already mentioned. I'll try to look how we can resolve that ugly traceback | 16:06 |
| tkajinam | amorin, you are correct. tbh most of the oslo cores are part-time and almost independent, and idk if any of us has something we want to discuss as a team | 16:07 |
| amorin | ack, it's because I initiated a topic around replacing rabbitmq with blazingmq long time ago https://review.opendev.org/c/openstack/oslo.messaging/+/988211 . I mostly did that on my spare time, not related to my employer yet, but I was wondering if that would make sense to be pushed a little bit further | 16:13 |
| cardoe | tkajinam: yeah I'm just pinging you cause you usually figure out that magic. I got lost. :/ | 16:13 |
| tkajinam | I wonder if we should add a validation within queue manager to validate lock_path. I think that's a common problem people face when they try the feature first | 16:14 |
| tkajinam | though that may still trigger the same traceback | 16:14 |
| opendevreview | Clif Houck proposed openstack/oslo.config master: Add TypeRegistry to types module https://review.opendev.org/c/openstack/oslo.config/+/1009821 | 17:12 |
| opendevreview | Clif Houck proposed openstack/oslo.config master: Add type validation to validator tool https://review.opendev.org/c/openstack/oslo.config/+/1009822 | 17:12 |
| clif | o/ I just posted two patches that adds a TypeRegistry type in types.py of oslo.config and uses it to do type validation in the validator tool. If anyone has any out-of-band questions feel free to ping me. I'm happy to discuss. | 17:14 |
| clif | I would also like to get to the point where olso.config type-validates all loaded values before first-access if possible. | 17:15 |
| tkajinam | clif, maybe it'd be useful if you can describe a few scenarios this is required in addition to the existing validation hook ? | 17:20 |
| clif | I wouldn't go so far as required, but I would think it's desirable. So imagine you have a config value defined in a config file that looks correct but when you go to access the value, it fails it's TypeConfig.__call__ method (as in it raises a ValueError because the value is mal-formed or incorrect somehow) | 17:21 |
| clif | then you're stuck with it blowing up at runtime | 17:22 |
| clif | Instead, if you type-validate the configuration value before you load and run it for real, you know it's incorrect early on | 17:22 |
| tkajinam | but even if you replace it by a different exception then it still goes up to the trace, right ? | 17:22 |
| clif | and can fix it before you have a run-time/deply problem/etc | 17:22 |
| clif | But I'm not replacing the exception | 17:23 |
| tkajinam | If you want to do run-time validation then it should validate not only types but constraints we define | 17:24 |
| tkajinam | otherwise that would eventually cause different problems ihmo | 17:24 |
| clif | right, that's the duty of a derived ConfigType.__call__ impl , which I call | 17:24 |
| clif | that's the point | 17:24 |
| clif | I want that __call__ to do whatever validation it's doing in the validator tool, so it's not a surprise come run time | 17:25 |
| tkajinam | I'm not yet sure if I understand your point really | 17:25 |
| tkajinam | we already have validations for each options, using __call__ interface of each ConfigType, right ? | 17:25 |
| tkajinam | hmm wait | 17:26 |
| clif | yes, but that __call__ interface is not used until first-access of the configuration value at runtime | 17:26 |
| tkajinam | I agree with that point | 17:26 |
| JayF | It's about timing. We discovered this when we added validation to a config type in Ironic -- it only failed at access time (late) and not in the validator. clif is working to add a feature to cause it to fail at oslo-config-validator runtime, so operators can test configs :). | 17:26 |
| clif | so you may have a type-validation problem that never surfaces until you first need that configuration value | 17:26 |
| tkajinam | one thing you probably have to consider is that not all options are loaded at start up | 17:27 |
| tkajinam | I mean some of these are loaded lazily. one good example is keystoneauth options which are loaded according to the plugin used there | 17:27 |
| clif | yeah that makes sense | 17:27 |
| clif | it may not be easy or feasible for all values | 17:28 |
| tkajinam | yeah | 17:28 |
| tkajinam | I just want to make sure I understand how it will be used actually later in real services | 17:28 |
| clif | with the current state of my patches, it only affects behavior in the validator tool | 17:29 |
| clif | but I'd like to add the option to do this validation at configuration load time in real services | 17:29 |
| clif | first as opt-in | 17:29 |
| clif | then maybe later as default-on if it proves itself worthwhile | 17:29 |
| clif | I'm also drafting an email to the list to invite further discussion | 17:30 |
| tkajinam | ok I think I now have a better view about the proposal after reading the code again based on the discussion above. | 17:33 |
| tkajinam | clif, do you mind explaining the details of the mistakes the new validation option can detect ( I assume existing one doesn't load options so do not validate their values, but only ensure these options actually are defined) | 17:34 |
| tkajinam | in the commit message of https://review.opendev.org/c/openstack/oslo.config/+/1009822 | 17:34 |
| tkajinam | in general I suggest explaining how the behavior is changed, instead of how the implementation is changed. | 17:34 |
| clif | Sure, I'll try to explain the behavior changed | 17:35 |
| JayF | tkajinam: your question makes me wonder if doing extensive type validation, like Ironic does (here's some examples https://opendev.org/openstack/ironic/src/branch/master/ironic/conf/types.py#L22 ) is even a pattern oslo.config expects | 17:36 |
| tkajinam | ok I might be confused by the existing default value validation and I agree it's annoying that it doesn't provide a way to validate values actually written in config files | 17:36 |
| clif | tkajinam: the option I added will actually validation a configuration file value if you provide it to the validator tool | 17:37 |
| clif | validate* | 17:37 |
| tkajinam | yes I understand that now | 17:37 |
| tkajinam | JayF, IMO it should, instead of passing that value to actual logic and then trigger wrong behavior. | 17:38 |
| tkajinam | I'm totally for adding validation at least in the validator cli. to be honest I'd thought it has been there (as I said I was confused by validation of default values there...) | 17:39 |
| JayF | :D | 17:39 |
| clif | we validate for ironic custom types here: https://opendev.org/openstack/ironic/src/branch/master/ironic/common/config.py#L49 | 17:40 |
| tkajinam | I see | 17:43 |
| tkajinam | clif, one more quick feedback. I think the current implementation only triggers the basic validation and does not honor options such as min, right ? | 17:45 |
| tkajinam | clif, I wonder if we should rather use class instances, not classes, to load these and run full validation. | 17:47 |
| tkajinam | I mean, if you have IntOpt('foo', min=0, max=10) then the validation ensures that given value can be parsed as an integer, but does not check min, max if I read the current implementation correctly. | 17:47 |
| clif | if it's not enforced/honored in ConfigType.__call__ then my change doesn't enforce it | 17:48 |
| tkajinam | you have to pass these options when you build an instance of ConfigType | 17:48 |
| clif | ah I see, good point | 17:49 |
| tkajinam | I guess the current logic results in calling IntOpt().__init__, (not IntOpt('foo', min=0, max=10).__call__ ) for any IntOpt | 17:49 |
| tkajinam | but we don't want to drop these options really. | 17:49 |
| clif | yes, agreed | 17:50 |
| clif | I will think about it | 17:50 |
| tkajinam | let me leave that comment in the base patch | 17:50 |
| clif | appreciated | 17:51 |
| opendevreview | Merged openstack/oslo.middleware master: Use middleware-owned provider for API tracing https://review.opendev.org/c/openstack/oslo.middleware/+/1008816 | 21:11 |
| opendevreview | Stephen Finucane proposed openstack/oslo.config master: trivial: Reindent docstrings https://review.opendev.org/c/openstack/oslo.config/+/1009868 | 21:42 |
| opendevreview | Stephen Finucane proposed openstack/oslo.config master: typing: Make ConfigType generic https://review.opendev.org/c/openstack/oslo.config/+/1009869 | 21:42 |
| opendevreview | Stephen Finucane proposed openstack/oslo.config master: typing: Make Opt generic https://review.opendev.org/c/openstack/oslo.config/+/1009870 | 21:42 |
| opendevreview | Stephen Finucane proposed openstack/oslo.config master: typing: Misc typing improvements https://review.opendev.org/c/openstack/oslo.config/+/1009871 | 21:42 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!