From 5026bfab9c354151358552dda6e1587fefc4300b Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Sat, 8 Aug 2020 20:25:53 -0700
Subject: [PATCH] tests/int: fix error handling and logging

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>
---
 tests/integration/checkpoint.bats | 15 +++++----------
 tests/integration/hooks.bats      |  2 --
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/tests/integration/checkpoint.bats b/tests/integration/checkpoint.bats
index c983f84c6..68c02ec35 100644
--- a/tests/integration/checkpoint.bats
+++ b/tests/integration/checkpoint.bats
@@ -97,7 +97,6 @@ function simple_cr() {
 
   # run busybox
   __runc run -d test_busybox <&${in_r} >&${out_w} 2>&${out_w}
-  [ $? -eq 0 ]
 
   testcontainer test_busybox running
 
@@ -120,8 +119,8 @@ function simple_cr() {
   testcontainer test_busybox checkpointed
 
   # restore from checkpoint
-  __runc --criu "$CRIU" restore -d --work-path ./work-dir --image-path ./image-dir test_busybox <&${in_r} >&${out_w} 2>&${out_w}
-  ret=$?
+  ret=0
+  __runc --criu "$CRIU" restore -d --work-path ./work-dir --image-path ./image-dir test_busybox <&${in_r} >&${out_w} 2>&${out_w} || ret=$?
   cat ./work-dir/restore.log | grep -B 5 Error || true
   [ $ret -eq 0 ]
 
@@ -149,7 +148,6 @@ function simple_cr() {
 
   # run busybox
   __runc run -d test_busybox <&${in_r} >&${out_w} 2>&${out_w}
-  [ $? -eq 0 ]
 
   testcontainer test_busybox running
 
@@ -191,8 +189,8 @@ function simple_cr() {
   # in time when the last page is lazily transferred to the destination.
   # Killing the CRIU on the checkpoint side will let the container
   # continue to run if the migration failed at some point.
-  __runc --criu "$CRIU" restore -d --work-path ./image-dir --image-path ./image-dir --lazy-pages test_busybox_restore <&${in_r} >&${out_w} 2>&${out_w}
-  ret=$?
+  ret=0
+  __runc --criu "$CRIU" restore -d --work-path ./image-dir --image-path ./image-dir --lazy-pages test_busybox_restore <&${in_r} >&${out_w} 2>&${out_w} || ret=$?
   cat ./work-dir/restore.log | grep -B 5 Error || true
   [ $ret -eq 0 ]
 
@@ -204,10 +202,8 @@ function simple_cr() {
   [[ ${output} == "ok" ]]
 
   wait $cpt_pid
-  [ $? -eq 0 ]
 
   wait $lp_pid
-  [ $? -eq 0 ]
   PIDS_TO_KILL=()
 
   check_pipes
@@ -253,9 +249,8 @@ function simple_cr() {
 
     # restore from checkpoint; this should restore the container into the existing network namespace
     runc --criu "$CRIU" restore -d --work-path ./work-dir --console-socket $CONSOLE_SOCKET test_busybox
-    ret=$?
     cat ./work-dir/restore.log | grep -B 5 Error || true
-    [ "$ret" -eq 0 ]
+    [ "$status" -eq 0 ]
 
     # busybox should be back up and running
     testcontainer test_busybox running
diff --git a/tests/integration/hooks.bats b/tests/integration/hooks.bats
index aa2f70bec..d64441f25 100644
--- a/tests/integration/hooks.bats
+++ b/tests/integration/hooks.bats
@@ -55,9 +55,7 @@ function teardown() {
 
 	echo "Checking create-runtime library"
 	echo $output | grep $HOOKLIBCR
-	[ "$?" -eq 0 ]
 
 	echo "Checking create-container library"
 	echo $output | grep $HOOKLIBCC
-	[ "$?" -eq 0 ]
 }