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

vendor latest c/{buildah,common,image,storage} #24447

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Nov 1, 2024

Also add a regression test for the TZDIR + --tz=local issue that I fixed in c/common containers/common#2219

Does this PR introduce a user-facing change?

Fixed an issue where --tz=local could not be used when the TZDIR environment variable was set. 

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 1, 2024
@Luap99 Luap99 marked this pull request as draft November 4, 2024 15:27
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2024
@edsantiago
Copy link
Member

There's one more change you might need from #13808, the skip.*secret

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 4, 2024

The buildah failure reproducer:

diff --git a/tests/bud/multiarch/Dockerfile.no-run b/tests/bud/multiarch/Dockerfile.no-run
index 959f4be2f..7313f2209 100644
--- a/tests/bud/multiarch/Dockerfile.no-run
+++ b/tests/bud/multiarch/Dockerfile.no-run
@@ -3,7 +3,7 @@
 ARG SAFEIMAGE
 
 # A base image that is known to be a manifest list.
-FROM docker.io/library/alpine
+FROM quay.io/libpod/alpine
 COPY Dockerfile.no-run /root/
 
 FROM $SAFEIMAGE
diff --git a/vendor/github.com/containers/common/libimage/pull.go b/vendor/github.com/containers/common/libimage/pull.go
index ad4699c60..dec3f82cb 100644
--- a/vendor/github.com/containers/common/libimage/pull.go
+++ b/vendor/github.com/containers/common/libimage/pull.go
@@ -5,6 +5,7 @@ package libimage
 import (
        "context"
        "errors"
+       "sync/atomic"
        "fmt"
        "io"
        "os"
@@ -686,6 +687,11 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
                        logrus.Errorf("Error recording short-name alias %q: %v", candidateString, err)
                }
 
+               pullCount := firstUse.Add(1)
+               logrus.Errorf("Pull count %d", pullCount)
+               if pullCount ==1 {
+                       time.Sleep(3*time.Minute)
+               }
                logrus.Debugf("Pulled candidate %s successfully", candidateString)
                ids, err := r.imageIDForPulledImage(candidate.Value, manifestBytes)
                if err != nil {
@@ -704,3 +710,5 @@ func (r *Runtime) copySingleImageFromRegistry(ctx context.Context, imageName str
 
        return "", resolved.FormatPullErrors(pullErrors)
 }
+
+var firstUse atomic.Int32

and

bin/buildah build --log-level=debug --jobs=0  --all-platforms --manifest localhost/testlist --build-arg SAFEIMAGE=quay.io/libpod/testimage:20221018  -f tests/bud/multiarch/Dockerfile.no-run tests/bud/multiarch/

I confirm the hypothesis: each per-platform pull moves the quay.io/libpod/alpine:latest tag, and with that, quay.io/libpod/alpine@sha256:… on the other platforms no longer matches anything.

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 4, 2024

Also confirming that reverting containers/common@aa722ef fixes this…

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 4, 2024

… and, that, alternatively, (today’s version of) containers/image#2609 + containers/common#2209 fixes this reproducer.

So I’d prefer this alternative (merging the two PRs) to the revert. I’ll also prepare a test Podman PR based on this one.

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 4, 2024

#24462 is running to POC the alternative of obtaining the image ID from c/image instead of doing a lookup.

Copy link

Cockpit tests failed for commit 8016809. @martinpitt, @jelly, @mvollmer please check.

@Luap99 Luap99 force-pushed the vendor branch 2 times, most recently from 14bd9c0 to 63dbf1a Compare November 6, 2024 16:25
Regression test for containers#23550. Setting the TZDIR env should make no
difference for the local timezone as this is not a real timezone name
that is resolved from that directory.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99 Luap99 marked this pull request as ready for review November 7, 2024 09:40
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2024
@Luap99
Copy link
Member Author

Luap99 commented Nov 7, 2024

@containers/podman-maintainers PTAL

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll defer to @edsantiago for the tests.

Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5, Luap99

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

@edsantiago
Copy link
Member

Tests LGTM. I'm curious why bud remote tests pass even without https://github.com/containers/podman/pull/13808/files#diff-8ba024261bda9c3e2b398f3af7416765e8b179f0de7470d3794095069d34463e but not curious enough to spend time on it.

@rhatdan
Copy link
Member

rhatdan commented Nov 7, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 6598a36 into containers:main Nov 7, 2024
86 checks passed
@Luap99 Luap99 deleted the vendor branch November 7, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants