Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

What changes were proposed in this pull request?

This PR aims to support executor id placeholder in spark.kubernetes.executor.volumes.persistentVolumeClaim.myname.options.claimName configuration like the following.

--conf spark.kubernetes.executor.volumes.persistentVolumeClaim.spark-local-dir-1.options.claimName=pvc-spark-SPARK_EXECUTOR_ID \

Why are the changes needed?

This is a convenient way to mount corresponding PV to the executor.

Does this PR introduce any user-facing change?

Yes, but this is a new feature and there is no regression because users don't use SPARK_EXECUTOR_ID in PVC claim name.

How was this patch tested?

Pass the newly added test case.

-c spark.kubernetes.executor.volumes.persistentVolumeClaim.spark-local-dir-1.options.claimName=pvc-spark-SPARK_EXECUTOR_ID \
-c spark.kubernetes.executor.volumes.persistentVolumeClaim.spark-local-dir-1.mount.path=/data \
-c spark.kubernetes.executor.volumes.persistentVolumeClaim.spark-local-dir-1.mount.readOnly=false \
@SparkQA
Copy link

SparkQA commented Aug 27, 2020

Test build #127953 has finished for PR 29557 at commit b5fe711.

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

@SparkQA
Copy link

SparkQA commented Aug 27, 2020

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

@SparkQA
Copy link

SparkQA commented Aug 27, 2020

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

@dongjoon-hyun
Copy link
Member Author

Could you review this please, @holdenk and @dbtsai ?

@holdenk
Copy link
Contributor

holdenk commented Aug 27, 2020

LGTM

@holdenk
Copy link
Contributor

holdenk commented Aug 27, 2020

Maybe consider a follow up to document this?

@dongjoon-hyun
Copy link
Member Author

Thanks. Sure! I have two related PRs. I'll make a K8s doc PR.
Merged to master.

holdenk pushed a commit to holdenk/spark that referenced this pull request Oct 27, 2020
### What changes were proposed in this pull request?

This PR aims to support executor id placeholder in `spark.kubernetes.executor.volumes.persistentVolumeClaim.myname.options.claimName` configuration like the following.
```
--conf spark.kubernetes.executor.volumes.persistentVolumeClaim.spark-local-dir-1.options.claimName=pvc-spark-SPARK_EXECUTOR_ID \
```

### Why are the changes needed?

This is a convenient way to mount corresponding PV to the executor.

### Does this PR introduce _any_ user-facing change?

Yes, but this is a new feature and there is no regression because users don't use `SPARK_EXECUTOR_ID` in PVC claim name.

### How was this patch tested?

Pass the newly added test case.

Closes apache#29557 from dongjoon-hyun/SPARK-PVC.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
dongjoon-hyun pushed a commit that referenced this pull request May 8, 2022
…dynamic allocation failure

### What changes were proposed in this pull request?

This PR aims to show a directional error message for executor PVC dynamic allocation failure.

### Why are the changes needed?

#29846 supports dynamic PVC creation/deletion for K8s executors.
#29557 support execId placeholder in executor PVC conf.
If not set `spark.kubernetes.executor.volumes.persistentVolumeClaim.spark-local-dir-1.options.claimName` with `onDemand` or `SPARK_EXECUTOR_ID`, spark will continue to try to create the executor pod.
After this PR, spark can show a directional error message for this situation.
```plain
ERROR ExecutorPodsSnapshotsStoreImpl: Going to stop due to IllegalArgumentException
java.lang.IllegalArgumentException: PVC ClaimName should contain OnDemand or SPARK_EXECUTOR_ID when multiple executors are required
```

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Add unit test.

Closes #36374 from dcoliversun/SPARK-39006.

Authored-by: Qian.Sun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants