| opendevreview | Luan Utimura proposed openstack/ironic master: Fix nexthop error when adding route to PUBLIC_SUBNET_IP https://review.opendev.org/c/openstack/ironic/+/960299 | 00:40 |
|---|---|---|
| kubajj | dtantsur: are you around? We are working on the refactor of https://review.opendev.org/c/openstack/ironic-python-agent/+/937342 - the extraction of the functions to RAID utils brings some problems - i.e. we would need to move other functions outside of hardware.py to avoid cyclic imports - would this be ok, or should we keep it in the hardware.py in that case? | 08:58 |
| kubajj | I do not know how much other operators change their downstream hardware managers, so do not want to break stuff (the change is a bug fix, so was thinking about backporting it) | 08:59 |
| kubajj | dtantsur: please ignore, I think there would be too much to move, so we are going to keep them in hardware | 09:06 |
| dtantsur | kubajj: *nod* there is a known problem: hardware.py has both the abstract hw manager interface and the specific hw manager, so circular imports are a pain | 11:47 |
| dtantsur | at some point, I'm tempted to move GenericHardwareManager out of it | 11:47 |
| kubajj | dtantsur: valid | 11:47 |
| koperg[m] | o/ Hello Ironic... (full message at <https://matrix.org/oftc/media/v1/media/download/AYkoxSAsYj9BZr2yWs4eu3jrugozMLhn9uqJefMiXTWEvmSFZsIr3ukpwk3C4fmwfWKAUfCR45bhOSNXt25r95VCeZe47NSwAG1hdHJpeC5vcmcvRHRnQmVGS2VqTkViTEVOV21NeVJxa2di>) | 11:49 |
| dtantsur | koperg[m]: please write shorter messages, otherwise the matrix bridge only posts a link | 11:50 |
| koperg[m] | dtantsur: sorry about that. Just saying i would appreciate a review on 2 relatively small NGS patches. | 11:51 |
| lutimura | JayF: hey! just to let you know, i did some adjustments on https://review.opendev.org/c/openstack/ironic/+/960299 and zuul seems to be happy with it :) | 11:59 |
| opendevreview | Pierre Riteau proposed openstack/bifrost master: Fix support for CentOS Stream 10 https://review.opendev.org/c/openstack/bifrost/+/958853 | 12:52 |
| opendevreview | Pierre Riteau proposed openstack/bifrost master: CI: Add testing for CentOS Stream 10 https://review.opendev.org/c/openstack/bifrost/+/959684 | 12:53 |
| opendevreview | Merged openstack/bifrost stable/2024.1: [2024.1 only] Drop upgrade jobs since 2023.2 brach doesn't exists https://review.opendev.org/c/openstack/bifrost/+/959317 | 12:54 |
| opendevreview | Verification of a change to openstack/networking-generic-switch master failed: Cast boolean Netmiko kwargs to native types https://review.opendev.org/c/openstack/networking-generic-switch/+/958079 | 13:10 |
| opendevreview | Merged openstack/ironic master: api: Fix off-by-one error https://review.opendev.org/c/openstack/ironic/+/960288 | 13:18 |
| JayF | dtantsur: kubajj: I would be+100000 to moving generic hardware manager to it's own module | 13:19 |
| kubajj | JayF: dtantsur: I think I have the workforce to do it (my intern is very keen to move stuff :D ) | 13:19 |
| kubajj | (mostepha: 😉) | 13:20 |
| JayF | https://review.opendev.org/c/openstack/ironic/+/960299 is needed to fix CI in some devstack configs, has my +2 | 14:01 |
| JayF | please review it soon or I will land as a single core to unblock openstacksdk ci | 14:01 |
| frickler | sdk made the job non-voting, so not a blocker currently, but don't let that stop you anyway, would still be good to revert to voting asap | 14:02 |
| dtantsur | kubajj, mostepha: please do then :) | 14:05 |
| kubajj | dtantsur: could we do it after we fix the safeguards though? 😉 | 14:06 |
| dtantsur | oh yeah, definitely. I was only worried about infinitely growing functions. | 14:07 |
| stephenfin | JayF: fwiw I'm making it voting again, but it was never an issue in the gate, only locally | 15:03 |
| JayF | ah | 15:03 |
| stephenfin | last time it was left unvoting for a while, quite a bit of broken stuff crept it | 15:03 |
| stephenfin | *in | 15:03 |
| opendevreview | Jakub Jelinek proposed openstack/ironic-python-agent master: Fix skip block devices for RAID arrays https://review.opendev.org/c/openstack/ironic-python-agent/+/937342 | 15:04 |
| JayF | I suspect there may be value in cross-gating it | 15:07 |
| JayF | since you clearly are more sensitive to microversion issues | 15:07 |
| JayF | especially since the schema changes are in line | 15:07 |
| kubajj | dtantsur: tried to refactor, let us know if this is more readable | 15:08 |
| kubajj | JayF: we went through the security issues you listed and just made it stricter 😈 | 15:08 |
| opendevreview | Stephen Finucane proposed openstack/ironic master: Add cross-gating job with openstacksdk https://review.opendev.org/c/openstack/ironic/+/960393 | 15:13 |
| stephenfin | JayF: good idea ^ | 15:14 |
| JayF | kubajj: I asked claude just to trace the code path for the one I found and it's like "which one of these 5 were you thinking about" lol | 15:20 |
| kubajj | JayF: 🫠| 15:20 |
| JayF | hint: some of those predated your code :D | 15:21 |
| JayF | I meant it more as I'm impressed by claude's review than anything else fwiw | 15:21 |
| kubajj | nice | 15:21 |
| kubajj | JayF: btw, the assumed behaviour is for a node to be cleaned to deploy these - that's why we are strict with enforcing volume names | 15:23 |
| JayF | kubajj: https://review.opendev.org/c/openstack/ironic-python-agent/+/937342/6..7/ironic_python_agent/hardware.py#3148 I am ... sketched out a little by that logic? | 15:26 |
| JayF | kubajj: I guess I'm not convinced that "fail cleaning" is the right behavior on a failure *that can be trivially induced by the previous tenant* | 15:26 |
| kubajj | JayF: but if they are using create configuration, it will fail to validate configuration | 15:32 |
| JayF | I don't understand how that relates | 15:32 |
| kubajj | if the previous tenant did something, the node needs to clean | 15:32 |
| JayF | _create_raid_ignore_list() is in the cleaning codepath, isn't it? | 15:32 |
| JayF | if it's only in the create path I can see maybe what you mean | 15:33 |
| kubajj | JayF: nope, it is for creating | 15:33 |
| JayF | aha, that's the bit I was misunderstanding | 15:33 |
| kubajj | JayF: delete configuration is only using _handle_raid_skip_list | 15:33 |
| JayF | which I assume/hope doesn't kaboom there | 15:33 |
| kubajj | JayF: create also calls that, but only to check that the existing raid devices are actually supposed to be there | 15:34 |
| JayF | yep 100% fewer kaboom | 15:34 |
| JayF | +2 | 15:34 |
| JayF | thanks for walking me thru it | 15:34 |
| kubajj | JayF: are you sure about the +2? I was actually going to ask about our tests. We did not figure out how to call _create_raid_ignore_list from test_hardware, so it does not have any test case for itself. We do include a couple of tests for the whole create_configuration | 15:36 |
| JayF | yeah it's awkward that the methods are nested, likely is what is making the testing harder | 15:36 |
| JayF | I'll put this plainly: You are the primary customer for this feature in my mind, and I trust to some extent you have a good bit of certainty it works for your use case :). If that's not true, please W-1 and add some tests or do more testing :) | 15:37 |
| JayF | If it is true, then maybe add more unit test coverage when you do that module move | 15:37 |
| kubajj | JayF: we did test it on real nodes and it worked as expected as before the small refactor | 15:41 |
| kubajj | so probs can do after the move | 15:41 |
| kubajj | thanks | 15:42 |
| JayF | I'll tell you what, I'll drop down to a +1 for now until you confirm you tested this version on real hardware or add unit tests to increase your confidence :) | 15:42 |
| kubajj | JayF: we already tested it 😉 | 15:44 |
| kubajj | (it worked the same as before the refactor, might have confused you with the wording) | 15:45 |
| JayF | oh, your statement was more "worked as expected [same] as before the small refactor" not "worked as expected before the small refactor" | 15:45 |
| kubajj | JayF: by small refactor I meant what Dmitry suggested - splitting functions as they were too huge. I would not call this -206 +1440 a small refactor :D | 15:46 |
| JayF | ahh | 15:46 |
| JayF | you know how much IPA code these days is raid code? | 15:46 |
| JayF | approximately all of it :P | 15:46 |
| kubajj | JayF: sorry, the person responsible is sitting a couple of offices away from me and is currently the boss of the boss of my supervisor 😉 | 15:47 |
| kubajj | (he also got me into OpenStack, so am not complaining) | 15:47 |
| JayF | hehehe it's not a bad thing, it's just a thing that was mildly surprising that I found when doing image security stuff | 15:48 |
| dtantsur | Everything that is not partition images handling is RAID code | 15:51 |
| JayF | a lot of that image handling code was unified during the security stuff | 15:59 |
| JayF | a lot of it could be unified further tbh | 15:59 |
| cardoe | Who let it be Wednesday already? | 16:52 |
| JayF | https://review.opendev.org/q/topic:%22ironic-releases-2025.2%22 one of our release managers must +1 these cc iurygregory rpittau | 17:10 |
| JayF | I just updated it to get the recent api and wsgi bugfixes | 17:10 |
| opendevreview | Merged openstack/ironic master: Fix nexthop error when adding route to PUBLIC_SUBNET_IP https://review.opendev.org/c/openstack/ironic/+/960299 | 18:33 |
| iurygregory | JayF, will look in a bit | 19:29 |
| iurygregory | tks | 19:29 |
| clif | I realize the release cycle is in full swing so I'm not expecting a full review but if someone could take a look at how I'm propogating changes from a portgroup to its constituent ports I'd appreciate it: https://review.opendev.org/c/openstack/ironic/+/955625/comment/50a04612_fe429db3/ | 20:19 |
| clif | in short: what's the most appropriate way to accomplish this? | 20:20 |
| JayF | I looked at this with Clif and our usage of RPC calls versus direct object calls in that layer is inconsistent | 20:37 |
| JayF | So I too am now curious why do one way or why do the other way | 20:37 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!