Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/exec: expect 127 exit code for missing executable #3290

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Sep 27, 2024

The Docker Engine has always returned 126 when starting an exec fails due to a missing binary, but this was due to a bug in the daemon causing the correct exit code to be overwritten in some cases – see: moby/moby#45795

Change tests to expect correct exit code (127).

Note: Must take some care to what's the best way to fix this without breaking engine or vice-versa, due to the engine running tests against docker-py and docker-py running tests against the engine. It might be necessary to 1st. skip the test, then fix the engine, then fix/unskip the test.

@laurazard
Copy link
Member Author

PR to fix the breaking docker-py test: #3290

I called it out there, but we'll need to TAL at the dependencies between the engine and docker-py. Docker-py runs integration tests against the engine, and the engine uses docker-py to run tests, so changing it in either place first is a bit annoying. We might need to skip the test in docker-py -> change the engine behavior -> unskip+fix the test in docker-py, and update the engine version it runs against.

Docker Engine has always returned `126` when starting an exec fails due
to a missing binary, but this was due to a bug in the daemon causing the
correct exit code to be overwritten in some cases – see: moby/moby#45795

Change tests to expect correct exit code (`127`).

Signed-off-by: Laura Brehm <[email protected]>
laurazard added a commit to laurazard/moby that referenced this pull request Sep 27, 2024
Temporarily skip the exec run failed exit code test in `docker-py` –
https://github.com/docker/docker-py/blob/a3652028b1ead708bd9191efb286f909ba6c2a49/tests/integration/models_containers_test.py#L356-L363

We can reenable this after the PR fixing the expected exit code in that
test is merged/released/included – docker/docker-py#3290

Signed-off-by: Laura Brehm <[email protected]>
@laurazard laurazard force-pushed the exec-no-executable-exit-code branch 3 times, most recently from 128ae9a to f0048c1 Compare September 30, 2024 13:06
Execs should return the exit code of the exec'd process, if it started.

Signed-off-by: Laura Brehm <[email protected]>
@laurazard
Copy link
Member Author

It's passing now @thaJeztah 🎉

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@thaJeztah thaJeztah merged commit e031cf0 into docker:main Sep 30, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants