[SPARK-27510][CORE] Avoid Master falls into dead loop while launching executor failed in Worker

[Ngone51]

What changes were proposed in this pull request?

This is a long standing issue which I met before and I've seen other people got trouble with it:
test cases stuck on "local-cluster mode" of ReplSuite?
Spark tests hang on local machine due to "testGuavaOptional" in JavaAPISuite

When running test under local-cluster mode with wrong SPARK_HOME(spark.test.home), test just get stuck and no response forever. After looking into SPARK_WORKER_DIR, I found there's endless executor directories under it. So, this explains what happens during test getting stuck.

The whole process looks like:

1. Driver submits an app to Master and asks for N executors
2. Master inits executor state with LAUNCHING and sends LaunchExecutor to Worker
3. Worker receives LaunchExecutor, launches ExecutorRunner asynchronously and sends ExecutorStateChanged(state=RUNNING) to Mater immediately
4. Master receives ExecutorStateChanged(state=RUNNING) and reset _retyCount to 0.
5. ExecutorRunner throws exception during executor launching, sends ExecutorStateChanged(state=FAILED) to Worker, Worker forwards the msg to Master
6. Master receives ExecutorStateChanged(state=FAILED). Since Master always reset _retyCount when it receives RUNNING msg, so, event if a Worker fails to launch executor for continuous many times, _retryCount would never exceed maxExecutorRetries. So, Master continue to launch executor and fall into the dead loop.

The problem exists in step 3. Worker sends ExecutorStateChanged(state=RUNNING) to Master immediately while executor is still launching. And, when Master receive that msg, it believes the executor has launched successfully, and reset _retryCount subsequently. However, that's not true.

This pr suggests to remove step 3 and requires Worker only send ExecutorStateChanged(state=RUNNING) after executor has really launched successfully.

How was this patch tested?

Tested Manually.

[Ngone51]

ping @cloud-fan @jiangxb1987 @jerryshao @srowen

please help review, thanks!

[SparkQA]

Test build #104712 has finished for PR 24408 at commit ea54dd4.

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

[srowen]

As usual I'm not super familiar with this code, but from reading your analysis and the code and the change, it does seem more correct to me.

[cloud-fan]

LGTM if tests pass. cc @jiangxb1987

[Ngone51]

Jenkins, retest this please.

[SparkQA]

Test build #104742 has finished for PR 24408 at commit ea54dd4.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[Ngone51]

Jenkins, retest this please.

[jiangxb1987]

One problem is if the ExecutorRunner got shutdown before calling fetchAndRunExecutor(), then it will send wrong state to the master. We shall also update the shutdownHook.

Ideally this change shall be on the right way, but the sync between Master and Workers are extremely subtle so I would be quite conservative to making any changes without adding some test cases.

[cloud-fan]

if the ExecutorRunner got shutdown before calling fetchAndRunExecutor()

Then the worker sends nothing, and master should timeout this worker eventually. +1 for adding some tests

[jiangxb1987]

if the ExecutorRunner got shutdown before calling fetchAndRunExecutor()

Then the worker sends nothing, and master should timeout this worker eventually. +1 for adding some tests

The worker will send ExecutorState.LAUNCHING again, while actually it shall send out ExecutorState.FAILED in this case. See

spark/core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala

Line 110 in ea54dd4

worker.send(ExecutorStateChanged(appId, execId, state, message, exitCode))

[SparkQA]

Test build #104744 has finished for PR 24408 at commit ea54dd4.

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

[SparkQA]

Test build #104749 has finished for PR 24408 at commit 0be6b91.

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

[srowen]

@jiangxb1987 are you OK with this change now?

[Ngone51]

ping @jiangxb1987

[srowen]

@jiangxb1987 (maybe @cloud-fan ) this is looking OK to me but would most like a second look at it.

[jiangxb1987]

LGTM

[jiangxb1987]

retest this please

[SparkQA]

Test build #105052 has finished for PR 24408 at commit 0be6b91.

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

[jiangxb1987]

Thanks! Merged to master!

[Ngone51]

Thanks @srowen @cloud-fan @jiangxb1987 !