[GitHub] [ozone] adoroszlai commented on a diff in pull request #5347: HDDS-7531. Intermittent error while removing docker network

["adoroszlai (via GitHub)" <gi...@apache.org>]

adoroszlai commented on code in PR #5347:
URL: https://github.com/apache/ozone/pull/5347#discussion_r1334153334


##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -337,7 +337,26 @@ stop_docker_env(){
   copy_daemon_logs
   save_container_logs
   if [ "${KEEP_RUNNING:-false}" = false ]; then
-     docker-compose --ansi never down
+    down_repeats=3
+    result=0
+    for i in $(seq 1 $down_repeats)
+    do
+      docker-compose --ansi never down
+      result=$?
+      if [[ $result -eq 0 ]]
+      then
+        echo "Docker-compose down successful"
+        break
+      else
+        echo "Error while trying to shut down docker containers"
+        sleep 5
+      fi
+    done
+
+    if [[ $result -ne 0 ]]
+    then
+      echo "Couldn't remove all docker containers after $down_repeats repeats."
+    fi

Review Comment:
   I think we should fail the test run if `docker-compose down` is unsuccessful even after several retries.  I guess this cannot happen due to the intermittent lag in stopping containers, and is most likely due to some error in the test script.
   
   ```suggestion
       echo "Failed to remove all docker containers in $down_repeats attempts."
       return 1
   ```



##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -337,7 +337,26 @@ stop_docker_env(){
   copy_daemon_logs
   save_container_logs
   if [ "${KEEP_RUNNING:-false}" = false ]; then
-     docker-compose --ansi never down
+    down_repeats=3
+    result=0

Review Comment:
   Not needed with the other suggested changes:
   
   ```suggestion
   ```



##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -337,7 +337,26 @@ stop_docker_env(){
   copy_daemon_logs
   save_container_logs
   if [ "${KEEP_RUNNING:-false}" = false ]; then
-     docker-compose --ansi never down
+    down_repeats=3
+    result=0
+    for i in $(seq 1 $down_repeats)
+    do
+      docker-compose --ansi never down
+      result=$?
+      if [[ $result -eq 0 ]]
+      then
+        echo "Docker-compose down successful"
+        break
+      else
+        echo "Error while trying to shut down docker containers"
+        sleep 5
+      fi

Review Comment:
   Since `exit-on-error` is enabled (`set -e` at the start of the script), this exits as soon as `docker-compose down` fails.  This can be fixed by running the command as an `if` condition.
   
   Also, I would omit these extra verbose messages, I think output from `docker-compose` is enough.
   
   ```suggestion
         if docker-compose --ansi never down; then
           return
         fi
         sleep 5
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org