You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rvesse <gi...@git.apache.org> on 2018/08/24 10:13:35 UTC
[GitHub] spark pull request #22215: [SPARK-25222][K8S][WIP] Improve container status ...
GitHub user rvesse opened a pull request:
https://github.com/apache/spark/pull/22215
[SPARK-25222][K8S][WIP] Improve container status logging
## What changes were proposed in this pull request?
Currently when running Spark on Kubernetes a logger is run by the client that watches the K8S API for events related to the Driver pod and logs them. However for the container status aspect of the logging this simply dumps the raw object which is not human readable e.g.
This is despite the fact that the logging class in question actually has methods to pretty print this information but only invokes these at the end of a job.
This PR improves the logging to always use the pretty printing methods, additionally modifying them to include further useful information provided by the K8S API.
A similar issue also exists when tasks are lost that will be addressed by further commits to this PR
- [x] Improved `LoggingPodStatusWatcher`
- [ ] Improved container status on task failure
## How was this patch tested?
Built and launched jobs with the updated Spark client and observed the new human readable output:
Suggested reviewers: @liyinan926 @mccheah
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rvesse/spark SPARK-25222
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22215.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22215
----
commit ebcbf05b212937aad21742bf14e592ffc1b14383
Author: Rob Vesse <rv...@...>
Date: 2018-08-24T09:32:14Z
[SPARK-25222][K8S] Improve container status logging
Actually log human readable container status information rather than
dumping the raw status object returned by the K8S API
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22215
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/2789/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on the issue:
https://github.com/apache/spark/pull/22215
Think this is pretty much ready to merge, can folks take another look when they get chance
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
**[Test build #95569 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95569/testReport)** for PR 22215 at commit [`6f6442f`](https://github.com/apache/spark/commit/6f6442f392717fe87002e9bc1b27c91ff387080e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/2753/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S][WIP] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/22215
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95569/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/2789/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the issue:
https://github.com/apache/spark/pull/22215
Yup this can merge now, thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S][WIP] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r214491177
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,81 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) : String = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
+ pairs.map {
+ case (k, v) => s"\n$indentStr $k: ${Option(v).filter(_.nonEmpty).getOrElse("N/A")}"
+ }.mkString("")
+ }
+
+ /**
+ * Given a pod, output a human readable representation of its state
+ *
+ * @param pod Pod
+ * @return Human readable pod state
+ */
+ def formatPodState(pod: Pod): String = {
+ val details = Seq[(String, String)](
+ // pod metadata
+ ("pod name", pod.getMetadata.getName),
+ ("namespace", pod.getMetadata.getNamespace),
+ ("labels", pod.getMetadata.getLabels.asScala.mkString(", ")),
+ ("pod uid", pod.getMetadata.getUid),
+ ("creation time", formatTime(pod.getMetadata.getCreationTimestamp)),
+
+ // spec details
+ ("service account name", pod.getSpec.getServiceAccountName),
+ ("volumes", pod.getSpec.getVolumes.asScala.map(_.getName).mkString(", ")),
+ ("node name", pod.getSpec.getNodeName),
+
+ // status
+ ("start time", formatTime(pod.getStatus.getStartTime)),
+ ("phase", pod.getStatus.getPhase),
+ ("container status", containersDescription(pod, 2))
+ )
+
+ formatPairsBundle(details)
+ }
+
+ def containersDescription(p: Pod, indent: Int = 1): String = {
+ p.getStatus.getContainerStatuses.asScala.map { status =>
+ Seq(
+ ("container name", status.getName),
+ ("container image", status.getImage)) ++
+ containerStatusDescription(status)
+ }.map(p => formatPairsBundle(p, indent)).mkString("\n\n")
+ }
+
+ def containerStatusDescription(containerStatus: ContainerStatus)
+ : Seq[(String, String)] = {
+ val state = containerStatus.getState
+ Option(state.getRunning)
--- End diff --
This is a really cool use of partial functions - wonder if there's other places where we should be matching this way (of course doesn't have to be done here).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/2703/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
**[Test build #95568 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95568/testReport)** for PR 22215 at commit [`6f6442f`](https://github.com/apache/spark/commit/6f6442f392717fe87002e9bc1b27c91ff387080e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/2703/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on the issue:
https://github.com/apache/spark/pull/22215
@mccheah Thanks for the review, have made the change you suggested to use N/A instead of empty string.
I have left indentation as tabs for now, as I said in a previous comment this was just what the existing code used and I am happy to change it if others also want the change to spaces made
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2703/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95616/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by nrchakradhar <gi...@git.apache.org>.
Github user nrchakradhar commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r212793906
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,90 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
+ pairs.map {
+ case (k, v) => s"\n$indentStr $k: ${Option(v).filter(_.nonEmpty).getOrElse("N/A")}"
+ }.mkString("")
+ }
+
+ /**
+ * Given a pod output a human readable representation of its state
--- End diff --
nit. Adding a "," after pod make it better reading. Given a pod, output ...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/2753/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r212713726
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,90 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
+ pairs.map {
+ case (k, v) => s"\n$indentStr $k: ${Option(v).filter(_.nonEmpty).getOrElse("N/A")}"
+ }.mkString("")
+ }
+
+ /**
+ * Given a pod output a human readable representation of its state
+ * @param pod Pod
+ * @return Human readable pod state
+ */
+ def formatPodState(pod: Pod): String = {
+ val details = Seq[(String, String)](
+ // pod metadata
+ ("pod name", pod.getMetadata.getName),
+ ("namespace", pod.getMetadata.getNamespace),
+ ("labels", pod.getMetadata.getLabels.asScala.mkString(", ")),
+ ("pod uid", pod.getMetadata.getUid),
+ ("creation time", formatTime(pod.getMetadata.getCreationTimestamp)),
+
+ // spec details
+ ("service account name", pod.getSpec.getServiceAccountName),
+ ("volumes", pod.getSpec.getVolumes.asScala.map(_.getName).mkString(", ")),
+ ("node name", pod.getSpec.getNodeName),
+
+ // status
+ ("start time", formatTime(pod.getStatus.getStartTime)),
+ ("container images",
+ pod.getStatus.getContainerStatuses
+ .asScala
+ .map(_.getImage)
+ .mkString(", ")),
+ ("phase", pod.getStatus.getPhase),
+ ("status", pod.getStatus.getContainerStatuses.asScala.map { status =>
+ Seq(
+ ("Container name", status.getName),
+ ("Container image", status.getImage)) ++
+ containerStatusDescription(status)
+ }.map(p => formatPairsBundle(p, 2)).mkString("\n\n"))
+ )
+
+ formatPairsBundle(details)
+ }
+
+ def containersDescription(p: Pod): String = {
+ p.getStatus.getContainerStatuses.asScala.map { status =>
+ Seq(
+ ("Container name", status.getName),
+ ("Container image", status.getImage)) ++
+ containerStatusDescription(status)
+ }.map(p => formatPairsBundle(p, 1)).mkString("\n\n")
+ }
+
+ def containerStatusDescription(containerStatus: ContainerStatus)
+ : Seq[(String, String)] = {
+ val state = containerStatus.getState
+ Option(state.getRunning)
+ .orElse(Option(state.getTerminated))
+ .orElse(Option(state.getWaiting))
+ .map {
+ case running: ContainerStateRunning =>
+ Seq(
+ ("Container state", "Running"),
--- End diff --
Ditto. Please use all lowercase.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2789/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r212713203
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,90 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
+ pairs.map {
+ case (k, v) => s"\n$indentStr $k: ${Option(v).filter(_.nonEmpty).getOrElse("N/A")}"
+ }.mkString("")
+ }
+
+ /**
+ * Given a pod output a human readable representation of its state
+ * @param pod Pod
+ * @return Human readable pod state
+ */
+ def formatPodState(pod: Pod): String = {
+ val details = Seq[(String, String)](
+ // pod metadata
+ ("pod name", pod.getMetadata.getName),
+ ("namespace", pod.getMetadata.getNamespace),
+ ("labels", pod.getMetadata.getLabels.asScala.mkString(", ")),
+ ("pod uid", pod.getMetadata.getUid),
+ ("creation time", formatTime(pod.getMetadata.getCreationTimestamp)),
+
+ // spec details
+ ("service account name", pod.getSpec.getServiceAccountName),
+ ("volumes", pod.getSpec.getVolumes.asScala.map(_.getName).mkString(", ")),
+ ("node name", pod.getSpec.getNodeName),
+
+ // status
+ ("start time", formatTime(pod.getStatus.getStartTime)),
+ ("container images",
+ pod.getStatus.getContainerStatuses
+ .asScala
+ .map(_.getImage)
+ .mkString(", ")),
+ ("phase", pod.getStatus.getPhase),
+ ("status", pod.getStatus.getContainerStatuses.asScala.map { status =>
+ Seq(
--- End diff --
Looks like you can use `containersDescription` here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r214490999
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,81 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) : String = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
--- End diff --
Can we prefer space-based indentation? Curious as to whether others have an opinion about this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95568/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
**[Test build #95569 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95569/testReport)** for PR 22215 at commit [`6f6442f`](https://github.com/apache/spark/commit/6f6442f392717fe87002e9bc1b27c91ff387080e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the issue:
https://github.com/apache/spark/pull/22215
@mccheah can you give `ok to test` to this one and help merge it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on the issue:
https://github.com/apache/spark/pull/22215
@liyinan926 @nrchakradhar Addressed all your comments, thanks for the reviews.
Is someone able to kick off the Jenkins testing on this PR?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
**[Test build #95568 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95568/testReport)** for PR 22215 at commit [`6f6442f`](https://github.com/apache/spark/commit/6f6442f392717fe87002e9bc1b27c91ff387080e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r214612510
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,81 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) : String = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
--- End diff --
I just preserved the original codes choice here, I would happily change to spaces if preferred
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2753/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r214491272
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala ---
@@ -151,13 +152,15 @@ private[spark] class ExecutorPodsLifecycleManager(
private def exitReasonMessage(podState: FinalPodState, execId: Long, exitCode: Int) = {
val pod = podState.pod
+ val reason = Option(pod.getStatus.getReason)
+ val message = Option(pod.getStatus.getMessage)
s"""
|The executor with id $execId exited with exit code $exitCode.
- |The API gave the following brief reason: ${pod.getStatus.getReason}
- |The API gave the following message: ${pod.getStatus.getMessage}
+ |The API gave the following brief reason: ${reason.getOrElse("")}
--- End diff --
Maybe default as `N/A`? Might be confusing to be left blank.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S][WIP] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22215
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r212713446
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala ---
@@ -60,4 +64,90 @@ private[spark] object KubernetesUtils {
}
def parseMasterUrl(url: String): String = url.substring("k8s://".length)
+
+ def formatPairsBundle(pairs: Seq[(String, String)], indent: Int = 1) = {
+ // Use more loggable format if value is null or empty
+ val indentStr = "\t" * indent
+ pairs.map {
+ case (k, v) => s"\n$indentStr $k: ${Option(v).filter(_.nonEmpty).getOrElse("N/A")}"
+ }.mkString("")
+ }
+
+ /**
+ * Given a pod output a human readable representation of its state
+ * @param pod Pod
+ * @return Human readable pod state
+ */
+ def formatPodState(pod: Pod): String = {
+ val details = Seq[(String, String)](
+ // pod metadata
+ ("pod name", pod.getMetadata.getName),
+ ("namespace", pod.getMetadata.getNamespace),
+ ("labels", pod.getMetadata.getLabels.asScala.mkString(", ")),
+ ("pod uid", pod.getMetadata.getUid),
+ ("creation time", formatTime(pod.getMetadata.getCreationTimestamp)),
+
+ // spec details
+ ("service account name", pod.getSpec.getServiceAccountName),
+ ("volumes", pod.getSpec.getVolumes.asScala.map(_.getName).mkString(", ")),
+ ("node name", pod.getSpec.getNodeName),
+
+ // status
+ ("start time", formatTime(pod.getStatus.getStartTime)),
+ ("container images",
+ pod.getStatus.getContainerStatuses
+ .asScala
+ .map(_.getImage)
+ .mkString(", ")),
+ ("phase", pod.getStatus.getPhase),
+ ("status", pod.getStatus.getContainerStatuses.asScala.map { status =>
+ Seq(
+ ("Container name", status.getName),
+ ("Container image", status.getImage)) ++
+ containerStatusDescription(status)
+ }.map(p => formatPairsBundle(p, 2)).mkString("\n\n"))
+ )
+
+ formatPairsBundle(details)
+ }
+
+ def containersDescription(p: Pod): String = {
+ p.getStatus.getContainerStatuses.asScala.map { status =>
+ Seq(
+ ("Container name", status.getName),
--- End diff --
Should use all lowercase for consistency with other rows.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
**[Test build #95616 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95616/testReport)** for PR 22215 at commit [`4c39a81`](https://github.com/apache/spark/commit/4c39a81cd93ea0a7e8ccfe51558d9756f3ea7ff3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22215: [SPARK-25222][K8S] Improve container status loggi...
Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on a diff in the pull request:
https://github.com/apache/spark/pull/22215#discussion_r214614277
--- Diff: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala ---
@@ -151,13 +152,15 @@ private[spark] class ExecutorPodsLifecycleManager(
private def exitReasonMessage(podState: FinalPodState, execId: Long, exitCode: Int) = {
val pod = podState.pod
+ val reason = Option(pod.getStatus.getReason)
+ val message = Option(pod.getStatus.getMessage)
s"""
|The executor with id $execId exited with exit code $exitCode.
- |The API gave the following brief reason: ${pod.getStatus.getReason}
- |The API gave the following message: ${pod.getStatus.getMessage}
+ |The API gave the following brief reason: ${reason.getOrElse("")}
--- End diff --
Fixed in latest commit
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22215: [SPARK-25222][K8S] Improve container status logging
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22215
**[Test build #95616 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95616/testReport)** for PR 22215 at commit [`4c39a81`](https://github.com/apache/spark/commit/4c39a81cd93ea0a7e8ccfe51558d9756f3ea7ff3).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org