| opendevreview | Kaifeng Wang proposed openstack/ironic master: Fix incorrect arg while getting ports by portgroup https://review.opendev.org/c/openstack/ironic/+/962043 | 01:18 |
|---|---|---|
| cardoe | alegacy: can we get what we need from neutron into neutron-lib? | 01:36 |
| opendevreview | Iury Gregory Melo Ferreira proposed openstack/ironic master: [WIP] iDRAC10 force Slot1 for VirtualMedia https://review.opendev.org/c/openstack/ironic/+/961989 | 03:01 |
| opendevreview | Merged openstack/sushy stable/2025.2: Improve Dell Asynchronous task handling https://review.opendev.org/c/openstack/sushy/+/961818 | 04:00 |
| rpittau | good morning ironic! o/ | 07:08 |
| abongale | good morning ironic 0/ | 08:06 |
| opendevreview | nidhi proposed openstack/sushy master: Add comprehensive PCIe device support to Sushy https://review.opendev.org/c/openstack/sushy/+/961982 | 08:08 |
| *** jroll01 is now known as jroll0 | 08:29 | |
| kubajj | Hello, I have a question related to inspector. Is it recommended to do the migration with Caracal or rather first upgrade to Epoxy and then migrate Inspector? | 11:29 |
| *** dking is now known as Guest27158 | 11:50 | |
| dtantsur | kubajj: both should be fine, but my gut feeling would be to move to Epoxy first. If you get any issues, it will be much easier to fix them. | 12:06 |
| alegacy | cardoe: not sure I understand what you mean... from a standalone networking point of view we don't need anything from neutron, in fact part of the point of this is to decouple from neutron. | 12:07 |
| kubajj | dtantsur: sounds good, thanks | 12:08 |
| cardoe | You had said you pulled in stuff from neutron. So I was saying rather than copying can we not share in their library | 12:13 |
| alegacy | cardoe: maybe there was a misunderstanding... what I meant was that the new dependency on ironic from ngs to get the base class and exception types was similar to the existing dependency on neutron to get its base class and exception types. completely different interfaces though. | 12:51 |
| cardoe | Ah okay | 12:52 |
| dtantsur | I'd really appreciate a diagram :) | 13:05 |
| *** Guest27158 is now known as dking | 13:08 | |
| dtantsur | Now that we got rid of eventlet monkey-patching, importing Ironic as a library is technically possible, although you're going to get a lot of dependencies with it. | 13:13 |
| opendevreview | Verification of a change to openstack/ironic master failed: Add cross-gating job with openstacksdk https://review.opendev.org/c/openstack/ironic/+/960393 | 13:33 |
| TheJulia | I still need to look, unfortunately I'm also just not "awake" yet either | 14:03 |
| alegacy | dtantsur: technically it is going to be ironic importing ironic since the standalone service is also a part of ironic. Is your concern around unit testing or when the package is used (potentially) in another project? | 14:04 |
| dtantsur | alegacy: okay, now you properly confused me :) what's the problem with parts of ironic using each other? | 14:05 |
| * dtantsur second lunch, brb | 14:06 | |
| alegacy | dtantsur: :) my concern is mostly a logistical one of getting the CI jobs to pass... I guess until my (not yet ready) ironic commit for standalone networking is merged, the ngs repo tox tests won't pass since it has a dependency on my ironic changes. | 14:11 |
| TheJulia | Have I mentioned how much I dislike miro ? | 14:13 |
| dtantsur | Miro is a crime | 14:22 |
| dtantsur | alegacy: you should be able to use Depends-On in your review | 14:22 |
| TheJulia | dtantsur: it is better than a cell in a spreadsheet.... | 14:32 |
| * TheJulia goes and walks the dog real quick before making a fresh batch of coffee to get back to code reviews | 14:39 | |
| opendevreview | Merged openstack/ironic master: Add cross-gating job with openstacksdk https://review.opendev.org/c/openstack/ironic/+/960393 | 15:32 |
| JayF | If https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/pxe_base.py#L357 blows up even with network_interface: noop, is that expected? | 15:39 |
| JayF | Shouldn't we be not validating network-specific things in an inspection interface? | 15:40 |
| JayF | https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/inspector/interface.py#L122 -> https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/pxe_base.py#L436 | 15:40 |
| dtantsur | JayF: it's required to decide if managed inspection is possible | 15:40 |
| JayF | So the old workflow, which worked on earlier ironic releases was: | 15:41 |
| dtantsur | You get to https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/pxe_base.py#L446 and that converts into unmanaged one | 15:41 |
| JayF | - Manually create neutron port for pxe boot | 15:41 |
| JayF | - Boot into inspection which sets up the real ports | 15:41 |
| JayF | I know this isn't technically supported, but I was a little surprised to find a codepath that was network-dependant that didn't toggle with network-interface | 15:41 |
| dtantsur | Network interface does not handle getting information from a BMC | 15:42 |
| JayF | in a noop network_interface case, managed inspection doesn't need hte macs | 15:42 |
| JayF | right? | 15:42 |
| dtantsur | Not really | 15:42 |
| dtantsur | Lookup still needs a MAC, especially if it's Redfish | 15:42 |
| JayF | hmm | 15:43 |
| dtantsur | Also, you need a MAC address for PXE configuration | 15:43 |
| dtantsur | I think this is was the original reason | 15:43 |
| JayF | you asked the right question to trigger my brain down the correct path | 15:44 |
| JayF | the "lookup needs a MAC" is a question I don't have a good story for around how my downstream is working | 15:44 |
| JayF | so thank you for the pointer and I'll keep digging :D | 15:44 |
| dtantsur | sure thing :) | 15:44 |
| dtantsur | If you use Redfish, we have a pre-inspection feature where we create ports from OOB information first, then go into in-band inspection | 15:44 |
| dtantsur | Won't help with IPMI or if this information is not available | 15:45 |
| JayF | yeah I have servers that don't have EthernetInterface populated on the system that Ironic sees | 15:45 |
| JayF | lolsob | 15:45 |
| dtantsur | sigh, why, why | 15:45 |
| JayF | I have a couple million (no joke) lines of JSON to sort through then I can show you | 15:45 |
| JayF | (JSON dumps from cursed hardware that I have to validate/redact if needed) | 15:46 |
| dtantsur | yeah, that's scary | 15:47 |
| JayF | dtantsur: https://github.com/openstack/ironic/blob/master/ironic/drivers/modules/inspect_utils.py#L377 last mile node lookup matching is by bmc_address | 15:48 |
| JayF | we don't have any populated mac address (or switch) info until post-inspection | 15:49 |
| dtantsur | BMC address sometimes work, sometimes does not | 15:49 |
| dtantsur | Also, how is your PXE going to work? We rely on MAC addresses there IIRC? | 15:49 |
| JayF | they are making direct neutron port create calls with explicit IP/network/etc assignments | 15:49 |
| JayF | they know what ports things are plugged into | 15:49 |
| JayF | they /don't/ know the port:mac mappings | 15:49 |
| JayF | which is why we avoid adding in the metadata until lldp gives us all the needed info | 15:50 |
| JayF | port with no local_link_connection = angry neutron | 15:50 |
| JayF | no port = angry new inspection | 15:50 |
| dtantsur | That does not really answer my question though? The right (i)PXE configuration is fetched from firmware using a MAC address. | 15:50 |
| JayF | hm | 15:50 |
| JayF | I think the high level thing is | 15:51 |
| dtantsur | https://opendev.org/openstack/ironic/src/commit/38605857ae6659cf44f31df20a1dcf7aa80f6901/ironic/drivers/modules/boot.ipxe#L10-L11 | 15:51 |
| JayF | we're doing something halfway between managed and unmanaged | 15:51 |
| JayF | and the line got drawn darker | 15:51 |
| JayF | and now we're in unhappy land | 15:51 |
| JayF | because I suspect the answer will be "oh we put it in our magic boot an agent for inspection vlan" | 15:51 |
| dtantsur | It does sound so :) Cannot you actually go unmanaged, except that you don't have a separate dnsmasq, you just configure neutron manually? | 15:51 |
| dtantsur | It's unmanaged from Ironic's perspective, but managed from neutron's? | 15:52 |
| JayF | I believe there's a desire to ensure the correct node<>BMC mappings | 15:52 |
| JayF | like the node names/bmc addresses are associated pre-ironic | 15:52 |
| JayF | hmm we might still have the info to do this | 15:52 |
| TheJulia | adamcarthur5: regarding bulk operations, you need to wrap your lines. See https://zuul.opendev.org/t/openstack/build/8b892eb616ab4bec83789c4158fd203f and/or run "tox -elinters" locally | 16:24 |
| * TheJulia ponders https://review.opendev.org/c/openstack/networking-generic-switch/+/962037 | 16:56 | |
| TheJulia | alegacy: thoughts on defining a couple of our own exception classes, and then in ironic check to see if the exception class defined in NGS is what is being triggered? | 17:03 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Fix incorrect arg while getting ports by portgroup https://review.opendev.org/c/openstack/ironic/+/962043 | 17:36 |
| opendevreview | Merged openstack/ironic master: Add WSGI alias under ironic.wsgi.api https://review.opendev.org/c/openstack/ironic/+/961753 | 17:37 |
| alegacy | TheJulia: If I'm understanding correctly you mean have NGS throw its own exception (that don't inherit from Ironic's) and have Ironic catch those? That locks Ironic in to directly knowing about NGS, and if another driver comes along (or many more) then Ironic needs to know about each of them rather than just the generic exception types. Maybe I haven't grasped what you were selling there...apologies. | 17:50 |
| TheJulia | Well, I'm thinking that because it allows ngs to be self contained and ironic can do whatever appropriate handling from a generic exception catch from there I guess more to the point, someone has to import something somewhere and its a common pattern to catch something in a library or helper we're calling in ironic, its a weirder pattern to define exceptions in ironic for use outside of ironic but then only ironic to | 17:53 |
| TheJulia | catch | 17:53 |
| TheJulia | I guess the question, and its not in my mind right now, who has the running process because the wip appears to just be importing exceptions | 17:56 |
| alegacy | dunno, I think it is more of a common pattern for many drivers to inherit from some common definition than it is for each of those drivers to expose their own stuff... that would break the abstraction layer. maybe what's needed is an ironic-lib where we throw the driver base classes and exception so that driver implementations can be externalized. | 17:57 |
| alegacy | NGS currently imports the base class and exceptions. The running process that would load that is the standalone networking process (what I'm calling ironic-networking... if I can finally finish cleaning it up and publish! ;) ) | 17:58 |
| iurygregory | funny that we got rid of ironic-lib lol | 17:58 |
| TheJulia | yeah.... | 18:00 |
| alegacy | sorry, didn't finish that sentence ... "so that driver implementations can be externalized...without importing all of Ironic" | 18:01 |
| TheJulia | alegacy: where is that process living code wise? | 18:01 |
| alegacy | ironic | 18:01 |
| TheJulia | so ironic will invoke the ngs code? | 18:01 |
| alegacy | The main ironic process will invoke an RPC over to the ironic-networking process. That process will invoke the NGS driver. | 18:02 |
| alegacy | ...and that first RPC will be done thru the network driver which lives in the ironic repo. | 18:02 |
| TheJulia | so then its an easy call to have try: ngs.magical() except ngs.exception.sadnessfoo: raise ironic.common.exception.sadnessfoo ? | 18:03 |
| TheJulia | Unless I'm misunderstanding the direct mapping to the ngs code | 18:04 |
| alegacy | ironic-networking loads whatever drivers match that entrypoint (ngs being one possible implementation). ngs is loaded and becomes callable, but that detail is abstracted ... sort-of. | 18:05 |
| TheJulia | ahhhhh | 18:05 |
| TheJulia | hmmmmm | 18:05 |
| TheJulia | hmmmmmmmm | 18:05 |
| TheJulia | JayF: https://review.opendev.org/c/openstack/ironic/+/952790 seems to be in good shape, just needs to webapi rst file changed changed | 18:22 |
| JayF | ack | 18:23 |
| JayF | I'm only half here today | 18:23 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Add node.instance_name https://review.opendev.org/c/openstack/ironic/+/952790 | 18:23 |
| TheJulia | There i fixed it | 18:23 |
| TheJulia | :) | 18:23 |
| JayF | thanks for the birfday present, a patchset, just what I wanted! | 18:23 |
| JayF | oh goodness I joke but you /did/ save me from having to spell Gaz... without an example in the file | 18:24 |
| JayF | lol | 18:24 |
| TheJulia | lol | 18:28 |
| TheJulia | I had the release series doc open on the other screen because I had to look it up too | 18:28 |
| TheJulia | Super quick reviews: https://review.opendev.org/c/openstack/ironic/+/962043 https://review.opendev.org/c/openstack/ironic/+/961996 https://review.opendev.org/c/openstack/ironic/+/955806 | 18:50 |
| JayF | -1'd one for a release note (on that patch as it's backportable), one landed | 18:56 |
| JayF | and the OCI doc I'll just let dmitry have a look | 18:56 |
| TheJulia | I'll whip a reno for that last one, it just doesn't make sense to cycle too much over it | 19:07 |
| TheJulia | clif: w/r/t https://review.opendev.org/c/openstack/ironic/+/955625, I concur, the *right* way is to trigger an rpc action. Maybe...split the change into distinct parts and do the rpc stuffs the right way there later on? | 19:08 |
| clif | As in split out controller logic into another change? | 19:09 |
| opendevreview | Merged openstack/bifrost stable/2024.1: Do not pass empty values to instance_info https://review.opendev.org/c/openstack/bifrost/+/959278 | 19:12 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Fix inspection IB port client-id https://review.opendev.org/c/openstack/ironic/+/955806 | 19:13 |
| TheJulia | split out the physical network chnage enforcement to a seprate change, or build the substate before it on the rpc, and then layer this change on top | 19:13 |
| clif | wdym by 'build the substate'? | 19:15 |
| TheJulia | well, so, your change is already kind of big | 19:16 |
| TheJulia | oh wait | 19:16 |
| TheJulia | your adding it all in one | 19:16 |
| TheJulia | I guess the cleanest way is to add an RPC method to handle this change | 19:16 |
| JayF | the thing that made me WTF looking at that code | 19:17 |
| JayF | is there are places where we work on objects directly | 19:17 |
| JayF | e.g. create_portgroup | 19:17 |
| TheJulia | ideally we would be doing this across several patches, but because its an all in one, it sort of has grown | 19:17 |
| clif | I'm happy to break it out into more than one if we can | 19:17 |
| TheJulia | The model is really "send the action which requires a lock to be held to the conductor" | 19:17 |
| TheJulia | Oh, you absolutely can, it just gets a little weird if your not used to the pattern | 19:18 |
| clif | part of the reason I'm doing it here is because of previous reviews saying we need to enforce port consistency with the portgroup | 19:18 |
| TheJulia | Then keep it all in one change then | 19:18 |
| TheJulia | I'm just trying to come up with a way to help move things along faster so we're not churning as much | 19:18 |
| clif | fair, so ideally the consistency is enforced through a single rpc call? | 19:19 |
| TheJulia | yeah | 19:19 |
| TheJulia | that way you can pull a task with a lock, update all the ports, and be done | 19:20 |
| clif | I'll pursue that route | 19:21 |
| JayF | so I think there's another thing here then, right? | 19:21 |
| JayF | can someone create_portgroup with a physical network? | 19:21 |
| JayF | if so you'll have to RPC-itize that as well | 19:21 |
| JayF | or specifically disallow it to punt the problem to later? | 19:22 |
| clif | seems like ideally yes | 19:22 |
| clif | hopefully I can design the rpc call to fit both create and update cases cleanly | 19:22 |
| TheJulia | Seems reasonable to me | 19:23 |
| clif | thanks for taking a look | 19:25 |
| opendevreview | Merged openstack/ironic master: Add SKU field to Redfish inspection https://review.opendev.org/c/openstack/ironic/+/960463 | 19:35 |
| TheJulia | cardoe: down to 30, but yeah, a lot of ironic-week-prio items open | 19:41 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Fix inspection IB port client-id https://review.opendev.org/c/openstack/ironic/+/955806 | 19:51 |
| TheJulia | ugh, typeo fix from codepell | 19:51 |
| opendevreview | Merged openstack/ironic master: Fix : AsRockRack Management via Redfish https://review.opendev.org/c/openstack/ironic/+/960551 | 20:02 |
| TheJulia | mirrors are not super happy today | 20:34 |
| iurygregory | TheJulia, do you think it's worth considering detecting if is idrac8/9/10 or only iDRAC10 on https://review.opendev.org/c/openstack/ironic/+/961989/2/ironic/drivers/modules/drac/boot.py ? | 21:12 |
| iurygregory | I'm wondering in which cases it would be useful having such information | 21:13 |
| TheJulia | We have a case where the version logic blows up on the redfish path because there is a firmware version check | 21:15 |
| TheJulia | so, we basiclaly need to detect generation *or* idrac revision to navigate | 21:15 |
| iurygregory | yeah for idrac9, in case we want to attempt using pure redfish | 21:16 |
| iurygregory | we would endup with it complaining about the firmware | 21:16 |
| iurygregory | I will consider that, since it can be useful for the future in case new Dell can use pure redfish instead of idrac =) | 21:18 |
| TheJulia | Yeah, there are a few other issues we can fix in the driver with that context | 21:34 |
| TheJulia | so it wouldn't be a bad idea to try and surface | 21:34 |
| iurygregory | TheJulia, ack, working on the patch now, so people can review tomorrow | 22:59 |
| TheJulia | cool cool! Thanks! | 23:05 |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!