ExitWithError() - continue tightening #22346

edsantiago · 2024-04-11T14:30:39Z

Followup to #22270 : wherever possible/practical, extend command
error checks to include explicit exit status codes and error strings.

A small number of tests were broken, as in, not actually testing
what they claimed to be testing. I've done my best to fix those.

Signed-off-by: Ed Santiago santiago@redhat.com

None

edsantiago

Pointing out a few areas that merit special attention

edsantiago · 2024-04-11T14:31:52Z

test/e2e/import_test.go

-		if IsRemote() {
-			Expect(result).To(ExitWithError())
-			Expect(result.ErrorToString()).To(ContainSubstring("unknown flag"))
-			result := podmanTest.Podman([]string{"import", "-q", outfile})
-			result.WaitWithDefaultTimeout()
-		}


Click the expand-up arrow. Note the SkipIfRemote().

edsantiago · 2024-04-11T14:32:53Z

test/e2e/inspect_test.go

@@ -197,7 +198,7 @@ var _ = Describe("Podman inspect", func() {
 	})

 	It("podman [image, container] inspect on container", func() {
-		ctrName := "testCtr"
+		ctrName := "testctr"


With caps, "image inspect" was barfing with "repository name must be lowercase"

edsantiago · 2024-04-11T14:33:54Z

test/e2e/load_test.go

-			result = podmanTest.Podman([]string{"load", "-i", outfile})
-			result.WaitWithDefaultTimeout()
-			Expect(result).Should(ExitCleanly())


Judgment call: I could not figure out the point of this load. That is already tested elsewhere.

edsantiago · 2024-04-11T14:35:10Z

test/e2e/logs_test.go

@@ -595,14 +595,10 @@ var _ = Describe("Podman logs", func() {
 		defer cleanup()
 		logc := podmanTest.Podman([]string{"run", "--log-driver", "journald", "--log-opt", "tag=äöüß", ALPINE, "echo", "podman"})
 		logc.WaitWithDefaultTimeout()
-		Expect(logc).To(Exit(126))
-		// FIXME-2023-09-26: conmon <2.1.8 logs to stdout; clean this up once >=2.1.8 is universal


conmon is 2.1.10 everywhere now.

edsantiago · 2024-04-11T14:36:35Z

test/e2e/network_connect_disconnect_test.go

-		dis := podmanTest.Podman([]string{"network", "connect", "foobar", "test"})
+		session := podmanTest.Podman([]string{"create", "--name", "testContainer", "--network", "bridge", ALPINE})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())


This test was completely broken. PLEASE REVIEW CAREFULLY! I don't know if this is the test intention.

edsantiago · 2024-04-11T14:37:18Z

test/e2e/network_create_test.go

@@ -427,15 +428,19 @@ var _ = Describe("Podman network create", func() {
 	})

 	It("podman network create with invalid IP", func() {
-		nc := podmanTest.Podman([]string{"network", "create", "--subnet", "10.11.0/17000", stringid.GenerateRandomID()})


Another test that was completely broken. PLEASE REVIEW THIS CAREFULLY.

chnage looks good but you may want to rename the test to include ip-range

Before I do that, is it possible that I misunderstood the point of the test? What would you expect "invalid IP" to mean in the context of "network create"??

My initial gut was running a container with --ip that is not part of the network subnet but AFAIK we already have tests for that. I also see that we handle invalid subnet before in a different test, so that leaves --gateway which also accepts a ip.

added gateway test. Renamed test.

edsantiago · 2024-04-11T14:38:04Z

test/e2e/network_create_test.go

 	})

 	It("podman network create with invalid network name", func() {
-		nc := podmanTest.Podman([]string{"network", "create", "foo "})


Extra space was making impossible to strcmp() error message. I could not figure out why.

Luap99 · 2024-04-11T14:49:08Z

test/e2e/load_test.go

 	})

 	It("podman load bogus file", func() {
 		save := podmanTest.Podman([]string{"load", "-i", "foobar.tar"})
 		save.WaitWithDefaultTimeout()
-		Expect(save).To(ExitWithError())
+		Expect(save).To(ExitWithError(125, "stat foobar.tar: no such file or directory"))


FYI all (most?) of these stat <name>: ... will conflict with #22334 and need to be changed to faccessat

Thanks for the heads-up. I've set to draft, to prevent this from merging. (Not that I expect it to pass remote/rootless/runc/etc tests anyway. I'm just trying to find out what the snags are).

Luap99 · 2024-04-11T14:50:39Z

test/e2e/network_connect_disconnect_test.go

-		dis := podmanTest.Podman([]string{"network", "connect", "foobar", "test"})
+		session := podmanTest.Podman([]string{"create", "--name", "testContainer", "--network", "bridge", ALPINE})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(ExitCleanly())


Luap99 · 2024-04-11T14:54:03Z

test/e2e/network_create_test.go

@@ -427,15 +428,19 @@ var _ = Describe("Podman network create", func() {
 	})

 	It("podman network create with invalid IP", func() {
-		nc := podmanTest.Podman([]string{"network", "create", "--subnet", "10.11.0/17000", stringid.GenerateRandomID()})


chnage looks good but you may want to rename the test to include ip-range

edsantiago

OMG it passed CI. A few more special-attention-please comments from my part.

This is still blocked and draft because I don't want to collide with #22334 but I think it's ready for review. Once 22334 merges I will rebase, update error messages as needed, and then post whether my rebase-repush was trivial or if anything else noteworthy was necessary.

edsantiago · 2024-04-11T18:27:04Z

libpod/boltdb_state.go

@@ -1234,7 +1234,7 @@ func (s *BoltState) networkModify(ctr *Container, network string, opts types.Per
 		netConnected := ctrNetworksBkt.Get([]byte(network))

 		if new && netConnected != nil {
-			return fmt.Errorf("container %s is already connected to network %q: %w", ctr.ID(), network, define.ErrNetworkConnected)
+			return fmt.Errorf("container %s is already connected to network %s: %w", ctr.ID(), network, define.ErrNetworkConnected)


DANGER DANGER DANGER DANGER! I am mucking with non-test code! My thinking is, I would really like consistent error messages.

How is this more consistent? The error 2 lines below still uses %q.
I am in favor of consistency but I would argue this needs much broader vetting of error messages first and a consent from the team on what the right way way is, i.e. when to use %q vs %s, right now this seems rather arbitrarily chosen.

I meant consistent between podman and podman-remote. Right now each one emits a different message, which is evil. I did not make that clear, and apologize for that.

I could instead add a conditional to the e2e test (if remote, Expect one thing, otherwise, Expect another). Or I could break this PR down into smaller parts. I'm tempted to do the latter. Suggestions welcome.

How the hell do podman vs podman-remote generate different messages here? This error is coming straight from the db backend, are you sure this is remote vs local not boltdb vs sqlite?

I see what you are trying to do but IMO changing a few messages makes things worse unless we have a overall agreement first on what the right way to change it is. And IMO changing one message only while leaving others the same way makes podman error messages less consistent overall.

Final (I hope) followup: I've removed my changes to the code base, and gone with if/else IsRemote() or boltb conditionals in the tests. I find that distasteful but I hope it will let us move forward.

pkg/domain/infra/abi/containers.go

edsantiago · 2024-04-11T18:28:28Z

pkg/domain/infra/abi/containers.go

@@ -525,7 +525,7 @@ func (ic *ContainerEngine) ContainerInspect(ctx context.Context, namesOrIds []st
 			// ErrNoSuchCtr is non-fatal, other errors will be
 			// treated as fatal.
 			if errors.Is(err, define.ErrNoSuchCtr) {
-				errs = append(errs, fmt.Errorf("no such container %s", name))
+				errs = append(errs, fmt.Errorf("no such container %q", name))


edsantiago · 2024-04-11T18:32:28Z

test/e2e/manifest_test.go

 		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
+
+		// FIXME-someday: figure out why message differs in podman-remote


Eh, I tried, got nowhere. Probably five more minutes would've gotten me there but that's what I said three times before that already.

edsantiago · 2024-04-11T18:33:37Z

test/e2e/network_create_test.go

@@ -427,15 +428,19 @@ var _ = Describe("Podman network create", func() {
 	})

 	It("podman network create with invalid IP", func() {
-		nc := podmanTest.Podman([]string{"network", "create", "--subnet", "10.11.0/17000", stringid.GenerateRandomID()})


added gateway test. Renamed test.

edsantiago · 2024-04-11T18:34:20Z

test/e2e/network_test.go

-		Expect(session).Should(ExitWithError())
-		Expect(session.ErrorToString()).To(ContainSubstring("network not found"))
+		expectMessage := fmt.Sprintf("network %s: ", netID[1:])
+		// FIXME-someday: figure out why this part does not show up in remote


Another rathole

Followup to containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. Because containers#22346 is stalled, these are some trivial easy-to-review changes that get us closer to the goal. Signed-off-by: Ed Santiago <santiago@redhat.com>

Followup to [1]containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. Just trying to shrink down containers#22346 to a manageable, reviewable size. Signed-off-by: Ed Santiago <santiago@redhat.com>

packit-as-a-service · 2024-04-25T21:36:09Z

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

edsantiago · 2024-04-30T13:44:18Z

@containers/podman-maintainers should I split this into smaller chunks?

test/e2e/network_connect_disconnect_test.go

Luap99 · 2024-04-30T14:12:39Z

@containers/podman-maintainers should I split this into smaller chunks?

I am fine with it, I have now reviewed everything anyway. Just noticed the one blocker above.

Luap99

LGTM but we really should work on consistent error messages in the near future...

openshift-ci · 2024-04-30T16:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, Luap99

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~~ [Luap99,edsantiago]

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

Followup to containers#22270 : wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. A small number of tests were broken, as in, not actually testing what they claimed to be testing. I've done my best to fix those. Signed-off-by: Ed Santiago <santiago@redhat.com>

mheon · 2024-05-08T17:39:38Z

/lgtm

openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 11, 2024

edsantiago commented Apr 11, 2024

View reviewed changes

edsantiago force-pushed the exitwitherror-part2 branch from dcc20c6 to 2a37039 Compare April 11, 2024 14:47

Luap99 reviewed Apr 11, 2024

View reviewed changes

edsantiago marked this pull request as draft April 11, 2024 14:56

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2024

edsantiago force-pushed the exitwitherror-part2 branch from 2a37039 to a554989 Compare April 11, 2024 17:17

edsantiago commented Apr 11, 2024

View reviewed changes

Luap99 mentioned this pull request Apr 12, 2024

use containers/storage/pkg/fileutils/(Exists,Lexists) #22334

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2024

edsantiago force-pushed the exitwitherror-part2 branch from a554989 to f42dfde Compare April 22, 2024 12:01

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024

edsantiago force-pushed the exitwitherror-part2 branch 2 times, most recently from a9274d5 to d01b207 Compare April 22, 2024 14:47

edsantiago marked this pull request as ready for review April 22, 2024 16:34

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2024

edsantiago mentioned this pull request Apr 24, 2024

ExitWithError() - low-hanging fruit #22485

Merged

edsantiago mentioned this pull request Apr 24, 2024

ExitWithError() - yet more low-hanging fruit #22489

Merged

edsantiago force-pushed the exitwitherror-part2 branch from d01b207 to 0aca6a0 Compare April 25, 2024 21:27

edsantiago force-pushed the exitwitherror-part2 branch 2 times, most recently from bf25023 to 734faf3 Compare April 29, 2024 21:15

Luap99 requested changes Apr 30, 2024

View reviewed changes

test/e2e/network_connect_disconnect_test.go Outdated Show resolved Hide resolved

edsantiago force-pushed the exitwitherror-part2 branch from 734faf3 to 0eacdc3 Compare April 30, 2024 14:34

edsantiago mentioned this pull request Apr 30, 2024

podman-machine uncategorized flakes #22551

Open

Luap99 approved these changes Apr 30, 2024

View reviewed changes

edsantiago force-pushed the exitwitherror-part2 branch from 0eacdc3 to 641cd7c Compare May 8, 2024 00:57

openshift-ci bot assigned mheon May 8, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2024

openshift-merge-bot bot merged commit 092d040 into containers:main May 8, 2024
91 checks passed

edsantiago deleted the exitwitherror-part2 branch May 8, 2024 19:39

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 7, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExitWithError() - continue tightening #22346

ExitWithError() - continue tightening #22346

edsantiago commented Apr 11, 2024

edsantiago left a comment

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

Luap99 Apr 11, 2024

edsantiago Apr 11, 2024

Luap99 Apr 11, 2024

edsantiago Apr 11, 2024

Luap99 Apr 11, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

Luap99 Apr 11, 2024

edsantiago Apr 11, 2024

Luap99 Apr 11, 2024

Luap99 Apr 11, 2024

edsantiago left a comment

edsantiago Apr 11, 2024

Luap99 Apr 22, 2024

edsantiago Apr 22, 2024

Luap99 Apr 22, 2024

edsantiago Apr 22, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

edsantiago Apr 11, 2024

packit-as-a-service bot commented Apr 25, 2024

edsantiago commented Apr 30, 2024

Luap99 commented Apr 30, 2024

Luap99 left a comment

openshift-ci bot commented Apr 30, 2024

mheon commented May 8, 2024

ExitWithError() - continue tightening #22346

ExitWithError() - continue tightening #22346

Conversation

edsantiago commented Apr 11, 2024

edsantiago 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago 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

packit-as-a-service bot commented Apr 25, 2024

edsantiago commented Apr 30, 2024

Luap99 commented Apr 30, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 30, 2024

mheon commented May 8, 2024