PR#31: Drop use of buildah for container manifests -

cloud-image-uploader

#31 Drop use of buildah for container manifests

Merged Oct 3, 2024 by jcline. Opened Oct 2, 2024 by adamwill.

adamwill/cloud-image-uploader container-no-buildah into main

Drop use of buildah for container manifests

Adam Williamson • Oct 3, 2024

7f97e98fde32ca2e129309064e5b0838d56045b9

adamwill commented Oct 2, 2024

When trying to deploy the container stuff in production, we found
that it blows up when running in an infra openshift container
because the security policy prevents creation of a user namespace,
which buildah does even for operations like 'login', 'manifest
create' and 'manifest push'. To solve that, this removes all use
of buildah. We construct the multi-arch manifest ourselves using
code adapted from koji_flatpak (thanks Owen Taylor). We publish
it with skopeo, not buildah. And we use skopeo inspect --raw
instead of buildah inspect in the missing arch detection.

Signed-off-by: Adam Williamson awilliam@redhat.com

adamwill commented Oct 2, 2024

@jcline @sgallagh @otaylor

adamwill commented Oct 2, 2024

I did consider wedging in tests of the actual manifest contents - we'd have to mock out json.dumps and file .write to do that, I guess, or sabotage tempfile.TemporaryDirectory() to produce a directory with a known name, maybe - but it seemed a bit excessive as mostly the test would just be a re-statement of the code and the code is mostly just templates. If you think it'd be worth the effort to do this, I can try it.

jcline commented Oct 2, 2024

It's very annoying to get a permalink out of pagure via the UI, but let's link to the version copied in case things change (doesn't seem like it will, but y'know):
https://pagure.io/koji-flatpak/raw/bb45b055c47d199b21c6b163e33048dda7684289/f/koji_flatpak/plugins/flatpak_builder_plugin.py

jcline commented Oct 2, 2024

In the event this fails it seems likely we'll want to retry the whole operation? Or do we want to give up and try again on the next published image?

Either way this should have a try/except that either logs or raises a Nack

jcline commented Oct 2, 2024

Beyond the two notes I left it looks good to me.

sgallagh commented Oct 2, 2024

Yeah, I think we do want to exception-check the inspection (and probably separately the json.loads() in case we get back bad JSON.

I did consider wedging in tests of the actual manifest contents - we'd have to mock out json.dumps and file .write to do that, I guess, or sabotage tempfile.TemporaryDirectory() to produce a directory with a known name, maybe - but it seemed a bit excessive as mostly the test would just be a re-statement of the code and the code is mostly just templates. If you think it'd be worth the effort to do this, I can try it.

Definitely not worth it for this patch, but maybe worth opening a ticket to get to it "someday".

rebased onto 148aa3928c573d064f798b62212fac84bd42cec8

Oct 3, 2024

adamwill commented Oct 3, 2024

Updated with those notes fixed. Plus I went ahead and did the 'hack up tempfile' thing. It's not especially pretty, but hey. I guess I should also have it check the other file we write, I'll do that later, gotta go to dinner.

sgallagh commented Oct 3, 2024

LGTM :thumbsup:

rebased onto 148aa3928c573d064f798b62212fac84bd42cec8

Oct 3, 2024

Pull-Request has been merged by jcline

Oct 3, 2024

jcline commented Oct 3, 2024

I've built this for staging and filed https://pagure.io/fedora-infra/ansible/pull-request/2282, when it's merged and the ansible playbook is run we can take this for a spin in staging.

adamwill commented Oct 3, 2024

I didn't get around to adding test of the other json file yet, but eh, it should be fine. let's see what it does in stg.