[SPARK-34764][CORE][K8S][UI] Propagate reason for exec loss to Web UI #32436

holdenk · 2021-05-04T17:39:34Z

What changes were proposed in this pull request?

Adds the exec loss reason to the Spark web UI & in doing so also fix the Kube integration to pass exec loss reason into core.

UI change:

Why are the changes needed?

Debugging Spark jobs is hard, making it clearer why executors have exited could help.

Does this PR introduce any user-facing change?

Yes a new column on the executor page.

How was this patch tested?

K8s unit test updated to validate exec loss reasons are passed through regardless of exec alive state, manual testing to validate the UI.

…n, but we want to propegate the loss reason in doRemoveExecutor so we can't gait on isExecutorActive

…gate it to the test as well

…ason, update the test to match this change

holdenk · 2021-05-04T18:38:09Z

cc @dongjoon-hyun & @BryanCutler

...ore/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala

...rc/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManagerSuite.scala

...ore/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala

dongjoon-hyun · 2021-05-04T20:24:16Z

Thank you for pinging me, @holdenk .
cc @attilapiros

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

dongjoon-hyun · 2021-05-04T20:26:57Z

core/src/main/resources/org/apache/spark/ui/static/executorspage-template.html

              Shuffle Write</span></th>
          <th>Logs</th>
          <th>Thread Dump</th>
+          <th>Exec Loss Reason</th>


Is this empty always in non-K8s resource managers?

No exec loss reason is populated for YARN as well :)

...ore/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala

dongjoon-hyun · 2021-05-04T20:32:10Z

...ore/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala

+      case 126 => "(not executable - possibly perm or arch)"
+      case 137 => "(SIGKILL, possible container OOM)"
+      case 139 => "(SIGSEGV: that's unexpected)"
+      case 255 => "(exit-1, your guess is as good as mine)"


I expected we have an error code for Evicted due to out of disk during worker decommission. What is the error code for that, @holdenk ?

So I think it's going to be inconsistent depending on how exactly it shows up (e.g. does the JVM have an uncaught exception trying to write a file or do we exceed the resource quota). So for now I don't have a clear exit code to map to it unfortunately. I could try and add a base handler for IO errors that are uncaught to exit with a specific code, but I'd rather do that in a separate PR.

SparkQA · 2021-05-05T01:04:12Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42666/

SparkQA · 2021-05-05T01:06:23Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42666/

SparkQA · 2021-05-05T03:28:41Z

Test build #138145 has finished for PR 32436 at commit 433ee83.

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

...ore/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

attilapiros · 2021-05-05T05:03:35Z

Can you suggest an event log we can use for checking the UI? If there is none checked in already as none is good enough (there is no interesting executor losses in the eventlog) then we should consider adding a new one. This would be helpful not only for the current reviewers but for future developers of this area could use it to check (at least by eye) their new changes: whether they have broken this feature or not.

WDYT?

…ark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala Co-authored-by: Reid Mewborne <[email protected]>

holdenk · 2021-05-05T16:40:55Z

So if you want it's pretty easy to trigger OOMs with the GroupByKey example (it's how I did the manual test). I'm not sure about event logs though, do we have those committed somewhere for folks to use in debugging w/history server?

attilapiros · 2021-05-05T17:09:04Z

We are storing event logs for unitest, here is the directory: https://github.com/apache/spark/tree/master/core/src/test/resources/spark-events

SparkQA · 2021-05-05T17:09:15Z

Test build #138178 has finished for PR 32436 at commit 19355d4.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-05-05T17:13:04Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42699/

SparkQA · 2021-05-11T18:26:48Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42913/

SparkQA · 2021-05-11T18:33:04Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42913/

SparkQA · 2021-05-11T20:20:55Z

Test build #138390 has finished for PR 32436 at commit a4263cd.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2021-05-11T20:35:30Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42923/

SparkQA · 2021-05-11T20:35:31Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42923/

SparkQA · 2021-05-11T22:05:54Z

Test build #138401 has finished for PR 32436 at commit 3ab5a09.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait ExtractValue extends Expression
trait ShuffledJoin extends JoinCodegenSupport

BryanCutler

Very nice to have this, thanks @holdenk, LGTM

holdenk · 2021-05-11T22:29:48Z

K8s failure is unrelated (The connection to the server localhost:8080 was refused - did you specify the right host or port?). I'll merge this Thursday unless anyone has additional suggestions :)

holdenk · 2021-05-13T22:56:27Z

Going to merge now, thanks everyone for the review and feedback. We can iterate on adding more exit codes in future PRs if we see any common questions on the user@ list :)

holdenk · 2021-05-13T23:03:08Z

Merged to the current dev branch :) Since it's a new feature not planning on back porting.

HyukjinKwon · 2021-05-14T01:07:27Z

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

  return threadDumpEnabled;
 }

+function formatLossReason(removeReason, type, row) {


Eh, uhoh. Seems like the JavaScript linter is broken by this:

https://github.com/apache/spark/runs/2579648547

added 118 packages in 1.482s /__w/spark/spark/core/src/main/resources/org/apache/spark/ui/static/executorspage.js 34:41 error 'type' is defined but never used. Allowed unused args must match /^_ignored_.*/u no-unused-vars 34:47 error 'row' is defined but never used. Allowed unused args must match /^_ignored_.*/u no-unused-vars 35:1 error Expected indentation of 2 spaces but found 4 indent 36:1 error Expected indentation of 4 spaces but found 7 indent 37:1 error Expected indentation of 2 spaces but found 4 indent 38:1 error Expected indentation of 4 spaces but found 7 indent 39:1 error Expected indentation of 2 spaces but found 4 indent 556:1 error Expected indentation of 14 spaces but found 16 indent 557:1 error Expected indentation of 14 spaces but found 16 indent

Mind taking a look please?

I just made a quick followup to fix up the build: #32541

…r JavaScript linter ### What changes were proposed in this pull request? This PR is a followup of #32436 which broke JavaScript linter. There was a logical conflict - the linter was added after the last successful test run in that PR. ``` added 118 packages in 1.482s /__w/spark/spark/core/src/main/resources/org/apache/spark/ui/static/executorspage.js 34:41 error 'type' is defined but never used. Allowed unused args must match /^_ignored_.*/u no-unused-vars 34:47 error 'row' is defined but never used. Allowed unused args must match /^_ignored_.*/u no-unused-vars 35:1 error Expected indentation of 2 spaces but found 4 indent 36:1 error Expected indentation of 4 spaces but found 7 indent 37:1 error Expected indentation of 2 spaces but found 4 indent 38:1 error Expected indentation of 4 spaces but found 7 indent 39:1 error Expected indentation of 2 spaces but found 4 indent 556:1 error Expected indentation of 14 spaces but found 16 indent 557:1 error Expected indentation of 14 spaces but found 16 indent ``` ### Why are the changes needed? To recover the build ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? Manually tested: ```bash ./dev/lint-js lint-js checks passed. ``` Closes #32541 from HyukjinKwon/SPARK-34764-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]>

Adds the exec loss reason to the Spark web UI & in doing so also fix the Kube integration to pass exec loss reason into core. UI change: ![image](https://user-images.githubusercontent.com/59893/117045762-b975ba80-acc4-11eb-9679-8edab3cfadc2.png) Debugging Spark jobs is *hard*, making it clearer why executors have exited could help. Yes a new column on the executor page. K8s unit test updated to validate exec loss reasons are passed through regardless of exec alive state, manual testing to validate the UI. Closes apache#32436 from holdenk/SPARK-34764-propegate-reason-for-exec-loss. Lead-authored-by: Holden Karau <[email protected]> Co-authored-by: Holden Karau <[email protected]> Signed-off-by: Holden Karau <[email protected]> Fix

holdenk and others added 8 commits May 3, 2021 11:10

Add the removeal reason to the status

b8eb7be

Work on adding exec loss reason

d46d3cd

Missing a + broke the JS

b5db580

isExecutorActive will return false if we are waiting for a loss reaso…

ae77e33

…n, but we want to propegate the loss reason in doRemoveExecutor so we can't gait on isExecutorActive

Format the loss reason properly

dee0963

We don't need a space after dead

4e478a0

Add a utility function for describing kubernetes exit codes and prope…

6ebcc55

…gate it to the test as well

We don't use alive to gate anymore so the UI can get the exec loss re…

433ee83

…ason, update the test to match this change

github-actions bot added CORE KUBERNETES WEB UI labels May 4, 2021

holdenk changed the title ~~[SPARK-34764][CORE][K8S] Propagate reason for exec loss to Web UI~~ [SPARK-34764][CORE][K8S][UI] Propagate reason for exec loss to Web UI May 4, 2021

8bitreid reviewed May 4, 2021

View reviewed changes

dongjoon-hyun reviewed May 4, 2021

View reviewed changes

core/src/main/resources/org/apache/spark/ui/static/executorspage.js Outdated Show resolved Hide resolved

dongjoon-hyun reviewed May 4, 2021

View reviewed changes

...ore/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed May 4, 2021

View reviewed changes

attilapiros reviewed May 5, 2021

View reviewed changes

Update resource-managers/kubernetes/core/src/main/scala/org/apache/sp…

19355d4

…ark/scheduler/cluster/k8s/ExecutorPodsLifecycleManager.scala Co-authored-by: Reid Mewborne <[email protected]>

JS changes (indentation and simplify data access)

e62e3b1

Simplify the describeExitCode and remove the duplication

a4263cd

Merge in latest changes

3ab5a09

BryanCutler reviewed May 11, 2021

View reviewed changes

asfgit closed this in 160b3be May 13, 2021

HyukjinKwon reviewed May 14, 2021

View reviewed changes

HyukjinKwon mentioned this pull request May 14, 2021

[SPARK-34764][UI][FOLLOW-UP] Fix indentation and missing arguments for JavaScript linter #32541

Closed

[SPARK-34764][CORE][K8S][UI] Propagate reason for exec loss to Web UI #32436

[SPARK-34764][CORE][K8S][UI] Propagate reason for exec loss to Web UI #32436

Uh oh!

Conversation

holdenk commented May 4, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

holdenk commented May 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented May 4, 2021

Uh oh!

Uh oh!

dongjoon-hyun May 4, 2021

Choose a reason for hiding this comment

Uh oh!

holdenk May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun May 4, 2021

Choose a reason for hiding this comment

Uh oh!

holdenk May 4, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 5, 2021

Uh oh!

SparkQA commented May 5, 2021

Uh oh!

SparkQA commented May 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

attilapiros commented May 5, 2021

Uh oh!

holdenk commented May 5, 2021

Uh oh!

attilapiros commented May 5, 2021

Uh oh!

SparkQA commented May 5, 2021

Uh oh!

SparkQA commented May 5, 2021

Uh oh!

SparkQA commented May 11, 2021

Uh oh!

SparkQA commented May 11, 2021

Uh oh!

SparkQA commented May 11, 2021

Uh oh!

SparkQA commented May 11, 2021

Uh oh!

SparkQA commented May 11, 2021

Uh oh!

SparkQA commented May 11, 2021

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

holdenk commented May 11, 2021

Uh oh!

holdenk commented May 13, 2021

Uh oh!

holdenk commented May 13, 2021

Uh oh!

HyukjinKwon May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon May 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

HyukjinKwon May 14, 2021 •

edited

Loading