From af0ef47f0c3aec909a1cb8c273f114a6ddbf55be Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Tue, 31 Oct 2023 15:21:15 -0400 Subject: [PATCH] Add status messages to podman --remote commit Fixes: https://github.com/containers/podman/issues/19947 Signed-off-by: Daniel J Walsh --- pkg/api/handlers/compat/images.go | 2 +- pkg/api/handlers/compat/images_build.go | 14 +--- pkg/api/handlers/libpod/images.go | 84 +++++++++++++++++-- pkg/api/server/register_images.go | 36 ++++---- pkg/bindings/containers/commit.go | 59 +++++++++++++ pkg/bindings/containers/types.go | 1 + .../containers/types_commit_options.go | 15 ++++ pkg/bindings/images/build.go | 19 +++-- pkg/domain/infra/tunnel/containers.go | 2 +- pkg/specgen/generate/config_linux.go | 1 + test/e2e/commit_test.go | 31 +++++-- 11 files changed, 218 insertions(+), 46 deletions(-) diff --git a/pkg/api/handlers/compat/images.go b/pkg/api/handlers/compat/images.go index 0dc0d2bdad..ff25e81808 100644 --- a/pkg/api/handlers/compat/images.go +++ b/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()}) diff --git a/pkg/api/handlers/compat/images_build.go b/pkg/api/handlers/compat/images_build.go index dffd33dcfc..5619a95033 100644 --- a/pkg/api/handlers/compat/images_build.go +++ b/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 { - 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{} + m := images.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) diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go index b1ef91a68d..8cef020bda 100644 --- a/pkg/api/handlers/libpod/images.go +++ b/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) } - 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)) + + 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, err) + return + } + utils.WriteResponse(w, http.StatusOK, entities.IDResponse{ID: commitImage.ID()}) return } - utils.WriteResponse(w, http.StatusOK, entities.IDResponse{ID: commitImage.ID()}) + + // 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) { diff --git a/pkg/api/server/register_images.go b/pkg/api/server/register_images.go index 0b9d0a500b..6264b192df 100644 --- a/pkg/api/server/register_images.go +++ b/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 + // name: author // type: string - // description: the repository name for the created image + // description: author of the image // - in: query - // name: tag - // type: string - // description: tag name for the created image + // 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: author + // name: format // type: string - // description: author of the image + // 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: changes - // description: instructions to apply while committing in Dockerfile format (i.e. "CMD=/bin/foo") - // type: array - // items: - // type: string + // name: squash + // type: boolean + // description: squash the container before committing it // - in: query - // name: format + // name: repo // type: string - // description: format of the image manifest and metadata (default "oci") + // 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: diff --git a/pkg/bindings/containers/commit.go b/pkg/bindings/containers/commit.go index 1a85bfc380..5138b13cb6 100644 --- a/pkg/bindings/containers/commit.go +++ b/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) } diff --git a/pkg/bindings/containers/types.go b/pkg/bindings/containers/types.go index 6275daf63d..6678a86ff3 100644 --- a/pkg/bindings/containers/types.go +++ b/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 diff --git a/pkg/bindings/containers/types_commit_options.go b/pkg/bindings/containers/types_commit_options.go index 7b4745eb88..d58630b924 100644 --- a/pkg/bindings/containers/types_commit_options.go +++ b/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 diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index 9ba899344a..b286b1a496 100644 --- a/pkg/bindings/images/build.go +++ b/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 { - Stream string `json:"stream,omitempty"` - Error string `json:"error,omitempty"` - } - + var s BuildResponse 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") } diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 008bb39fde..55dc6a0dfb 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/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 { diff --git a/pkg/specgen/generate/config_linux.go b/pkg/specgen/generate/config_linux.go index f9d0aca732..feaa9fa85d 100644 --- a/pkg/specgen/generate/config_linux.go +++ b/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" diff --git a/test/e2e/commit_test.go b/test/e2e/commit_test.go index 3c1a8af00b..6a9a7e614f 100644 --- a/test/e2e/commit_test.go +++ b/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") - } + 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() {