diff --git a/.cirrus.yml b/.cirrus.yml index 8a79ade78c..a55b7aa45c 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -229,7 +229,7 @@ validate_task: bindings_task: name: "Test Bindings" alias: bindings - only_if: ¬_docs $CIRRUS_CHANGE_TITLE !=~ '.*CI:DOCS.*' + only_if: ¬_docs $CIRRUS_CHANGE_TITLE =~ '.*CI:DOCS.*' skip: *branches_and_tags depends_on: - build @@ -590,7 +590,7 @@ buildah_bud_test_task: name: *std_name_fmt alias: buildah_bud_test skip: *tags - only_if: *not_docs +# only_if: *not_docs depends_on: - local_integration_test env: @@ -601,7 +601,11 @@ buildah_bud_test_task: CTR_FQIN: ${FEDORA_CONTAINER_FQIN} # ID for re-use of build output _BUILD_CACHE_HANDLE: ${FEDORA_NAME}-build-${CIRRUS_BUILD_ID} - PODBIN_NAME: podman + matrix: + - env: + PODBIN_NAME: podman + - env: + PODBIN_NAME: remote gce_instance: *standardvm timeout_in: 45m clone_script: *noop diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f44b0ea424..91c0fef878 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -4,8 +4,15 @@ repos: - repo: https://github.com/pre-commit/pre-commit-hooks.git rev: v3.4.0 hooks: + # buildah-tests.diff is generated by 'git format-patch' and includes + # trailing whitespace as part of its format. We can work around that, + # but unfortunately the buildah repo has some files with tabs, which + # git-diff formats as '[+/-]', which these hooks choke on. + # Just disable checks on this diff file as a special case. - id: end-of-file-fixer + exclude: test/buildah-bud/buildah-tests.diff - id: trailing-whitespace + exclude: test/buildah-bud/buildah-tests.diff - id: mixed-line-ending - id: check-byte-order-marker - id: check-executables-have-shebangs diff --git a/Makefile b/Makefile index 8326713375..32d2f44b72 100644 --- a/Makefile +++ b/Makefile @@ -214,7 +214,7 @@ endif .PHONY: .gitvalidation .gitvalidation: .gopathok @echo "Validating vs commit '$(call err_if_empty,EPOCH_TEST_COMMIT)'" - GIT_CHECK_EXCLUDE="./vendor:docs/make.bat" $(GOBIN)/git-validation -run DCO,short-subject,dangling-whitespace -range $(EPOCH_TEST_COMMIT)..$(HEAD) + GIT_CHECK_EXCLUDE="./vendor:docs/make.bat:test/buildah-bud/buildah-tests.diff" $(GOBIN)/git-validation -run DCO,short-subject,dangling-whitespace -range $(EPOCH_TEST_COMMIT)..$(HEAD) .PHONY: lint lint: golangci-lint diff --git a/pkg/api/handlers/compat/images_build.go b/pkg/api/handlers/compat/images_build.go index 64805b7fa4..2c98a5361e 100644 --- a/pkg/api/handlers/compat/images_build.go +++ b/pkg/api/handlers/compat/images_build.go @@ -393,16 +393,16 @@ func BuildImage(w http.ResponseWriter, r *http.Request) { defer auth.RemoveAuthfile(authfile) // Channels all mux'ed in select{} below to follow API build protocol - stdout := channel.NewWriter(make(chan []byte, 1)) + stdout := channel.NewWriter(make(chan []byte)) defer stdout.Close() - auxout := channel.NewWriter(make(chan []byte, 1)) + auxout := channel.NewWriter(make(chan []byte)) defer auxout.Close() - stderr := channel.NewWriter(make(chan []byte, 1)) + stderr := channel.NewWriter(make(chan []byte)) defer stderr.Close() - reporter := channel.NewWriter(make(chan []byte, 1)) + reporter := channel.NewWriter(make(chan []byte)) defer reporter.Close() runtime := r.Context().Value("runtime").(*libpod.Runtime) @@ -529,7 +529,7 @@ func BuildImage(w http.ResponseWriter, r *http.Request) { enc := json.NewEncoder(body) enc.SetEscapeHTML(true) -loop: + for { m := struct { Stream string `json:"stream,omitempty"` @@ -543,13 +543,13 @@ loop: stderr.Write([]byte(err.Error())) } flush() - case e := <-auxout.Chan(): + case e := <-reporter.Chan(): m.Stream = string(e) if err := enc.Encode(m); err != nil { stderr.Write([]byte(err.Error())) } flush() - case e := <-reporter.Chan(): + case e := <-auxout.Chan(): m.Stream = string(e) if err := enc.Encode(m); err != nil { stderr.Write([]byte(err.Error())) @@ -561,8 +561,8 @@ loop: logrus.Warnf("Failed to json encode error %v", err) } flush() + return case <-runCtx.Done(): - flush() if success { if !utils.IsLibpodRequest(r) { m.Stream = fmt.Sprintf("Successfully built %12.12s\n", imageID) @@ -579,7 +579,8 @@ loop: } } } - break loop + flush() + return case <-r.Context().Done(): cancel() logrus.Infof("Client disconnect reported for build %q / %q.", registry, query.Dockerfile) diff --git a/pkg/bindings/connection.go b/pkg/bindings/connection.go index fd93c5ac7e..62b1655acf 100644 --- a/pkg/bindings/connection.go +++ b/pkg/bindings/connection.go @@ -327,7 +327,7 @@ func (c *Connection) DoRequest(httpBody io.Reader, httpMethod, endpoint string, uri := fmt.Sprintf("http://d/v%d.%d.%d/libpod"+endpoint, params...) logrus.Debugf("DoRequest Method: %s URI: %v", httpMethod, uri) - req, err := http.NewRequest(httpMethod, uri, httpBody) + req, err := http.NewRequestWithContext(context.WithValue(context.Background(), clientKey, c), httpMethod, uri, httpBody) if err != nil { return nil, err } @@ -337,7 +337,6 @@ func (c *Connection) DoRequest(httpBody io.Reader, httpMethod, endpoint string, for key, val := range header { req.Header.Set(key, val) } - req = req.WithContext(context.WithValue(context.Background(), clientKey, c)) // Give the Do three chances in the case of a comm/service hiccup for i := 0; i < 3; i++ { response, err = c.Client.Do(req) // nolint diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index 142204f277..a35f461a77 100644 --- a/pkg/bindings/images/build.go +++ b/pkg/bindings/images/build.go @@ -391,42 +391,50 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO dec := json.NewDecoder(body) var id string - var mErr error for { var s struct { Stream string `json:"stream,omitempty"` Error string `json:"error,omitempty"` } - if err := dec.Decode(&s); err != nil { - if errors.Is(err, io.EOF) { - if mErr == nil && id == "" { - mErr = errors.New("stream dropped, unexpected failure") - } - break - } - s.Error = err.Error() + "\n" - } select { + // FIXME(vrothberg): it seems we always hit the EOF case below, + // even when the server quit but it seems desirable to + // distinguish a proper build from a transient EOF. case <-response.Request.Context().Done(): - return &entities.BuildReport{ID: id}, mErr + return &entities.BuildReport{ID: id}, nil default: // non-blocking select } + if err := dec.Decode(&s); err != nil { + if errors.Is(err, io.ErrUnexpectedEOF) { + return nil, errors.Wrap(err, "server probably quit") + } + // EOF means the stream is over in which case we need + // to have read the id. + if errors.Is(err, io.EOF) && id != "" { + break + } + return &entities.BuildReport{ID: id}, errors.Wrap(err, "decoding stream") + } + switch { case s.Stream != "": - stdout.Write([]byte(s.Stream)) - if iidRegex.Match([]byte(s.Stream)) { + raw := []byte(s.Stream) + stdout.Write(raw) + if iidRegex.Match(raw) { id = strings.TrimSuffix(s.Stream, "\n") } case s.Error != "": - mErr = errors.New(s.Error) + // If there's an error, return directly. The stream + // will be closed on return. + return &entities.BuildReport{ID: id}, errors.New(s.Error) default: return &entities.BuildReport{ID: id}, errors.New("failed to parse build results stream, unexpected input") } } - return &entities.BuildReport{ID: id}, mErr + return &entities.BuildReport{ID: id}, nil } func nTar(excludes []string, sources ...string) (io.ReadCloser, error) { diff --git a/pkg/domain/infra/runtime_abi.go b/pkg/domain/infra/runtime_abi.go index ca201b5aed..177e9cff40 100644 --- a/pkg/domain/infra/runtime_abi.go +++ b/pkg/domain/infra/runtime_abi.go @@ -33,6 +33,7 @@ func NewImageEngine(facts *entities.PodmanConfig) (entities.ImageEngine, error) r, err := NewLibpodImageRuntime(facts.FlagSet, facts) return r, err case entities.TunnelMode: + // TODO: look at me! ctx, err := bindings.NewConnectionWithIdentity(context.Background(), facts.URI, facts.Identity) return &tunnel.ImageEngine{ClientCtx: ctx}, err } diff --git a/test/buildah-bud/apply-podman-deltas b/test/buildah-bud/apply-podman-deltas index 41c84257f9..0d6d2dcb4a 100755 --- a/test/buildah-bud/apply-podman-deltas +++ b/test/buildah-bud/apply-podman-deltas @@ -56,22 +56,31 @@ function errmsg() { done } -# skip: used to add a 'skip' to one specific test -function skip() { +# _skip: used to add a 'skip' or 'skip_if_remote' to one specific test +function _skip() { + local skip=$1; shift local reason=$1; shift # All further arguments are test names for t in "$@"; do if fgrep -qx "@test \"$t\" {" $BUD; then - $ECHO "@test \"$t\" : skip \"$reason\"" + $ECHO "@test \"$t\" : $skip \"$reason\"" t=${t//\//\\/} - sed -i -e "/^\@test \"$t\" {/ a \ \ skip \"$reason\"" $BUD + sed -i -e "/^\@test \"$t\" {/ a \ \ $skip \"$reason\"" $BUD else - warn "[skip] Did not find test \"$t\" in $BUD" + warn "[$skip] Did not find test \"$t\" in $BUD" fi done } +function skip() { + _skip "skip" "$@" +} + +function skip_if_remote() { + _skip "skip_if_remote" "$@" +} + # END handlers ############################################################################### # BEGIN user-customizable section @@ -79,14 +88,14 @@ function skip() { # These are the hand-maintained exceptions. This is what you want to edit # or update as needed. # -# There are two directives you can use below: +# There are three directives you can use below: # # errmsg "old-message" "new-message" "test name" ["test name"...] # # This replaced "old-message" with "new-message" in @test "test name". # It is used when a podman error message differs from buildah's. # -# skip "reason" "test name" ["test name"...] +# [skip | skip_if_remote] "reason" "test name" ["test name"...] # # This adds a 'skip' statement as the first line of @test "test name". # It is used when a test does not work in podman, either for permanent @@ -126,6 +135,7 @@ errmsg "no such file or directory" \ ############################################################################### # BEGIN tests that don't make sense under podman due to fundamental differences + skip "N/A under podman" \ "bud-flags-order-verification" @@ -147,13 +157,13 @@ skip "Too much effort to spin up a local registry" \ skip "FIXME FIXME FIXME: argument-order incompatible with podman" \ "bud-squash-hardlinks" -skip "FIXME FIXME FIXME we'll figure these out later" \ - "bud-multi-stage-nocache-nocommit" \ - "bud with --cgroup-parent" +skip "FIXME FIXME FIXME: this passes on Ed's laptop, fails in CI??" \ + "bud-multi-stage-nocache-nocommit" -# see https://github.com/containers/podman/pull/10147#issuecomment-832503633 -skip "FIXME FIXME FIXME podman save/load has been fixed (but not yet used in Buildah CI)" \ - "bud with --layers and --no-cache flags" +# This will probably never work: buildah and podman have incompatible defaults +# Documented in https://github.com/containers/podman/issues/10412 +skip "buildah runs with --cgroup-manager=cgroupfs, podman with systemd" \ + "bud with --cgroup-parent" # see https://github.com/containers/podman/pull/10829 skip "FIXME FIXME FIXME - requires updated CI images (#10829)" \ @@ -163,6 +173,43 @@ skip "FIXME FIXME FIXME - requires updated CI images (#10829)" \ # BEGIN tests which are skipped due to actual podman bugs. +############################################################################### +# BEGIN tests which are skipped because they make no sense under podman-remote + +skip_if_remote "--target does not work with podman-remote" \ + "bud-target" + +skip_if_remote "--runtime not meaningful under podman-remote" \ + "bud with --runtime and --runtime-flag" + +skip_if_remote "secret files not implemented under podman-remote" \ + "bud with containerfile secret" \ + "bud with containerfile secret accessed on second RUN" \ + "bud with containerfile secret options" + +skip_if_remote "volumes don't work with podman-remote" \ + "buildah bud --volume" \ + "buildah-bud-policy" + +# See podman #9890 for discussion +skip_if_remote "--stdin option will not be implemented in podman-remote" \ + "bud test no --stdin" + +############################################################################### +# BEGIN tests which are skipped due to actual podman-remote bugs. + +#skip_if_remote "FIXME: podman #10907 (complicated .dockerignore file)" \ +# "bud with .dockerignore" +# +#skip_if_remote "FIXME: podman #10908 (.containerignore + .dockerignore)" \ +# "bud with .containerignore" + +# Warning messages go to server, not client +# FIXME FIXME FIXME: this is now buildah #3285, which merged 2021-06-15. +# FIXME FIXME FIXME: remove this once we vendor in a buildah > v1.21.1 +skip_if_remote "FIXME: podman #10616 - cpp stderr shown on server end" \ + "bud with preprocessor error" + ############################################################################### # Done. diff --git a/test/buildah-bud/buildah-tests.diff b/test/buildah-bud/buildah-tests.diff index 1c8592f7f8..ff91cad95e 100644 --- a/test/buildah-bud/buildah-tests.diff +++ b/test/buildah-bud/buildah-tests.diff @@ -1,23 +1,75 @@ -From a00508656599b24776982996fdb44d4874338fd4 Mon Sep 17 00:00:00 2001 +From dc44d1c82c87bf49183b23a99dc61272f934d6d6 Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Tue, 9 Feb 2021 17:28:05 -0700 Subject: [PATCH] tweaks for running buildah tests under podman Signed-off-by: Ed Santiago --- - tests/helpers.bash | 28 +++++++++++++++++++++++++--- - 1 file changed, 25 insertions(+), 3 deletions(-) + tests/helpers.bash | 71 +++++++++++++++++++++++++++++++++++++++++++--- + 1 file changed, 67 insertions(+), 4 deletions(-) diff --git a/tests/helpers.bash b/tests/helpers.bash -index 11deb367..08e73954 100644 +index 11deb367..44c71dad 100644 --- a/tests/helpers.bash +++ b/tests/helpers.bash -@@ -164,15 +164,37 @@ function run_buildah() { +@@ -34,6 +34,23 @@ function setup() { + ROOTDIR_OPTS="--root ${TESTDIR}/root --runroot ${TESTDIR}/runroot --storage-driver ${STORAGE_DRIVER}" + BUILDAH_REGISTRY_OPTS="--registries-conf ${TESTSDIR}/registries.conf --registries-conf-dir ${TESTDIR}/registries.d --short-name-alias-conf ${TESTDIR}/cache/shortnames.conf" + PODMAN_REGISTRY_OPTS="--registries-conf ${TESTSDIR}/registries.conf" ++ ++ PODMAN_SERVER_PID= ++ PODMAN_NATIVE="${PODMAN_BINARY} ${ROOTDIR_OPTS} ${PODMAN_REGISTRY_OPTS}" ++ if [[ -n "$REMOTE" ]]; then ++ PODMAN_NATIVE="${PODMAN_BINARY%%-remote} ${ROOTDIR_OPTS} ${PODMAN_REGISTRY_OPTS}" ++ # static CONTAINERS_CONF needed for capabilities test. As of 2021-07-01 ++ # no tests in bud.bats override this; if at some point any test does ++ # so, it will probably need to be skip_if_remote()d. ++ env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} $PODMAN_NATIVE system service --timeout=0 & ++ PODMAN_SERVER_PID=$! ++ local timeout=10 ++ while ((timeout > 0)); do ++ test -S /run/podman/podman.sock && return ++ sleep 0.2 ++ done ++ die "podman server never came up" ++ fi + } + + function starthttpd() { +@@ -57,6 +74,12 @@ function stophttpd() { + function teardown() { + stophttpd + ++ if [[ -n "$PODMAN_SERVER_PID" ]]; then ++ kill $PODMAN_SERVER_PID ++ wait $PODMAN_SERVER_PID ++ rm -f /run/podman/podman.sock ++ fi ++ + # Workaround for #1991 - buildah + overlayfs leaks mount points. + # Many tests leave behind /var/tmp/.../root/overlay and sub-mounts; + # let's find those and clean them up, otherwise 'rm -rf' fails. +@@ -129,7 +152,13 @@ function copy() { + } + + function podman() { +- command podman ${PODMAN_REGISTRY_OPTS} ${ROOTDIR_OPTS} "$@" ++ echo "# ... podman $*" >&3 ++ ${PODMAN_BINARY} ${PODMAN_REGISTRY_OPTS} ${ROOTDIR_OPTS} "$@" ++} ++ ++function podman-remote() { ++ echo "# ... podman-remote $*" >&3 ++ ${PODMAN_BINARY} ${ROOTDIR_OPTS} "$@" + } + + ################# +@@ -164,15 +193,40 @@ function run_buildah() { --retry) retry=3; shift;; # retry network flakes esac - + + local podman_or_buildah=${BUILDAH_BINARY} -+ local registry_opts=${BUILDAH_REGISTRY_OPTS} ++ local _opts="${ROOTDIR_OPTS} ${BUILDAH_REGISTRY_OPTS}" + if [[ $1 == "bud" || $1 == "build-using-dockerfile" ]]; then + shift + # podman defaults to --layers=true; buildah to --false. @@ -29,7 +81,10 @@ index 11deb367..08e73954 100644 + set "build" "--force-rm=false" "--layers=false" "$@" + fi + podman_or_buildah=${PODMAN_BINARY} -+ registry_opts=${PODMAN_REGISTRY_OPTS} ++ _opts="${ROOTDIR_OPTS} ${PODMAN_REGISTRY_OPTS}" ++ if [[ -n "$REMOTE" ]]; then ++ _opts= ++ fi + + # podman always exits 125 where buildah exits 1 or 2 + case $expected_rc in @@ -41,17 +96,31 @@ index 11deb367..08e73954 100644 # Remember command args, for possible use in later diagnostic messages - MOST_RECENT_BUILDAH_COMMAND="buildah $*" + MOST_RECENT_BUILDAH_COMMAND="$cmd_basename $*" - + while [ $retry -gt 0 ]; do retry=$(( retry - 1 )) - + # stdout is only emitted upon error; this echo is to help a debugger - echo "\$ $BUILDAH_BINARY $*" - run env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} timeout --foreground --kill=10 $BUILDAH_TIMEOUT ${BUILDAH_BINARY} ${BUILDAH_REGISTRY_OPTS} ${ROOTDIR_OPTS} "$@" + echo "\$ $cmd_basename $*" -+ run env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} timeout --foreground --kill=10 $BUILDAH_TIMEOUT ${podman_or_buildah} ${registry_opts} ${ROOTDIR_OPTS} "$@" ++ run env CONTAINERS_CONF=${CONTAINERS_CONF:-$(dirname ${BASH_SOURCE})/containers.conf} timeout --foreground --kill=10 $BUILDAH_TIMEOUT ${podman_or_buildah} ${_opts} "$@" # without "quotes", multiple lines are glommed together into one if [ -n "$output" ]; then echo "$output" --- +@@ -396,3 +450,12 @@ function skip_if_no_docker() { + skip "this test needs actual docker, not podman-docker" + fi + } ++ ++#################### ++# skip_if_remote # (only applicable for podman) ++#################### ++function skip_if_remote() { ++ if [[ -n "$REMOTE" ]]; then ++ skip "${1:-test does not work with podman-remote}" ++ fi ++} +-- 2.31.1 + diff --git a/test/buildah-bud/make-new-buildah-diffs b/test/buildah-bud/make-new-buildah-diffs index 11987e3760..3d0a770083 100644 --- a/test/buildah-bud/make-new-buildah-diffs +++ b/test/buildah-bud/make-new-buildah-diffs @@ -56,8 +56,6 @@ if [[ -n "$patch2" ]]; then die "Internal error: I thought I checked for squashed commits, but still see $patch2" fi -# All looks good. Now write that patch into its proper place in the -# podman repo. The sed and tac mess strips trailing whitespace and -# empty lines; we need to do this to pass github CI checks. -sed -e 's/ \+$//' <0001-*.patch |\ - tac | sed -e '/./,$!d' | tac >| ../test/buildah-bud/buildah-tests.diff +# All looks good. We can now copy that patch into its proper place in the +# podman repo. +cp 0001-*.patch ../test/buildah-bud/buildah-tests.diff diff --git a/test/buildah-bud/run-buildah-bud-tests b/test/buildah-bud/run-buildah-bud-tests index a37e90dc43..eb8de56185 100755 --- a/test/buildah-bud/run-buildah-bud-tests +++ b/test/buildah-bud/run-buildah-bud-tests @@ -69,8 +69,7 @@ REMOTE= # If remote, start server & change path if [[ "${PODBIN_NAME:-}" = "remote" ]]; then REMOTE=1 - echo "$ME: remote tests are not working yet" >&2 - exit 1 + PODMAN_BINARY+="-remote" fi function die() { @@ -214,6 +213,7 @@ review the test failure and double-check your changes. (set -x;sudo env TMPDIR=/var/tmp \ PODMAN_BINARY=$PODMAN_BINARY \ + REMOTE=$REMOTE \ BUILDAH_BINARY=$(pwd)/bin/buildah \ COPY_BINARY=$(pwd)/bin/copy \ bats "${bats_filter[@]}" tests/bud.bats) diff --git a/test/system/070-build.bats b/test/system/070-build.bats index 7b76c585f9..26113e45c7 100644 --- a/test/system/070-build.bats +++ b/test/system/070-build.bats @@ -749,16 +749,9 @@ RUN echo $random_string EOF run_podman 125 build -t build_test --pull-never $tmpdir - # FIXME: this is just ridiculous. Even after #10030 and #10034, Ubuntu - # remote *STILL* flakes this test! It fails with the correct exit status, - # but the error output is 'Error: stream dropped, unexpected failure' - # Let's just stop checking on podman-remote. As long as it exits 125, - # we're happy. - if ! is_remote; then - is "$output" \ - ".*Error: error creating build container: quay.io/libpod/nosuchimage:nosuchtag: image not known" \ - "--pull-never fails with expected error message" - fi + is "$output" \ + ".*Error: error creating build container: quay.io/libpod/nosuchimage:nosuchtag: image not known" \ + "--pull-never fails with expected error message" } @test "podman build --logfile test" {