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

Do not print an error message on non-0 exec exit code #3462

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zouyee
Copy link

@zouyee zouyee commented Sep 24, 2024

This was added with an earlier exec, and it is very confusing. the error had nothing to do with Containerd; it was the executable we ran inside the container that errored, and per task run convention we should set the Containerd exit code to the process's exit code and print no error.

ctr exec

 ctr --debug -n k8s.io task exec -t --exec-id nginx XXX bash -c  'bash ./faketime.sh xxx';echo $?
tomcat: stopped
tomcat: started
1

nerdctl exec

nerdctl -n k8s.io exec  xxx bash -c  'bash ./faketime.sh xxx';echo $?
tomcat: stopped
tomcat: started
FATA[0005] exec failed with exit code 1
1

docker exec

docker exec xxx  bash -c  'bash ./faketime.sh xxx';echo $?
tomcat: stopped
tomcat: started
1

This was added with an earlier exec, and it is very confusing. the error had nothing to do with Containerd; it was the executable we ran inside the container that errored, and per task run convention we should set the Containerd exit code to the process's exit code and print no error.

Signed-off-by: Zou Nengren <[email protected]>
Signed-off-by: Zou Nengren <[email protected]>
@AkihiroSuda
Copy link
Member

I believe this was purposely printed for consistency with Docker, but can't verify it on phone

@apostasie
Copy link
Contributor

FWIW:

docker exec -ti foo bash -c 'bash ./faketime.sh xxx'; echo $?
bash: ./faketime.sh: No such file or directory
127


nerdctl exec -ti bar bash -c 'bash ./faketime.sh xxx'; echo $?
bash: ./faketime.sh: No such file or directory
FATA[0000] exec failed with exit code 127
1

It looks like this may have been introduced in:
fae1696#diff-5878c73318981892177c543bc6b07cb0e70f9b3989b533f285ee51c0dc57bf05R202

Not clear what was the reason, as this is part of a refactor.

if err != nil {
return err
}
if code != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

@zouyee not displaying the message is one thing, but clearly we still need to error out.

Copy link
Author

Choose a reason for hiding this comment

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

The executable we ran inside the container that errored, we need to print error ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to return an error from this method, otherwise nerdctl will exit 0.

Copy link
Author

Choose a reason for hiding this comment

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

Why does exec function care about the results of execution inside the container?

Copy link
Member

Choose a reason for hiding this comment

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

It should be just same as Docker

Copy link
Author

Choose a reason for hiding this comment

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

nerdctl exec

nerdctl -n k8s.io exec  xxx bash -c  'bash ./faketime.sh xxx';echo $?
tomcat: stopped
tomcat: started
FATA[0005] exec failed with exit code 1
1

docker exec

docker exec xxx  bash -c  'bash ./faketime.sh xxx';echo $?
tomcat: stopped
tomcat: started
1

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should do the same thing as docker.
With your patch right now, we no longer return exit 1 because you do not return a go error.

Copy link
Member

@fahedouch fahedouch Oct 6, 2024

Choose a reason for hiding this comment

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

@zouyee you may write stderr to /dev/null as workaround

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.

4 participants