| opendevreview | Merged openstack/networking-baremetal master: prevent break on communications failure https://review.opendev.org/c/openstack/networking-baremetal/+/933149 | 00:04 |
|---|---|---|
| rpittau | good morning ironic! o/ | 08:08 |
| opendevreview | Scott Solkhon proposed openstack/ironic-python-agent master: Add support for burnin-gpu https://review.opendev.org/c/openstack/ironic-python-agent/+/925415 | 11:44 |
| dtantsur | TheJulia: I've never considered anything like that. What's the use case? | 12:22 |
| *** dking is now known as Guest1121 | 13:27 | |
| *** Guest1121 is now known as dking | 13:27 | |
| dking | Good morning. If anybody is in, or when they get in, I would like to ask some questions about https://opendev.org/openstack/requirements, specifically regarding branch master vs stable/2024.1 | 13:47 |
| dking | I mean 2024.2 | 13:47 |
| iurygregory | dking, feel free to ask your question, hopefully we will have the answer for you =) | 13:50 |
| iurygregory | but normally questions about requirements would be in #openstack-requirements | 13:50 |
| dking | iurygregory: Fair enough. But perhaps it might be helpful to start in here since it is affecting an ironic tool directly. The other day I was suggested to use upper constraints from 2024.2 because the ironic-lib version had been changed. I see that change is still in master, so it's causing ironic-python-agent-builder to build broken IPA images. | 13:54 |
| iurygregory | we had a release of ironic-lib last week if I recall, wouldn't that be the reason? | 13:55 |
| * iurygregory checks ipa-b | 13:55 | |
| dking | dib/ironic-python-agent-ramdisk/source-repository-requirements pulls requirements at master, and therefore gets an ironic-lib==7.0.0, rather than 6.2.0, so when I try to deploy an IPA image built with ironic-python-agent-builder, it fails with an error from ironic_lib. | 13:56 |
| dking | I see the problem, and for now I should be able to override the REPOREF, which I'm hoping will work, but it seems like there's a problem, and I don't know which is the correct way forward. | 13:57 |
| iurygregory | well, 7.0.0 was released for master https://review.opendev.org/c/openstack/releases/+/934598/1/deliverables/epoxy/ironic-lib.yaml | 13:57 |
| iurygregory | so yeah, requirements from master will pull it directly | 13:57 |
| dking | Was it premature if ironic-python-agent still requires 6.2.0? | 13:57 |
| iurygregory | I would say it was because at the time you tested we haven't released new IPA... | 13:59 |
| iurygregory | and maybe we should update requirements in IPA to >= 7.0.0 cc rpittau | 13:59 |
| iurygregory | if I recall 7.0.0 ironic-lib had some breaking changes | 14:00 |
| dking | Do you mean <=7.0.0 ? | 14:08 |
| TheJulia | good morning | 14:09 |
| TheJulia | dtantsur: well, under current modeling we would sort of have to expect it... kind of | 14:10 |
| iurygregory | for ipa master we have >= 6.0.0 https://github.com/openstack/ironic-python-agent/blob/master/requirements.txt#L14 | 14:10 |
| * TheJulia attempts to caffinate | 14:19 | |
| rpittau | dking: new version of IPA fixes that behavior | 14:26 |
| rpittau | or should fix :D | 14:26 |
| rpittau | ironic-lib 7.0.0 has breaking changes, as iurygregory mentioned | 14:28 |
| rpittau | we recently released ironic 27.0 and ipa 10.0 to comply with those changes | 14:28 |
| dking | rpittau: Fair enough. If we do have ipa updated, then I suppose that was the fix. | 14:31 |
| rpittau | dking: in short ipa < 10 is incompatible with ironic-lib >= 7 | 14:31 |
| JayF | We probably /should/ ensure stable IPA-b gets stable requirements though | 15:04 |
| JayF | and/or that we document to set requirements repo to a ref that makes sense for your IPA ref | 15:04 |
| iurygregory | I'm trying to figure out why firmware updates via servicing works but we don't update the DB with the newer info, I though it was because we wasn't caching the node information in servicing https://review.opendev.org/c/openstack/ironic/+/936009 but it didn't help on my tests =( , after the update finishes I don't see references for https://opendev.org/openstack/ironic/src/branch/master/ironic/drivers/modules/redfish/firmware.py#L66 (I | 15:07 |
| iurygregory | added some debug locally) https://paste.opendev.org/show/b1dHkz1i1f5SnZZuTa9n/ , the only error in the ironic logs when we finish is about the Agent driver requiring agent_url, does anyone think this could be related to not having the DB updated? | 15:07 |
| JayF | that's just failed to collect system logs, probably doesn't matter | 15:08 |
| iurygregory | yeah, same feeling here | 15:09 |
| JayF | I'd put a log in to make sure that method is being called at all | 15:09 |
| JayF | I bet there's some caching somewhere, perhaps | 15:09 |
| JayF | https://opendev.org/openstack/ironic/src/branch/master/ironic/drivers/modules/redfish/firmware.py#L150 honestly I'm perplexed at this | 15:09 |
| JayF | as since it's a decorator, doesn't it have to take the function as an argument and return a function | 15:10 |
| JayF | when that method doesn't look like it's doing that at all? | 15:10 |
| JayF | IDK if maybe there's some syntax around that I'm not familiar with though | 15:10 |
| iurygregory | hummm | 15:13 |
| JayF | I think there's a functools which will do what seemingly is intended to be done here | 15:14 |
| JayF | but again, this is the sorta thing I'd look up the one way to do it and do it that way, so there's a chance this is a red herring | 15:15 |
| iurygregory | tks JayF o/ | 15:15 |
| iurygregory | funny that with cleaning it works "as expected" :D | 15:18 |
| JayF | I think we explicitly cache firmware | 15:19 |
| JayF | during the cleaning step | 15:19 |
| JayF | outside of the decorator iirc | 15:19 |
| JayF | but imbw | 15:19 |
| iurygregory | we do | 15:19 |
| iurygregory | in the cleaning we have utils.node_update_cache(task), we don't have in servicing, but after adding https://review.opendev.org/c/openstack/ironic/+/936009/1/ironic/conductor/servicing.py it didn't help XD | 15:20 |
| JayF | I think we're better off answering why the decorator is seemingly not working instead? | 15:21 |
| JayF | Rather than adding more explicit calls | 15:21 |
| JayF | hmmm I would expect that to work, though | 15:22 |
| JayF | I'd strongly suggest print line debugging if not actual debugging | 15:23 |
| JayF | add so many log.debug around those calls that your head will spin :D | 15:23 |
| iurygregory | yeah, I'm planning on doing that :D | 15:23 |
| dtantsur | TheJulia: sorry, I don't quite get it. What modelling? Why? | 15:30 |
| dtantsur | Is it about OCI/docker images? | 15:31 |
| TheJulia | So presently we had to add "get checksum from a URL support, as the code base stands today, we'll likely need to emulate it or make the callers more available of the backend image services and what they can/cannot do/get | 15:31 |
| dtantsur | I only care about built-in checksums | 15:31 |
| dtantsur | Hmm. I might see where you going with this. | 15:32 |
| TheJulia | I can tell, and I'm also worried we'll create ourselves another CVE or hard break by just focusing on that as well | 15:32 |
| dtantsur | Support for remote checksums was added very specifically because remote checksums are a thing | 15:32 |
| dtantsur | already | 15:32 |
| dtantsur | with the OCI work, it's a greenfield effort to a much larger extent | 15:32 |
| TheJulia | from my point of view, I've been bitten twice by stuff like this in the last few months, so I'm a bit... as is sometimes said, gunshy | 15:33 |
| dtantsur | I'd say, if you care seriously, provide an explicit value as string | 15:33 |
| TheJulia | True, but greenfield stuff sometimes doesn't age well along side other patterns as well | 15:33 |
| dtantsur | If care, but not super-super seriously, provide your docker:// URL using its hash | 15:33 |
| dtantsur | if you don't care at all.. well, we don't permit missing checksums right? | 15:33 |
| TheJulia | that is just the artifact hint to the layer manifest | 15:33 |
| TheJulia | but yeah :) | 15:33 |
| JayF | This is a tough thing because | 15:33 |
| JayF | you completely throw away any MITM type benefit of the checksum | 15:34 |
| dtantsur | AFAIK it's also a checksum of the layer | 15:34 |
| JayF | when you pull it in the same place | 15:34 |
| TheJulia | it is the checksum of the upper manifest file | 15:34 |
| TheJulia | AIUI | 15:34 |
| dtantsur | JayF: same with remote checksums. It's a known trade-off | 15:34 |
| TheJulia | based upon google's diagram, not the actual artifacts | 15:34 |
| dtantsur | if you care about MITM, you must use an explicit value | 15:34 |
| TheJulia | *but* you can use the checksums all the way down from that point | 15:34 |
| TheJulia | which is nice | 15:34 |
| dtantsur | TheJulia: I think the reason docker/podman can de-duplicate layers, even among several images, is because their ID is their checksum? | 15:35 |
| dtantsur | otherwise, how does it work? | 15:35 |
| JayF | dtantsur: I wasn't thrilled about support for those, either ;) | 15:35 |
| dtantsur | JayF: we need to accept that not everyone has MITM in their threat model | 15:36 |
| JayF | Note that I didn't say -1 or -2 or "no"; just that it's tough | 15:36 |
| JayF | Bluntly; I don't always trust users to even think about threat models in many cases :) | 15:37 |
| JayF | "I want X because it's easier" beats security in 7 outta 10 corporate environments I've been around :) | 15:37 |
| TheJulia | dtantsur: that is how it works, but that is a completely different discusison than the internal details of how ironic handles things like adjacent features | 15:40 |
| dtantsur | btw I've just found https://github.com/opencontainers/image-spec/blob/main/manifest.md#guidelines-for-artifact-usage (I assume you've seen it already, maybe worth linking in the spec?) | 15:42 |
| dtantsur | TheJulia: https://github.com/opencontainers/image-spec/blob/main/descriptor.md#digests | 15:43 |
| dtantsur | Digest "uniquely identifies content by taking a collision-resistant hash of the bytes. If the digest can be communicated in a secure manner, one can verify content from an insecure source by recalculating the digest independently, ensuring the content has not been modified." | 15:43 |
| TheJulia | ... I'm not sure why your focusing on that when I'm trying to deal with the reality that when we fixed the last cve, we had to add url support into ironic to verify provided checksums. We're still going to get close to that code path, and what I'm ultimately worried about has nothing to do with the capabilities of the url form descriptor of a dockerv2+oci bits url form. I'm trying to focus on the existing technical debt | 15:46 |
| TheJulia | and if we're going to have to add "can I do this thing" logic to image services | 15:46 |
| TheJulia | which it is kind of clear, to cleanly guard, I'm going to have to | 15:46 |
| dtantsur | Then I'm still not sure what you're trying to do, and my answer to the "file attached to container" question is "rather no, unless someone is begging for that". | 15:47 |
| dtantsur | I'm pasting the links because they seem to prove (to me) that the only checksum we need to support is the one provided in the image reference. But I won't stop you or anyone from supporting as explicit image_checksum, as long as it's opt-in. | 15:49 |
| iurygregory | | bios | 2.19.1 | 2.17.1 | 2.17.1 | 2024-11-26T12:20:26+00:00 | 2024-11-26T15:47:42+00:00 | | 16:03 |
| iurygregory | FINALLY! | 16:03 |
| TheJulia | dtantsur: I'll just have to make image service generally slightly more visible about interfaces supportable by url types for patterns of behavior | 16:06 |
| TheJulia | ... which might actually be good... maybe | 16:06 |
| iurygregory | going to do more tests to be sure it's really working, because the firmware installed was 2.18.1 not 2.17.1 :facepalm: | 16:08 |
| TheJulia | iurygregory: congrats | 16:10 |
| iurygregory | TheJulia, tks!, I'm just trying to understand why commenting https://opendev.org/openstack/ironic/src/branch/master/ironic/conductor/servicing.py#L183-L184 helped lol :D | 16:10 |
| dtantsur | sooo strange | 16:16 |
| dtantsur | I mean, it cannot possibly help :D | 16:16 |
| rpittau | yeah that doesn't make sense :/ | 16:18 |
| iurygregory | yup totally agree | 16:21 |
| iurygregory | I'm redeploying to see how it goes | 16:21 |
| JayF | Why is it throwing an exception to end up in that codepath at all? | 16:24 |
| * TheJulia is soooooooo confused | 16:25 | |
| * TheJulia goes back to email | 16:25 | |
| iurygregory | I'm still trying to understand that JayF | 16:32 |
| JayF | I'm tied up until around noonish my time, but if you're still around poking at it then and need a rubber duck I'm happy to jump in a video call | 16:33 |
| JayF | (that is +3.5 hours from now) | 16:33 |
| iurygregory | ack o/ | 16:36 |
| iurygregory | ok, I need coffee, the reason for the new data in the DB was because I restarted the ironic service :D (not related the L183 184) | 16:48 |
| rpittau | good night! o/ | 16:57 |
| TheJulia | bllllaarg | 17:03 |
| opendevreview | Julia Kreger proposed openstack/networking-baremetal stable/2024.2: prevent break on communications failure https://review.opendev.org/c/openstack/networking-baremetal/+/936289 | 17:30 |
| opendevreview | Julia Kreger proposed openstack/networking-baremetal stable/2024.1: prevent break on communications failure https://review.opendev.org/c/openstack/networking-baremetal/+/936290 | 17:30 |
| opendevreview | Julia Kreger proposed openstack/networking-baremetal stable/2023.2: prevent break on communications failure https://review.opendev.org/c/openstack/networking-baremetal/+/936291 | 17:31 |
| opendevreview | Julia Kreger proposed openstack/networking-baremetal stable/2023.1: prevent break on communications failure https://review.opendev.org/c/openstack/networking-baremetal/+/936292 | 17:32 |
| opendevreview | Adam McArthur proposed openstack/ironic-tempest-plugin master: Testing bad microversions on v1/allocations https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/935827 | 17:41 |
| opendevreview | Adam McArthur proposed openstack/ironic-tempest-plugin master: Testing bad microversions on v1/nodes/{uuid}/firmware https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/936071 | 17:48 |
| opendevreview | Adam McArthur proposed openstack/ironic-tempest-plugin master: Microversion Test Generator https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/936293 | 18:11 |
| opendevreview | Adam McArthur proposed openstack/ironic-tempest-plugin master: Microversion Test Generator https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/936293 | 18:19 |
| opendevreview | Adam McArthur proposed openstack/ironic master: api: Add schema validation framework https://review.opendev.org/c/openstack/ironic/+/928920 | 18:42 |
| opendevreview | Adam McArthur proposed openstack/ironic-tempest-plugin master: Testing bad microversions on v1/allocations https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/935827 | 18:54 |
| opendevreview | Adam McArthur proposed openstack/ironic-tempest-plugin master: Microversion Test Generator https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/936293 | 18:56 |
| *** iurygregory__ is now known as iurygregory | 23:37 | |
| JayF | adamcarthur5: cid: testcase version of assertRaises doesn't support the context manager syntax :-O | 23:44 |
| JayF | very annoying | 23:44 |
| * JayF trying another construct for cid's I-T-P change | 23:48 | |
| opendevreview | Jay Faulkner proposed openstack/ironic-tempest-plugin master: Test double encoding of error message https://review.opendev.org/c/openstack/ironic-tempest-plugin/+/935740 | 23:49 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!