build_and_run_docker.sh cleanup and simplification (#29468)

* unify DOCKER_TTY_ARGS in docker scripts

* improvements and cleanup in build_and_run_docker.sh

* fix shellcheck

* make sure python sdist artifact is readable

tools/internal_ci/linux/grpc_bloat_diff.sh

@@ -22,7 +22,4 @@ source tools/internal_ci/helper_scripts/prepare_build_linux_rc
 
 export DOCKERFILE_DIR=tools/dockerfile/test/cxx_debian11_x64
 export DOCKER_RUN_SCRIPT=tools/internal_ci/linux/grpc_bloat_diff_in_docker.sh
-# The check_on_pr.py needs access to the key to post status on github PRs,
-# so we mount the keystore dir to the docker container.
-export EXTRA_DOCKER_ARGS="-v ${KOKORO_KEYSTORE_DIR}:/kokoro_keystore -e KOKORO_KEYSTORE_DIR=/kokoro_keystore"
 exec tools/run_tests/dockerize/build_and_run_docker.sh

tools/internal_ci/linux/grpc_memory_diff.sh

@@ -22,7 +22,4 @@ source tools/internal_ci/helper_scripts/prepare_build_linux_rc
 
 export DOCKERFILE_DIR=tools/dockerfile/test/cxx_debian11_x64
 export DOCKER_RUN_SCRIPT=tools/internal_ci/linux/grpc_memory_diff_in_docker.sh
-# The check_on_pr.py needs access to the key to post status on github PRs,
-# so we mount the keystore dir to the docker container.
-export EXTRA_DOCKER_ARGS="-v ${KOKORO_KEYSTORE_DIR}:/kokoro_keystore -e KOKORO_KEYSTORE_DIR=/kokoro_keystore"
 exec tools/run_tests/dockerize/build_and_run_docker.sh

tools/run_tests/artifacts/build_artifact_python.sh

@@ -117,6 +117,7 @@ tar xzf "${GRPCIO_TAR_GZ}" -C "${GRPCIO_STRIP_TEMPDIR}"
     clean_non_source_files "${dir}" || true
   done
   tar czf "${GRPCIO_STRIPPED_TAR_GZ}" -- *
+  chmod ugo+r "${GRPCIO_STRIPPED_TAR_GZ}"
 )
 mv "${GRPCIO_STRIPPED_TAR_GZ}" "${GRPCIO_TAR_GZ}"
 

tools/run_tests/dockerize/build_and_run_docker.sh

@@ -26,10 +26,11 @@ cd -
 
 # Inputs
 # DOCKERFILE_DIR - Directory in which Dockerfile file is located.
-# DOCKER_RUN_SCRIPT - Script to run under docker (relative to grpc repo root)
-# OUTPUT_DIR - Directory (relatively to git repo root) that will be copied from inside docker container after finishing.
-# DOCKERHUB_ORGANIZATION - If set, pull a prebuilt image from given dockerhub org.
-# $@ - Extra args to pass to the "docker run" command.
+# OUTPUT_DIR - (optional) Directory under the git repo root that will be copied from inside docker container after finishing.
+# DOCKERHUB_ORGANIZATION - (optional) If set, pull a prebuilt image from given dockerhub org (instead of with "docker build" locally)
+# DOCKER_RUN_SCRIPT - (optional) Script to run under docker (relative to grpc repo root). If specified, the cmdline args
+#     passed on the commadline (a.k.a. $@) will be interpreted as extra args to pass to the "docker run" command.
+#     If DOCKER_RUN_SCRIPT is not set, $@ will be interpreted as the command and args to run under the docker container.
 
 # Use image name based on Dockerfile location checksum
 DOCKER_IMAGE_NAME=$(basename "$DOCKERFILE_DIR"):$(sha1sum "$DOCKERFILE_DIR/Dockerfile" | cut -f1 -d\ )
@@ -43,45 +44,175 @@ else
   docker build -t "$DOCKER_IMAGE_NAME" "$DOCKERFILE_DIR"
 fi
 
+# If TTY is available, the running container can be conveniently terminated with Ctrl+C.
 if [[ -t 0 ]]; then
-  DOCKER_TTY_ARGS="-it"
+  DOCKER_TTY_ARGS=("-it")
 else
   # The input device on kokoro is not a TTY, so -it does not work.
-  DOCKER_TTY_ARGS=
+  DOCKER_TTY_ARGS=()
 fi
 
+if [ "${DOCKER_RUN_SCRIPT}" != "" ]
+then
+  # Execute a well-known script inside the docker container.
+  # The script will act as a "wrapper" for the actual test command we want to run.
+  # TODO(jtattermusch): is the -l flag still necessary?
+  DOCKER_CMD_AND_ARGS=( bash -l "/var/local/jenkins/grpc/${DOCKER_RUN_SCRIPT}" )
+  DOCKER_RUN_SCRIPT_ARGS=(
+    # propagate the variable with command to be run by the DOCKER_RUN_SCRIPT
+    "-e=DOCKER_RUN_SCRIPT_COMMAND=${DOCKER_RUN_SCRIPT_COMMAND}"
+    # Interpret any cmdline args as extra docker args.
+    # TODO(jtattermusch): remove this hack once there are no places where build_and_run_docker.sh is invoked
+    # with args.
+    "$@"
+  )
+else
+  # Interpret any cmdline args as the command to be run inside the docker container.
+  DOCKER_CMD_AND_ARGS=( "$@" )
+  DOCKER_RUN_SCRIPT_ARGS=(
+    # TODO(jtattermusch): remove the hack
+    "-w=/var/local/jenkins/grpc"
+  )
+fi
+
+# If available, make KOKORO_KEYSTORE_DIR accessible from the container (as readonly)
+if [ "${KOKORO_KEYSTORE_DIR}" != "" ]
+then
+  MOUNT_KEYSTORE_DIR_ARGS=(
+    "-v=${KOKORO_KEYSTORE_DIR}:/kokoro_keystore:ro"
+    "-e=KOKORO_KEYSTORE_DIR=/kokoro_keystore"
+  )
+else
+  MOUNT_KEYSTORE_DIR_ARGS=()
+fi
+
+# If available, make KOKORO_GFILE_DIR accessible from the container (as readonly)
+if [ "${KOKORO_GFILE_DIR}" != "" ]
+then
+  MOUNT_GFILE_DIR_ARGS=(
+    "-v=${KOKORO_GFILE_DIR}:/kokoro_gfile:ro"
+    "-e=KOKORO_GFILE_DIR=/kokoro_gfile"
+  )
+else
+  MOUNT_GFILE_DIR_ARGS=()
+fi
+
+# If available, make KOKORO_ARTIFACTS_DIR accessible from the container
+if [ "${KOKORO_ARTIFACTS_DIR}" != "" ]
+then
+  MOUNT_ARTIFACTS_DIR_ARGS=(
+    "-v=${KOKORO_ARTIFACTS_DIR}:/kokoro_artifacts"
+    "-e=KOKORO_ARTIFACTS_DIR=/kokoro_artifacts"
+  )
+else
+  MOUNT_ARTIFACTS_DIR_ARGS=()
+fi
+
+# args required to be able to run gdb/strace under the docker container
+DOCKER_PRIVILEGED_ARGS=(
+  # TODO(jtattermusch): document why exactly is this option needed.
+  "--cap-add=SYS_PTRACE"
+)
+
+# propagate the well known variables to the docker container
+DOCKER_PROPAGATE_ENV_ARGS=(
+  "--env-file=tools/run_tests/dockerize/docker_propagate_env.list"
+)
+
+DOCKER_CLEANUP_ARGS=(
+  # delete the container when the containers exits
+  # (otherwise the container will not release the disk space it used)
+  "--rm=true"
+)
+
+DOCKER_NETWORK_ARGS=(
+  # enable IPv6
+  "--sysctl=net.ipv6.conf.all.disable_ipv6=0"
+)
+
+DOCKER_IMAGE_IDENTITY_ARGS=(
+  # make the info about docker image used available from inside the docker container
+  "-e=GRPC_TEST_DOCKER_IMAGE_IDENTITY=${DOCKER_IMAGE_NAME}"
+)
+
+# TODO: silence complaint about lack of quotes in some other way
+# shellcheck disable=SC2206
+DOCKER_EXTRA_ARGS_FROM_ENV=(
+  # Not quoting the variable is intentional, since the env variable may contain
+  # multiple arguments and we want to interpret it as such.
+  # TODO: get rid of EXTRA_DOCKER_ARGS occurrences and replace with DOCKER_EXTRA_ARGS
+  ${EXTRA_DOCKER_ARGS}
+  ${DOCKER_EXTRA_ARGS}
+)
+
 # Git root as seen by the docker instance
 # TODO(jtattermusch): rename to a more descriptive directory name
 # currently that's nontrivial because the name is hardcoded in many places.
 EXTERNAL_GIT_ROOT=/var/local/jenkins/grpc
 
+MOUNT_GIT_ROOT_ARGS=(
+  "-v=${git_root}:${EXTERNAL_GIT_ROOT}"
+  "-e=EXTERNAL_GIT_ROOT=${EXTERNAL_GIT_ROOT}"
+)
+
+# temporary directory that will be mounted to the docker container
+# as a way to persist report files.
+TEMP_REPORT_DIR="$(mktemp -d)"
+# make sure the "reports" dir exists and is owned by current user.
+mkdir -p "${TEMP_REPORT_DIR}/reports"
+mkdir -p "${git_root}/reports"
+
+MOUNT_REPORT_DIR_ARGS=(
+  # mount the temporary directory as the "report dir".
+  "-v=${TEMP_REPORT_DIR}:/var/local/report_dir"
+  # make the reports/ directory show up under the mounted git root
+  "-v=${TEMP_REPORT_DIR}/reports:${EXTERNAL_GIT_ROOT}/reports"
+  # set GRPC_TEST_REPORT_BASE_DIR to make sure that when XML test reports
+  # are generated, they will be stored in the report dir.
+  "-e=GRPC_TEST_REPORT_BASE_DIR=/var/local/report_dir"
+)
+
+if [ "${OUTPUT_DIR}" != "" ]
+then
   # temporary directory that will be mounted to the docker container
   # as a way to persist output files.
   # use unique name for the output directory to prevent clash between concurrent
   # runs of multiple docker containers
-TEMP_REPORT_DIR="$(mktemp -d)"
   TEMP_OUTPUT_DIR="$(mktemp -d)"
 
+  # make sure the "${OUTPUT_DIR}" dir exists and is owned by current user.
+  mkdir -p "${TEMP_OUTPUT_DIR}/${OUTPUT_DIR}"
+  mkdir -p "${git_root}/${OUTPUT_DIR}"
+
+  MOUNT_OUTPUT_DIR_ARGS=(
+    # the OUTPUT_DIR refers to a subdirectory of the git root.
+    "-v=${TEMP_OUTPUT_DIR}/${OUTPUT_DIR}:${EXTERNAL_GIT_ROOT}/${OUTPUT_DIR}"
+    "-e=OUTPUT_DIR=${OUTPUT_DIR}"
+  )
+else
+  MOUNT_OUTPUT_DIR_ARGS=()
+fi
+
 # Run tests inside docker
 DOCKER_EXIT_CODE=0
-# TODO: silence complaint about $DOCKER_TTY_ARGS expansion in some other way
-# shellcheck disable=SC2086,SC2154
+
 docker run \
-  "$@" \
-  ${DOCKER_TTY_ARGS} \
-  ${EXTRA_DOCKER_ARGS} \
-  --cap-add SYS_PTRACE \
-  -e "DOCKER_RUN_SCRIPT_COMMAND=${DOCKER_RUN_SCRIPT_COMMAND}" \
-  -e "EXTERNAL_GIT_ROOT=${EXTERNAL_GIT_ROOT}" \
-  -e "OUTPUT_DIR=${OUTPUT_DIR}" \
-  --env-file tools/run_tests/dockerize/docker_propagate_env.list \
-  --rm \
-  --sysctl net.ipv6.conf.all.disable_ipv6=0 \
-  -v "${git_root}:${EXTERNAL_GIT_ROOT}" \
-  -v "${TEMP_REPORT_DIR}:/var/local/report_dir" \
-  -v "${TEMP_OUTPUT_DIR}:/var/local/output_dir" \
+  "${DOCKER_TTY_ARGS[@]}" \
+  "${DOCKER_RUN_SCRIPT_ARGS[@]}" \
+  "${MOUNT_KEYSTORE_DIR_ARGS[@]}" \
+  "${MOUNT_GFILE_DIR_ARGS[@]}" \
+  "${MOUNT_ARTIFACTS_DIR_ARGS[@]}" \
+  "${DOCKER_PRIVILEGED_ARGS[@]}" \
+  "${DOCKER_PROPAGATE_ENV_ARGS[@]}" \
+  "${DOCKER_CLEANUP_ARGS[@]}" \
+  "${DOCKER_NETWORK_ARGS[@]}" \
+  "${DOCKER_IMAGE_IDENTITY_ARGS[@]}" \
+  "${MOUNT_GIT_ROOT_ARGS[@]}" \
+  "${MOUNT_REPORT_DIR_ARGS[@]}" \
+  "${MOUNT_OUTPUT_DIR_ARGS[@]}" \
+  "${DOCKER_EXTRA_ARGS_FROM_ENV[@]}" \
   "${DOCKER_IMAGE_NAME}" \
-  bash -l "/var/local/jenkins/grpc/${DOCKER_RUN_SCRIPT}" || DOCKER_EXIT_CODE=$?
+  "${DOCKER_CMD_AND_ARGS[@]}" || DOCKER_EXIT_CODE=$?
 
 # Copy reports stored by the container (if any)
 if [ "${GRPC_TEST_REPORT_BASE_DIR}" != "" ]
@@ -95,8 +226,6 @@ fi
 # Copy contents of OUTPUT_DIR back under the git repo root
 if [ "${OUTPUT_DIR}" != "" ]
 then
-  # create the directory if it doesn't exist yet.
-  mkdir -p "${TEMP_OUTPUT_DIR}/${OUTPUT_DIR}"
   cp -r "${TEMP_OUTPUT_DIR}/${OUTPUT_DIR}" "${git_root}" || DOCKER_EXIT_CODE=$?
 fi
 

tools/run_tests/dockerize/build_interop_image.sh

@@ -97,11 +97,12 @@ else
   docker build -t "$BASE_IMAGE" --force-rm=true "tools/dockerfile/interoptest/$BASE_NAME" || exit $?
 fi
 
+# If TTY is available, the running container can be conveniently terminated with Ctrl+C.
 if [[ -t 0 ]]; then
-  DOCKER_TTY_ARGS="-it"
+  DOCKER_TTY_ARGS=("-it")
 else
   # The input device on kokoro is not a TTY, so -it does not work.
-  DOCKER_TTY_ARGS=
+  DOCKER_TTY_ARGS=()
 fi
 
 CONTAINER_NAME="build_${BASE_NAME}_$(uuidgen)"
@@ -114,7 +115,7 @@ CONTAINER_NAME="build_${BASE_NAME}_$(uuidgen)"
 (docker run \
   --cap-add SYS_PTRACE \
   --env-file "tools/run_tests/dockerize/docker_propagate_env.list" \
-  $DOCKER_TTY_ARGS \
+  "${DOCKER_TTY_ARGS[@]}" \
   $MOUNT_ARGS \
   $BUILD_INTEROP_DOCKER_EXTRA_ARGS \
   --name="$CONTAINER_NAME" \

tools/run_tests/dockerize/docker_run.sh

@@ -34,30 +34,17 @@ fi
 
 cd /var/local/git/grpc
 
-# ensure the "reports" directory exists
-mkdir -p reports
-
-exit_code=0
-${DOCKER_RUN_SCRIPT_COMMAND} || exit_code=$?
-
-# copy reports/ dir and files matching one of the patterns to the well-known
-# location of report dir mounted to the docker container.
-# --parents preserves the directory structure for files matched by find.
-cp -r reports/ /var/local/report_dir
-find . -name report.xml -exec cp --parents {} /var/local/report_dir \;
-find . -name sponge_log.xml -exec cp --parents {} /var/local/report_dir \;
-find . -name 'report_*.xml' -exec cp --parents {} /var/local/report_dir \;
-chmod -R ugo+r /var/local/report_dir || true
-
-# Move contents of OUTPUT_DIR from under the workspace to a directory that will be visible to the docker host.
+# whatever is written to the reports/ dir will be made available outside of the docker container.
+ln -s "${EXTERNAL_GIT_ROOT}/reports" reports
+# if OUTPUT_DIR is specified, whatever is written to ./${OUTPUT_DIR}/ will be made available outside of the docker container.
 if [ "${OUTPUT_DIR}" != "" ]
 then
-  # create the directory if it doesn't exist yet.
-  mkdir -p "${OUTPUT_DIR}"
-  mv "${OUTPUT_DIR}" /var/local/output_dir || exit_code=$?
-  chmod -R ugo+r /var/local/output_dir || true
+  ln -s "${EXTERNAL_GIT_ROOT}/${OUTPUT_DIR}" "${OUTPUT_DIR}"
 fi
 
+exit_code=0
+${DOCKER_RUN_SCRIPT_COMMAND} || exit_code=$?
+
 if [ -x "$(command -v ccache)" ]
 then
   ccache --show-stats || true