Various patches to build extensions container using `podman build` in production by jlebon · Pull Request #4044 · coreos/coreos-assembler

jlebon · 2025-03-17T14:26:38Z

We're getting ready to build the extensions container in production as part of the layered node image work. To be able to build it using podman build directly and not via cosa build-extensions-container, a few adjustments are needed.

See individual commit messages.

Another one of those that's assumed by some userspace apps (in this case, apparently, `podman run`).

jbtrystram · 2025-03-17T15:12:59Z

        '--git-sub-dir', default='', required=False,
        help='Git sub directory to use for container build')
+    parser.add_argument(
+        '--git-file', default='', required=False,


minor: I think the name of the argument is confusing, containerfile is enough, with the help text as is IMO

I kind of agree.. alternatively.. --contextdir= where you can specify the full path to the containerfile as an option.

I want something with git in it to make it clear it's about the git repo. Maybe just --git-containerfile?

OK, just went with --git-containerfile for now.

dustymabe · 2025-03-17T17:09:18Z

-            raise Exception("Error encountered when checking if manifest exists")
-        if cp.returncode == 0:
-            return True
+    cmd = ["podman", "image", "exists", f"{repo}:{tag}"]


I wouldn't make this change unless we've tested it fully.

My experience here was that we needed them both: 44901a6. If I could have used just one I definitely would have.

Right, meant to point you at this for a sanity-check.

OK so testing this in depth now:

if an image is not a manifest list:

podman manifest exists returns 125

podman image exists returns 0

if an image is a manifest list:

podman manifest exists returns 0

podman image exists returns 1

if an image doesn't exist

podman manifest exists returns 1

podman image exists returns 1

I think there's a bug there and podman image exists should probably actually return 0 if the image exists but it's a manifest.

But anyway... I think we can work around this by reversing the order in which we call the two commands?

If it doesn't exist, then both iterations will pass. If it's a manifest, the second iteration returns True. If it's an image, the first iteration returns True.

Sorry for not thinking critically on this. Today has had a lot going on.

Are you saying there is a bug in the current code?

Yes. I summarized it in the commit message.

Well, I think it's debatable whether it's a bug in our code or a bug in podman that our code is hitting. I'd argue more for the latter.

yeah. I'm surprised. I was pretty sure I tested all permutations when I added this code because we were hitting an issue where someone had pulled the image on the node (i.e. not manifest listed) and I needed to update it and I thought I handled all cases.

Either way, it sounds like you've got all cases covered so LGTM

dustymabe

LGTM - merge when you're ready

Instead of bind-mounting `/tmp/extensions.json`, just extract it from the image now that it's baked in there.

For building the extensions, we need to be able to point buildah at the root of the git repo for the context dir but the Dockerfile in the `extensions/` subdirectory. Add a `--git-containerfile` for this.

When cloning the git repo to pass to buildah, make sure to initiatlize submodules. E.g. building the extensions container requires parsing the manifest, which references bits in the fedora-coreos-config submodule.

There's an asymmetry between `podman image exists` and `podman manifest exists`. The first will return 1 if the image is a manifest. The second will return 125 if the image is _not_ a manifest. I think this is probably a bug in podman, but for now we can work around this by reverting the order in which we do the checking. That way, if the image exists but isn't a manifest, we'll return `True` and not actually call `podman manifest exists` on it.

jlebon · 2025-03-18T20:46:25Z

I reworked this to make the build-extensions-container.sh change compatible with both the old and the new way so that we can merge it now so that we can merge this PR first so that in turn we don't need to override tests in openshift/os#1772 to merge that one.

dustymabe · 2025-03-18T20:56:50Z

I reworked this to make the build-extensions-container.sh change compatible with both the old and the new way so that we can merge it now so that we can merge this PR first so that in turn we don't need to override tests in openshift/os#1772 to merge that one.

I think the updated change to support this is fine, but would like to PR later this week or something to drop the extra code/workaround.

dustymabe

/lgtm

supermin-init-prelude: also create /dev/std{in,out,err}

ef40632

Another one of those that's assumed by some userspace apps (in this case, apparently, `podman run`).

jlebon mentioned this pull request Mar 17, 2025

NO-JIRA: Various patches to build extensions container using podman build in production openshift/os#1772

Merged

jbtrystram reviewed Mar 17, 2025

View reviewed changes

dustymabe reviewed Mar 17, 2025

View reviewed changes

Comment thread src/build-extensions-container.sh Outdated

jlebon force-pushed the pr/extensions-build branch 2 times, most recently from 3c6feeb to e2c2709 Compare March 17, 2025 19:30

dustymabe previously approved these changes Mar 18, 2025

View reviewed changes

jlebon added 4 commits March 18, 2025 16:41

build-extensions-container.sh: extract extensions.json from image

3929853

Instead of bind-mounting `/tmp/extensions.json`, just extract it from the image now that it's baked in there.

cmd-remote-build-container: add --git-containerfile

131a929

For building the extensions, we need to be able to point buildah at the root of the git repo for the context dir but the Dockerfile in the `extensions/` subdirectory. Add a `--git-containerfile` for this.

cmd-remote-build-container: initialize submodules in git clone

0c8d8d4

When cloning the git repo to pass to buildah, make sure to initiatlize submodules. E.g. building the extensions container requires parsing the manifest, which references bits in the fedora-coreos-config submodule.

jlebon dismissed dustymabe’s stale review via f0da6d2 March 18, 2025 20:45

jlebon force-pushed the pr/extensions-build branch from e2c2709 to f0da6d2 Compare March 18, 2025 20:45

dustymabe approved these changes Mar 18, 2025

View reviewed changes

jlebon enabled auto-merge (rebase) March 18, 2025 21:15

jlebon merged commit e61093c into coreos:main Mar 18, 2025
5 checks passed

jlebon deleted the pr/extensions-build branch March 18, 2025 23:20

Conversation

jlebon commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dustymabe Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dustymabe Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dustymabe left a comment

Choose a reason for hiding this comment

Uh oh!

jlebon commented Mar 18, 2025

Uh oh!

dustymabe commented Mar 18, 2025

Uh oh!

dustymabe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dustymabe Mar 17, 2025 •

edited

Loading

dustymabe Mar 17, 2025 •

edited

Loading