[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-19 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/21572


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread erikerlandson 
Github user erikerlandson commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195866275
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -37,11 +37,17 @@ if [ -z "$uidentry" ] ; then
 fi
 
 SPARK_K8S_CMD="$1"
-if [ -z "$SPARK_K8S_CMD" ]; then
-  echo "No command to execute has been provided." 1>&2
-  exit 1
-fi
-shift 1
+case "$SPARK_K8S_CMD" in
+driver | driver-py | executor)
+  shift 1
+  ;;
+"")
+  ;;
+*)
+  echo "No SPARK_K8S_CMD provided: proceeding in pass-through mode..."
--- End diff --

Overall this looks good to me.  Looking at this simplified logic, the 
logging message should probably be more like:

"Non-spark-k8s command provided, proceeding in pass-through mode..."


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread erikerlandson 
Github user erikerlandson commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195811228
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
--- End diff --

@tmckayus that seems wrong, since the `CMD` that gets executed after the 
`case` is now missing the first element, am I missing something?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread tmckayus 
Github user tmckayus commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195807560
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
--- End diff --

@erikerlandson yes, after a shift the leading arg is gone from "$@"


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread tmckayus 
Github user tmckayus commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195806962
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
 if [ -z "$SPARK_K8S_CMD" ]; then
-  echo "No command to execute has been provided." 1>&2
-  exit 1
+  echo "No command to execute has been provided. Ignoring spark-on-k8s 
workflow..." 1>&2
+else
+  shift 1
--- End diff --

this doesn't quite work, the -z test is effectively checking only whether 
$1 was empty or not.
If it's non-empty, but it is *not* a recognized spark-on-k8s command (ie 
driver, driver-py, or executor), it's a passthrough command and therefore we 
cannot shift anything.  As it is, this would consume something like 
"/usr/libexec/s2i/assembly.sh" and make it disappear.

Personally, I would do somethng like this and take an early out in the 
unsupported case, skipping all the other environment processing

```bash
case "$SPARK_K8S_CMD in
driver | driver-py | executor)
   shift 1
   ;;
*)
   echo "No SPARK_K8S_CMD provided: proceeding in pass-through mode..."
   exec /sbin/tini -s -- "$@"
  ;;
esac


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread erikerlandson 
Github user erikerlandson commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195804610
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
--- End diff --

And it does the right thing w.r.t. `shift` in both cases?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread tmckayus 
Github user tmckayus commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195803416
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
--- End diff --

ack, I concur, this is the way the script works historically


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread rimolive 
Github user rimolive commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195796105
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
--- End diff --

This is handled by the case block, because pass-through mode will be used 
either if SPARK_K8S_CMD is empty or has a non spark-on-k8s command. I tested 
both scenarios.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread rimolive 
Github user rimolive commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195795650
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
 if [ -z "$SPARK_K8S_CMD" ]; then
-  echo "No command to execute has been provided." 1>&2
-  exit 1
+  echo "No command to execute has been provided. Ignoring spark-on-k8s 
workflow..." 1>&2
--- End diff --

Good idea. This message is better.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread rimolive 
Github user rimolive commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195795697
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -110,8 +110,7 @@ case "$SPARK_K8S_CMD" in
 ;;
 
   *)
-echo "Unknown command: $SPARK_K8S_CMD" 1>&2
-exit 1
+CMD=("$@")
--- End diff --

+1, I'll make the change.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread erikerlandson 
Github user erikerlandson commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195794051
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
--- End diff --

I see two possible pass-through conditions here: one is "empty 
SPARK_K8S_CMD" and the other is "SPARK_K8S_CMD is non empty but has non-spark 
command in it"  Is that the convention, or is the pass-through case always 
expected to be "empty SPARK_K8S_CMD" ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread erikerlandson 
Github user erikerlandson commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195793358
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -110,8 +110,7 @@ case "$SPARK_K8S_CMD" in
 ;;
 
   *)
-echo "Unknown command: $SPARK_K8S_CMD" 1>&2
-exit 1
+CMD=("$@")
--- End diff --

Should log a message here too, about "executing in pass-through mode" since 
this is the guaranteed code path for pass through


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread erikerlandson 
Github user erikerlandson commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195787809
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -38,10 +38,10 @@ fi
 
 SPARK_K8S_CMD="$1"
 if [ -z "$SPARK_K8S_CMD" ]; then
-  echo "No command to execute has been provided." 1>&2
-  exit 1
+  echo "No command to execute has been provided. Ignoring spark-on-k8s 
workflow..." 1>&2
--- End diff --

I'd propose:
"No SPARK_K8S_CMD provided: proceeding in pass-through mode..."


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21572: [SPARK-24534][K8S] Bypass non spark-on-k8s comman...

2018-06-15 Thread tmckayus 
Github user tmckayus commented on a diff in the pull request:

https://github.com/apache/spark/pull/21572#discussion_r195777382
  
--- Diff: 
resource-managers/kubernetes/docker/src/main/dockerfiles/spark/entrypoint.sh ---
@@ -109,9 +81,16 @@ case "$SPARK_K8S_CMD" in
 )
 ;;
 
+  init)
+CMD=(
+  "$SPARK_HOME/bin/spark-class"
+  "org.apache.spark.deploy.k8s.SparkPodInitContainer"
+  "$@"
--- End diff --

any place in this entrypoint where $@ is being referenced by a spark-on-k8s 
command still needs the shift that was taken out on line 44, correct?  For the 
unknown command case we just want a passthrough, but the main case statement 
historically is expecting the spark-on-k8s command to be stripped. The 
easiest/clearest way to preserve this might just be an extra case statement 
like this at line 44:

```bash
case "$SPARK_K8S_CMD" in
driver | executor | init)
   shift 1
   ;;
esac


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org