Update contrib/docker builds with features for ngraph-tensorflow integration builds (#195)

* Refactored to run builds as user, not as root.  Added make install.  Added parallel build.  Expanded make clean to remove BUILD directory.  Moved log generation to makefile, so Jenkins job can let make do all of the work.

* Changes/fixes to address code review.  Mainly adding double-quotes around variables and using $(...) expansion where appropriate (and semantically identical to back-quotes).

* Add double-quotes for variable expansion safety.  Write error message in run_as_user.sh to stderr.

* Remove chown_files.sh, made obsolete by running as user inside docker container.  Add set -e and set -u to run_as_user.sh, for further protection.

contrib/docker/Dockerfile.ngraph_cpp_cpu

@@ -11,7 +11,4 @@ RUN apt-get update && apt-get install -y \
 RUN apt-get clean autoclean && \
     apt-get autoremove -y
 
-# Add chown_files script to avoid 
-ADD contrib/docker/chown_files.sh /tmp/chown_files.sh
-
-WORKDIR /root/ngraph-cpp-test
+WORKDIR /home

contrib/docker/Makefile

 # Basic Makefile for contrib/docker. This can be expanded later as more targets
 # are added.
 
+# Default is to build with -j for parallel builds.  Turn off with
+#   make PARELLEL=
+PARALLEL=-j
+
+# DIR is an internal variable that serves as an anchor to this cloned git
+# repository.  DIR is mounted into the docker container, so that builds
+# can occur within the container on this cloned git repository.  DIR should
+# not be modified - if it is, then the build system will not work.
 DIR = $(realpath ../..)
-# Use /tmp/ngraph-cpp-test, because we run as the user (and not root)
+
+# DOCKUSER_HOME is the location of the home directory of the fabricated
+# "dockuser" user, used only within the docker containers.  "dockuser" is
+# created (from the passed-in RUN_UID) to map the docker-caller user's UID to a
+# first-class user (/etc/passwd entry, member of sudo group, proper home dir)
+# /home/dockuser is also used in other scripts, notably run_as_user.sh, so if
+# changed it must be done in other areas for the builds to work.
+DOCKUSER_HOME=/home/dockuser
+
+# Use /home/dockuser/ngraph-cpp-test, because we run as the user (and not root)
 # /root/ngraph-cpp-test is not used, because /root is not accessible to user
-VOLUME = -v ${DIR}:/tmp/ngraph-cpp-test
+VOLUME = -v "${DIR}:${DOCKUSER_HOME}/ngraph-cpp-test"
 GIT_COMMIT = $(shell git rev-parse HEAD)
 BUILD_VERSION = ${GIT_COMMIT}_${PYTHON_VERSION}
 BUILD_DIR = ${DIR}/contrib/docker/.build-${BUILD_VERSION}
@@ -16,56 +33,71 @@ CALLER_GID := $(shell id -g)
 # line
 PYTHON_VERSION = 2
 
-.PHONY: clean build_ngraph_cpp_cpu check_cpu shell build_all
+.PHONY: clean build_ngraph_cpp_cpu check_cpu install shell build_all
 
 DOCKER_BUILD=docker build --rm=true
 
 ifdef http_proxy
 DOCKER_BUILD+=--build-arg http_proxy=$(http_proxy)
-DOCKER_RUN_ENV+=--env http_proxy=$(http_proxy)
+DOCKER_RUN_ENV+=--env "http_proxy=$(http_proxy)"
 endif
 
 ifdef https_proxy
 DOCKER_BUILD+=--build-arg https_proxy=$(https_proxy)
-DOCKER_RUN_ENV+=--env https_proxy=$(https_proxy)
+DOCKER_RUN_ENV+=--env "https_proxy=$(https_proxy)"
 endif
 
 expand_dockerfile_templates:
-	cd ${DIR}/contrib/docker
-	mkdir ${BUILD_DIR} || true
-	sed -e 's/\(FROM ngraph.*\)/\1:${BUILD_VERSION}/' Dockerfile.ngraph_cpp_cpu > ${BUILD_DIR}/Dockerfile.ngraph_cpp_cpu
+	cd "${DIR}"/contrib/docker
+	mkdir "${BUILD_DIR}" || true
+	sed -e 's/\(FROM ngraph.*\)/\1:${BUILD_VERSION}/' Dockerfile.ngraph_cpp_cpu > "${BUILD_DIR}"/Dockerfile.ngraph_cpp_cpu
 
 clean:
-	rm -f ${DIR}/contrib/docker/.build-*/Dockerfile.* || echo "keep going if files are not present"
-	rmdir ${DIR}/contrib/docker/.build-* || echo "keep going if directory is not present"
+	rm -f "${DIR}"/contrib/docker/.build-*/Dockerfile.* || echo "keep going if files are not present"
+	rmdir "${DIR}"/contrib/docker/.build-* || echo "keep going if directory is not present"
+	rm -fr "${DIR}"/BUILD
 
 build_ngraph_cpp_cpu: expand_dockerfile_templates
-	$(DOCKER_BUILD) -f=${BUILD_DIR}/Dockerfile.ngraph_cpp_cpu --build-arg python_version=${PYTHON_VERSION} -t=ngraph_cpp_cpu:${BUILD_VERSION} ${DIR}
+	$(DOCKER_BUILD) -f="${BUILD_DIR}"/Dockerfile.ngraph_cpp_cpu --build-arg python_version="${PYTHON_VERSION}" -t=ngraph_cpp_cpu:"${BUILD_VERSION}" "${DIR}"
 	# remove the tag for the previous latest image
 	docker rmi ngraph_cpp_cpu:latest || echo "keep going if docker rmi command fails"
-	docker tag `docker images -q ngraph_cpp_cpu:${BUILD_VERSION}` ngraph_cpp_cpu:latest
+	docker tag `docker images -q "ngraph_cpp_cpu:${BUILD_VERSION}"` ngraph_cpp_cpu:latest
 
 build_all: build_ngraph_cpp_cpu
 
 check_cpu: build_ngraph_cpp_cpu
+	# Remove old distribution directory if present
+	( test -d "${DIR}"/BUILD/ngraph_dist && rm -fr "${DIR}"/BUILD/ngraph_dist && echo "Removed old ${DIR}/BUILD/ngraph_dist directory" ) || echo "Previous ngraph_dist directory not found"
 	# Make BUILD directory as user
-	mkdir -p ${DIR}/BUILD
-	chmod ug+rwx ${DIR}/BUILD
-	# Need to use /tmp/ngraph-cpp-test/BUILD, because running as user
-	# Can't use /root/ngraph-cpp-test/BUILD, because /root not accessible to user
-	docker run \
-	    --rm --user ${CALLER_UID}:${CALLER_GID} \
-	    ${DOCKER_RUN_ENV} ${VOLUME} -w /tmp/ngraph-cpp-test/BUILD -t ngraph_cpp_cpu:${BUILD_VERSION} \
-	    sh -c "cmake -DCMAKE_CXX_COMPILER=clang++-3.9 -DCMAKE_C_COMPILER=clang-3.9 .. ; env VERBOSE=1 make check"
-	# update the files to be owned by the calling user instead of root, to avoid docker mount problems with file ownership
-	docker run --rm ${VOLUME} \
-	--env MY_UID=${CALLER_UID} \
-	--env MY_GID=${CALLER_GID} \
-	--env MY_ROOT_DIR=/root/ngraph-cpp-test \
-	-t ngraph_cpp_cpu \
-	/tmp/chown_files.sh
+	mkdir -p "${DIR}"/BUILD
+	chmod ug+rwx "${DIR}"/BUILD
+	docker run --rm --tty \
+            ${VOLUME} \
+	    ${DOCKER_RUN_ENV} \
+            --env RUN_UID="$(shell id -u)" \
+            --env RUN_CMD="set -e ; set -o pipefail ; cd ${DOCKUSER_HOME}/ngraph-cpp-test/BUILD; cmake -DCMAKE_CXX_COMPILER=clang++-3.9 -DCMAKE_C_COMPILER=clang-3.9 .. 2>&1 | tee cmake.log ; env VERBOSE=1 make ${PARALLEL} 2>&1 | tee make.log ; env VERBOSE=1 make check 2>&1 | tee make_check.log" \
+            "ngraph_cpp_cpu:${BUILD_VERSION}" \
+	    sh -c "${DOCKUSER_HOME}/ngraph-cpp-test/contrib/docker/run_as_user.sh"
 
 shell: build_ngraph_cpp_cpu
-	docker run --rm ${VOLUME} -it ngraph_cpp_cpu:${BUILD_VERSION} /bin/bash
+	# "make shell" runs an interactive shell in the docker image, for debugging
+	docker run --rm --tty --interactive \
+            ${VOLUME} \
+	    ${DOCKER_RUN_ENV} \
+            --env RUN_UID="$(shell id -u)" \
+            "ngraph_cpp_cpu:${BUILD_VERSION}" \
+            sh -c "cd ${DOCKUSER_HOME} ; ${DOCKUSER_HOME}/ngraph-cpp-test/contrib/docker/run_as_user.sh"
+
+install:
+	# Puts ngraph_dist in BUILD directory.  This is used by Jenkins ngraph-tensorflow batch job.
+	# Note: We currently have a bug where cmake only installs in $HOME.  Jira NGTF-205 is opened
+	#       for this.  For now, here we install to $HOME, then move the directory.
+	docker run --rm --tty \
+            ${VOLUME} \
+	    ${DOCKER_RUN_ENV} \
+            --env RUN_UID="$(shell id -u)" \
+            --env RUN_CMD="set -e ; set -o pipefail; cd ${DOCKUSER_HOME}/ngraph-cpp-test/BUILD ; test -d ngraph_dist && rm -fr ngraph_dist && echo 'Removed old ngraph_dist directory' ; make install 2>&1 | tee make_install.log ; mv -v ${DOCKUSER_HOME}/ngraph_dist ${DOCKUSER_HOME}/ngraph-cpp-test/BUILD" \
+            "ngraph_cpp_cpu:${BUILD_VERSION}" \
+	    sh -c "${DOCKUSER_HOME}/ngraph-cpp-test/contrib/docker/run_as_user.sh"
 
 all: build_ngraph_cpp_cpu

contrib/docker/chown_files.sh

-#!/bin/bash
-
-# the docker run commands leave output files with root ownership
-# modify the file ownership with the UID of the calling user  
-
-if [ -z $MY_UID ];then
-    MY_UID=`id -u`
-fi
-
-if [ -z $MY_GID ];then
-    MY_GID=`id -g`
-fi
-
-if [ -z $MY_ROOT_DIR ];then
-    MY_ROOT_DIR=/root/ngraph-test
-fi
-
-cd $MY_ROOT_DIR
-find . -user root > files_to_chown.txt
-cat files_to_chown.txt | xargs chown ${MY_UID} ${1}
-cat files_to_chown.txt | xargs chgrp ${MY_GID} ${1}
-rm files_to_chown.txt

contrib/docker/docker_cleanup.sh

@@ -7,7 +7,7 @@ echo
 
 # clean up old docker containers
 echo "Removing Exited docker containers..." 
-docker ps -a | grep Exited | cut -f 1 -d ' ' | xargs docker rm -f ${1}
+docker ps -a | grep Exited | cut -f 1 -d ' ' | xargs docker rm -f "${1}"
 echo
 
 #list docker images for ngraph
@@ -17,4 +17,4 @@ echo
 
 # clean up docker images no longer in use
 echo "Removing docker images for ngraph..." 
-docker images -qa ngraph_* | xargs docker rmi -f ${1}
+docker images -qa ngraph_* | xargs docker rmi -f "${1}"

contrib/docker/run_as_user.sh

+#!  /bin/bash
+
+# This script is designed to simulate running as a user with a particular UID
+# within a docker container.
+#
+# Normally a docker container runs as root, which can cause problems with file
+# ownership when a host directory tree is mounted into the docker container.
+# There are other problems with building and running software as root as
+# well.  Good practice when validating software builds in a docker container
+# is to run as a normal user, since many (most?) end users will not be building
+# and installing software as root.
+#
+# This script should be run using "docker run", with RUN_UID (set to the user
+# you want to run as) passed into the docker container as an environment
+# variable.  The script will then add the UID as user "dockuser" to
+# /etc/passwd (important for some software, like bazel), add the new dockuser
+# to the sudo group (whether or not sudo is installed), and su to a new shell
+# as the dockuser (passing in the existing environment, which is important).
+#
+# If the environment variable RUN_CMD is passed into the docker container, then
+# this script will use RUN_CMD as a command to run when su'ing.  If RUN_CMD is
+# not defined, then /bin/bash will run, which effectively provides an
+# interactive shell in the docker container, for debugging.
+
+set -e  # Make sure we exit on any command that returns non-zero
+set -u  # No unset variables
+
+if [ -z "$RUN_UID" ] ; then
+
+    # >&2 redirects echo output to stderr.
+    # See: https://stackoverflow.com/questions/2990414/echo-that-outputs-to-stderr
+    ( >&2 echo 'ERROR: Environment variable RUN_UID was not set when run-as-user.sh was run' )
+    ( >&2 echo '       Running as default user (root, in docker)' )
+    ( >&2 echo ' ' )
+
+    exit 1
+
+else
+
+    # The username used in the docker container to map the caller UID to
+    #
+    # Note 'dockuser' is used in other scripts, notably Makefile.  If you
+    # choose to change it here, then you need to change it in all other
+    # scripts, or else the builds will break.
+    #
+    DOCK_USER='dockuser'
+
+    # We will be su'ing using a non-login shell or command, and preserving
+    # the environment.  This is done so that env. variables passed in with
+    # "docker run --env ..." are honored.
+    # Therefore, we need to reset at least HOME=/root ...
+    #
+    # Note also that /home/dockuser is used in other scripts, notably
+    # Makefile.  If you choose to change it here, then you need to change it
+    # in all other scripts, or else the builds will break.
+    #
+    export HOME="/home/${DOCK_USER}"
+
+    # Make sure the home directory is owned by the new user
+    if [ -d "${HOME}" ] ; then
+      chown "${RUN_UID}" "${HOME}"
+    fi
+
+    # Add a user with UID of person running docker (in ${RUN_UID})
+    # If $HOME does not yet exist, then it will be created
+    adduser --disabled-password --gecos 'Docker-User' -u "${RUN_UID}" "${DOCK_USER}"
+    # Add dockuser to the sudo group
+    adduser "${DOCK_USER}" sudo
+
+    # If root access is needed in the docker image while running as a normal
+    # user, uncomment this and add 'sudo' as a package installed in Dockerfile
+    # echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
+
+    if [ -z "${RUN_CMD+x}" ] ; then  # Launch a shell as dockuser
+      su -m "${DOCK_USER}" -c /bin/bash
+    else                         # Run command as dockuser
+      su -m "${DOCK_USER}" -c "${RUN_CMD}"
+    fi
+
+fi