WIP - remote build: fix streaming and error handling

.cirrus.yml

@@ -229,7 +229,7 @@ validate_task:
 bindings_task:
     name: "Test Bindings"
     alias: bindings
-    only_if: &not_docs $CIRRUS_CHANGE_TITLE !=~ '.*CI:DOCS.*'
+    only_if: &not_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

.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 '[+/-]<space><tab>', 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

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

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)

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

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) {

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
 	}

test/buildah-bud/apply-podman-deltas

@@ -56,37 +56,46 @@ 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
 #
 # 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.