Fix BUILDPLATFORM redefinition issue in some Containerfiles

[frobware]

Removed the hardcoded ARG BUILDPLATFORM=linux/amd64 from
Containerfile.bpfman-agent{.openshift} and Containerfile.bpfman-operator{.openshift}.

This change addresses an issue where Podman fails to build the images if
BUILDPLATFORM is redefined. By removing the redefinition, the build
process is made compatible with Podman.

This change remains compatible with Docker, which is more lenient with
ARG redefinitions.

I didn't bisect which version of Podman made this a failure; it doesn't occur with:

% podman version
Client:       Podman Engine
Version:      5.0.3
API Version:  5.0.3
Go Version:   go1.22.5
Built:        Tue Jan  1 00:00:00 1980
OS/Arch:      linux/amd64

but does with:

% podman version
Client:       Podman Engine
Version:      5.2.2
API Version:  5.2.2
Go Version:   go1.22.6
Built:        Wed Aug 21 01:00:00 2024
OS/Arch:      linux/amd64

Fixes: #120

[frobware]

@msherif1234 I didn't change either of:

• Containerfile.bpfman-agent.openshift
• Containerfile.bpfman-operator.openshift

These two files also redefine BUILDPLATFORM twice. Should we change the OpenShift files too?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.61%. Comparing base (7bac877) to head (1dcd857).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #121   +/-   ##
=======================================
  Coverage   27.61%   27.61%           
=======================================
  Files          81       81           
  Lines        6999     6999           
=======================================
  Hits         1933     1933           
  Misses       4877     4877           
  Partials      189      189           

Flag	Coverage Δ
unittests	27.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[frobware]

Is there any issue with my change w.r.t. multi-arch builds?

[msherif1234]

@msherif1234 I didn't change either of:

• Containerfile.bpfman-agent.openshift
• Containerfile.bpfman-operator.openshift

These two files also redefine BUILDPLATFORM twice. Should we change the OpenShift files too?

Yes make sense while u there to remove the duplicate as well for openshift version

[anfredette]

I've seen the following warning when building the operator and agent images:

WARN: RedundantTargetPlatform: Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior (line 27)

Is this a similar issue?

[frobware]

I've seen the following warning when building the operator and agent images:

WARN: RedundantTargetPlatform: Setting platform to predefined $TARGETPLATFORM in FROM is redundant as this is the default behavior (line 27)

Is this a similar issue?

I was wary of "fixing" this in the same way in case that breaks multi-arch builds. But if you're game we can find out.

[anfredette]

I was wary of "fixing" this in the same way in case that breaks multi-arch builds. But if you're game we can find out.

I was too. It still seems to work, so let's leave it as is until we get multi-arch builds all worked out, then we can give fixing it a try.

[frobware]

@msherif1234 I didn't change either of:

• Containerfile.bpfman-agent.openshift
• Containerfile.bpfman-operator.openshift

These two files also redefine BUILDPLATFORM twice. Should we change the OpenShift files too?

Yes make sense while u there to remove the duplicate as well for openshift version

Done in https://github.com/bpfman/bpfman-operator/compare/092556513f397958167b9dcf2cf9014eb7a71946..7b708a9045d54bb99f8a2b549f9b91c55aff518d.

[msherif1234]

I wonder if this PR fixes konflux CI issues in #122
/LGTM

[anfredette]

@Billy99 do you have any concerns with merging this PR?

[Billy99]

@Billy99 do you have any concerns with merging this PR?

@frobware Did you test with docker? I didn't try podman, but a similar change in bpfman repo causes a problem:

$ cat Containerfile.bpfman
ARG BUILDPLATFORM

FROM --platform=$BUILDPLATFORM redhat/ubi9-minimal AS bpfman-build
:

$ docker buildx build --progress=plain --platform linux/arm64 -f ./Containerfile.bpfman . -t quay.io/billy99/bpfman:test
#0 building with "default" instance using docker driver

#1 [internal] load build definition from Containerfile.bpfman
#1 transferring dockerfile: 2.02kB done
#1 DONE 0.0s
Containerfile.bpfman:4
--------------------
   2 |     ARG BUILDPLATFORM
   3 |     
   4 | >>> FROM --platform=$BUILDPLATFORM redhat/ubi9-minimal AS bpfman-build
   5 |     
   6 |     # The following ARGs are set internally by docker/build-push-action in github actions
--------------------
ERROR: failed to solve: failed to parse platform : "" is an invalid component of "": platform specifier component must match "^[A-Za-z0-9_-]+$": invalid argument

Still playing with it.

[frobware]

I pushed changes today - need to test again with docker. But I think we should augment the CI workflow to test both docker and podman. It’s much more efficient than manual testing.

…

On Thu, 5 Sep 2024 at 19:21, Billy McFall ***@***.***> wrote: @Billy99 <https://github.com/Billy99> do you have any concerns with merging this PR? @frobware <https://github.com/frobware> Did you test with docker? I didn't try podman, but a similar change in bpfman repo causes a problem: $ cat Containerfile.bpfman ARG BUILDPLATFORM FROM --platform=$BUILDPLATFORM redhat/ubi9-minimal AS bpfman-build : $ docker buildx build --progress=plain --platform linux/arm64 -f ./Containerfile.bpfman . -t quay.io/billy99/bpfman:test #0 <http://quay.io/billy99/bpfman:test#0> building with "default" instance using docker driver #1 [internal] load build definition from Containerfile.bpfman #1 transferring dockerfile: 2.02kB done #1 DONE 0.0s Containerfile.bpfman:4 -------------------- 2 | ARG BUILDPLATFORM 3 | 4 | >>> FROM --platform=$BUILDPLATFORM redhat/ubi9-minimal AS bpfman-build 5 | 6 | # The following ARGs are set internally by docker/build-push-action in github actions -------------------- ERROR: failed to solve: failed to parse platform : "" is an invalid component of "": platform specifier component must match "^[A-Za-z0-9_-]+$": invalid argument Still playing with it. — Reply to this email directly, view it on GitHub <#121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHGOHYAYE55MVMCOISD223ZVCOLXAVCNFSM6AAAAABNPYE3A6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZSGM3TGOBRGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Billy99]

There is a comment in my bpfman Containerfiles about not using --platform to fill in this ARG, but I didn't write it:

# We do not use --platform feature to auto fill this ARG because of incompatibility between podman and docker
ARG BUILDPLATFORM=linux/amd64

But I think we should augment the CI workflow to test both docker and podman. It’s much more efficient than manual testing.

So we could update CI to use both podman and docker (maybe, would have to look at the github actions and make sure there is equivalent for all the actions we are using), but base CI on a push doesn't test for either. Today, images are only built when the PR merges (build and push) or if image-build.yaml is updated (build). So your green CI for this PR didn't test docker or podman, unless I misunderstood your comment.

[frobware]

Today, images are only built when the PR merges (build and push) or if image-build.yaml is updated (build). So your green CI for this PR didn't test docker or podman, unless I misunderstood your comment.

It would have tested Docker because the make build-images step is executed prior to running the integration tests. Therefore, even though images are only pushed after a PR merges, the image build itself is still performed during the CI process before the integration tests are executed.

I updated the CI steps in commit to run:

• make build-images
• make OCI_BIN=podman build-images

This goals was to ensure we validate the image builds with both Docker and Podman. However, the version of Podman that gets installed from the Ubuntu archives (v3.4) is outdated and doesn't catch the redefinition issue, which only appears in Podman 5.2. As a result, while the CI change is valid, it wouldn't have detected the failure due to the older Podman version.

The CI output shows that podman is now used in addition to docker for the build-images step.

I was trying to avoid manual testing for future PRs by ensuring that make OCI_BIN=podman build-images works on every pull request. It's debatable whether it's worth pursuing without having a means to install newer versions of podman on ubuntu.

[frobware]

/hold

I want to further understand whether we should be setting any buildplatform/targetplaform/targetarch related ARGS in global scope. Perhaps we only need to define these in build stages and rely on the defaults.

[frobware]

/hold

I want to further understand whether we should be setting any buildplatform/targetplaform/targetarch related ARGS in global scope. Perhaps we only need to define these in build stages and rely on the defaults.

/hold cancel

I simplified the change in https://github.com/bpfman/bpfman-operator/compare/9f219922bbdc20f27a92518189a449cc96825ad7..1dcd8577e793acd8813ca2c0ba45eb39aa4957c4.

We still need to decide whether to keep the CI commit that uses both Podman and Docker for building images. At the very least, it provides some CI coverage for Podman compatibility, even though the version of Podman is very outdated.

[frobware]

I wonder if this PR fixes konflux CI issues in #122 /LGTM

Seems like the same issue looking at the build failure from: https://github.com/bpfman/bpfman-operator/pull/122/checks?check_run_id=29757802714.

[anfredette]

I suspect it's not related. #122 passed multiple integration test cases, then failed on the application one. I wonder if it's just a flake, or if something else changed that's causing that one to fail. I just restarted that test.

[frobware]

I pulled the CI change out as #130. Given how old the version of Podman is on Ubuntu, I'm doubtful of the value of running Podman per the build-images step. This simplifies this PR to focus solely on fixing day-to-day development when using Podman.

[anfredette]

I suspect it's not related. #122 passed multiple integration test cases, then failed on the application one.

I was focused on the integration test failure, which is cleared up now, but there are still two Konflux failures, so those may be related.

[anfredette]

LGTM