Merge pull request #20377 from rhatdan/commit

Add status messages to podman --remote commit

pkg/api/handlers/compat/images.go

@@ -165,7 +165,7 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 
 	commitImage, err := ctr.Commit(r.Context(), destImage, options)
 	if err != nil && !strings.Contains(err.Error(), "is not running") {
-		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("CommitFailure: %w", err))
+		utils.Error(w, http.StatusInternalServerError, err)
 		return
 	}
 	utils.WriteResponse(w, http.StatusCreated, entities.IDResponse{ID: commitImage.ID()})

pkg/api/handlers/compat/images_build.go

@@ -22,6 +22,7 @@ import (
 	"github.com/containers/podman/v4/pkg/api/handlers/utils"
 	api "github.com/containers/podman/v4/pkg/api/types"
 	"github.com/containers/podman/v4/pkg/auth"
+	"github.com/containers/podman/v4/pkg/bindings/images"
 	"github.com/containers/podman/v4/pkg/channel"
 	"github.com/containers/podman/v4/pkg/rootless"
 	"github.com/containers/podman/v4/pkg/util"
@@ -781,14 +782,7 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
 	var stepErrors []string
 
 	for {
-		type BuildResponse struct {
+		m := images.BuildResponse{}
-			Stream string                 `json:"stream,omitempty"`
-			Error  *jsonmessage.JSONError `json:"errorDetail,omitempty"`
-			// NOTE: `error` is being deprecated check https://github.com/moby/moby/blob/master/pkg/jsonmessage/jsonmessage.go#L148
-			ErrorMessage string          `json:"error,omitempty"` // deprecate this slowly
-			Aux          json.RawMessage `json:"aux,omitempty"`
-		}
-		m := BuildResponse{}
 
 		select {
 		case e := <-stdout.Chan():
@@ -818,7 +812,7 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
 			// output all step errors irrespective of quiet
 			// flag.
 			for _, stepError := range stepErrors {
-				t := BuildResponse{}
+				t := images.BuildResponse{}
 				t.Stream = stepError
 				if err := enc.Encode(t); err != nil {
 					stderr.Write([]byte(err.Error()))
@@ -827,7 +821,7 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
 			}
 			m.ErrorMessage = string(e)
 			m.Error = &jsonmessage.JSONError{
-				Message: m.ErrorMessage,
+				Message: string(e),
 			}
 			if err := enc.Encode(m); err != nil {
 				logrus.Warnf("Failed to json encode error %v", err)

pkg/api/handlers/libpod/images.go

@@ -1,6 +1,8 @@
 package libpod
 
 import (
+	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -18,6 +20,8 @@ import (
 	"github.com/containers/podman/v4/pkg/api/handlers"
 	"github.com/containers/podman/v4/pkg/api/handlers/utils"
 	api "github.com/containers/podman/v4/pkg/api/types"
+	"github.com/containers/podman/v4/pkg/bindings/images"
+	"github.com/containers/podman/v4/pkg/channel"
 	"github.com/containers/podman/v4/pkg/domain/entities"
 	"github.com/containers/podman/v4/pkg/domain/entities/reports"
 	"github.com/containers/podman/v4/pkg/domain/infra/abi"
@@ -30,7 +34,9 @@ import (
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/chrootarchive"
 	"github.com/containers/storage/pkg/idtools"
+	"github.com/docker/docker/pkg/jsonmessage"
 	"github.com/gorilla/schema"
+	"github.com/sirupsen/logrus"
 )
 
 // Commit
@@ -442,6 +448,7 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 		Pause     bool     `schema:"pause"`
 		Squash    bool     `schema:"squash"`
 		Repo      string   `schema:"repo"`
+		Stream    bool     `schema:"stream"`
 		Tag       string   `schema:"tag"`
 	}{
 		Format: "oci",
@@ -480,7 +487,6 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 		SystemContext:         sc,
 		PreferredManifestType: mimeType,
 	}
-
 	if len(query.Tag) > 0 {
 		tag = query.Tag
 	}
@@ -498,12 +504,80 @@ func CommitContainer(w http.ResponseWriter, r *http.Request) {
 	if len(query.Repo) > 0 {
 		destImage = fmt.Sprintf("%s:%s", query.Repo, tag)
 	}
+
+	if !query.Stream {
 		commitImage, err := ctr.Commit(r.Context(), destImage, options)
 		if err != nil && !strings.Contains(err.Error(), "is not running") {
-		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("CommitFailure: %w", err))
+			utils.Error(w, http.StatusInternalServerError, err)
 			return
 		}
 		utils.WriteResponse(w, http.StatusOK, entities.IDResponse{ID: commitImage.ID()})
+		return
+	}
+
+	// Channels all mux'ed in select{} below to follow API commit protocol
+	stdout := channel.NewWriter(make(chan []byte))
+	defer stdout.Close()
+	// Channels all mux'ed in select{} below to follow API commit protocol
+	options.CommitOptions.ReportWriter = stdout
+	var (
+		commitImage *libimage.Image
+		commitErr   error
+	)
+	runCtx, cancel := context.WithCancel(r.Context())
+	go func() {
+		defer cancel()
+		commitImage, commitErr = ctr.Commit(r.Context(), destImage, options)
+	}()
+
+	flush := func() {
+		if flusher, ok := w.(http.Flusher); ok {
+			flusher.Flush()
+		}
+	}
+
+	enc := json.NewEncoder(w)
+
+	statusWritten := false
+	writeStatusCode := func(code int) {
+		if !statusWritten {
+			w.WriteHeader(code)
+			w.Header().Set("Content-Type", "application/json")
+			flush()
+			statusWritten = true
+		}
+	}
+
+	for {
+		m := images.BuildResponse{}
+
+		select {
+		case e := <-stdout.Chan():
+			writeStatusCode(http.StatusOK)
+			m.Stream = string(e)
+			if err := enc.Encode(m); err != nil {
+				logrus.Errorf("%v", err)
+			}
+			flush()
+		case <-runCtx.Done():
+			if commitErr != nil {
+				m.Error = &jsonmessage.JSONError{
+					Message: commitErr.Error(),
+				}
+			} else {
+				m.Stream = commitImage.ID()
+			}
+			if err := enc.Encode(m); err != nil {
+				logrus.Errorf("%v", err)
+			}
+			flush()
+			return
+		case <-r.Context().Done():
+			cancel()
+			logrus.Infof("Client disconnect reported for commit")
+			return
+		}
+	}
 }
 
 func UntagImage(w http.ResponseWriter, r *http.Request) {

pkg/api/server/register_images.go

@@ -1281,35 +1281,43 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error {
 	//    description: the name or ID of a container
 	//    required: true
 	//  - in: query
-	//    name: repo
-	//    type: string
-	//    description: the repository name for the created image
-	//  - in: query
-	//    name: tag
-	//    type: string
-	//    description: tag name for the created image
-	//  - in: query
-	//    name: comment
-	//    type: string
-	//    description: commit message
-	//  - in: query
 	//    name: author
 	//    type: string
 	//    description: author of the image
 	//  - in: query
-	//    name: pause
-	//    type: boolean
-	//    description: pause the container before committing it
-	//  - in: query
 	//    name: changes
 	//    description: instructions to apply while committing in Dockerfile format (i.e. "CMD=/bin/foo")
 	//    type: array
 	//    items:
 	//       type: string
 	//  - in: query
+	//    name: comment
+	//    type: string
+	//    description: commit message
+	//  - in: query
 	//    name: format
 	//    type: string
 	//    description: format of the image manifest and metadata (default "oci")
+	//  - in: query
+	//    name: pause
+	//    type: boolean
+	//    description: pause the container before committing it
+	//  - in: query
+	//    name: squash
+	//    type: boolean
+	//    description: squash the container before committing it
+	//  - in: query
+	//    name: repo
+	//    type: string
+	//    description: the repository name for the created image
+	//  - in: query
+	//    name: stream
+	//    type: boolean
+	//    description: output from commit process
+	//  - in: query
+	//    name: tag
+	//    type: string
+	//    description: tag name for the created image
 	// produces:
 	// - application/json
 	// responses:

pkg/bindings/containers/commit.go

@@ -2,12 +2,21 @@ package containers
 
 import (
 	"context"
+	"errors"
+	"fmt"
+	"io"
 	"net/http"
+	"os"
+	"strings"
 
 	"github.com/containers/podman/v4/pkg/bindings"
+	"github.com/containers/podman/v4/pkg/bindings/images"
 	"github.com/containers/podman/v4/pkg/domain/entities"
+	"github.com/containers/storage/pkg/regexp"
 )
 
+var iidRegex = regexp.Delayed(`^[0-9a-f]{12}`)
+
 // Commit creates a container image from a container.  The container is defined by nameOrID.  Use
 // the CommitOptions for finer grain control on characteristics of the resulting image.
 func Commit(ctx context.Context, nameOrID string, options *CommitOptions) (entities.IDResponse, error) {
@@ -30,5 +39,55 @@ func Commit(ctx context.Context, nameOrID string, options *CommitOptions) (entit
 	}
 	defer response.Body.Close()
 
+	if !response.IsSuccess() {
+		return id, response.Process(err)
+	}
+
+	if !options.GetStream() {
+		return id, response.Process(&id)
+	}
+	stderr := os.Stderr
+	body := response.Body.(io.Reader)
+	dec := json.NewDecoder(body)
+	for {
+		var s images.BuildResponse
+		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 id, nil
+		default:
+			// non-blocking select
+		}
+
+		if err := dec.Decode(&s); err != nil {
+			if errors.Is(err, io.ErrUnexpectedEOF) {
+				return id, fmt.Errorf("server probably quit: %w", err)
+			}
+			// EOF means the stream is over in which case we need
+			// to have read the id.
+			if errors.Is(err, io.EOF) && id.ID != "" {
+				break
+			}
+			return id, fmt.Errorf("decoding stream: %w", err)
+		}
+
+		switch {
+		case s.Stream != "":
+			raw := []byte(s.Stream)
+			stderr.Write(raw)
+			if iidRegex.Match(raw) {
+				id.ID = strings.TrimSuffix(s.Stream, "\n")
+				return id, nil
+			}
+		case s.Error != nil:
+			// If there's an error, return directly.  The stream
+			// will be closed on return.
+			return id, errors.New(s.Error.Message)
+		default:
+			return id, errors.New("failed to parse build results stream, unexpected input")
+		}
+	}
 	return id, response.Process(&id)
 }

pkg/bindings/containers/types.go

@@ -32,6 +32,7 @@ type CommitOptions struct {
 	Comment *string
 	Format  *string
 	Pause   *bool
+	Stream  *bool
 	Squash  *bool
 	Repo    *string
 	Tag     *string

pkg/bindings/containers/types_commit_options.go

@@ -92,6 +92,21 @@ func (o *CommitOptions) GetPause() bool {
 	return *o.Pause
 }
 
+// WithStream set field Stream to given value
+func (o *CommitOptions) WithStream(value bool) *CommitOptions {
+	o.Stream = &value
+	return o
+}
+
+// GetStream returns value of field Stream
+func (o *CommitOptions) GetStream() bool {
+	if o.Stream == nil {
+		var z bool
+		return z
+	}
+	return *o.Stream
+}
+
 // WithSquash set field Squash to given value
 func (o *CommitOptions) WithSquash(value bool) *CommitOptions {
 	o.Squash = &value

pkg/bindings/images/build.go

@@ -27,6 +27,7 @@ import (
 	"github.com/containers/storage/pkg/fileutils"
 	"github.com/containers/storage/pkg/ioutils"
 	"github.com/containers/storage/pkg/regexp"
+	"github.com/docker/docker/pkg/jsonmessage"
 	"github.com/docker/go-units"
 	"github.com/hashicorp/go-multierror"
 	jsoniter "github.com/json-iterator/go"
@@ -40,6 +41,14 @@ type devino struct {
 
 var iidRegex = regexp.Delayed(`^[0-9a-f]{12}`)
 
+type BuildResponse struct {
+	Stream string                 `json:"stream,omitempty"`
+	Error  *jsonmessage.JSONError `json:"errorDetail,omitempty"`
+	// NOTE: `error` is being deprecated check https://github.com/moby/moby/blob/master/pkg/jsonmessage/jsonmessage.go#L148
+	ErrorMessage string          `json:"error,omitempty"` // deprecate this slowly
+	Aux          json.RawMessage `json:"aux,omitempty"`
+}
+
 // Build creates an image using a containerfile reference
 func Build(ctx context.Context, containerFiles []string, options entities.BuildOptions) (*entities.BuildReport, error) {
 	if options.CommonBuildOpts == nil {
@@ -603,11 +612,7 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO
 
 	var id string
 	for {
-		var s struct {
+		var s BuildResponse
-			Stream string `json:"stream,omitempty"`
-			Error  string `json:"error,omitempty"`
-		}
-
 		select {
 		// FIXME(vrothberg): it seems we always hit the EOF case below,
 		// even when the server quit but it seems desirable to
@@ -637,10 +642,10 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO
 			if iidRegex.Match(raw) {
 				id = strings.TrimSuffix(s.Stream, "\n")
 			}
-		case s.Error != "":
+		case s.Error != nil:
 			// If there's an error, return directly.  The stream
 			// will be closed on return.
-			return &entities.BuildReport{ID: id, SaveFormat: saveFormat}, errors.New(s.Error)
+			return &entities.BuildReport{ID: id, SaveFormat: saveFormat}, errors.New(s.Error.Message)
 		default:
 			return &entities.BuildReport{ID: id, SaveFormat: saveFormat}, errors.New("failed to parse build results stream, unexpected input")
 		}

pkg/domain/infra/tunnel/containers.go

@@ -346,7 +346,7 @@ func (ic *ContainerEngine) ContainerCommit(ctx context.Context, nameOrID string,
 			return nil, fmt.Errorf("invalid image name %q", opts.ImageName)
 		}
 	}
-	options := new(containers.CommitOptions).WithAuthor(opts.Author).WithChanges(opts.Changes).WithComment(opts.Message).WithSquash(opts.Squash)
+	options := new(containers.CommitOptions).WithAuthor(opts.Author).WithChanges(opts.Changes).WithComment(opts.Message).WithSquash(opts.Squash).WithStream(!opts.Quiet)
 	options.WithFormat(opts.Format).WithPause(opts.Pause).WithRepo(repo).WithTag(tag)
 	response, err := containers.Commit(ic.ClientCtx, nameOrID, options)
 	if err != nil {

pkg/specgen/generate/config_linux.go

@@ -15,6 +15,7 @@ import (
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/pkg/rootless"
 	"github.com/containers/podman/v4/pkg/util"
+
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/opencontainers/runtime-tools/generate"
 	"github.com/sirupsen/logrus"

test/e2e/commit_test.go

@@ -18,22 +18,37 @@ var _ = Describe("Podman commit", func() {
 		Expect(ec).To(Equal(0))
 		Expect(podmanTest.NumberOfContainers()).To(Equal(1))
 
-		session := podmanTest.Podman([]string{"commit", "test1", "foobar.com/test1-image:latest"})
+		session := podmanTest.Podman([]string{"commit", "test1", "--change", "BOGUS=foo", "foobar.com/test1-image:latest"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Exit(125))
+		Expect(session.ErrorToString()).To(Equal("Error: invalid change \"BOGUS=foo\" - invalid instruction BOGUS"))
+
+		session = podmanTest.Podman([]string{"commit", "test1", "foobar.com/test1-image:latest"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(Exit(0))
 
-		if !IsRemote() {
 		messages := session.ErrorToString()
 		Expect(messages).To(ContainSubstring("Getting image source signatures"))
 		Expect(messages).To(ContainSubstring("Copying blob"))
 		Expect(messages).To(ContainSubstring("Writing manifest to image destination"))
 		Expect(messages).To(Not(ContainSubstring("level=")), "Unexpected logrus messages in stderr")
-		}
 
 		check := podmanTest.Podman([]string{"inspect", "foobar.com/test1-image:latest"})
 		check.WaitWithDefaultTimeout()
 		data := check.InspectImageJSON()
 		Expect(data[0].RepoTags).To(ContainElement("foobar.com/test1-image:latest"))
+
+		// commit second time with --quiet, should not write to stderr
+		session = podmanTest.Podman([]string{"commit", "--quiet", "test1", "foobar.com/test1-image:latest"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Exit(0))
+		Expect(session.ErrorToString()).To(BeEmpty())
+
+		// commit second time with --quiet, should not write to stderr
+		session = podmanTest.Podman([]string{"commit", "--quiet", "bogus", "foobar.com/test1-image:latest"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Exit(125))
+		Expect(session.ErrorToString()).To(Equal("Error: no container with name or ID \"bogus\" found: no such container"))
 	})
 
 	It("podman commit single letter container", func() {