[SPARK-22960][k8s] Make build-push-docker-images.sh more dev-friendly. #20154

vanzin · 2018-01-04T17:52:59Z

Make it possible to build images from a git clone.
Make it easy to use minikube to test things.

Also fixed what seemed like a bug: the base image wasn't getting the tag
provided in the command line. Adding the tag allows users to use multiple
Spark builds in the same kubernetes cluster.

Tested by deploying images on minikube and running spark-submit from a dev
environment; also by building the images with different tags and verifying
"docker images" in minikube.

- Make it possible to build images from a git clone. - Make it easy to use minikube to test things. Also fixed what seemed like a bug: the base image wasn't getting the tag provided in the command line. Adding the tag allows users to use multiple Spark builds in the same kubernetes cluster. Tested by deploying images on minikube and running spark-submit from a dev environment; also by building the images with different tags and verifying "docker images" in minikube.

vanzin · 2018-01-04T17:53:21Z

@foxish @liyinan926

foxish

Thanks @vanzin. I think this is a definite improvement for the dev workflow. I have a few comments on the minikube specific bits however.

foxish · 2018-01-04T18:00:01Z

docs/running-on-kubernetes.md

@@ -16,6 +16,8 @@ Kubernetes scheduler that has been added to Spark.
 you may setup a test cluster on your local machine using
 [minikube](https://kubernetes.io/docs/getting-started-guides/minikube/).
  * We recommend using the latest release of minikube with the DNS addon enabled.
+  * Be aware that the default minikube configuration is not enough for running Spark applications.
+  You will need to increase the available memory and number of CPUs.


I think if we're going into detail, we should specify a certain config here. 6G of memory and at least 2 CPUs? @liyinan926, do you recall what we typically need for SparkPi?

Driver + default minikube overhead uses 1.25 CPUs from what I remember seeing in the dashboard. Don't remember the memory usage. So I'd say 4 CPUs + 4g of memory (to allow driver + single executor), 6g if you want two executors.

I remember 3 cpus are the minimum, considering that kube-system pods will use some CPU cores. For me, 4G of memory worked fine.

foxish · 2018-01-04T18:11:26Z

sbin/build-push-docker-images.sh

+   if ! which minikube 1>/dev/null; then
+     error "Cannot find minikube."
+   fi
+   eval $(minikube docker-env)


I think building docker images right into the minikube VM's docker daemon is uncommon and not something we'd want to recommend. Users on minikube should also use a proper registry - (for example, there is a registry addon) that could be used.

While this might be good to document as a local developer workflow, I'm apprehensive about adding a new flag just for this particular mode. Also one could invoke eval $(minikube docker-env) and then use the build command to get the same effect.

I started calling that command separately, but it's really annoying. This option is useful not just for Spark devs, but for people who want to try their own apps on minikube before trying them on a larger cluster, for example.

building docker images right into the minikube VM's docker daemon is uncommon

What's the alternative? Deploying your own registry? I struggled with that for hours and it's nearly impossible to get docker to talk to an insecure registry (or one with a self signed cert like minikube's). This approach just worked (tm).

I see your point - this is considerably easier. I spoke with a minikube maintainer and it seems this is not as uncommon as I initially thought. So, this change looks good, but I'd prefer that we add some more explanation to the usage section, that this will build an image within the minikube environment - and also linking to https://kubernetes.io/docs/getting-started-guides/minikube/#reusing-the-docker-daemon.

cc/ @aaron-prindle

foxish · 2018-01-04T18:16:25Z

sbin/build-push-docker-images.sh

+fi
+
+if [ ! -d "$IMG_PATH" ]; then
+  error "Cannot find docker images. This script must be run from a runnable distribution of Apache Spark."


Update this comment? I presume now it should say runnable distribution, or from source.

The source directory is sort of a "runnable distribution" if Spark is built. I'd rather keep the message simple since it's mostly targeted at end users (not devs).

foxish · 2018-01-04T21:00:57Z

sbin/build-push-docker-images.sh

+  -m          Use minikube's Docker daemon.
+
+Using minikube when building images will do so directly into minikube's Docker daemon.
+There is no need to push the images into minikube int that case, they'll be automatically


typo int -> in.

foxish

LGTM after nits. Thanks!

foxish · 2018-01-04T21:02:42Z

docs/running-on-kubernetes.md

-Status and logs of failed executor pods can be checked in similar ways. Finally, deleting the driver pod will clean up the entire spark 
-application, includling all executors, associated service, etc. The driver pod can be thought of as the Kubernetes representation of 
+Status and logs of failed executor pods can be checked in similar ways. Finally, deleting the driver pod will clean up the entire spark
+application, includling all executors, associated service, etc. The driver pod can be thought of as the Kubernetes representation of


also just noticed another typo here. includling -> including

SparkQA · 2018-01-04T21:33:42Z

Test build #85685 has finished for PR 20154 at commit 2ce48f9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-04T22:11:50Z

Test build #85688 has finished for PR 20154 at commit e1ea8a7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-05T00:03:20Z

Test build #85697 has finished for PR 20154 at commit ef99214.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-05T00:29:24Z

Test build #85696 has finished for PR 20154 at commit 9a2d6d3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-01-05T00:34:13Z

Merging to master / 2.3.

- Make it possible to build images from a git clone. - Make it easy to use minikube to test things. Also fixed what seemed like a bug: the base image wasn't getting the tag provided in the command line. Adding the tag allows users to use multiple Spark builds in the same kubernetes cluster. Tested by deploying images on minikube and running spark-submit from a dev environment; also by building the images with different tags and verifying "docker images" in minikube. Author: Marcelo Vanzin <[email protected]> Closes #20154 from vanzin/SPARK-22960. (cherry picked from commit 0428368) Signed-off-by: Marcelo Vanzin <[email protected]>

felixcheung

just to note, to publish to the ASF official project on Docker Hub https://hub.docker.com/u/apache/, ASF INFRA supports the use of Docker Hub's automated build, which doesn't use script or ARGS and typically only with the Dockerfile itself docker/hub-feedback#508 (workaround suggested in this)

vanzin · 2018-01-05T17:40:14Z

ARGs can have default values, so we could do that if we decide to use the Docker Hub infra. Also the docker file depends on the Spark build being present locally, so that's probably another thing that would not work as expected there.

liyinan926 · 2018-01-05T22:24:09Z

@vanzin it seems using ARG in FROM is only supported since Docker version 17.06. It does not work for earlier versions of Docker. I verified that it didn't work on an older version of Docker. See https://stackoverflow.com/questions/40273070/docker-build-arg-in-source-file. I think we should revert the base_image arg.

vanzin · 2018-01-05T22:27:49Z

That kinda sucks. It means the base image cannot have a tag so working with multiple Spark versions will be a little weird. Anyway, feel free to open a PR to revert that part.

## What changes were proposed in this pull request? This PR reverts the `ARG base_image` before `FROM` in the images of driver, executor, and init-container, introduced in #20154. The reason is Docker versions before 17.06 do not support this use (`ARG` before `FROM`). ## How was this patch tested? Tested manually. vanzin foxish kimoonkim Author: Yinan Li <[email protected]> Closes #20170 from liyinan926/master. (cherry picked from commit bf65cd3) Signed-off-by: Marcelo Vanzin <[email protected]>

felixcheung · 2018-01-06T07:56:40Z

that's good, I think we should still address the finer point of #20154 (review)

if docker hub can't build spark-base then pretty much we are crossing out the possibility of releasing the docker images with 2.3.0 release as ASF.

foxish reviewed Jan 4, 2018

View reviewed changes

Marcelo Vanzin added 3 commits January 4, 2018 10:30

Make recommendation explicit.

13d5fc0

Typo.

e1ea8a7

More docs.

9a2d6d3

foxish reviewed Jan 4, 2018

View reviewed changes

Typos.

ef99214

asfgit closed this in 0428368 Jan 5, 2018

felixcheung reviewed Jan 5, 2018

View reviewed changes

foxish mentioned this pull request Jan 5, 2018

Added some coverage for executors and test cases for secret mounting and init-containers apache-spark-on-k8s/spark-integration#15

Merged

vanzin deleted the SPARK-22960 branch January 5, 2018 22:35

liyinan926 mentioned this pull request Jan 5, 2018

[SPARK-22960][K8S] Revert use of ARG base_image in images #20170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22960][k8s] Make build-push-docker-images.sh more dev-friendly. #20154

[SPARK-22960][k8s] Make build-push-docker-images.sh more dev-friendly. #20154

vanzin commented Jan 4, 2018

vanzin commented Jan 4, 2018

foxish left a comment

foxish Jan 4, 2018

vanzin Jan 4, 2018

liyinan926 Jan 4, 2018

foxish Jan 4, 2018 •

edited

Loading

vanzin Jan 4, 2018 •

edited

Loading

foxish Jan 4, 2018 •

edited

Loading

foxish Jan 4, 2018

vanzin Jan 4, 2018

foxish Jan 4, 2018

foxish Jan 4, 2018

foxish left a comment •

edited

Loading

foxish Jan 4, 2018

SparkQA commented Jan 4, 2018

SparkQA commented Jan 4, 2018

SparkQA commented Jan 5, 2018

SparkQA commented Jan 5, 2018

vanzin commented Jan 5, 2018

felixcheung left a comment •

edited

Loading

vanzin commented Jan 5, 2018 •

edited

Loading

liyinan926 commented Jan 5, 2018 •

edited

Loading

vanzin commented Jan 5, 2018

felixcheung commented Jan 6, 2018

[SPARK-22960][k8s] Make build-push-docker-images.sh more dev-friendly. #20154

[SPARK-22960][k8s] Make build-push-docker-images.sh more dev-friendly. #20154

Conversation

vanzin commented Jan 4, 2018

vanzin commented Jan 4, 2018

foxish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxish Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

vanzin Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

foxish Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxish left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 4, 2018

SparkQA commented Jan 4, 2018

SparkQA commented Jan 5, 2018

SparkQA commented Jan 5, 2018

vanzin commented Jan 5, 2018

felixcheung left a comment • edited Loading

Choose a reason for hiding this comment

vanzin commented Jan 5, 2018 • edited Loading

liyinan926 commented Jan 5, 2018 • edited Loading

vanzin commented Jan 5, 2018

felixcheung commented Jan 6, 2018

foxish Jan 4, 2018 •

edited

Loading

vanzin Jan 4, 2018 •

edited

Loading

foxish Jan 4, 2018 •

edited

Loading

foxish left a comment •

edited

Loading

felixcheung left a comment •

edited

Loading

vanzin commented Jan 5, 2018 •

edited

Loading

liyinan926 commented Jan 5, 2018 •

edited

Loading