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

runc exec: fail with exit code of 255 #3073

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 8, 2021

Noticing that runc exec exit status is not documented, I ended up with this.

Currently there's no way to distinguish between the two cases:

  • runc exec failed;
  • the command executed returned 1.

This was possible before commit 8477638, as runc exec exited with
the code of 255 if exec itself has failed. The code of 255 is the same
convention as used by e.g. ssh.

Re-introduce the feature, document it, and add some tests so it won't be
broken again.

Changelog entry

New functionality:
* runc exec now produces exit code of 255 when the exec failed.
  This may help in distinguishing between runc exec failures
  (such as invalid options, non-running container or non-existent
  binary etc.) and failures of the command being executed. (#3073)

@kolyshkin
Copy link
Contributor Author

CI failure in centos7 is a glitch; restarted.

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM(nb)

exec.go Outdated
@@ -101,7 +101,8 @@ following will output a list of processes running in the container:
if err == nil {
os.Exit(status)
}
return fmt.Errorf("exec failed: %w", err)
fatalWithCode(fmt.Errorf("exec failed: %w", err), -1)
Copy link
Member

Choose a reason for hiding this comment

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

-1

Can't we use 255 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will probably look a tad better.

Fixed, PTAL @AkihiroSuda

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Let's try again

AkihiroSuda
AkihiroSuda previously approved these changes Jul 12, 2021
AkihiroSuda
AkihiroSuda previously approved these changes Jul 12, 2021
@kolyshkin kolyshkin requested a review from hqhq July 12, 2021 20:19
@kolyshkin kolyshkin requested review from cyphar and mrunalp July 19, 2021 21:56
@kolyshkin
Copy link
Contributor Author

close/reopen to kick new cirrus ci

@kolyshkin kolyshkin closed this Jul 20, 2021
@kolyshkin kolyshkin reopened this Jul 20, 2021
@kolyshkin
Copy link
Contributor Author

OK, it's not working that way for cirrus-ci apparently, need to rebase.

Currently there's no way to distinguish between the two cases:
 - runc exec failed;
 - the command executed returned 1.

This was possible before commit 8477638, as runc exec exited with
the code of 255 if exec itself has failed. The code of 255 is the same
convention as used by e.g. ssh.

Re-introduce the feature, document it, and add some tests so it won't be
broken again.

Signed-off-by: Kir Kolyshkin <[email protected]>
@mrunalp mrunalp merged commit c9b8b4f into opencontainers:master Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants