fix: handle 127 error code for podman compatibility #2778

vchandela · 2024-09-12T14:34:47Z

What does this PR do?

Handle 127 error code for podman compatibility.
After merging this, we still need to disable ryuk for tests to work with Podman. Raised an issue for this: [Bug]: Some tests are not working on Podman due to Ryuk #2781

Why is it important?

Docker has a bug where it overrides exit code = 127 with exit code = 126. However, Podman handles these error codes separately.
While testing etcd module, my container was returning exit code = 127 on Podman. test-containers.go didn't know what to do with 127 error code as it follows the moby project so, it was stuck in an infinite loop.
Although, the container was up but the code was panicking.

Related issues

Some relevant PRs in other projects:
- podman run: exit status 126/127 seem to be reversed in sense containers/podman#367
- Exit code for failed exec start is inconsistent moby/moby#45795

How to test this PR

Output for podman exec -ti <containerid> /bin/sh -c "echo test"; echo $?

podman exec -ti 3604e691a9efbaa916bdeaa365f7481a04aca79722c430d3cc285b671215cfec /bin/sh -c "echo test"; echo $?
Error: crun: executable file `/bin/sh` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found
127

netlify · 2024-09-12T14:35:06Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`d76ecc1`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/66eaf10c0ef23a0008a9cf78
😎 Deploy Preview	https://deploy-preview-2778--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

wait/host_port.go

stevenh

Thanks for these improvements, some more suggestions

wait/host_port.go

stevenh

Looking good, we just need to handle the new error on return from the internalCheck function and we should be good

vchandela · 2024-09-13T01:31:41Z

Looking good, we just need to handle the new error on return from the internalCheck function and we should be good

do we also need to write a test in host_port_test.go for exitCode = 127?

mdelapenya · 2024-09-13T05:38:55Z

do we also need to write a test in host_port_test.go for exitCode = 127?

Yes please, once added, I think this PR is good to merge, thanks!

wait/host_port_test.go

kiview · 2024-09-13T12:39:13Z

Just to double check, if moby/moby#45795 would get fixed in the upstream, this PR would still be valid, correct?

stevenh · 2024-09-13T13:15:16Z

Just to double check, if moby/moby#45795 would get fixed in the upstream, this PR would still be valid, correct?

Technically it should only check 127, as 126 is an access issue which should not happen for /bin/sh so it could hide an issue if for some reason /bin has the wrong permissions preventing the internal check from being successful.

That said we would likely want this for compatibility with older versions of docker which have the issue as people won't upgrade instantly even if it is fixed.

stevenh

Sorry I think there was a miss understanding about my comment to use require, the intent was only for the new test, not to do it globally, which while desired could cause a lot of conflicts with other PRs that are currently in flight.

docker_files_test.go

wait/host_port.go

wait/host_port_test.go

mdelapenya · 2024-09-18T14:25:23Z

@vchandela can you run make lint so that all files are gofumpt-ed? 🙏

vchandela · 2024-09-18T15:25:49Z

@vchandela can you run make lint so that all files are gofumpt-ed? 🙏

Done. Thanks for pointing out.

…error and switch case instead of if-else-if

…ategyTarget to MockStrategyTarget to avoid export errors; run make lint

stevenh

Looks good to me, thank you for all your effort on the back and forward on this most appreciated.

stevenh · 2024-09-18T17:58:41Z

@vchandela can you check into the CI failures to make sure they aren't fall out from this.

vchandela · 2024-09-18T18:02:27Z

@vchandela can you check into the CI failures to make sure they aren't fall out from this.

They seem unrelated to this PR. One is for K3s and another is for rabbitmq.

mdelapenya

LGTM, thanks!

* main: feat: support databend module (#2779) chore: golangci-lint 1.61.0 (#2787) fix(mssql): bump Docker image version (#2786) fix: handle 127 error code for podman compatibility (#2778) fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (#2775) feat(mongodb): Wait for mongodb module with a replicaset to finish (#2777) fix(postgres): Apply default snapshot name if no name specified (#2783)

* main: chore: use a much smaller image for testing (testcontainers#2795) fix: parallel containers clean race (testcontainers#2790) fix(registry): wait for (testcontainers#2793) fix: container timeout test (testcontainers#2792) docs: document redpanda options (testcontainers#2789) feat: support databend module (testcontainers#2779) chore: golangci-lint 1.61.0 (testcontainers#2787) fix(mssql): bump Docker image version (testcontainers#2786) fix: handle 127 error code for podman compatibility (testcontainers#2778) fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (testcontainers#2775) feat(mongodb): Wait for mongodb module with a replicaset to finish (testcontainers#2777) fix(postgres): Apply default snapshot name if no name specified (testcontainers#2783) fix: resource clean up for tests and examples (testcontainers#2738) ci: add generate for mocks (testcontainers#2774) fix: docker config error handling when config file does not exist (testcontainers#2772)

* main: chore: use a much smaller image for testing (testcontainers#2795) fix: parallel containers clean race (testcontainers#2790) fix(registry): wait for (testcontainers#2793) fix: container timeout test (testcontainers#2792) docs: document redpanda options (testcontainers#2789) feat: support databend module (testcontainers#2779) chore: golangci-lint 1.61.0 (testcontainers#2787) fix(mssql): bump Docker image version (testcontainers#2786) fix: handle 127 error code for podman compatibility (testcontainers#2778) fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (testcontainers#2775) feat(mongodb): Wait for mongodb module with a replicaset to finish (testcontainers#2777) fix(postgres): Apply default snapshot name if no name specified (testcontainers#2783) fix: resource clean up for tests and examples (testcontainers#2738)

* main: (29 commits) fix: template for code generation (#2800) fix: update module path (#2797) fix: container logging deadlocks (#2791) chore: use a much smaller image for testing (#2795) fix: parallel containers clean race (#2790) fix(registry): wait for (#2793) fix: container timeout test (#2792) docs: document redpanda options (#2789) feat: support databend module (#2779) chore: golangci-lint 1.61.0 (#2787) fix(mssql): bump Docker image version (#2786) fix: handle 127 error code for podman compatibility (#2778) fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (#2775) feat(mongodb): Wait for mongodb module with a replicaset to finish (#2777) fix(postgres): Apply default snapshot name if no name specified (#2783) fix: resource clean up for tests and examples (#2738) ci: add generate for mocks (#2774) fix: docker config error handling when config file does not exist (#2772) docs: refine heading badges in README (#2770) feat(wait): for file (#2731) ...

vchandela requested a review from a team as a code owner September 12, 2024 14:34

vchandela mentioned this pull request Sep 12, 2024

Unable to use podman with Testcontainers for Go on M1 Mac containers/podman#23821

Closed

stevenh requested changes Sep 12, 2024

View reviewed changes

wait/host_port.go Outdated Show resolved Hide resolved

vchandela force-pushed the vchandela-handle-127-error-code branch from fe93655 to 9a6847f Compare September 12, 2024 14:58

stevenh requested changes Sep 12, 2024

View reviewed changes

wait/host_port.go Outdated Show resolved Hide resolved

wait/host_port.go Outdated Show resolved Hide resolved

wait/host_port.go Outdated Show resolved Hide resolved

wait/host_port.go Outdated Show resolved Hide resolved

stevenh requested changes Sep 13, 2024

View reviewed changes

vchandela force-pushed the vchandela-handle-127-error-code branch from d3f2a75 to dca8e0f Compare September 13, 2024 02:10

mdelapenya reviewed Sep 13, 2024

View reviewed changes

wait/host_port_test.go Show resolved Hide resolved

stevenh requested changes Sep 13, 2024

View reviewed changes

wait/host_port_test.go Outdated Show resolved Hide resolved

wait/host_port_test.go Show resolved Hide resolved

wait/host_port_test.go Outdated Show resolved Hide resolved

vchandela force-pushed the vchandela-handle-127-error-code branch from 70dc8d7 to 36a4e89 Compare September 14, 2024 06:26

stevenh requested changes Sep 14, 2024

View reviewed changes

docker_files_test.go Outdated Show resolved Hide resolved

vchandela force-pushed the vchandela-handle-127-error-code branch from a58712d to d967bfd Compare September 15, 2024 04:19

stevenh requested changes Sep 15, 2024

View reviewed changes

vchandela force-pushed the vchandela-handle-127-error-code branch 2 times, most recently from 5ff9e57 to c3cccd4 Compare September 18, 2024 09:48

vchandela force-pushed the vchandela-handle-127-error-code branch from c3cccd4 to d7c1fd7 Compare September 18, 2024 15:25

vchandela added 7 commits September 18, 2024 20:56

fix: handle 127 error code for podman compatibility

dbe193c

fix: remove magic numbers; handle 126 and 127 error code separately

c0801bf

chore: handle comments for removing exitStatus type; handle returned …

3f5bf92

…error and switch case instead of if-else-if

feat: add unit test for exit code 127

409e6f3

chore: add internalCheck() tests for exit code = 126/127

9f7c298

chore: replace t.fatal() with require.NoError()

79b4a85

chore: merge internal check tests with existing tests; revert mockStr…

d76ecc1

…ategyTarget to MockStrategyTarget to avoid export errors; run make lint

vchandela force-pushed the vchandela-handle-127-error-code branch from d7c1fd7 to d76ecc1 Compare September 18, 2024 15:26

stevenh approved these changes Sep 18, 2024

View reviewed changes

mdelapenya approved these changes Sep 18, 2024

View reviewed changes

mdelapenya merged commit 069f724 into testcontainers:main Sep 18, 2024
112 checks passed

vchandela mentioned this pull request Sep 21, 2024

feat: add podman support spirit-labs/tektite#253

Merged

mdelapenya self-assigned this Oct 1, 2024

mdelapenya added the bug An issue with the library label Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle 127 error code for podman compatibility #2778

fix: handle 127 error code for podman compatibility #2778

vchandela commented Sep 12, 2024 •

edited

Loading

netlify bot commented Sep 12, 2024 •

edited

Loading

stevenh left a comment

stevenh left a comment

vchandela commented Sep 13, 2024

mdelapenya commented Sep 13, 2024

kiview commented Sep 13, 2024

stevenh commented Sep 13, 2024 •

edited

Loading

stevenh left a comment

mdelapenya commented Sep 18, 2024

vchandela commented Sep 18, 2024

stevenh left a comment

stevenh commented Sep 18, 2024

vchandela commented Sep 18, 2024

mdelapenya left a comment

fix: handle 127 error code for podman compatibility #2778

fix: handle 127 error code for podman compatibility #2778

Conversation

vchandela commented Sep 12, 2024 • edited Loading

What does this PR do?

Why is it important?

Related issues

How to test this PR

netlify bot commented Sep 12, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

stevenh left a comment

Choose a reason for hiding this comment

stevenh left a comment

Choose a reason for hiding this comment

vchandela commented Sep 13, 2024

mdelapenya commented Sep 13, 2024

kiview commented Sep 13, 2024

stevenh commented Sep 13, 2024 • edited Loading

stevenh left a comment

Choose a reason for hiding this comment

mdelapenya commented Sep 18, 2024

vchandela commented Sep 18, 2024

stevenh left a comment

Choose a reason for hiding this comment

stevenh commented Sep 18, 2024

vchandela commented Sep 18, 2024

mdelapenya left a comment

Choose a reason for hiding this comment

vchandela commented Sep 12, 2024 •

edited

Loading

netlify bot commented Sep 12, 2024 •

edited

Loading

stevenh commented Sep 13, 2024 •

edited

Loading