| opendevreview | Doug Goldstein proposed openstack/ironic master: add types to driver interfaces base class for steps and task https://review.opendev.org/c/openstack/ironic/+/995571 | 01:14 |
|---|---|---|
| opendevreview | Doug Goldstein proposed openstack/ironic master: annotate the base driver/interface methods as stubs https://review.opendev.org/c/openstack/ironic/+/995572 | 01:14 |
| opendevreview | Doug Goldstein proposed openstack/ironic master: add type hints to arginfo for the steps and annotate step helpers https://review.opendev.org/c/openstack/ironic/+/995573 | 01:14 |
| opendevreview | Adam Harwell proposed openstack/ironic master: Add ASGI application entry point for Ironic API https://review.opendev.org/c/openstack/ironic/+/995168 | 04:09 |
| opendevreview | Adam Harwell proposed openstack/ironic master: Add ASGI application entry point for Ironic API https://review.opendev.org/c/openstack/ironic/+/995168 | 06:03 |
| opendevreview | Merged openstack/ironic-python-agent master: Enable centos10 publish job https://review.opendev.org/c/openstack/ironic-python-agent/+/992571 | 06:25 |
| opendevreview | Elod Illes proposed openstack/ironic-python-agent unmaintained/zed: Fix CI issues https://review.opendev.org/c/openstack/ironic-python-agent/+/995201 | 09:06 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: feat: add state, target_provision_state, and duration_seconds to node history https://review.opendev.org/c/openstack/ironic/+/989994 | 10:38 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: feat: add state, target_provision_state, and duration_seconds to node history https://review.opendev.org/c/openstack/ironic/+/989994 | 11:04 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: feat: populate node_history state fields https://review.opendev.org/c/openstack/ironic/+/993208 | 11:04 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: Add request context user tracking to node history records https://review.opendev.org/c/openstack/ironic/+/993901 | 11:41 |
| opendevreview | Merged openstack/ironic-tempest-plugin master: Drop python 3.10 https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/991186 | 14:09 |
| Sandzwerg[m] | cardoe: That's something. My idea for now would be to give them insight into the ironic side, so they see if nodes are in cleaning etc but not sure how good that would work with Terraform | 14:28 |
| opendevreview | Jed Preist proposed openstack/tenks master: Add Redfish support https://review.opendev.org/c/openstack/tenks/+/995697 | 14:47 |
| JayF | Sandzwerg[m]: in my experience, terraform makes lots of bad assumptions about openstack which makes it not great for ironic usage. Specifically the behaviors around partial failures are pretty miserable. | 14:49 |
| JayF | I would strongly suggest you look into rebuild patterning to see if you can avoid the massive deletes and creates | 14:49 |
| rpittau | I think we forgot to drop python 3.8 support from IPE in 2025.1 :D | 14:53 |
| Sandzwerg[m] | JayF: Yeah, not my choice or decision. But currently rebuilds are not possible as they need to connect these nodes to a vault and either Openstack or our installation is missing something that would allow them to rebuild. (I think it was host verification) | 14:55 |
| Sandzwerg[m] | In principle we should be able to handle the deletions and creations. But because they delete all nodes on a batch before creating the new one we run into limits. Currently that limit is the subnet of the cleaning network (which for us is the same as the deployment network) | 14:56 |
| JayF | yeah I mean, this is just a scaling issue and your only options are to change the user behavior or to scale up, I'm afraid :| | 14:58 |
| rpittau | considering that we don't test pytho n3.8 in CI and that it was dropped in 2025.1 from all openstack, should we just drop it from IPE in stable/2025.1 ? I'm actually ok with that | 15:26 |
| opendevreview | Merged openstack/sushy-tools master: Add optional ServiceRoot Vendor via SUSHY_EMULATOR_VENDOR https://review.opendev.org/c/openstack/sushy-tools/+/991904 | 15:48 |
| JayF | rpittau: I'd suggest we not intentionally merge py3.8 breaking code | 15:53 |
| JayF | rpittau: but testing it? nah | 15:53 |
| JayF | Reminder that most US-ians will be out Friday | 15:57 |
| rpittau | JayF: then I guess we can't explicitly remove support for Py 3.8 :) | 15:57 |
| JayF | what I'm saying is | 15:58 |
| JayF | nobody should say "great, we can use this fancy new syntax now" | 15:58 |
| JayF | or "awesome, I can use library X which I was waiting b/c it was py3.8" | 15:58 |
| JayF | not "we should maintain py3.8 compat via code review" :D | 15:58 |
| JayF | just we should not make any intentional changes to break py3.8... but I won't cry if we do :) | 15:58 |
| JayF | I mixed a lot of negatives up there: all I mean is we shouldn't *intentionally* break it, but I'm OK with not testing it | 15:59 |
| rpittau | I left out a particular that might be fundamenetal here :D | 16:06 |
| rpittau | IPE is already broken in 2025.1 for Python 3.8 because of the requirements, lot of them do not work with Python 3.8 | 16:06 |
| JayF | Sounds like we never supported py3.8 | 16:06 |
| JayF | we just false advertising'd | 16:06 |
| JayF | so I am +1 to fixing the docs/setup.cfg/whatever to match | 16:08 |
| rpittau | ok | 16:08 |
| opendevreview | Riccardo Pittau proposed openstack/ironic-prometheus-exporter stable/2025.1: [stable only] Replace pkg_resources with importlib.resources https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/994768 | 16:10 |
| opendevreview | Riccardo Pittau proposed openstack/ironic-prometheus-exporter stable/2025.1: [stable only] Replace pkg_resources with importlib.resources https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/994768 | 16:11 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: feat: add state, target_provision_state, and duration_seconds to node history https://review.opendev.org/c/openstack/ironic/+/989994 | 16:42 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: feat: populate node_history state fields https://review.opendev.org/c/openstack/ironic/+/993208 | 16:55 |
| opendevreview | Esther Domfeh proposed openstack/ironic master: Add request context user tracking to node history records https://review.opendev.org/c/openstack/ironic/+/993901 | 18:19 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add a custom configuration type KernelParameterString https://review.opendev.org/c/openstack/ironic/+/992306 | 20:41 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add kernel parameter parsing to _prepare_iso_image https://review.opendev.org/c/openstack/ironic/+/992307 | 20:41 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add kernel parameter parsing to build_extra_pxe_options https://review.opendev.org/c/openstack/ironic/+/992308 | 20:41 |
| opendevreview | Clif Houck proposed openstack/ironic master: Utilize KernelCommmandLine.parse() instead of ironic.common.utils.parse_kernel_params https://review.opendev.org/c/openstack/ironic/+/992309 | 20:41 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add a tool to parse kernel command lines https://review.opendev.org/c/openstack/ironic/+/992641 | 20:42 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add a ParsedKernelCommandLine type https://review.opendev.org/c/openstack/ironic/+/995748 | 20:42 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add UnsafeKernelCommandLine class https://review.opendev.org/c/openstack/ironic/+/995749 | 20:42 |
| opendevreview | Clif Houck proposed openstack/ironic master: Add a deprecation notice for disable_kernel_parameter_parsing https://review.opendev.org/c/openstack/ironic/+/995750 | 20:42 |
| clif | many commits ftw :) | 20:42 |
| JayF | so easy to review them one by one | 20:42 |
| JayF | it's like magic | 20:43 |
| clif | mostly, there's at least one 'L' chonker in there | 20:44 |
| JayF | https://review.opendev.org/c/openstack/ironic/+/992306/4/ironic/conf/types.py#80 makes me VERY curious about the semantics behind using a CONF object mid-initialization | 20:44 |
| JayF | not just from a perspective of "will it break", but also from the perspective of "can we load it in such a way that we can ENSURE this option exists first" | 20:45 |
| JayF | I would assume if we have confidence that it's not going to be breaky to use a CONF object inside a CONF type | 20:45 |
| JayF | that it's likely we can figure out how to conquer #2 as well | 20:45 |
| JayF | (not saying we should remove the defensive code even if we get there, but I'd be surprised if this isn't a situation where it's either working/non-working pretty consistently based on import ordering) | 20:46 |
| clif | So I don't see any easy way around the problem I encounterd there | 20:49 |
| clif | I think it's reasonable to short-circuit and not validate the default value because the default value should have a reasonably high chance of being correct | 20:49 |
| JayF | How do you know that's operating against the default value? | 20:50 |
| JayF | That's sorta where I am, is I was under the impression that CONF was loaded at import time | 20:50 |
| JayF | not lazily imported opt or group by group | 20:50 |
| clif | so think about the order of operations | 20:50 |
| clif | ironic/conf/__init__.py is the first out the gate | 20:50 |
| clif | and it loads *everything* under conf basically | 20:51 |
| clif | and types is imported by whatever configuration options use it | 20:51 |
| JayF | is it? | 20:51 |
| clif | yes | 20:51 |
| JayF | we have from oslo_config.cfg import CONF in some places | 20:51 |
| JayF | that will entrypoint via ironic.conf first, too? | 20:51 |
| clif | types exists under ironic/conf | 20:52 |
| clif | therefore its __init__.py *will* be loaded first thing | 20:52 |
| clif | as its part of the conf module | 20:52 |
| JayF | conductor.register_opts(CONF) gets called, and that calls register_opts on the conductor group | 20:52 |
| JayF | of which **both** of these options are a member, yeah? | 20:52 |
| clif | yes | 20:52 |
| JayF | I'm confused as to how it could *ever* be nondeterministic: it seems like it should get NoOptError everytime OR work everytime | 20:53 |
| JayF | the weirdness for me is that it's coded defensively in a way that suggests we don't know the behavior to expect | 20:53 |
| clif | oslo.config evals the default values before it evals the loaded configuration values | 20:53 |
| clif | the config doesn't get loaded until after all the import business gets resolved | 20:54 |
| clif | once the importing is done, that option is available and things don't blow up | 20:54 |
| clif | but until then I can't safely referennce it | 20:54 |
| JayF | so then it looks like: 1) first load of CONF happens with default values only 2) this means we take the "nosuchopt" fork in the validator | 20:54 |
| clif | from what I can tell olso.config evals the default value as soon as it ingests the option | 20:55 |
| clif | during import time | 20:55 |
| JayF | 3) load of custom CONF values happens in list() order, which the disable_kernel_parameter_parsing is NEWER than the existing ones (earlier in the list) so this is where it breaks for me | 20:55 |
| JayF | how does #3 work | 20:56 |
| JayF | does our code in ir_types trigger CONF to load the boolean values? Is it done in an order other than listed orders? | 20:56 |
| clif | depends on what you mean by 'to load' | 20:58 |
| JayF | so CONF.conductor.disable_kernel_parameter_parsing, if NoSuchOptError doesn't fire, can be presenting two values: | 20:58 |
| JayF | 1) a default or explicit false | 20:58 |
| JayF | 2) an explicit true | 20:59 |
| JayF | I think right now, we'll have different behavior depending on when the type/conf opt gets instantiated | 20:59 |
| JayF | if we hit register_opt(disable_parsing) before register_opt(kernel_params) it works as expected, if inverted, I think we'll always operate on the default value | 20:59 |
| clif | that doesn't make sense to me | 21:00 |
| JayF | I'm explaining it badly | 21:01 |
| JayF | There are three cases: | 21:01 |
| clif | KernelParameterString.__call__ gets called in several places | 21:01 |
| JayF | 1) NoSuchOptError (oslo doesn't even know CONF.conductor.whatever exists yet) | 21:01 |
| JayF | 2) False (explicit by user configuration) | 21:01 |
| JayF | 3) True (default, or explicit by user configuration) | 21:01 |
| JayF | right now we differentiate 1 from (2+3) | 21:01 |
| JayF | but we do NOTHING to ensure we're differentiating, or avoiding, the case where we use the default value for CONF.conductor.disable_kernel_parameter_parsing when deciding whether or not to parse, for instance, CONF.inspector.extra_kernel_params | 21:02 |
| JayF | right now it's my belief that in some cases, we'll decide to parse based on a default of true, but later, once the proper operator-chosen value is set for that (false) the behavior can change | 21:02 |
| clif | ok I think I understand now what you're saying | 21:04 |
| clif | but I don't know what to do about it | 21:05 |
| JayF | me either, but the code as written is -1 because it's worse than if that conf type didn't exist (inconsistently disobeying a user conf entry) | 21:06 |
| clif | other than maybe moving disable_kernel_parameter_parsing to some option group that is guaranteed to be loaded before anything that uses the KernelParameterString type | 21:06 |
| JayF | yes | 21:06 |
| JayF | that's what I'm looking at | 21:06 |
| JayF | we also don't have to register the whole group at once | 21:06 |
| JayF | you could split the kernel_params ones and register them into the groups separately at the end | 21:06 |
| JayF | that is EXACTLY what I'm trying to see is feasible right now | 21:06 |
| clif | ok I get it | 21:07 |
| opendevreview | cid proposed openstack/ironic-specs master: Docker console container provider https://review.opendev.org/c/openstack/ironic-specs/+/992741 | 21:07 |
| clif | this is what code review is for after all | 21:07 |
| JayF | if claude is to be believed, and I think I'm worked through this problem with it pretty well | 21:07 |
| JayF | it's suggesting that CONF.whatever.kernel_params will not be validated until it's accessed | 21:07 |
| JayF | if true; it means we should be OK | 21:07 |
| clif | .... really? that sounds weird to me | 21:08 |
| JayF | but we should document this so it's not a footgun/hidden dependency later if so | 21:08 |
| clif | but I don't profess to fully understand oslo.config | 21:08 |
| clif | what's the point of typing config values if you're not validating them at config load time | 21:08 |
| clif | and waiting until runtime for them to blow up in your face | 21:08 |
| JayF | I think you can I see "config load time" as import CONF from ironic.conf | 21:08 |
| JayF | but oslo.config may see it as when CONF.blah is wanted | 21:08 |
| clif | do you know when oslo.config actually loads configuration files? | 21:11 |
| JayF | https://etherpad.opendev.org/p/ironic-claudes-take-on-conf-dep-stuff I had claude dump my chat with citations in etherpad. I haven't reviewed it yet and am now. | 21:12 |
| clif | well claude seems to like the way I did it :p | 21:15 |
| JayF | yeah for sure, it thinks it's fine, which I'm coming around to as well | 21:16 |
| clif | not that I trust that clanker any further than I can throw it | 21:16 |
| JayF | yep | 21:16 |
| JayF | I use the clanker for ideas I validate with my own neurons lol | 21:16 |
| clif | valid | 21:16 |
| JayF | and afaict it's right in this case | 21:16 |
| JayF | but this does signal to me it might be wise to put an informational comment there about how this works | 21:16 |
| JayF | just to save someone else the half-hour of WTF in 2036 | 21:17 |
| clif | maybe, I don't like the fact that oslo.config's behavior is to not run the type validation until it's accessed, if that's true | 21:17 |
| JayF | might imply there's value in an ironic/conf/__init__.py having a list of conf variables to _ = CONF.group.maybe_fail | 21:18 |
| opendevreview | Verification of a change to openstack/ironic-python-agent unmaintained/zed failed: Fix CI issues https://review.opendev.org/c/openstack/ironic-python-agent/+/995201 | 21:24 |
| JayF | elodilles: I don't know if your employer has interest, but I know Ironic in the past has mostly curated our own unmaintained branches. As a core team, we're not really going to be working on anything older than um/2023.1 anymore. Should we close the branches or leave 'em open for you? | 21:26 |
| clif | JayF: I'm pretty sure claude is wrong, and that ConfigOpts._convert_value gets called on all configuration options at configuration load time, not just access time | 21:33 |
| JayF | I could easily belive that, can you point me to the code where that happens? | 21:34 |
| JayF | I was looking for it but couldn't find it | 21:34 |
| clif | _convert_value is what ends up calling opt.type() (which invokes the type's __call__ method) | 21:34 |
| * JayF looking | 21:35 | |
| JayF | _convert_value() called three places: one is in _do_get() (in the convert method inside that method) | 21:35 |
| clif | ConfigOpts._parse_config_files calls ConfigOpts._validate_cli_options which calls ConfigOpts._convert_value(value, opt) for all valid opt, group pairs | 21:35 |
| JayF | guess what's on my screen right now (That code, I just found it :C) | 21:36 |
| * clif punts claude into the sun | 21:36 | |
| JayF | #@$%@!# | 21:36 |
| JayF | clif: hard for me to be annoyed at claude when it means my original brain was right | 21:36 |
| JayF | clif: I'd rather claude have been right than have the annoying problem lol | 21:37 |
| JayF | so how do we get outta this tangled mess, then? | 21:37 |
| clif | um | 21:37 |
| JayF | Wait, is this still OK? as long as register_opt(conductor) happens first, it probably is? | 21:37 |
| JayF | We *have* to be able to write a unit test that would exercise this properly, it would just take careful mocking | 21:38 |
| clif | I mean, that can be my next step at least. | 21:41 |
| clif | write a test, watch it fail (or pass maybe) then try to find a not-sucky way of solving this | 21:42 |
| JayF | I mean, at the end of the day if the answer is "we can't add the type validator until we deprecate disable_parameter_parsing" it's not awesome but it'd be OK | 21:42 |
| clif | sure, but at least one commit after that one depends on the assumption that the configuration type exists and is validating configuration inputs for kernel parameters | 21:44 |
| clif | so I'd rather solve this and have the type available | 21:45 |
| * cardoe peeks from under the safety rock to see if its okay to come out yet/ | 22:12 | |
| JayF | it's never been okay | 22:19 |
| cardoe | I'm squirreled on custom hardware types | 22:21 |
| cardoe | and steps | 22:21 |
| cardoe | It's a side quest to make the surface more clear to folks. | 22:22 |
| JayF | don't get too deep without having a conversation with someone | 22:22 |
| JayF | and that someone is probably me :) | 22:22 |
| cardoe | You got 10 minutes? :D | 22:23 |
| cardoe | You just mark this side quest as failed and I'll move on quick. | 22:24 |
| JayF | sure | 22:25 |
| JayF | https://meetpad.opendev.org/cardoe-jay-wtf | 22:26 |
| cardoe | https://review.opendev.org/c/openstack/ironic/+/995571 what spawned this | 22:46 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!