From da25fdb8826d59c30a851e4e936b09e450247301 Mon Sep 17 00:00:00 2001
From: Sree Kuchibhotla <sreek@google.com>
Date: Fri, 26 Feb 2016 13:41:06 -0800
Subject: [PATCH] Address code review comments

---
 tools/gke/kubernetes_api.py                 |  4 ++--
 tools/gke/run_stress_tests_on_gke.py        | 18 ++++--------------
 tools/jenkins/build_interop_stress_image.sh |  3 +++
 tools/run_tests/stress_test/run_client.py   |  1 -
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/tools/gke/kubernetes_api.py b/tools/gke/kubernetes_api.py
index e1017e9da6..2d3f771e93 100755
--- a/tools/gke/kubernetes_api.py
+++ b/tools/gke/kubernetes_api.py
@@ -228,7 +228,7 @@ def delete_pod(kube_host, kube_port, namespace, pod_name):
 def create_pod_and_service(kube_host, kube_port, namespace, pod_name,
                            image_name, container_port_list, cmd_list, arg_list,
                            env_dict, is_headless_service):
-  """A simple helper function that creates a pod and a service (if pod creation was successful)."""
+  """A helper function that creates a pod and a service (if pod creation was successful)."""
   is_success = create_pod(kube_host, kube_port, namespace, pod_name, image_name,
                           container_port_list, cmd_list, arg_list, env_dict)
   if not is_success:
@@ -253,7 +253,7 @@ def create_pod_and_service(kube_host, kube_port, namespace, pod_name,
 
 
 def delete_pod_and_service(kube_host, kube_port, namespace, pod_name):
-  """ A simple helper function that calls delete_pod and delete_service """
+  """ A helper function that calls delete_pod and delete_service """
   is_success = delete_pod(kube_host, kube_port, namespace, pod_name)
   if not is_success:
     print 'Error in deleting pod %s' % pod_name
diff --git a/tools/gke/run_stress_tests_on_gke.py b/tools/gke/run_stress_tests_on_gke.py
index cf0ef595a6..d126e3db43 100755
--- a/tools/gke/run_stress_tests_on_gke.py
+++ b/tools/gke/run_stress_tests_on_gke.py
@@ -178,28 +178,19 @@ class StressClientSettings:
 
 
 def _build_docker_image(image_name, tag_name):
-  """ Build the docker image and add a tag """
+  """ Build the docker image and add tag it to the GKE repository """
   print 'Building docker image: %s' % image_name
   os.environ['INTEROP_IMAGE'] = image_name
+  os.environ['INTEROP_IMAGE_REPOSITORY_TAG'] = tag_name
   # Note that 'BASE_NAME' HAS to be 'grpc_interop_stress_cxx' since the script
   # build_interop_stress_image.sh invokes the following script:
   #   tools/dockerfile/$BASE_NAME/build_interop_stress.sh
   os.environ['BASE_NAME'] = 'grpc_interop_stress_cxx'
   cmd = ['tools/jenkins/build_interop_stress_image.sh']
-  p = subprocess.Popen(args=cmd)
-  retcode = p.wait()
+  retcode = subprocess.call(args=cmd)
   if retcode != 0:
     print 'Error in building docker image'
     return False
-
-  print 'Adding an additional tag %s to the image %s' % (tag_name, image_name)
-  cmd = ['docker', 'tag', '-f', image_name, tag_name]
-  p = subprocess.Popen(args=cmd)
-  retcode = p.wait()
-  if retcode != 0:
-    print 'Error in creating the tag %s for %s' % (tag_name, image_name)
-    return False
-
   return True
 
 
@@ -207,8 +198,7 @@ def _push_docker_image_to_gke_registry(docker_tag_name):
   """Executes 'gcloud docker push <docker_tag_name>' to push the image to GKE registry"""
   cmd = ['gcloud', 'docker', 'push', docker_tag_name]
   print 'Pushing %s to GKE registry..' % docker_tag_name
-  p = subprocess.Popen(args=cmd)
-  retcode = p.wait()
+  retcode = subprocess.call(args=cmd)
   if retcode != 0:
     print 'Error in pushing docker image %s to the GKE registry' % docker_tag_name
     return False
diff --git a/tools/jenkins/build_interop_stress_image.sh b/tools/jenkins/build_interop_stress_image.sh
index 92f2dab5e3..4c8e998a8a 100755
--- a/tools/jenkins/build_interop_stress_image.sh
+++ b/tools/jenkins/build_interop_stress_image.sh
@@ -35,6 +35,8 @@ set -x
 
 # Params:
 #  INTEROP_IMAGE - name of tag of the final interop image
+#  INTEROP_IMAGE_TAG - Optional. If set, the created image will be tagged using
+#    the command: 'docker tag $INTEROP_IMAGE $INTEROP_IMAGE_REPOSITORY_TAG'
 #  BASE_NAME - base name used to locate the base Dockerfile and build script
 #  TTY_FLAG - optional -t flag to make docker allocate tty
 #  BUILD_INTEROP_DOCKER_EXTRA_ARGS - optional args to be passed to the
@@ -77,6 +79,7 @@ CONTAINER_NAME="build_${BASE_NAME}_$(uuidgen)"
   $BASE_IMAGE \
   bash -l /var/local/jenkins/grpc/tools/dockerfile/$BASE_NAME/build_interop_stress.sh \
   && docker commit $CONTAINER_NAME $INTEROP_IMAGE \
+  && ( if [ -n $INTEROP_IMAGE_REPOSITORY_TAG ]; then docker tag $INTEROP_IMAGE $INTEROP_IMAGE_REPOSITORY_TAG ; fi ) \
   && echo "Successfully built image $INTEROP_IMAGE")
 EXITCODE=$?
 
diff --git a/tools/run_tests/stress_test/run_client.py b/tools/run_tests/stress_test/run_client.py
index 33958bce49..0fa1bf1cb9 100755
--- a/tools/run_tests/stress_test/run_client.py
+++ b/tools/run_tests/stress_test/run_client.py
@@ -142,7 +142,6 @@ def run_client():
     # Check if stress_client is still running. If so, collect metrics and upload
     # to BigQuery status table
     if stress_p.poll() is not None:
-      # TODO(sree) Upload completion status to BigQuery
       end_time = datetime.datetime.now().isoformat()
       event_type = EventType.SUCCESS
       details = 'End time: %s' % end_time
-- 
GitLab