Add status messages to podman --remote commit #20377

rhatdan · 2023-10-16T21:09:33Z

Fixes: #19947

Does this PR introduce a user-facing change?

podman --remote commit now shows additional information when committing.

packit-as-a-service · 2023-10-16T21:37:42Z

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

martinpitt · 2023-10-17T04:53:07Z

The cockpit-podman failure is right, this breaks commit error messages in the API. Create a container (immediately stops), enable the API, and commit the container:

podman run --name c1 quay.io/libpod/busybox true
systemctl start podman.socket
curl --unix /run/podman/podman.sock -XPOST 'http://none/v1.12/libpod/commit?container=c1&repo=localhost/FOO'

With current podman it fails properly, with a 500 status code:

{"cause":"repository name must be lowercase","message":"CommitFailure: repository name must be lowercase","response":500}

And the journal logs the failure:

podman[1751]: @ - - [17/Oct/2023:04:51:43 +0000] "POST /v1.12/libpod/commit?container=c1&repo=localhost/FOO HTTP/1.1" 500 122 "" "curl/8.0.1"

With this PR, the call claims success (but of course does not actually create an image):

{"stream":"repository name must be lowercase\n"}

i.e. no real error code; and the journal logs a success, too:

podman[1300]: @ - - [17/Oct/2023:04:49:26 +0000] "POST /v1.12/libpod/commit?container=c1&repo=localhost/FOO HTTP/1.1" 200 49 "" "curl/8.0.1"

martinpitt · 2023-10-17T05:05:07Z

pkg/api/handlers/libpod/images.go

+		if err == nil {
+			success = true
+		} else {
+			stdout.Write([]byte(err.Error() + "\n"))


This seems to be the problem: merely printing the error is not the same as actually posting it through utils.Error as in line 524 -- I don't know how Go works, but I suspect that this err is only scoped inside this go func, and doesn't exist any more outside, so this construction short-circuits the error handling below.

pkg/api/handlers/libpod/images.go

vrothberg

Some comments on the error handling. We should also add a test for an error scenario and make sure that the local and remote clients return the same error.

vrothberg · 2023-10-18T08:02:08Z

pkg/api/handlers/libpod/images.go

+	go func() {
+		defer cancel()
+		commitImage, err = ctr.Commit(r.Context(), destImage, options)
+		if err == nil {


I think we should preserve the error. The error can be returned in case we didn´t stream yet. The pull endpoints do that.

Could write it to an error channel and use that in the switch table below.

I do not know how to trigger a failure on this. If a user specifies a bad container name, this is caught before the commit line. It would be a very strange occurance like OOM or OODisk space to cause this to fail. Unless you have an idea.

I'd try specifying some bogus change values:

podman (main) $ ./bin/podman commit --change BOGUS=FOO 123 Error: invalid change "BOGUS=FOO" - invalid instruction BOGUS

Using this branch, I get the following output with podman-remote:

podman (commit) $ ./bin/podman-remote commit --change BOGUS=FOO 123 Error:

With 4.7, I get:

podman (commit) $ /usr/bin/podman-remote commit --change BOGUS=FOO 123 Error: CommitFailure: invalid change "BOGUS=FOO" - invalid instruction BOGU

pkg/api/handlers/libpod/images.go

vrothberg · 2023-10-19T08:28:51Z

pkg/api/server/register_images.go

-	//    description: format of the image manifest and metadata (default "oci")
+	//    description: the repository name for the created image
+	//  - in: query
+	//    name: Stream


Suggested change

// name: Stream

// name: stream

vrothberg · 2023-10-19T08:31:11Z

pkg/api/handlers/libpod/images.go

+			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


Looks like a copy-paste issue: Why add this as a new field when the plan is to deprecate it?

Cut and paste from Build
Fixed.

pkg/bindings/containers/commit.go

vrothberg · 2023-10-19T08:33:38Z

pkg/api/handlers/libpod/images.go

+	}
+	w.WriteHeader(http.StatusOK)
+	w.Header().Set("Content-Type", "application/json")
+	flush()


This happens too early. At that point, we may already have an error from Commit(), so we should only send OK and the content-type on first successful stream.

Moved to right after commit message above.

vrothberg · 2023-10-19T08:34:55Z

pkg/api/handlers/libpod/images.go

+	// Channels all mux'ed in select{} below to follow API commit protocol
+	stdout := channel.NewWriter(make(chan []byte))
+	defer stdout.Close()
+	stderr := channel.NewWriter(make(chan []byte))


IMO this should just be a chan error so we could - in theory - do error checks later on.

vrothberg · 2023-10-19T08:35:57Z

pkg/api/handlers/libpod/images.go

+	// Channels all mux'ed in select{} below to follow API commit protocol
+	options.CommitOptions.ReportWriter = stdout
+	var commitImage *libimage.Image
+	success := false


I don't think we need this anymore. The order in the switch table will hit the error before streaming.

vrothberg · 2023-10-19T08:36:54Z

pkg/api/handlers/libpod/images.go

+				logrus.Errorf("%v", err)
+			}
+			flush()
+		case e := <-stderr.Chan():


This is not being tested at the moment. I desire a test where the commit fails. Then we can make sure that the error reporting works correctly.

vrothberg

Error handling isn't working yet. Let's please add a new test to check for errors.

vrothberg · 2023-10-20T08:19:43Z

pkg/api/handlers/libpod/images.go

+	// Channels all mux'ed in select{} below to follow API commit protocol
+	options.CommitOptions.ReportWriter = stdout
+	var commitImage *libimage.Image
+	failed := false


We don't need that variable. The error channel signals when there was an error and it will be checked before the stream case in the switch table.

vrothberg · 2023-10-20T08:21:24Z

pkg/api/handlers/libpod/images.go

+			flush()
+		case err := <-errorChan:
+			writeStatusCode(http.StatusInternalServerError)
+			m.Stream = err.Error()


Suggested change

m.Stream = err.Error()

m.ErrorMessage = err.Error()

vrothberg · 2023-10-20T08:22:31Z

pkg/bindings/containers/commit.go

+	body := response.Body.(io.Reader)
+	dec := json.NewDecoder(body)
+	for {
+		var s struct {


The structs and JSON identifiers still differ. Let's move it into a place that both can share.

@rhatdan that's still open.

packit-as-a-service · 2023-10-23T18:56:58Z

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

martinpitt · 2023-10-23T19:43:14Z

So that cockpit test failure is interesting, I think we need to discuss this a bit. This happens in a test which tries to commit a container with name "TEST". This is invalid, and is expected to report an error "repository name must be lowercase" through the API. The test functionally passes, but it stumbles over an additional logging of the API error on the browser console. I will adjust cockpit-podman's tests to ignore this browser error, which will unblock this PR.

However, there's one weirdness: The full error message is now:

repository name must be lowercase: Internal Server Error

which is a bit smelly? Why not just the first half? That "Internal Server Error" is misleading. Is that intended?

martinpitt · 2023-10-23T19:58:43Z

To clarify: The current test expects the error message with this pattern:

 "Failed to commit container .*: CommitFailure: repository name must be lowercase"

This PR drops the CommitFailure: part, and I'll adjust cockpit-podman's tests. However, the test does not expect the "Internal Server Error" suffix. Of course we can make the test ignore it, but it doesn't feel intended?

Drop the "CommitFailure:" part from the expected browser error when trying to commit a container with an invalid name. This is unnecessarily strict, and containers/podman#20377 is going to change it.

vrothberg · 2023-10-24T06:49:51Z

pkg/api/handlers/utils/images.go

+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


Same comment as above: Why use/introduce a new struct with already deprecated fields?

@rhatdan that's still open.

Drop the "CommitFailure:" part from the expected browser error when trying to commit a container with an invalid name. This is unnecessarily strict, and containers/podman#20377 is going to change it.

packit-as-a-service · 2023-10-24T16:50:38Z

Ephemeral COPR build failed. @containers/packit-build please check.

rhatdan · 2023-11-01T12:43:49Z

@vrothberg @martinpitt PTANL
@mheon @Luap99 @giuseppe @baude @umohnani8 @flouthoc PTAL

mheon · 2023-11-01T13:07:45Z

LGTM, though there are a few open comments still

vrothberg

Added new comments on the few remaining open ones.

vrothberg · 2023-11-01T13:17:40Z

pkg/api/handlers/utils/images.go

+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


@rhatdan that's still open.

vrothberg · 2023-11-01T13:17:55Z

pkg/bindings/containers/commit.go

+	body := response.Body.(io.Reader)
+	dec := json.NewDecoder(body)
+	for {
+		var s struct {


@rhatdan that's still open.

vrothberg · 2023-11-01T13:18:19Z

pkg/bindings/images/build.go

-			Stream string `json:"stream,omitempty"`
-			Error  string `json:"error,omitempty"`
+			Stream string                 `json:"stream,omitempty"`
+			Error  *jsonmessage.JSONError `json:"errorDetail,omitempty"`
 		}


This could re-use the new struct, could it?

The deprecated warning is still being used in the docker compat bindings, so I can't remove.

Can we use another/new struct? I would have hard time trying to understand the reasoning in the future. A new struct wouldn't raise the question of why only parts are used and what's deprecated etc.

Fixes: containers#19947 Signed-off-by: Daniel J Walsh <[email protected]>

TomSweeneyRedHat · 2023-11-02T00:08:48Z

LGTM
once @vrothberg is hip with it.

vrothberg

/lgtm

Let's get it in. I bikesheded enough. Thanks, @rhatdan !

openshift-ci · 2023-11-02T09:18:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, vrothberg

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:

~~OWNERS~~ [rhatdan,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2023-11-02T11:09:59Z

test/e2e/commit_test.go

+		Expect(session).Should(Exit(0))
+		Expect(session.ErrorToString()).To(BeEmpty())


Shortcut (for future reference): To(ExitCleanly()). My bad for not catching this in time.

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 16, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 16, 2023

martinpitt reviewed Oct 17, 2023

View reviewed changes

vrothberg reviewed Oct 17, 2023

View reviewed changes

pkg/api/handlers/libpod/images.go Show resolved Hide resolved

rhatdan force-pushed the commit branch 2 times, most recently from cc9f27c to b813806 Compare October 17, 2023 18:37

vrothberg reviewed Oct 18, 2023

View reviewed changes

rhatdan force-pushed the commit branch from b813806 to 19f8afe Compare October 18, 2023 19:24

vrothberg reviewed Oct 19, 2023

View reviewed changes

rhatdan force-pushed the commit branch from 19f8afe to 626b819 Compare October 19, 2023 20:40

vrothberg reviewed Oct 20, 2023

View reviewed changes

rhatdan force-pushed the commit branch from 626b819 to 7b30130 Compare October 23, 2023 18:30

martinpitt mentioned this pull request Oct 23, 2023

test: Relax expected browser error cockpit-project/cockpit-podman#1460

Merged

vrothberg reviewed Oct 24, 2023

View reviewed changes

rhatdan force-pushed the commit branch 5 times, most recently from d7343b1 to 53d92f1 Compare October 24, 2023 16:44

rhatdan force-pushed the commit branch 2 times, most recently from f5da8a1 to f799a01 Compare October 24, 2023 20:21

rhatdan force-pushed the commit branch from f799a01 to a1d287b Compare October 31, 2023 19:21

vrothberg reviewed Nov 1, 2023

View reviewed changes

Add status messages to podman --remote commit

af0ef47

Fixes: containers#19947 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the commit branch from a1d287b to af0ef47 Compare November 1, 2023 18:32

vrothberg approved these changes Nov 2, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Nov 2, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2023

openshift-ci bot merged commit 54fca1f into containers:main Nov 2, 2023
100 checks passed

edsantiago reviewed Nov 2, 2023

View reviewed changes

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Feb 1, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024

		Expect(session).Should(Exit(0))
		Expect(session.ErrorToString()).To(BeEmpty())

Add status messages to podman --remote commit #20377

Add status messages to podman --remote commit #20377

Conversation

rhatdan commented Oct 16, 2023

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Oct 16, 2023

martinpitt commented Oct 17, 2023

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Oct 23, 2023

martinpitt commented Oct 23, 2023

martinpitt commented Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Oct 24, 2023

rhatdan commented Nov 1, 2023

mheon commented Nov 1, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Nov 2, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 2, 2023

Choose a reason for hiding this comment

martinpitt commented Oct 23, 2023 •

edited

Loading