test/e2e: fix default signal exit code test #24274
Conversation
openshift-ci
bot
added
release-note-none
approved
|
Ephemeral COPR build failed. @containers/packit-build please check. |
| @@ -112,7 +108,8 @@ var _ = Describe("Podman run with --sig-proxy", func() { | |||
| Expect(killSession).Should(ExitCleanly()) | |||
|
|
|||
| session.WaitWithDefaultTimeout() | |||
| Expect(session).To(ExitWithError(2, "SIGFPE: floating-point exception")) | |||
| errorMsg := "SIGFPE: floating-point exception" | |||
| Expect(session).To(Or(ExitWithError(2, errorMsg), ExitWithError(134, errorMsg))) | |||
There was a problem hiding this comment.
@edsantiago Not sure if you like this like that? It is certainly more duplication that I wanted but the alternative is to switch ExitWithError() to accept a list of exit codes which seems much more complicated.
There was a problem hiding this comment.
I'm not sure I see a better alternative, or even any alternative.
Since this is very confusing to a reader, would you mind adding a one-line comment?
There was a problem hiding this comment.
Done, the alternative is to patch ExitWithError() to accept any as first arg and then use it to pass an int like we do now or an array of int's to allow several exit codes. But this is much more complicated and for a single test overkill.
By default golang programs exit 2 on special exit signals that can be cought and produce a stack trace. However this is behavior that can be modfied via GOTRACEBACK=crash[1], in that case it does not exit(2) but rather sends itself SIGABRT to the parent sees the signal exit and out test sees that es exit code 134, 128 + 6 (SIGABRT), like most shells do. As it turns out GOTRACEBACK=crash is the default mode on all fedora and RHEL rpm builds as they patch the build with a special "rpm_crashtraceback" go build tag. While that change is old and existing for a very long time it was never caught until commit 5e240ab, which switched the old ExitWithError() check that accepted anything > 0, to just accept 2. And as CI only test upstream builds that are build without rpm_crashtraceback we did not catch in CI either. Only once a user actually used distro build against the source e2e test it failed. I like to highlight that running distro builds against upstream e2e tests is not something we really support or plan to support but given this is a easy fix I decided to just fix it here as any user with GOTRACEBACK=crash set would face the same issue. While I touch this test remove the unnecessary RestoreArtifact() call which is not needed at all as we do nothing with the image and just slows the test down for now reason. [1] https://pkg.go.dev/runtime#section-sourcefiles Fixes containers#24213 Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
fix-signal-exit
branch
from
6ee2b11 to
b0f2ebb
Compare
|
[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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 1dcb4c5
into
containers:main
By default golang programs exit 2 on special exit signals that can be cought and produce a stack trace. However this is behavior that can be modfied via GOTRACEBACK=crash[1], in that case it does not exit(2) but rather sends itself SIGABRT to the parent sees the signal exit and out test sees that es exit code 134, 128 + 6 (SIGABRT), like most shells do.
As it turns out GOTRACEBACK=crash is the default mode on all fedora and RHEL rpm builds as they patch the build with a special "rpm_crashtraceback" go build tag.
While that change is old and existing for a very long time it was never caught until commit 5e240ab, which switched the old ExitWithError() check that accepted anything > 0, to just accept 2. And as CI only test upstream builds that are build without rpm_crashtraceback we did not catch in CI either. Only once a user actually used distro build against the source e2e test it failed.
I like to highlight that running distro builds against upstream e2e tests is not something we really support or plan to support but given this is a easy fix I decided to just fix it here as any user with GOTRACEBACK=crash set would face the same issue.
While I touch this test remove the unnecessary RestoreArtifact() call which is not needed at all as we do nothing with the image and just slows the test down for now reason.
[1] https://pkg.go.dev/runtime#section-sourcefiles
Fixes #24213
Does this PR introduce a user-facing change?