| cardoe | JayF: clif: The issue is that https://www.irccloud.com/pastebin/lKP1f6dV/ won't actually work. Looking for some ideas on how to make this work. | 01:48 |
|---|---|---|
| cardoe | So lets say I have 2 ports with category = network and 1 port group with both of those and its category is network and I run the following | 01:49 |
| cardoe | openstack baremetal node vif attach 784e38e1-62c8-4d0f-8c0b-210eecb3a270 f7dac560-6533-4729-b8fa-b8c05cae0405 --port-uuid 56f0e289-8f4a-44d9-82e5-083c1da31539 | 01:49 |
| cardoe | where 56f0e is ONE of the two ports in the port group. It will actually bind the port group first and then it will attempt to bind the port and fail. Since with TBN that all happens in 1 API call, the end result is a failure. | 01:51 |
| cardoe | I see two issues here. It attempted two bindings. The second issue is that it didn't respect my request. | 01:51 |
| cardoe | Now I turn that around the other way and do | 01:51 |
| cardoe | openstack baremetal node vif attach 784e38e1-62c8-4d0f-8c0b-210eecb3a270 06edec49-b86b-4359-b61a-32e996c44c9e --portgroup-uuid 17ad8171-5e62-4332-890e-bb552c98283e | 01:52 |
| cardoe | It does indeed bind to the port group but then it attempts to bind to the individual port and once again it fails the vif_attach() call. | 01:52 |
| cardoe | So yes, there's residual binding info like you suggested JayF. Just it's actual binding info. | 01:54 |
| opendevreview | Doug Goldstein proposed openstack/ironic master: Only execute the first matching TBN action on VIF attach https://review.opendev.org/c/openstack/ironic/+/1008897 | 02:19 |
| opendevreview | Doug Goldstein proposed openstack/ironic master: Raise an error for unhandled TBN actions on VIF attach https://review.opendev.org/c/openstack/ironic/+/1008898 | 02:19 |
| * cardoe table flips. | 02:32 | |
| cardoe | https://review.opendev.org/c/openstack/ironic/+/1005807 results in a 500 error in the TBN path. 'Proxy' object has no attribute 'close' (HTTP 500) | 02:32 |
| opendevreview | Nuutti Hakala proposed openstack/ironic-python-agent-builder master: Disable Glean DHCP fallback by default https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/1008917 | 04:44 |
| opendevreview | Riccardo Pittau proposed openstack/bifrost stable/2026.2: [CI] Upgrade non-SLURP 2026.2 from 2026.1 https://review.opendev.org/c/openstack/bifrost/+/1007855 | 07:39 |
| *** amorin_ is now known as amorin | 07:45 | |
| opendevreview | Riccardo Pittau proposed openstack/bifrost stable/2026.2: [CI] Upgrade non-SLURP 2026.2 from 2026.1 https://review.opendev.org/c/openstack/bifrost/+/1007855 | 09:48 |
| opendevreview | Elod Illes proposed openstack/ironic-python-agent-builder unmaintained/2023.1: DNM: CI health test https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/1009020 | 10:57 |
| TheJulia | good morning | 13:32 |
| opendevreview | Thomas Goirand proposed openstack/ironic master: Mock cache_firmware_components() in redfish tests https://review.opendev.org/c/openstack/ironic/+/1009046 | 13:34 |
| * cardoe flips a table at TheJulia. | 13:37 | |
| TheJulia | cardoe: what did I do? | 13:37 |
| TheJulia | Shields currently reading 98% | 13:37 |
| cardoe | I dunno. Your name was on 1005807 | 13:37 |
| TheJulia | uhhh, good, bad, or ugly? | 13:38 |
| TheJulia | in other words, do you call the FSM or Cthulhu upon me? | 13:39 |
| JayF | We should figure out what we need to do in CI to cause it to fail properly | 13:40 |
| JayF | It seems like TBN is pretty breakable given our current testing approaches | 13:40 |
| zigo | o/ | 13:41 |
| zigo | Good morning Ironic. | 13:41 |
| TheJulia | Okay, I feel like I'm missing context to the discussion | 13:41 |
| TheJulia | OH | 13:41 |
| zigo | Can someone have a look at my patch above? | 13:41 |
| zigo | Not sure we should keep all the comments I wrote in the test suite though... | 13:42 |
| TheJulia | cardoe: I get it now, I'll dig at it | 13:42 |
| iurygregory_ | good morning | 13:43 |
| TheJulia | zigo: sorry about that! | 13:44 |
| opendevreview | Clif Houck proposed openstack/ironic stable/2026.2: Don't raise if an object's attribute is None in _retrieve_attribute https://review.opendev.org/c/openstack/ironic/+/1007757 | 13:49 |
| TheJulia | cardoe: I've got one annoying emergent thing going on first thing htis morning, once that is sorted I'll be on top of that issue | 13:49 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Add configurable namespace for Kubernetes console provider https://review.opendev.org/c/openstack/ironic/+/1009053 | 13:51 |
| TheJulia | stevebaker[m]: ^^^^ | 13:52 |
| TheJulia | May that be useful based on the discussion your going to wake up to. | 13:52 |
| TheJulia | cardoe: do your two patches from yesterday completley suppress that issue, or is more needed? | 13:58 |
| * TheJulia needs more coffee but thinking its chicken/egg | 13:59 | |
| opendevreview | Elod Illes proposed openstack/ironic-python-agent-builder unmaintained/2024.1: DNM: CI health test https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/1009056 | 13:59 |
| cardoe | I had to delete client.close() in the vif_attach | 14:04 |
| TheJulia | .... | 14:05 |
| cardoe | JayF: yeah the CI not catching this stuff is what's concerning to me | 14:05 |
| TheJulia | oh my | 14:05 |
| TheJulia | well, we don't drive CI testing with TBN which is more so an issue | 14:05 |
| TheJulia | cardoe: did you just have that one bit of error text, or get an entire backtrace? | 14:06 |
| cardoe | That's actually all I got back. | 14:08 |
| cardoe | ah hang on it's in the api logs | 14:09 |
| TheJulia | k | 14:10 |
| TheJulia | I'm digging on it now that I don't have a epic sinus headache and have sprint planning | 14:10 |
| cardoe | https://www.irccloud.com/pastebin/Y43AiDRl/Logs-2026-10-06%2009_09_36.txt | 14:11 |
| TheJulia | I guess the whole idea behind close() is more so make it explicit so we're just "done", instead of hoping gc eventually frees it | 14:11 |
| cardoe | Well we call get_client() so many times and don't pass the client around. I'm not understanding why. | 14:12 |
| TheJulia | Differing times, plus just the evolution | 14:12 |
| TheJulia | somehow you got a bare client on the proxy, funky... | 14:13 |
| cardoe | So I'm running stable/2026.1 plus https://review.opendev.org/c/openstack/ironic/+/1008516 and https://review.opendev.org/c/openstack/ironic/+/1008897 plus the delete of client.close() | 14:14 |
| cardoe | but that could be that we didn't put the wrapper into 2026.1 yet. | 14:14 |
| TheJulia | I think it should be, but... ugh | 14:15 |
| cardoe | ah yes it's not been backported | 14:16 |
| cardoe | So that's on me locally backporting | 14:16 |
| cardoe | I didn't backport that wrapper locally but when I grabbed those 2 patches it included client.close() which it should not have. | 14:16 |
| TheJulia | okay, yeah, claude just sort of came to that conclusion with what I had given it so far | 14:19 |
| TheJulia | Ahhh!, okay | 14:20 |
| *** elodille1 is now known as elodilles | 14:23 | |
| cardoe | I intended to take 1/2 days this week since we're in Florida and so just trying to juggle my time so I did a bad job backporting it. | 14:28 |
| cardoe | So I'm hoping that someone else can look at https://bugs.launchpad.net/ironic/+bug/2169647 and agree if my conclusion is correct? | 14:34 |
| cardoe | Reading the TBN docs we've got 3 different pages. They roughly all say the same thing but not fully and refer to each other. I'd like to merge those. | 14:35 |
| TheJulia | ++ | 14:39 |
| opendevreview | Elod Illes proposed openstack/ironic-python-agent-builder stable/2025.1: DNM: CI health test https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/1009065 | 14:41 |
| opendevreview | Merged openstack/ironic master: Mock cache_firmware_components() in redfish tests https://review.opendev.org/c/openstack/ironic/+/1009046 | 14:42 |
| JayF | clif: ^^ cardoe's bug | 14:42 |
| clif | O | 14:44 |
| clif | I'll take a look | 14:44 |
| clif | I thought the methods I wrote filter out attached stuff, but maybe I didn't filter out attached vifs | 14:45 |
| TheJulia | thats my reading of the other issue noted and the basic gist of one of cardoe's changes | 14:46 |
| clif | oh does cardoe already have a change up to address attached vifs being reconsidered/attached more than once? | 15:10 |
| clif | I see it now | 15:11 |
| cardoe | clif: yeah does it make sense? | 15:27 |
| cardoe | Only attach once? but multiple rules can match | 15:28 |
| cardoe | I still don't know if this is a bug or not but if I had a port group created for a node and it has 0 ports assigned to it... it'll attach the port group | 15:28 |
| clif | it does, but it also means that things like 'min_count' and 'max_count' are essentially nonsense when it applies to attach_{port,portgroup} | 15:29 |
| clif | because of the way TBN has been hammered into the existing flow | 15:29 |
| clif | seems like a bug? probably | 15:29 |
| clif | should empty portgroups exist? | 15:29 |
| cardoe | Well I'm cool with us attaching multiple times in on vif_attach() flow but we'd need to now understand that this $THING is already attached and skip that. Which would mean we'd have to re-evaluate the rules or actions after each action. | 15:30 |
| cardoe | Well you have to create a port group empty via the API first and then attach ports to it. TBN's dynamic creation even does that.. So an empty port group can exist. | 15:31 |
| cardoe | The other "I dunno if that's a bug" is if I as the user want to attach to a port and not a port group, the existing documented way is to pass "--port-uuid" to the vif attach but TBN ignores those inputs entirely. | 15:32 |
| clif | cardoe: the way it is structured now we can't attach muiltiple times in vif_attach because we agreed that it should only consider one vif at a time | 15:32 |
| cardoe | oh true | 15:32 |
| cardoe | So then we should do my patch to remove the for loop. | 15:33 |
| clif | probably | 15:33 |
| clif | it is right given the current state of things | 15:33 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Close neutron client connections on error paths https://review.opendev.org/c/openstack/ironic/+/1009078 | 15:34 |
| TheJulia | cardoe: ^ some cleanup on the handling but more structural handling than anything else | 15:34 |
| clif | but I'm not really happy with it because I feel we should reach a state where TBN can plan everything at once, considering all ports and vifs avaiable against the applicable traits | 15:34 |
| clif | currently feels like a shadow of what it was meant to be | 15:35 |
| JayF | The best laid plans are run into the rocks by having to be stuffed through the teeny tiny API interfaces invented back when "cloud" was still a word people said with the same disdain they say "AI" now | 15:35 |
| JayF | spaghettification of features as it crosses the event horizon of the API | 15:36 |
| cardoe | clif: I agree. I'd like a attach_all_vifs() | 15:37 |
| cardoe | But right now vif_attach() just takes 1 VIF only. | 15:38 |
| clif | yep | 15:38 |
| cardoe | Like if I have 4 VIFs, I would like to pass them all in 1 go. | 15:38 |
| clif | agreed | 15:39 |
| JayF | I think that's reflective of an API issue, yeah? | 15:39 |
| JayF | like the API is "attach one vif" shaped(?) | 15:39 |
| * TheJulia is worried about spaghettification and who is running the temporal loom... | 15:40 | |
| clif | JayF: yea | 15:41 |
| clif | that's what we ran into trying to hook TBN into the existing flow | 15:41 |
| cardoe | So I propose roughly... let's get the existing flow working properly through TBN. | 15:43 |
| clif | yep, I'm just cranky | 15:44 |
| * JayF checks off the "[x] getting cranky about API interfaces" box from the path to core bingo card for clif | 15:44 | |
| opendevreview | Merged openstack/ironic stable/2026.1: Fix TBN Portgroup creation accessing nonexistent vendor field https://review.opendev.org/c/openstack/ironic/+/1008888 | 15:50 |
| cardoe | Let's get to the point that we can deprecate the old flow and run everything through TBN. | 16:01 |
| cardoe | Once we're comfortable enough there then we can add an attach_all_vifs() API which will realize the true TBN goals? | 16:02 |
| JayF | TBN doesn't even work with non-neutron networks, fwiw | 16:04 |
| JayF | so we need to figure that out if you wanna make it the one path | 16:04 |
| cardoe | JayF: maybe my suggestion doesn't make sense then... I was just thinking getting to the place that we can reduce the matrix down. | 16:08 |
| cardoe | before expanding it again | 16:08 |
| JayF | I'm just saying there are bridges to build before we get there | 16:09 |
| JayF | and saying "neutron's default behavior is now encoded in a default TBN config" might be a more achievable goal | 16:10 |
| cardoe | Yeah that makes sense. | 16:30 |
| JayF | https://zuul.opendev.org/t/openstack/build/43c277ae07884b088447c0ee34888105 https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/1007320 | 17:13 |
| JayF | is complaining there's no yum-utils in debian | 17:13 |
| JayF | I think there might be some real upstream failure issue that I am not digging into right now | 17:13 |
| opendevreview | Jay Faulkner proposed openstack/ironic master: Reject Glance encrypted images during validation https://review.opendev.org/c/openstack/ironic/+/1008314 | 17:16 |
| opendevreview | Jay Faulkner proposed openstack/ironic master: Reject Glance encrypted images during validation https://review.opendev.org/c/openstack/ironic/+/1008314 | 17:17 |
| cardoe | another possible TBN oddity... I need to run to join the family cause PTO and all... but | 17:50 |
| cardoe | openstack server create failed and the port group didn't have category = network set on it but the ports did | 17:50 |
| cardoe | the vif_attach() from nova worked but then we hit https://github.com/openstack/ironic/blob/stable/2026.1/ironic/drivers/modules/network/neutron.py#L194 | 17:51 |
| cardoe | I'll see if I have cycles to look more tonight | 17:51 |
| cardoe | A quick glance at the code... TBN allows the ports as part of a port group to be considered | 17:54 |
| cardoe | But the plug_port_to_tenant_network() explicitly removes ports that are part of a port group from consideration | 17:54 |
| cardoe | so vif_attach() worked by attaching to an individual port and not the port group because category wasn't set. But then the build failed later because plug_port_to_tenant_network excluded the ports part of a port group. | 17:55 |
| cardoe | So we probably need to decide which direction is correct. | 17:55 |
| JayF | we should not be considering members of a static portgroup as individual ports in TBN | 18:06 |
| JayF | because we can't double dip | 18:06 |
| opendevreview | Merged openstack/ironic stable/2026.2: Don't raise if an object's attribute is None in _retrieve_attribute https://review.opendev.org/c/openstack/ironic/+/1007757 | 18:32 |
| cardoe | JayF: agreed. I'll file a bug to that effect. | 19:24 |
| cardoe | So another question... "port group" or "portgroup" can we decide on terminology? | 19:24 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Change image_server_auth_permit_unknown_hosts default to False https://review.opendev.org/c/openstack/ironic/+/999907 | 20:08 |
| opendevreview | Julia Kreger proposed openstack/ironic master: Guard checksum URL host and cap checksum file size https://review.opendev.org/c/openstack/ironic/+/1009114 | 20:08 |
| TheJulia | cardoe: I can see the difference, but is there an example to ground discussions upon ? | 20:11 |
| cardoe | well the CLI command is "baremetal port group". Some functions are blah_port_group() and some are blah_portgroup() | 20:13 |
| cardoe | TBN uses portgroup in its config and docs. And then inspection rules use "port group". | 20:14 |
| cardoe | I was just trying to fix up the docs. Any user facing things I would likely make some aliases to the one pattern we'd get behind. | 20:15 |
| TheJulia | so, cli is for cli reasons | 20:16 |
| TheJulia | under the hood, acting on portgroups themselves, since they are specific api resource, and all, I think ought to be consistent on portgroup | 20:17 |
| opendevreview | Doug Goldstein proposed openstack/python-ironicclient master: Add --instance option to baremetal node list https://review.opendev.org/c/openstack/python-ironicclient/+/1009115 | 20:20 |
| opendevreview | Doug Goldstein proposed openstack/python-ironicclient master: Fix baremetal port group create --node with node names https://review.opendev.org/c/openstack/python-ironicclient/+/1009116 | 20:20 |
| opendevreview | Doug Goldstein proposed openstack/python-ironicclient master: Add baremetal port group add and remove commands https://review.opendev.org/c/openstack/python-ironicclient/+/1009117 | 20:20 |
| opendevreview | Julia Kreger proposed openstack/ironic master: trivial: note that it is any shim, not red hat shim. https://review.opendev.org/c/openstack/ironic/+/1009122 | 20:34 |
| opendevreview | Julia Kreger proposed openstack/ironic-python-agent-builder master: Don't require epel for dib build https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/1009125 | 21:01 |
| shermanm | possibly a silly question, but if I'm running into what I think are bugs in the nova ironic driver, should I ask about it here or in the nova irc? | 21:13 |
| TheJulia | Likely both places, whats up? | 21:28 |
| shermanm | I'm running into issues where if a user deletes their instance during provisioning, or if the ironic conductor gets restarted, we start seeing nodes left in state "active" or "deploy_failed" with instance_uuid still set, but no matching nova instance, and it never gets resolved until an operator calls undeploy. | 21:29 |
| shermanm | I filed https://bugs.launchpad.net/nova/+bug/2169200 already for why nova doesn't clean it up eventually, but I think I've found a few different cases that can trigger the problem | 21:30 |
| TheJulia | shermanm: so the first is likely we never got the abort request across or the abort happened when its locked out | 21:30 |
| TheJulia | the latter issue on that yeah, it requires reconciling somewhere since somewhere along the way we didn't get or couldn't process the undeploy reqeust | 21:31 |
| TheJulia | instance_uuid still being set is the big give away, I'm assuming what is happening is the node is just locked for a long while processing... something. | 21:32 |
| shermanm | thanks, I'll mention over there too, and I can add extra info to the bug report now that I keep finding different ways to trigger the problem | 21:36 |
| TheJulia | The root issue is ironic has a lock held for a while, We don't know what or why, but nova-compute's ironic driver is supposed to try for a long time to remove it | 21:36 |
| shermanm | well I found at least one place where I don't think nova's driver retries at atll | 21:36 |
| TheJulia | that is obviously failing, so you sort of need to find a solid example of what is happening (or three) and we need to go from there | 21:36 |
| shermanm | I think I can whip up a reproducer | 21:37 |
| cardoe | shermanm: I’m happy to try and get some patches in. | 21:37 |
| TheJulia | shermanm: did the cleanup get chagned or is it just gone now | 21:38 |
| TheJulia | (read the bug you opened) | 21:38 |
| shermanm | the cleanup broke when sharding was added | 21:38 |
| TheJulia | ahh. | 21:39 |
| shermanm | because once the instances are removed from the nova side, they're not in the cache anymore | 21:39 |
| shermanm | https://opendev.org/openstack/nova/src/commit/b5c2fded48f03fd5887256e3a2dd151a4819e73d/nova/virt/ironic/driver.py#L816-L832 | 21:40 |
| TheJulia | yup, makes sense now | 21:40 |
| shermanm | the problem I think is figuring out how to make only the correct shard attempt the cleanup | 21:40 |
| shermanm | without breaking anything else that reads that cache | 21:40 |
| TheJulia | ... I think the fix is almost going to need to look at deleted instances from the last x time which mapped to it and mapped to nodes to do reconcile | 21:42 |
| TheJulia | which deja vu, I think i floated to saga ages ago | 21:42 |
| shermanm | does it have to look at deleted instances, or can it just check what exists *now* on the ironic side and check whether each has a match in nova? | 21:43 |
| TheJulia | Great question | 21:43 |
| shermanm | my workaround for the moment is a cronjob that basically does that via the ironic and nova apis | 21:44 |
| shermanm | but we're in a single conductor, all nova-managed case, so there's clearly a bunch of edge cases not accounted for | 21:44 |
| TheJulia | thinking outloud: If I'm surfacing all nodes which map to my nova-compute, and I don't have any other users to conflict with, perhaps. But the key why you want to look at the deleted instances is to verify it was an orpahned instance and can be released | 21:44 |
| shermanm | e.g. instance_uuid used to map to something, instead of being totally bogus? | 21:45 |
| TheJulia | that deleted instance's UUID would still be stamped on the node | 21:45 |
| TheJulia | now, you left with nodes in deploy_fail also shouldn't really happen | 21:45 |
| TheJulia | shermanm: it has always mapped to something | 21:45 |
| TheJulia | the key is if there is a record of the same uuid in nova's instances records | 21:45 |
| shermanm | right, I mean that if instance_uuid is set, and doesn't map to anything in nova now, when do we care if/when it used to map to something | 21:46 |
| TheJulia | ironic itself has no way of knowing/reconciling that short of something like deferred actions getting recorded to nuke the deployment later or we figure out why your its not getting cleared out to begin with | 21:47 |
| cardoe | TheJulia: https://bugs.launchpad.net/ironic/+bug/2169761 thoughts? feels? | 21:48 |
| TheJulia | cardoe: so, static portgroup exists, you attempt to deploy but tbn is choosing ports in that portgroup right now? | 21:49 |
| cardoe | Yes | 22:24 |
| TheJulia | Yeah, I'd concur with that then | 22:30 |
| JayF | shermanm: I'm not working right this second, please file a bug with details against both nova and ironic, please cc or assign me | 22:56 |
| shermanm | @JayF +1, will do | 22:57 |
| opendevreview | Merged openstack/ironic master: trivial: note that it is any shim, not red hat shim. https://review.opendev.org/c/openstack/ironic/+/1009122 | 23:28 |
| opendevreview | Merged openstack/ironic master: Only execute the first matching TBN action on VIF attach https://review.opendev.org/c/openstack/ironic/+/1008897 | 23:55 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!