Skip to content

tests/int: fix error handling and logging#2548

Merged
mrunalp merged 1 commit intoopencontainers:masterfrom
kolyshkin:int-cr-fix
Aug 10, 2020
Merged

tests/int: fix error handling and logging#2548
mrunalp merged 1 commit intoopencontainers:masterfrom
kolyshkin:int-cr-fix

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

TL;DR: this allows to show logs from failed runc restore.

Bats scripts are run with set -e. This is well known and obvious,
and yet there are a few errors with respect to that, including a few
"gems" by yours truly :(

  1. bats scripts are run with set -e, meaning that [ $? -eq 0 ] is
    useless since the execution won't ever reach this line in case of
    non-zero exit code from a preceding command. So, remove all such
    checks, they are useless and misleading.

  2. bats scripts are run with set -e, meaning that ret=$? is useless
    since the execution won't ever reach this line in case of non-zero
    exit code from a preceding command.

In particular, the code that calls runc restore needs to save the exit
code, show the errors in the log, and only when check the exit code and
fail if it's non-zero. It can not use run (or runc which uses run)
because of shell redirection that we need to set up.

The solution, implemented in this patch, is to use code like this:

ret=0
__runc ... || ret=$?
show_logs
[ $ret -eq 0 ]

In case __runc ... fails (i.e. exits with non-zero exit code), ret=$? is
executed, and it always succeeds, so we won't fail just yet and have
a chance to show logs before checking the value of $ret.

In case __runc ... succeeds, ret=$? is never executed, so $ret will still
be zero (this is the reason why it needs to be set explicitly).

Should help with investigating #2475

TL;DR: this allows to show logs from failed runc restore.

Bats scripts are run with `set -e`. This is well known and obvious,
and yet there are a few errors with respect to that, including a few
"gems" by yours truly.

1. bats scripts are run with `set -e`, meaning that `[ $? -eq 0 ]` is
   useless since the execution won't ever reach this line in case of
   non-zero exit code from a preceding command. So, remove all such
   checks, they are useless and misleading.

2. bats scripts are run with `set -e`, meaning that `ret=$?` is useless
   since the execution won't ever reach this line in case of non-zero
   exit code from a preceding command.

In particular, the code that calls runc restore needs to save the exit
code, show the errors in the log, and only when check the exit code and
fail if it's non-zero. It can not use `run` (or `runc` which uses `run`)
because of shell redirection that we need to set up.

The solution, implemented in this patch, is to use code like this:

```bash
ret=0
__runc ... || ret=$?
show_logs
[ $ret -eq 0 ]
```

In case __runc exits with non-zero exit code, `ret=$?` is executed, and
it always succeeds, so we won't fail just yet and have a chance to show
logs before checking the value of $ret.

In case __runc succeeds, `ret=$?` is never executed, so $ret will still
be zero (this is the reason why it needs to be set explicitly).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

@RenaudWasTaken
Copy link
Copy Markdown
Contributor

Makes sense, thanks for fixing this!

@adrianreber
Copy link
Copy Markdown
Contributor

Nice. LGTM.

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.

5 participants