Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "libimage: pull: do not enforce pull if local image matches" #1299

Merged

Conversation

vrothberg
Copy link
Member

This reverts commit ba3a9c0 as it caused a nasty flake on Buildah [1]

[1] containers/buildah#4527

Signed-off-by: Valentin Rothberg [email protected]

This reverts commit ba3a9c0 as it
caused a nasty flake on Buildah [1]

[1] containers/buildah#4527

Signed-off-by: Valentin Rothberg <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nalind
Copy link
Member

nalind commented Jan 18, 2023

/lgtm
/hold

@vrothberg
Copy link
Member Author

/hold cancel

@akostadinov
Copy link

So because of bad test design in buildah, this fix cannot be implemented? Doesn't sound like viable long-term 😞

@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2023

That is kind of you to say.

But the real issue is that images report bogus data in the arch, so enforcing that the image ARCH match the host ARCH, proved problematic. Lots of images at docker.io and other registries, report the wrong arch in the image manifest.

@vrothberg
Copy link
Member Author

vrothberg commented Feb 16, 2023

So because of bad test design in buildah, this fix cannot be implemented? Doesn't sound like viable long-term disappointed

The real issue for having reverted this commit is that the dependency we use for platform parsing treats linux/arm/v6 as linux/arm/v7. This issue was revealed by the Buildah tests.

I am looking again into this issue but I need to reach out to the containerd folks before. The issue also impacts Docker and containerd.

@akostadinov
Copy link

I don't want to be rude, I apologize if it sounded like that. I didn't know what to say when I was left with the impression that things were reverted because of test flakiness. Apparently I was wrong that this was the reason but I felt the need to question the decision. And didn't choose the best words.

Just for a context, I love podman/buildah and sometimes I've spent a lot of time to use it locally and in CI for the job and private projects I worked on. Documentation/blogs are more often than not about docker and I have to figure out more things by myself. Also debugging a CI workflow is very time wasting because of the long turnaround times. So it was frustrating to see that a known issue was abandoned for a reason that didn't sound compelling (as it turned out, that was not the reason).

So I'm ready to take some amount of pain to make our projects better. And I appreciate that usually (or rather always) the podman team is very responsive. And podman really rocks, very solid and nice to use.

P.S. another pain point with CI, in particular github actions, is redhat-actions/buildah-build#115 because it makes builds much slower compared to buildx, which does get in the way. (I know buildah supports that in general, just the action doesn't). Might be also because buildah/podman are old in ubuntu, I don't know if there is any way for podman team to get them updated in the default github runner.

@vrothberg
Copy link
Member Author

@akostadinov, no worries at all. I appreciate your honesty and feedback!

For this specific issue, the best workaround is probably to use --pull=never. This way, the local image will be used. We're currently discussing a solution in #1295.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants