| iurygregory | oh perfect, tox -e py3 complains about psycopg2 .-. | 00:53 |
|---|---|---|
| iurygregory | in py3.10 and py 3.11... | 00:53 |
| opendevreview | Winicius Allan Bezerra da Silva proposed openstack/ironic master: Allow usage of virtual media via System https://review.opendev.org/c/openstack/ironic/+/912336 | 01:03 |
| iurygregory | \o/ | 01:09 |
| TheJulia | iurygregory: Postgres support right? | 01:11 |
| iurygregory | not 100% sure, it's when trying to run unit tests locally | 01:11 |
| iurygregory | since we pull requirements, for 3.10 and 3.11 my OS complains, 3.9 is fine | 01:11 |
| opendevreview | Jacob Anders proposed openstack/sushy-tools master: [DNM] - testing CI https://review.opendev.org/c/openstack/sushy-tools/+/912832 | 01:30 |
| *** jroll05 is now known as jroll0 | 05:22 | |
| rpittau | good morning ironic! o/ | 09:18 |
| rpittau | iurygregory: I had a similar issue, since py3.10 dependencies installation slightly change, you need to double-check that you have all system dev libraries correctly installed and in the library path | 09:23 |
| rpittau | when someone is available please check https://review.opendev.org/c/openstack/ironic/+/912785 to unblock CI | 09:35 |
| opendevreview | Mohammed Boukhalfa proposed openstack/sushy-tools master: Add fake_ipa inspection, lookup and heartbeater to fake system https://review.opendev.org/c/openstack/sushy-tools/+/875366 | 10:08 |
| dtantsur | rpittau: done | 10:11 |
| rpittau | dtantsur: thanks! | 10:11 |
| iurygregory | morning Ironic | 11:14 |
| iurygregory | rpittau, ack I will double check | 11:14 |
| iurygregory | metalsmith is still blocking our CI right? just saw it failed in https://review.opendev.org/c/openstack/ironic/+/912336 | 11:15 |
| opendevreview | OpenStack Release Bot proposed openstack/bifrost master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/bifrost/+/912914 | 11:16 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-inspector master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-inspector/+/912916 | 11:17 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-prometheus-exporter master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/912919 | 11:17 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-python-agent master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-python-agent/+/912921 | 11:18 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-ui master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-ui/+/912923 | 11:18 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic/+/912925 | 11:19 |
| opendevreview | OpenStack Release Bot proposed openstack/metalsmith master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/metalsmith/+/912927 | 11:19 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-baremetal master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/networking-baremetal/+/912929 | 11:19 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-generic-switch stable/victoria: Update .gitreview for stable/victoria https://review.opendev.org/c/openstack/networking-generic-switch/+/912930 | 11:19 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-generic-switch stable/victoria: Update TOX_CONSTRAINTS_FILE for stable/victoria https://review.opendev.org/c/openstack/networking-generic-switch/+/912931 | 11:19 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-generic-switch master: Update master for stable/victoria https://review.opendev.org/c/openstack/networking-generic-switch/+/912932 | 11:19 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironic-inspector-client master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/912934 | 11:20 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironicclient stable/victoria: Update .gitreview for stable/victoria https://review.opendev.org/c/openstack/python-ironicclient/+/912935 | 11:20 |
| dtantsur | NGS changes look very wrong | 11:20 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironicclient stable/victoria: Update TOX_CONSTRAINTS_FILE for stable/victoria https://review.opendev.org/c/openstack/python-ironicclient/+/912936 | 11:20 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironicclient master: Update master for stable/victoria https://review.opendev.org/c/openstack/python-ironicclient/+/912937 | 11:20 |
| opendevreview | OpenStack Release Bot proposed openstack/sushy master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/sushy/+/912939 | 11:20 |
| opendevreview | OpenStack Release Bot proposed openstack/bifrost master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/bifrost/+/912941 | 11:21 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-inspector master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-inspector/+/912943 | 11:21 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-prometheus-exporter master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/912946 | 11:21 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-python-agent-builder master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/912948 | 11:22 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-python-agent master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-python-agent/+/912950 | 11:22 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-ui master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-ui/+/912952 | 11:22 |
| opendevreview | Merged openstack/ironic master: Temporary move metalsmith legacy CI job to non-voting https://review.opendev.org/c/openstack/ironic/+/912785 | 11:23 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic/+/912954 | 11:23 |
| opendevreview | OpenStack Release Bot proposed openstack/metalsmith master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/metalsmith/+/912956 | 11:23 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-baremetal master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/networking-baremetal/+/912958 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-generic-switch stable/wallaby: Update .gitreview for stable/wallaby https://review.opendev.org/c/openstack/networking-generic-switch/+/912959 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-generic-switch stable/wallaby: Update TOX_CONSTRAINTS_FILE for stable/wallaby https://review.opendev.org/c/openstack/networking-generic-switch/+/912960 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-generic-switch master: Update master for stable/wallaby https://review.opendev.org/c/openstack/networking-generic-switch/+/912961 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironic-inspector-client master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/912963 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironicclient stable/wallaby: Update .gitreview for stable/wallaby https://review.opendev.org/c/openstack/python-ironicclient/+/912964 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironicclient stable/wallaby: Update TOX_CONSTRAINTS_FILE for stable/wallaby https://review.opendev.org/c/openstack/python-ironicclient/+/912965 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironicclient master: Update master for stable/wallaby https://review.opendev.org/c/openstack/python-ironicclient/+/912966 | 11:24 |
| opendevreview | OpenStack Release Bot proposed openstack/sushy master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/sushy/+/912968 | 11:25 |
| opendevreview | OpenStack Release Bot proposed openstack/bifrost master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/bifrost/+/912970 | 11:25 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-inspector master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-inspector/+/912972 | 11:25 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-prometheus-exporter master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/912976 | 11:26 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-python-agent-builder master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/912978 | 11:26 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-python-agent master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-python-agent/+/912980 | 11:27 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic-ui master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-ui/+/912982 | 11:27 |
| opendevreview | OpenStack Release Bot proposed openstack/ironic master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic/+/912984 | 11:28 |
| opendevreview | OpenStack Release Bot proposed openstack/metalsmith master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/metalsmith/+/912986 | 11:28 |
| opendevreview | OpenStack Release Bot proposed openstack/networking-baremetal master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/networking-baremetal/+/912988 | 11:28 |
| opendevreview | OpenStack Release Bot proposed openstack/python-ironic-inspector-client master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/912990 | 11:28 |
| opendevreview | OpenStack Release Bot proposed openstack/sushy master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/sushy/+/912992 | 11:29 |
| * dtantsur has approved all valid reno changes | 11:42 | |
| dtantsur | Please don't approve the invalid ones (resurrecting stable/wallaby), the release team has been notified | 11:42 |
| opendevreview | Merged openstack/ironic-python-agent-builder master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/912948 | 11:45 |
| iurygregory | jesus | 11:46 |
| iurygregory | I went to grab another coffee and this is what I see on irc | 11:46 |
| opendevreview | Merged openstack/networking-baremetal master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/networking-baremetal/+/912929 | 11:47 |
| opendevreview | Merged openstack/python-ironic-inspector-client master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/912934 | 11:48 |
| opendevreview | Merged openstack/sushy master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/sushy/+/912939 | 11:50 |
| iurygregory | rpittau, fyi you probably want to check https://review.opendev.org/c/openstack/ironic/+/912336 | 11:51 |
| opendevreview | Merged openstack/ironic-inspector master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-inspector/+/912916 | 11:51 |
| opendevreview | Merged openstack/bifrost master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/bifrost/+/912914 | 11:53 |
| opendevreview | Merged openstack/ironic-ui master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-ui/+/912923 | 11:53 |
| opendevreview | Merged openstack/ironic-ui master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-ui/+/912982 | 11:56 |
| opendevreview | Merged openstack/ironic-prometheus-exporter master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/912919 | 11:56 |
| opendevreview | Merged openstack/ironic-prometheus-exporter master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/912976 | 11:56 |
| opendevreview | Merged openstack/ironic-prometheus-exporter master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-prometheus-exporter/+/912946 | 11:56 |
| opendevreview | Merged openstack/bifrost master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/bifrost/+/912970 | 11:57 |
| opendevreview | Merged openstack/networking-baremetal master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/networking-baremetal/+/912958 | 11:57 |
| opendevreview | Merged openstack/metalsmith master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/metalsmith/+/912927 | 11:57 |
| opendevreview | Merged openstack/ironic-python-agent master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-python-agent/+/912980 | 11:57 |
| opendevreview | Merged openstack/ironic-inspector master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-inspector/+/912943 | 11:57 |
| opendevreview | Merged openstack/ironic-python-agent-builder master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-python-agent-builder/+/912978 | 11:57 |
| opendevreview | Merged openstack/python-ironic-inspector-client master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/912963 | 11:58 |
| opendevreview | Merged openstack/ironic-python-agent master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-python-agent/+/912950 | 12:00 |
| opendevreview | Merged openstack/metalsmith master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/metalsmith/+/912986 | 12:02 |
| opendevreview | Merged openstack/networking-baremetal master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/networking-baremetal/+/912988 | 12:02 |
| opendevreview | Merged openstack/bifrost master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/bifrost/+/912941 | 12:03 |
| opendevreview | Merged openstack/ironic-python-agent master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic-python-agent/+/912921 | 12:05 |
| opendevreview | Merged openstack/python-ironic-inspector-client master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/912990 | 12:09 |
| opendevreview | Merged openstack/ironic master: reno: Update master for unmaintained/victoria https://review.opendev.org/c/openstack/ironic/+/912925 | 12:09 |
| opendevreview | Merged openstack/ironic master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic/+/912954 | 12:09 |
| opendevreview | Merged openstack/sushy master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/sushy/+/912968 | 12:10 |
| opendevreview | Merged openstack/sushy master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/sushy/+/912992 | 12:10 |
| opendevreview | Merged openstack/ironic-inspector master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic-inspector/+/912972 | 12:13 |
| opendevreview | Merged openstack/metalsmith master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/metalsmith/+/912956 | 12:17 |
| opendevreview | Merged openstack/ironic master: reno: Update master for unmaintained/xena https://review.opendev.org/c/openstack/ironic/+/912984 | 12:17 |
| opendevreview | Merged openstack/ironic-ui master: reno: Update master for unmaintained/wallaby https://review.opendev.org/c/openstack/ironic-ui/+/912952 | 12:20 |
| *** dking is now known as Guest2749 | 12:49 | |
| *** Guest2749 is now known as dking | 12:55 | |
| dking | I think that I need to ask some questions about the ironic-python-agent unit testing. It seems that calling `dispatch_to_managers` hits utils.execute, which throws an error. It also seems that no part of that is generally mocked. Does anybody know the best practice on having a method call dispatch_to_managers for the unit testing? | 12:58 |
| Nisha_Agarwal | rpittau, i have replied on the patch. Please have a look. | 13:18 |
| rpittau | Nisha_Agarwal: I checked there and I can confirm that working pyasn1-lextudio versions were removed from pypi, so that looks good, thanks | 13:18 |
| Nisha_Agarwal | rpittau, thnaks | 13:19 |
| Nisha_Agarwal | rpittau, TheJulia in this context the pinning done in driver-requirements.txt in stable branches can be removed | 13:20 |
| Nisha_Agarwal | Now proliantutils version 2.16.1 is already released and the installation doesnt fail | 13:20 |
| rpittau | Nisha_Agarwal: we usually don't touch requirements from stable branches, need to see if we can make an exception here considering the reason behind that | 13:22 |
| Nisha_Agarwal | it is already done because of this issue...thats why just to revert back the change | 13:23 |
| dtantsur | dking: dispatch_to_managers does not directly execute() anything, but the actual call behind it might | 13:40 |
| rpittau | Nisha_Agarwal: I'm going to bump proliantutils version in master, then we can discuss about the stable branches | 13:40 |
| opendevreview | Riccardo Pittau proposed openstack/ironic master: Move back to plain pyasn1 https://review.opendev.org/c/openstack/ironic/+/910342 | 13:41 |
| rpittau | pyasn1-lextudio does not exist anymore so moving back to pyasn1 now ^ :/ | 13:42 |
| dtantsur | wow | 13:42 |
| Nisha_Agarwal | rpittau, ok | 13:42 |
| rpittau | yeah, quite bad | 13:42 |
| dking | dtantsur: Yeah. I suppose that I should dig into it deeper. Do you, or does anybody know if there's something better I should be doing? Essentially, it looks like the unit tests assume that nothing called in most of the code is going to be calling dispatch_to_managers, which is unfortunate if something calls that is called by a lot of things. | 13:46 |
| dking | It feels to me like something should be mocked for all or most of the tests. | 13:47 |
| dtantsur | dking: while we do tend to mock dispatch_to_managers most of the time, it depends on what you're trying to do? | 13:47 |
| dtantsur | If you're testing your hardware manager addition, why not test it directly? If it needs to call dispatch_to_managers, you may mock it or most the manager call being invoked or even let it be sometimes (although most real calls will execute()) | 13:48 |
| dking | dtantsur: I put in a feature request: https://bugs.launchpad.net/ironic-python-agent/+bug/2057668 What I am wanting to do is to make a hook that allows users with custom hardware managers to be able to programmatically add block devices to the skip list to not be cleaned. | 13:51 |
| dking | dtantsur: For my own code, it is mocked. However, I'm replacing (wrapping) `get_skip_list_from_node` which gets called by a lot of other methods, which means that anything which currently calls `get_skip_list_from_node` will end up calling dispatch_to_managers, and so all of those places would need to mock something. | 13:53 |
| dtantsur | yeah, probably dispatch_to_managers is the thing to mock | 13:54 |
| dking | The problem, though, is that means a lot of adding on patches to test methods, which I suspect will start to get ugly. | 13:55 |
| dking | IMHO, dispatch_to_managers should probably mocked a bit more intelligently, but I might need help from somebody more familiar with the existing unit tests. | 13:56 |
| dking | If it were me on my own code, I would probably write a full fake class to handle it, including perhaps some ways to add in test methods to fake calling, and then use that as the mock globally. | 13:58 |
| dtantsur | That's not a bad idea, but you probably don't want to test this responsibility :) | 13:58 |
| dking | Correct. It's a hard call, and probably something a little beyond what I would want to decide by myself, especially on my first edit directly to this particular file. | 14:00 |
| TheJulia | good morning | 14:12 |
| dking | TheJulia: o/ Good morning | 14:14 |
| samcat116 | Do folks here generally only deploy qcows when deploying with ironic? Wondering what the fastest/most efficient format for deploying here is | 14:38 |
| TheJulia | samcat116: generally qcows, but it sort of depends on the exact use case | 14:40 |
| TheJulia | samcat116: fastest is a ramdisk style deploy, but your not actually writing to a disk at that point. | 14:40 |
| TheJulia | qcows are generally fairly quick as long as they have been compressed and whitespace removed | 14:40 |
| samcat116 | yeah I have some folks trying to deploy 60G raws... which is causing issues | 14:41 |
| TheJulia | ouch, yeah | 14:41 |
| TheJulia | qcow2 will somewhat inherently handle that if you make the conversion | 14:41 |
| TheJulia | now, if they have 60G of data in those raw images, eek | 14:44 |
| TheJulia | There are still paths there, but that is a ton to move over a wire | 14:44 |
| samcat116 | The ironic stack that im building now is replacing one that still uses ISCSI deploy, so I am hoping that moving to direct deploy means the overhead of unpacking the qcow moves from the conductor to the actual baremetal host | 14:44 |
| Sandzwerg[m] | We have vmdks but mostly because the VMs run vmware. But we also have nodes with much ram (usually >1TB), so the time to write the image itself does not really matter when the boot alone is 5-10mins | 14:45 |
| samcat116 | Only thing that sucks is that Qcows aren't really optimal for ceph backed nova, so we end up with the same image as a raw and qcow in glance | 14:46 |
| TheJulia | samcat116: You can use raw, and depending on your config it can still stream through the conductor. tl;dr the conductor will download the file, and then the http service for the conductor *can* have compression enabled. It wouldn't be a cure-all, but it could address/speed the overall transfer and write out to the disk. The challenge with iscsi is, basically, zeros would still get written across the wire | 14:50 |
| dking | Who on the team would be most familiar with the ironic-python-agent unit tests? | 15:18 |
| dking | Oh, TheJulia, it looks like you have the most lines in. Would I be able to get your thoughts on it, to know if I'm going in the right direction? | 15:21 |
| TheJulia | Heh, rutro | 15:22 |
| TheJulia | what is going on? | 15:22 |
| dking | TheJulia: Nothing really bad, but I'm wanting to make some changes and I realized that things are getting dirty in the unit testing, so I want to get some thoughts on the best way to proceed. Specifically, with regard to dispatch_to_manages. | 15:23 |
| dking | It seems like the way that the unit testing is handled, it seems like there may be an expectation that dispatch_to_managers is called fairly high up and rarely. I have a change that I want to make which calls dispatch_to_managers, and in the places where I use the code directly, it's fine as I can easily mock_dispatch. However, when I add my method to things which get called a lot deeper down (get_skip_list_from_node), then ver | 15:26 |
| dking | y many (about 13?) tests fail because somewhere in the process dispatch_to_managers calls utils.execute. | 15:26 |
| TheJulia | hmm | 15:28 |
| dking | TheJulia: So, I'm either 1) doing something very wrong and need to rethink what I'm doing, 2) will need to mock dispatch_to_managers in a lot of tests, or 3) perhaps do something more clever with dispatch_to_managers which mocks it more or less globally in the unit test, or 4) there's something better I'm missing. | 15:28 |
| TheJulia | I think I've actually hit this before as well | 15:28 |
| TheJulia | I think in the cases I hit it, my mocks were wrong to begin with and depending on what is being done, since you are explicitly triggering a dispatch_to_managers it sounds like you'll need to mock out everything that is not what you want to execute (and maybe some further calls).... | 15:29 |
| TheJulia | Since it is trying to to the thing across the board | 15:29 |
| TheJulia | without having code in front of me it is a little hard to picture/visualize, but I hope what I just typed makes sense?!? | 15:30 |
| JayF | That is a good way to explain it, I think. You gotta mock out *all the things* dispatch_to_managers would call | 15:32 |
| dking | That may be the case, but it looks like it might mean adding a mock to 13 different tests, none of which call the new method directly. That's fine and probably the quick and dirty way to do it. However, the next time somebody wants to add code that calls dispatch_to_managers, then that new code will have to be mocked in a bunch of places also. That is, of course, if anybody ever really modifies that code or adds in a new hook. | 15:33 |
| TheJulia | Yeah, that is kind of the side effect since dispatch_to_managers tries to call everything loaded | 15:35 |
| TheJulia | I suspect we're fast approaching the need to look at code though | 15:35 |
| TheJulia | (and my mind is on policy-ish words at the moment) | 15:35 |
| dking | In my personal unit testing, I would likely do something like add a helper class to the test which allows for adding and removing dispatched methods on the fly and then mock the whole method globally to use something from that class. We can't really know what potential custom hardware managers will do, and we don't really assume that there would even be any. This would allow the initial assumption to be that there aren't any cu | 15:35 |
| dking | stom hardware managers, and then allow specific tests for what overrides they end up using. | 15:35 |
| JayF | That' | 15:36 |
| JayF | Hmm. | 15:36 |
| JayF | You'd almost need to replace dispatch_to_managers itself with a mocked version that fed it a list of HWMs to test | 15:36 |
| TheJulia | yeah, you would need to | 15:37 |
| TheJulia | .. and truth be told, I think at least a couple tests in IPA *do* just replace dispatch_to_managers | 15:37 |
| TheJulia | for the purpose of mocking | 15:37 |
| JayF | https://opendev.org/openstack/ironic-python-agent/src/branch/master/ironic_python_agent/hardware.py#L3259 | 15:37 |
| TheJulia | It has been a while since I've looked | 15:37 |
| JayF | nope, it's even easier | 15:37 |
| JayF | replace get_managers() | 15:37 |
| dking | Yes, but not really needing a list of HWMs, just add or remove methods as you need to test them. Since it's mocked, it wouldn't really need a whole HWM behind it. | 15:37 |
| TheJulia | bahahahah, yeah, get_managers | 15:37 |
| JayF | dking: you'll need to mock out get_managers with a version of it that only returns the HWM under test | 15:38 |
| JayF | dking: I'm not 100% sure how to do that, but I suspect you can load an extension from stevedore by name | 15:38 |
| dking | Is get_managers what ends up calling utils.execute() under the hood? | 15:38 |
| JayF | So you gotta step back a little and look at a bigger picture | 15:39 |
| JayF | if you draw out the map of what's happening; dispatch to managers: 1) gets a list of all possible managers 2) tries to execute [dispatched method] on those in prio order | 15:39 |
| JayF | the execute is coming from $thing_you_dont_care_about being dispatched to | 15:40 |
| JayF | if you hijack step #1 though, to only return *the manager under test* | 15:40 |
| JayF | you're only going to dispatch to the manager you're testing, and if *that* calls an execute you'll have to mock it out | 15:40 |
| JayF | but basically we're saying "short circuit it higher up the stack" | 15:40 |
| dking | BTW, I'm not looking at testing HWMs. I'm just currently interested in things which call dispatch_to_managers under the hood, presumably just getting GenericHardwareManager. Currently, I don't think there's any testing going on for custom HWMs. | 15:40 |
| JayF | I don't think that's a correct statement | 15:41 |
| JayF | we test dispatch to managers dispatches across all of them, and each custom hwm has items under test separately | 15:41 |
| dking | Nothing added at the moment calls an execute. It seems to only be something that happens from the call to dispatch_to_managers directly. And I'm not worried about testing any specific HWM. So there's no manger under test (except the GenericHardwareManger). | 15:43 |
| dking | I may not be explaining myself fully. Is that making sense? The tests for things I'm adding directly work as expected and I can mock and test the dispatch fine. For the most part, the dispatch just returns an empty list from the GenericHardwareManager that doesn't really get into the code. However, there is a wrapper function which makes that call and that replaces an existing method (really, just returning the same thing that | 15:48 |
| dking | the original method returned), but since that gets called in a lot of places, all of those places end up having to call dispatch_to_managers. | 15:48 |
| dking | I have some code, but I'm not entirely sure where I could upload it. The tests fail, of course, once the wrapper is used. | 15:49 |
| JayF | I think to get further you'd have to get that code somewhere, with CI results we could examine | 15:51 |
| iurygregory | metal3-integration failed .-. Error response from daemon: received unexpected HTTP status: 502 Bad Gateway | 15:51 |
| iurygregory | nice start for Thursday | 15:51 |
| rpittau | iurygregory: which patch ? | 15:52 |
| iurygregory | https://review.opendev.org/c/openstack/ironic/+/912336 | 15:52 |
| dking | JayF: I could probably fork the repo into my personal Github. Give me just a bit. | 15:52 |
| iurygregory | it was green in the run where metalsmith failed, I'm following the recheck status https://zuul.opendev.org/t/openstack/build/dc4e801fcbf445cc8c12fdb7cdf3215a | 15:52 |
| JayF | dking: if it's an IPA change, just push it up to gerrit? | 15:53 |
| JayF | dking: that way CI will run. If it's a custom HWM, it's not super useful to see the code without the unit test failure output alongside, so without CI it'll be tough | 15:53 |
| rpittau | iurygregory: it's an error from quay.io when trying to get the metal3-io ironic-client image, I would check if it works locally and just recheck the test | 15:54 |
| iurygregory | yeah | 15:54 |
| dtantsur | mmm, the dnsmasq patch has been backported all the way to Zed, right TheJulia? | 15:56 |
| TheJulia | dtantsur: huh, what, | 15:56 |
| dtantsur | sudo dpkg -r dnsmasq-base | 15:57 |
| JayF | the force-install of the older version has been merged quite a ways back | 15:57 |
| dking | JayF: Okay. It feels a bit dirty to push a known-failing commit, but that's probably most efficient. | 15:57 |
| TheJulia | yeah, they put the breaking change all the way down through focal by just updating the package version | 15:57 |
| JayF | dking: it's not a big deal, if you need to revise it multiple times, you can go into zuul.d/project.yaml and disable all the tests except unit tests (just make sure you comment them outta both check and gate) | 15:57 |
| TheJulia | so if we want to run unit tests, we had to put in place changes | 15:58 |
| dtantsur | I wonder why the gophercloud CI only started failing now.. | 15:58 |
| TheJulia | on stable branches? I think we disabled it | 15:58 |
| TheJulia | oh, gophercloud! | 15:58 |
| TheJulia | they mostly started landing the end of last week/early this week | 15:59 |
| TheJulia | only now sort of blocked due to branch renames | 15:59 |
| JayF | it's in everything but xena, I think? | 15:59 |
| TheJulia | xena/wallaby | 15:59 |
| JayF | it landed in wallaby | 15:59 |
| TheJulia | oh, yeha | 16:00 |
| TheJulia | well, I ahve a list of patches | 16:00 |
| JayF | that's part of why they were WTF'ing in release, it landed in violation of stable policy before the rest | 16:00 |
| dtantsur | well, everything up to yoga is gone now, so it's not my concern :) | 16:00 |
| JayF | which usually doesn't matter for us but ...yeah | 16:00 |
| TheJulia | yeah, there is a high horse there, and a topic one doesn't want to get on a call and discuss with me right now :) | 16:00 |
| * TheJulia gets off the soapbox and goes back to writing an example commit message for a policy | 16:00 | |
| JayF | I don't care where the horse is, or who is riding it, I just try to go with the current because it's easier than swimming up or cross stream ;) | 16:01 |
| TheJulia | ++ | 16:01 |
| opendevreview | Merged openstack/ironic master: Move back to plain pyasn1 https://review.opendev.org/c/openstack/ironic/+/910342 | 16:09 |
| JayF | I'm looking at Ironic master now, seeing if we're good to cut a release | 16:10 |
| JayF | Your "no wait, X needs to land" comments would be appropriate now ;) | 16:11 |
| iurygregory | https://review.opendev.org/c/openstack/ironic/+/912336 needs to land =) | 16:11 |
| iurygregory | I can take care of the release if you want JayF =) | 16:11 |
| dtantsur | and https://review.opendev.org/c/openstack/ironic/+/912516 | 16:11 |
| JayF | I was expecting that to be on the list lol | 16:11 |
| iurygregory | we mentioned on monday I think | 16:12 |
| JayF | iurygregory: you're welcome to, but we need to cut it today | 16:12 |
| JayF | er | 16:12 |
| JayF | this week | 16:12 |
| JayF | so today or tomorrow | 16:12 |
| iurygregory | yeah, CI was miss behaving a bit I would say | 16:12 |
| iurygregory | we only landed the nv for metalsmith today | 16:12 |
| JayF | my concern is that it happens | 16:13 |
| JayF | if you want to babysit patches and comb thru the open reviews for things that should land or can land please do :) | 16:13 |
| JayF | I'll go audit release notes instead, which needs doing too | 16:13 |
| iurygregory | ++ | 16:13 |
| JayF | ugh | 16:14 |
| JayF | codespell isn't in :( | 16:14 |
| JayF | https://review.opendev.org/c/openstack/ironic/+/906807 someone wanna approve this and the "Adding CI target for..." patch? | 16:15 |
| rpittau | approved, I thought I approved all of them but that was updated | 16:15 |
| JayF | yeah it's all good, I didn't notice until I wanted to spell check release notes lol | 16:16 |
| rpittau | :D | 16:16 |
| * JayF adds a line item for PTG: get consensus on getting codepsell voting | 16:16 | |
| rpittau | oh yeah, wanted to add that too | 16:16 |
| JayF | adamcarthur5: fyi https://etherpad.opendev.org/p/ironic-ptg-april-2024 I added codespell voting ci, line 149 | 16:17 |
| JayF | Also, just introducing Dave Welsch from Expert Support to the community (he's Reverbverbverb ) -- he's working on a report with some actionable bugs to help improve the organization of Ironic docs. | 16:20 |
| JayF | Please treat him kindly by reporting all docs bugs to him personally /s (JUST KIDDING) :D | 16:20 |
| rpittau | JayF: sounds good to me :D | 16:21 |
| JayF | as I've said before, the wheels grind but they grind slow, I said we'd try to get this going at last ptg | 16:21 |
| JayF | and right now we're targetting the report being done for being presented at the ptg | 16:21 |
| rpittau | awesome | 16:22 |
| TheJulia | hmm, scheduling? https://ce240375b9fb134f8b36-edfb11d7c283f3d767689d2badcfe4a2.ssl.cf5.rackcdn.com/912516/5/check/ironic-tempest-uefi-redfish-vmedia/edca7e7/testr_results.html | 16:32 |
| iurygregory | TheJulia, I think I saw the same error on janders patch on sushy-tools https://review.opendev.org/c/openstack/sushy-tools/+/909487 https://693ae3b551562860f1e3-7b446e9d0ed6b9dfc63ac80c2f10e41b.ssl.cf2.rackcdn.com/909487/7/check/sushy-tools-tempest-uefi-redfish-vmedia/c6febf7/testr_results.html | 16:53 |
| iurygregory | but after recheck the job went green | 16:54 |
| TheJulia | I guess we will see since riccardo already rechecked dmitry's patch | 16:55 |
| rpittau | I rechecked mine too https://review.opendev.org/c/openstack/ironic/+/912516 | 16:57 |
| rpittau | see ya tomorrow! o/ | 16:57 |
| TheJulia | same patch :) | 16:58 |
| opendevreview | Merged openstack/ironic master: [codespell] Fixing Spelling Mistakes https://review.opendev.org/c/openstack/ironic/+/906806 | 17:13 |
| opendevreview | Merged openstack/ironic master: [codespell] Adding Tox Target for Codespell https://review.opendev.org/c/openstack/ironic/+/906807 | 17:13 |
| JayF | well, something-CI is happy at least :D | 17:14 |
| TheJulia | Did we give it cookies while convincing it to come to the dark side? | 17:14 |
| opendevreview | Daniel King proposed openstack/ironic-python-agent master: Add get_additional_skip_list and get_skip_list https://review.opendev.org/c/openstack/ironic-python-agent/+/913202 | 17:18 |
| opendevreview | Daniel King proposed openstack/ironic-python-agent master: Cause failures by adding get_skip_list https://review.opendev.org/c/openstack/ironic-python-agent/+/913203 | 17:18 |
| dking | JayF: That last one is the one that is giving me the errors. It should show the problem in the unit tests. | 17:21 |
| JayF | ack; it's not useful to look at until the unit tests failed upstream | 17:22 |
| JayF | so if you see zuul has commented later, ping again and I'll look? | 17:22 |
| JayF | I'm not so good at remembering :D | 17:23 |
| dking | JayF: Sounds good! I imagine that will be a while, so I'm going to step away for a bit. | 17:23 |
| opendevreview | Merged openstack/ironic master: [codespell] Adding CI target for Tox Codespell https://review.opendev.org/c/openstack/ironic/+/906808 | 17:38 |
| Reverbverbverb | jayf: This is why I never let anyone stand behind me when a bus is approaching. :D | 17:40 |
| Reverbverbverb | Seriously though, if anyone has high-level suggestions for Ironic doc improvement or wants to be involved in the analysis discussion, I'd love to hear from you | 17:41 |
| * TheJulia feels like she should go start up the diesel engine of the bus | 17:44 | |
| iurygregory | rpittau, dtantsur or any other core https://review.opendev.org/c/openstack/ironic/+/912336 is green \o/ | 17:49 |
| dking | JayF, TheJulia: Here's the failure: https://zuul.opendev.org/t/openstack/build/fb416b12698b4106b74aece2bafef76f | 18:07 |
| dtantsur | iurygregory: approved | 18:26 |
| JayF | dking: > File "/home/zuul/src/opendev.org/openstack/ironic-python-agent/ironic_python_agent/hardware_managers/cna.py", line 78, in evaluate_hardware_support | 18:33 |
| JayF | so you'll have to mock out the CNA hwm or prevent it from loading by mocking out get_managers as mentioned earlier | 18:33 |
| dking | JayF: Should that be mocked out for the whole test_hardware.py? And if so, my bigger question is, shouldn't we then think about just mocking dispatch_to_managers and/or get_managers? | 19:09 |
| dking | I mean, for the whole file | 19:10 |
| JayF | I would suggest finding a version of that change that gets your test to pass with his little modification of the other code AS possible, and if it looks like at that point we could do something to improve the tests overall, that would be a separate change imo | 19:11 |
| JayF | **with as little modification of other code as possible | 19:11 |
| dking | In this case, "as little as possible" is probably going to be the global mocking. There's 7 failures here, two more in the next change, and then probably a few more for the third. Meaning that a single mock might be better. However, if it's better to make it dirty before cleaning it, then I can go through with that. | 19:14 |
| iurygregory | dtantsur, ty! | 19:14 |
| JayF | dking: more of a general practice of "don't mix a refactor and a feature" :) You could do them in opposite order, but that gives you a longer runway to the piece you care about | 19:17 |
| dking | Fair. I usually do the refactor first, when it's something that's generally good, but I get the point. | 19:18 |
| dking | I'm thinking that I will just mock get_managers, once I check over the method. | 19:18 |
| JayF | I think that's the best route tbh | 19:19 |
| opendevreview | Verification of a change to openstack/ironic master failed: Allow usage of virtual media via System https://review.opendev.org/c/openstack/ironic/+/912336 | 19:32 |
| iurygregory | TheJulia, same error in the vmedia job test_baremetal_server_ops_partition_image | 19:36 |
| opendevreview | Verification of a change to openstack/ironic master failed: Allow usage of virtual media via System https://review.opendev.org/c/openstack/ironic/+/912336 | 19:53 |
| opendevreview | Jay Faulkner proposed openstack/ironic master: Fix new codespell issues; tweak config https://review.opendev.org/c/openstack/ironic/+/913265 | 19:58 |
| opendevreview | Jay Faulkner proposed openstack/ironic master: Codespell voting in CI https://review.opendev.org/c/openstack/ironic/+/913266 | 19:58 |
| JayF | 913265 is an easy review, 913266 might be more controversial :D | 19:59 |
| opendevreview | Jay Faulkner proposed openstack/ironic master: Release notes prelude for 2024.1/24.1 https://review.opendev.org/c/openstack/ironic/+/912679 | 20:08 |
| JayF | I'm happy with release notes once the prelude lands | 20:08 |
| opendevreview | Jay Faulkner proposed openstack/ironic master: Release notes prelude for 2024.1/24.1 https://review.opendev.org/c/openstack/ironic/+/912679 | 20:12 |
| TheJulia | iurygregory: so the issue is the partition image | 20:50 |
| TheJulia | https://www.irccloud.com/pastebin/rlRYaGbL/ | 20:54 |
| iurygregory | yeah | 20:55 |
| iurygregory | ouch =O | 20:56 |
| TheJulia | so, my guess is the instance image is tinycore based | 20:56 |
| iurygregory | you mean the instance where devstack is running? | 20:57 |
| TheJulia | https://dd930d07009d7df5be20-ea76e3e710a8219c6c723d407750283e.ssl.cf5.rackcdn.com/912336/2/gate/ironic-tempest-uefi-redfish-vmedia/2dfb615/controller/logs/ironic-bm-logs/node-0_console_2024-03-14-19%3A01%3A26_log.txt | 20:57 |
| iurygregory | because we always used tinycore in some jobs no? | 20:57 |
| TheJulia | The image it is trying to write | 20:57 |
| TheJulia | oh | 20:58 |
| TheJulia | 0df539ba-1442-42f1-9f71-84ef23af3c59 is cirros 6.1 | 20:58 |
| TheJulia | that error is expected | 20:58 |
| iurygregory | in cirros 6.1? | 21:00 |
| iurygregory | don't we have a var that sets the min cirros version we will use? | 21:00 |
| TheJulia | ... why is this suddenly fatal | 21:01 |
| iurygregory | yup, before everything was working without problems | 21:02 |
| iurygregory | oh ovn-uefi-ipmi-pxe job failed https://zuul.opendev.org/t/openstack/build/76202771eb2447a9830d99109949251b | 21:02 |
| iurygregory | I only saw now | 21:02 |
| JayF | is this one of the jobs whose name got changed? and do we ever match on job name for that stuff? | 21:03 |
| iurygregory | test_baremetal_server_ops_partition_image \o/ yay | 21:03 |
| JayF | re: ovn-uefi-ipmi-pxe | 21:03 |
| iurygregory | JayF, not 100% sure atm (need more coffee to think about the issues lol) | 21:04 |
| iurygregory | first failure for ironic-tempest-uefi-redfish-vmedia was on Mar 11 | 21:07 |
| TheJulia | so I think we should just force the job on to "wholedisk" only | 21:16 |
| iurygregory | +1 to that | 21:16 |
| iurygregory | :D | 21:16 |
| TheJulia | The source image appears to be unchanged | 21:16 |
| TheJulia | something about the image build appears to be failing, and unfortuantely that is in a legitimate way | 21:16 |
| TheJulia | I wonder if it is disk latency or soemthing with that build process, I think i have a general idea of what is being done, I just don't knwo how it ever knows when it is "done" | 21:17 |
| iurygregory | I was wondering if can be related to the Cloud provider... | 21:19 |
| TheJulia | latency could do it | 21:19 |
| TheJulia | basically it boots a vm to extract the contents | 21:19 |
| TheJulia | if you just want to post a change to lock CI to the one test, I'll +2+A it | 21:21 |
| iurygregory | should we do in the tempest plugin and just add a skip on it ? | 21:26 |
| TheJulia | set the test regex so it is just the wholedisk job | 21:26 |
| iurygregory | oh right | 21:26 |
| iurygregory | should I also change for ironic-tempest-ovn-uefi-ipmi-pxe ? | 21:30 |
| TheJulia | only if you've seen it fail | 21:30 |
| iurygregory | yeah I saw the same failure on it | 21:34 |
| TheJulia | okay | 21:35 |
| iurygregory | https://zuul.opendev.org/t/openstack/build/76202771eb2447a9830d99109949251b | 21:35 |
| TheJulia | filed https://bugs.launchpad.net/ironic/+bug/2057972 | 21:37 |
| opendevreview | Iury Gregory Melo Ferreira proposed openstack/ironic master: Tempest test with only wholedisk for some jobs https://review.opendev.org/c/openstack/ironic/+/913270 | 21:38 |
| iurygregory | let me add in the commit for reference | 21:38 |
| opendevreview | Merged openstack/ironic master: Implement generic redfish vmedia attach detach https://review.opendev.org/c/openstack/ironic/+/912516 | 21:39 |
| opendevreview | Iury Gregory Melo Ferreira proposed openstack/ironic master: Tempest test with only wholedisk for some jobs https://review.opendev.org/c/openstack/ironic/+/913270 | 21:40 |
| iurygregory | TheJulia, sorry D: I added Related-Bug in the commit message | 21:41 |
| TheJulia | I think I'm going to call it a day, I'm feeling super tired | 21:57 |
| iurygregory | same here | 22:02 |
| opendevreview | Merged openstack/ironic master: Allow usage of virtual media via System https://review.opendev.org/c/openstack/ironic/+/912336 | 22:14 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!