Bring image building logic into the Bash library

Previously, useful logic for building images like the choice to tag images with `:latest` as well as `:sha`, automatic re-try logic, `git` cleanup, nice output printing, etc, were all local to the top-level `hack/build-images.sh` entrypoint. There's no reason for that to be the case and as other people find themselves needing those features, we should just expose all of that with `os::build::image`. This is not an entirely backwards compatible patch. The following changes were made: - support for explicitly providing a Dockerfile was removed from the signature of `os::build::image` as this was an entirely unused feature. - the order of arguments was re-arranged so that all of the image build utilites were in alignment -- first the tag, then the dir, etc. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
openshift · Jun 13, 2017 · 9753bf5 · 9753bf5
1 parent c09c601
commit 9753bf5
Show file tree

Hide file tree

Showing 5 changed files with 155 additions and 120 deletions.
diff --git a/hack/build-base-images.sh b/hack/build-base-images.sh
@@ -11,7 +11,7 @@ tag_prefix="${OS_IMAGE_PREFIX:-"openshift/origin"}"
 os::util::ensure::gopath_binary_exists imagebuilder
 
 # Build the base image without the default image args
-OS_BUILD_IMAGE_ARGS="${OS_BUILD_IMAGE_BASE_ARGS-}" os::build::image "${OS_ROOT}/images/source" "${tag_prefix}-source"
-OS_BUILD_IMAGE_ARGS="${OS_BUILD_IMAGE_BASE_ARGS-}" os::build::image "${OS_ROOT}/images/base" "${tag_prefix}-base"
+os::build::image "${tag_prefix}-source" "${OS_ROOT}/images/source"
+os::build::image "${tag_prefix}-base"   "${OS_ROOT}/images/base"
 
 ret=$?; ENDTIME=$(date +%s); echo "$0 took $(($ENDTIME - $STARTTIME)) seconds"; exit "$ret"
diff --git a/hack/build-dind-images.sh b/hack/build-dind-images.sh
@@ -8,8 +8,8 @@
 STARTTIME=$(date +%s)
 source "$(dirname "${BASH_SOURCE}")/lib/init.sh"
 
-os::build::image "${OS_ROOT}/images/dind"        openshift/dind
-os::build::image "${OS_ROOT}/images/dind/node"   openshift/dind-node
-os::build::image "${OS_ROOT}/images/dind/master" openshift/dind-master
+os::build::image openshift/dind        "${OS_ROOT}/images/dind"
+os::build::image openshift/dind-node   "${OS_ROOT}/images/dind/node"
+os::build::image openshift/dind-master "${OS_ROOT}/images/dind/master"
 
 ret=$?; ENDTIME=$(date +%s); echo "$0 took $(($ENDTIME - $STARTTIME)) seconds"; exit "$ret"
diff --git a/hack/build-images.sh b/hack/build-images.sh
@@ -6,9 +6,15 @@
 # NOTE:  you only need to run this script if your code changes are part of
 # any images OpenShift runs internally such as origin-sti-builder, origin-docker-builder,
 # origin-deployer, etc.
-STARTTIME=$(date +%s)
 source "$(dirname "${BASH_SOURCE}")/lib/init.sh"
 
+function cleanup() {
+    return_code=$?
+    os::util::describe_return_code "${return_code}"
+    exit "${return_code}"
+}
+trap "cleanup" EXIT
+
 os::util::ensure::gopath_binary_exists imagebuilder
 # image builds require RPMs to have been built
 os::build::release::check_for_rpms
@@ -32,54 +38,6 @@ function ln_or_cp {
 	fi
 }
 
-
-# image-build is wrapped to allow output to be captured
-function image-build() {
-	local tag=$1
-	local dir=$2
-	local dest="${tag}"
-	local extra=
-	if [[ ! "${tag}" == *":"* ]]; then
-		dest="${tag}:latest"
-		# tag to release commit unless we specified a hardcoded tag
-		extra="${tag}:${OS_RELEASE_COMMIT}"
-	fi
-
-	local STARTTIME
-	local ENDTIME
-	STARTTIME="$(date +%s)"
-
-	# build the image
-	if ! os::build::image "${dir}" "${dest}" "" "${extra}"; then
-		os::log::warning "Retrying build once"
-		if ! os::build::image "${dir}" "${dest}" "" "${extra}"; then
-			return 1
-		fi
-	fi
-
-	# ensure the temporary contents are cleaned up
-	git clean -fdx "${dir}"
-
-	ENDTIME="$(date +%s)"
-	echo "Finished in $(($ENDTIME - $STARTTIME)) seconds"
-}
-
-# builds an image and tags it two ways - with latest, and with the release tag
-function image() {
-	local tag=$1
-	local dir=$2
-	local out
-	mkdir -p "${BASETMPDIR}"
-	out="$( mktemp "${BASETMPDIR}/imagelogs.XXXXX" )"
-	if ! image-build "${tag}" "${dir}" > "${out}" 2>&1; then
-		sed -e "s|^|$1: |" "${out}" 1>&2
-		os::log::error "Failed to build $1"
-		return 1
-	fi
-	sed -e "s|^|$1: |" "${out}"
-	return 0
-}
-
 # Link or copy image binaries to the appropriate locations.
 ln_or_cp "${OS_OUTPUT_BINPATH}/linux/amd64/hello-openshift" examples/hello-openshift/bin
 ln_or_cp "${OS_OUTPUT_BINPATH}/linux/amd64/gitserver"       examples/gitserver/bin
@@ -88,32 +46,28 @@ ln_or_cp "${OS_OUTPUT_BINPATH}/linux/amd64/gitserver"       examples/gitserver/b
 tag_prefix="${OS_IMAGE_PREFIX:-"openshift/origin"}"
 
 # images that depend on "${tag_prefix}-source"
-image "${tag_prefix}-pod"                   images/pod
-image "${tag_prefix}-cluster-capacity"      images/cluster-capacity
-image "${tag_prefix}-service-catalog"       images/service-catalog
+os::build::image "${tag_prefix}-pod"                   images/pod
+os::build::image "${tag_prefix}-cluster-capacity"      images/cluster-capacity
+os::build::image "${tag_prefix}-service-catalog"       images/service-catalog
 
 # images that depend on "${tag_prefix}-base"
-image "${tag_prefix}"                       images/origin
-image "${tag_prefix}-haproxy-router"        images/router/haproxy
-image "${tag_prefix}-keepalived-ipfailover" images/ipfailover/keepalived
-image "${tag_prefix}-docker-registry"       images/dockerregistry
-image "${tag_prefix}-egress-router"         images/egress/router
-image "${tag_prefix}-egress-http-proxy"     images/egress/http-proxy
-image "${tag_prefix}-federation"            images/federation
+os::build::image "${tag_prefix}"                       images/origin
+os::build::image "${tag_prefix}-haproxy-router"        images/router/haproxy
+os::build::image "${tag_prefix}-keepalived-ipfailover" images/ipfailover/keepalived
+os::build::image "${tag_prefix}-docker-registry"       images/dockerregistry
+os::build::image "${tag_prefix}-egress-router"         images/egress/router
+os::build::image "${tag_prefix}-egress-http-proxy"     images/egress/http-proxy
+os::build::image "${tag_prefix}-federation"            images/federation
 # images that depend on "${tag_prefix}
-image "${tag_prefix}-gitserver"             examples/gitserver
-image "${tag_prefix}-deployer"              images/deployer
-image "${tag_prefix}-recycler"              images/recycler
-image "${tag_prefix}-docker-builder"        images/builder/docker/docker-builder
-image "${tag_prefix}-sti-builder"           images/builder/docker/sti-builder
-image "${tag_prefix}-f5-router"             images/router/f5
-image "openshift/node"                      images/node
+os::build::image "${tag_prefix}-gitserver"             examples/gitserver
+os::build::image "${tag_prefix}-deployer"              images/deployer
+os::build::image "${tag_prefix}-recycler"              images/recycler
+os::build::image "${tag_prefix}-docker-builder"        images/builder/docker/docker-builder
+os::build::image "${tag_prefix}-sti-builder"           images/builder/docker/sti-builder
+os::build::image "${tag_prefix}-f5-router"             images/router/f5
+os::build::image "openshift/node"                      images/node
 # images that depend on "openshift/node"
-image "openshift/openvswitch"               images/openvswitch
+os::build::image "openshift/openvswitch"               images/openvswitch
 
 # extra images (not part of infrastructure)
-image "openshift/hello-openshift"           examples/hello-openshift
-
-echo
-
-ret=$?; ENDTIME=$(date +%s); echo "$0 took $(($ENDTIME - $STARTTIME)) seconds"; exit "$ret"
+os::build::image "openshift/hello-openshift"           examples/hello-openshift
diff --git a/hack/dind-cluster.sh b/hack/dind-cluster.sh
@@ -526,7 +526,7 @@ function build-image() {
   local build_root=$1
   local image_name=$2
 
-  os::build::image "${build_root}" "${image_name}"
+  os::build::image "${image_name}" "${build_root}"
 }
 
 function os::build::get-bin-output-path() {

diff --git a/hack/lib/build/images.sh b/hack/lib/build/images.sh
@@ -2,47 +2,128 @@
 
 # This library holds utility functions for building container images.
 
-# os::build::image builds an image from a directory, to a tag, with an optional dockerfile to
-# use as the third argument. The environment variable OS_BUILD_IMAGE_ARGS adds additional
-# options to the command. The default is to use the imagebuilder binary if it is available
-# on the path with fallback to docker build if it is not available.
+# os::build::image builds an image from a directory, to a tag or tags The default
+# behavior is to use the imagebuilder binary if it is available on the path with
+# fallback to docker build if it is not available.
+#
+# Globals:
+#  - OS_BUILD_IMAGE_ARGS
+#  - OS_BUILD_IMAGE_NUM_RETRIES
+# Arguments:
+#  - 1: the directory in which to build
+#  - 2: the tag to apply to the image
+# Returns:
+#  None
 function os::build::image() {
-  local directory=$1
-  local tag=$2
-  local dockerfile="${3-}"
-  local extra_tag="${4-}"
-  local options="${OS_BUILD_IMAGE_ARGS-}"
-  local mode="${OS_BUILD_IMAGE_TYPE:-imagebuilder}"
-
-  if [[ "${mode}" == "imagebuilder" ]]; then
-    if os::util::find::system_binary 'imagebuilder'; then
-      if [[ -n "${extra_tag}" ]]; then
-        extra_tag="-t '${extra_tag}'"
-      fi
-      if [[ -n "${dockerfile}" ]]; then
-        eval "imagebuilder -f '${dockerfile}' -t '${tag}' ${extra_tag} ${options} '${directory}'"
-        return $?
-      fi
-      eval "imagebuilder -t '${tag}' ${extra_tag} ${options} '${directory}'"
-      return $?
-    fi
-
-    os::log::warning "Unable to locate 'imagebuilder' on PATH, falling back to Docker build"
-    # clear options since we were unable to select imagebuilder
-    options=""
-  fi
-
-  if [[ -n "${dockerfile}" ]]; then
-    eval "docker build -f '${dockerfile}' -t '${tag}' ${options} '${directory}'"
-    if [[ -n "${extra_tag}" ]]; then
-      docker tag "${tag}" "${extra_tag}"
-    fi
-    return $?
-  fi
-  eval "docker build -t '${tag}' ${options} '${directory}'"
-  if [[ -n "${extra_tag}" ]]; then
-    docker tag "${tag}" "${extra_tag}"
-  fi
-  return $?
+	local tag=$1
+	local directory=$2
+	local extra_tag
+
+	if [[ ! "${tag}" == *":"* ]]; then
+		# if no tag was specified in the image name,
+		# tag with :latest and the release commit, if
+		# available, falling back to the last commit
+		# if no release commit is recorded
+		local release_commit
+		release_commit="${OS_RELEASE_COMMIT:-"$( git log -1 --pretty=%h )"}"
+		extra_tag="${tag}:${release_commit}"
+
+		tag="${tag}:latest"
+	fi
+
+	local result='1'
+	local image_build_log
+	image_build_log="$( mktemp "${BASETMPDIR}/imagelogs.XXXXX" )"
+	for (( i = 0; i < "${OS_BUILD_IMAGE_NUM_RETRIES:-2}"; i++ )); do
+		if [[ "${i}" -gt 0 ]]; then
+			os::log::warning "Retrying image build for ${tag}, attempt ${i}..."
+		fi
+
+		if os::build::image::internal::generic "${tag}" "${directory}" "${extra_tag:-}" >>"${image_build_log}" 2>&1; then
+			result='0'
+			break
+		fi
+	done
+
+	os::log::internal::prefix_lines "[${tag%:*}]" "$( cat "${image_build_log}" )"
+	return "${result}"
+}
+readonly -f os::build::image
+
+# os::build::image::internal::generic builds a container image using either imagebuilder
+# or docker, defaulting to imagebuilder if present
+#
+# Globals:
+#  - OS_BUILD_IMAGE_ARGS
+# Arguments:
+#  - 1: the directory in which to build
+#  - 2: the tag to apply to the image
+#  - 3: optionally, extra tags to add
+# Returns:
+#  None
+function os::build::image::internal::generic() {
+	local directory=$2
+
+	if os::util::find::system_binary 'imagebuilder' >/dev/null; then
+		os::build::image::internal::imagebuilder "$@"
+	else
+		os::log::warning "Unable to locate 'imagebuilder' on PATH, falling back to Docker build"
+		os::build::image::internal::docker "$@"
+	fi
+
+	# ensure the temporary contents are cleaned up
+	git clean -fdx "${directory}"
+}
+readonly -f os::build::image::internal::generic
+
+# os::build::image::internal::imagebuilder builds a container image using imagebuilder
+#
+# Globals:
+#  - OS_BUILD_IMAGE_ARGS
+# Arguments:
+#  - 1: the directory in which to build
+#  - 2: the tag to apply to the image
+#  - 3: optionally, extra tags to add
+# Returns:
+#  None
+function os::build::image::internal::imagebuilder() {
+	local tag=$1
+	local directory=$2
+	local extra_tag="${3-}"
+	local options=()
+
+	if [[ -n "${OS_BUILD_IMAGE_ARGS:-}" ]]; then
+		options=( ${OS_BUILD_IMAGE_ARGS} )
+	fi
+
+	if [[ -n "${extra_tag}" ]]; then
+		options+=( -t "${extra_tag}" )
+	fi
+
+	imagebuilder "${options[@]:-}" -t "${tag}" "${directory}"
+}
+readonly -f os::build::image::internal::imagebuilder
+
+# os::build::image::internal::docker builds a container image using docker
+#
+# Globals:
+#  - OS_BUILD_IMAGE_ARGS
+# Arguments:
+#  - 1: the directory in which to build
+#  - 2: the tag to apply to the image
+#  - 3: optionally, extra tags to add
+# Returns:
+#  None
+function os::build::image::internal::docker() {
+	local tag=$1
+	local directory=$2
+	local extra_tag="${3-}"
+	local options=()
+
+	docker build ${OS_BUILD_IMAGE_ARGS:-} -t "${tag}" "${directory}"
+
+	if [[ -n "${extra_tag}" ]]; then
+		docker tag "${tag}" "${extra_tag}"
+	fi
 }
-readonly -f os::build::image
+readonly -f os::build::image::internal::docker