[SPARK-37331][K8S] Add the ability to create resources before driverPod creating #34599

Yikun · 2021-11-15T09:26:34Z

What changes were proposed in this pull request?

This patch adds a new method getAdditionalPreKubernetesResources for KubernetesFeatureConfigStep. It returns any additional Kubernetes resources that should be added to support this feature and resources would be setup before driver pod
creating.

After this patch:

getAdditionalPreKubernetesResources: Devs should return resources in here when they want to create resources before pod creating
getAdditionalKubernetesResources: Devs should return resources in here when they can accept the resources create after pod, and spark will also help to refresh owner reference after resources created, that means if any resource is expected to refresh the owner pod reference, it should be added it here, even if it already in the getAdditionalPreKubernetesResources as same.

Why are the changes needed?

We need to setup K8S resources or extension resources before driver pod creating, and then create pod, after the pod created, the owner refernce would be owner to this Pod.

These pre-resources are usually necessary in pod creation and scheduling, they should be ready before pod creation, should be deleted when user delete pod, the lifecycle of these resource is same with pod, such as the customized batch scheduler scenario, the user need to create the addtional pre resources (such as pod group in Volcano) to help the specific spark job complete batch scheduling.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

Yikun · 2021-11-15T09:36:53Z

cc @dongjoon-hyun @holdenk

SparkQA · 2021-11-15T09:46:33Z

Test build #145228 has finished for PR 34599 at commit 68b5cf3.

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

SparkQA · 2021-11-15T10:16:21Z

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

SparkQA · 2021-11-15T10:55:42Z

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

SparkQA · 2021-11-15T11:58:37Z

Test build #145231 has finished for PR 34599 at commit f8f2dd4.

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

SparkQA · 2021-11-15T12:40:31Z

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

SparkQA · 2021-11-15T13:27:09Z

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

Yikun · 2021-11-17T07:23:52Z

Intergration test failed is unrelated, ready for review.

holdenk · 2021-11-17T21:03:55Z

Cool, I'll take a look. I've also got a PR to cleanup the integration test failures ( #34636 )

holdenk

Some minor nits, but LGTM if @dongjoon-hyun doesn't have any concerns.

...s/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

Yikun · 2021-11-18T02:09:51Z

@holdenk Thanks for your review!

SparkQA · 2021-11-18T02:53:08Z

Test build #145362 has finished for PR 34599 at commit 7d7d98d.

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

SparkQA · 2021-11-18T03:26:31Z

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

SparkQA · 2021-11-18T04:10:20Z

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

Yikun · 2021-11-19T06:29:52Z

@dongjoon-hyun Would you mind giving some suggestion on this? Sorry to ping wrong person in #34599 (comment) . : )

dongjoon-hyun · 2021-12-01T07:05:55Z

Sorry for being late. I'll review now.

...ce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala

...s/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

dongjoon-hyun

I have two comments.

In theory, this feature looks unsafe because there is a chance to leak the pre-populated resources because they have no owner yet.
Please minimize the change. For example, we had better keep the existing resource management code for the non-pre-polulated code.

Yikun · 2021-12-01T14:16:59Z

In theory, this feature looks unsafe because there is a chance to leak the pre-populated resources because they have no owner yet.

Yes, that's a good point, and the cleanup operations of pre-populated resource is required, will add cleaup pre-populated resource logic

Please minimize the change. For example, we had better keep the existing resource management code for the non-pre-polulated code.

Thanks for your suggestion, will address in next commit.

SparkQA · 2021-12-01T14:43:31Z

Test build #145816 has finished for PR 34599 at commit a48e2fd.

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

SparkQA · 2021-12-01T15:16:36Z

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

SparkQA · 2021-12-01T16:18:29Z

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

SparkQA · 2021-12-16T09:01:52Z

Test build #146272 has finished for PR 34599 at commit 7e3c0ae.

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

Yikun · 2021-12-16T09:02:12Z

This PR doesn't have a test case. Could you add a test coverage for your code path please?

Sure, thanks for reminder, I have added the testcase in latest commit.

could you elaborate why we need to create some resource before driver pod creation?

These resources are usually necessary in pod creation and scheduling, they should be ready before pod creation, should be deleted when user delete pod, the lifecycle of these resource is same with pod, there are some scenarioes:

Extension custom resources, they are usually created in k8s or cncf extension compoent, such as the customized batch scheduler scenario, the user need to create the addtional pre resources (such as pod group in Volcano) to help the specific spark job complete batch scheduling.
The temporary priority class, the priority class is created in custom feature step for specific job and also is epected to be deleted when job deleted.

@dongjoon-hyun Much thank for your patient and guidance.

SparkQA · 2021-12-16T09:37:18Z

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

SparkQA · 2021-12-16T10:43:27Z

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

SparkQA · 2021-12-16T10:45:19Z

Test build #146280 has finished for PR 34599 at commit 5aba6eb.

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

SparkQA · 2021-12-16T11:25:33Z

Kubernetes integration test unable to build dist.

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

Yikun · 2021-12-16T11:31:28Z

Retest this please

SparkQA · 2021-12-16T11:48:12Z

Test build #146285 has finished for PR 34599 at commit 5aba6eb.

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

SparkQA · 2021-12-16T11:54:20Z

Test build #146286 has finished for PR 34599 at commit 5aba6eb.

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

SparkQA · 2021-12-16T13:23:51Z

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

SparkQA · 2021-12-16T13:59:53Z

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

dongjoon-hyun · 2021-12-16T17:26:40Z

Thank you for update, @Yikun !

SparkQA · 2021-12-16T17:45:08Z

Test build #146294 has finished for PR 34599 at commit ebbfea6.

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

dongjoon-hyun · 2021-12-16T17:49:58Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+      kubernetesClient.resourceList(preKubernetesResources: _*).createOrReplace()
+    } catch {
+      case NonFatal(e) =>
+        kubernetesClient.resourceList(preKubernetesResources: _*).delete()


Since this is the first error causing Job failure, could you add a user-friendly error message with logError?

Sure, how about:

logError("Please check \"kubectl auth can-i create [resource]\" first." + " It should be yes. And please also check your feature step implementation.")

dongjoon-hyun · 2021-12-16T17:52:26Z

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala

+    } catch {
+      case NonFatal(e) =>
+        kubernetesClient.pods().delete(createdDriverPod)
+        kubernetesClient.resourceList(preKubernetesResources: _*).delete()


What happens when the previous line deletes some of the resources of preKubernetesResources? For the non-existing resource, delete() API is okay?

What happens when the previous line deletes some of the resources of preKubernetesResources? For the non-existing resource, delete() API is okay?

Yes, it's okay, false return in here instead of exception rasing. We could find answer in kubernetes-client delete implementation and resource list delete test.

I also write a simple test to delete non-existing crd in real env, it passed as expected.

test("Deleting nonExisting crds") { val crd = new CustomResourceDefinitionBuilder() .withNewMetadata() .withName("nonExisting1") .endMetadata() .build() val crd2 = new CustomResourceDefinitionBuilder() .withNewMetadata() .withName("nonExisting") .endMetadata() .build() val crds = Seq(crd, crd2) val client = new DefaultKubernetesClient("https://127.0.0.1:52878") assert(client.inNamespace("default").resourceList(crd).delete() === false) assert(client.inNamespace("default").resourceList(crds: _*).delete() == false) }

...managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala

SparkQA · 2021-12-16T18:09:34Z

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

SparkQA · 2021-12-16T19:13:05Z

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

SparkQA · 2021-12-17T03:47:14Z

Test build #146312 has finished for PR 34599 at commit 2613a92.

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

Yikun · 2021-12-17T03:52:24Z

@dongjoon-hyun Thanks for your review, addressed your comments in latest commit.

SparkQA · 2021-12-17T04:35:46Z

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

SparkQA · 2021-12-17T05:35:22Z

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

dongjoon-hyun

+1, LGTM. Thank you for your patience, @Yikun and @holdenk .
Merged to master for Apache Spark 3.3.

Yikun · 2021-12-18T07:38:13Z

@dongjoon-hyun Much thanks for your help!

github-actions bot added the KUBERNETES label Nov 15, 2021

Yikun mentioned this pull request Nov 15, 2021

[SPARK-36061][K8S] Add support for PodGroup #34456

Closed

Yikun force-pushed the SPARK-37331 branch from 68b5cf3 to f8f2dd4 Compare November 15, 2021 11:40

holdenk approved these changes Nov 17, 2021

View reviewed changes

dongjoon-hyun reviewed Dec 1, 2021

View reviewed changes

...ce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Dec 1, 2021

View reviewed changes

...ce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Dec 1, 2021

View reviewed changes

...s/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Dec 1, 2021

View reviewed changes

...tes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala Outdated Show resolved Hide resolved

dongjoon-hyun requested changes Dec 1, 2021

View reviewed changes

Yikun force-pushed the SPARK-37331 branch from 7d7d98d to a48e2fd Compare December 1, 2021 14:23

Yikun force-pushed the SPARK-37331 branch from 7a9cc33 to 9b14667 Compare December 2, 2021 13:21

Yikun force-pushed the SPARK-37331 branch from 7e3c0ae to 5aba6eb Compare December 16, 2021 10:20

Yikun added 2 commits December 17, 2021 01:13

Add the ability to creating resources before driver pod

2f4d6e9

Address comments and add test cases

ebbfea6

Yikun force-pushed the SPARK-37331 branch from 5aba6eb to ebbfea6 Compare December 16, 2021 17:14

dongjoon-hyun reviewed Dec 16, 2021

View reviewed changes

...managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala Outdated Show resolved Hide resolved

Address nits

2613a92

dongjoon-hyun approved these changes Dec 18, 2021

View reviewed changes

dongjoon-hyun closed this in 1922798 Dec 18, 2021

[SPARK-37331][K8S] Add the ability to create resources before driverPod creating #34599

[SPARK-37331][K8S] Add the ability to create resources before driverPod creating #34599

Uh oh!

Conversation

Yikun commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Yikun commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

SparkQA commented Nov 15, 2021

Uh oh!

Yikun commented Nov 17, 2021

Uh oh!

holdenk commented Nov 17, 2021

Uh oh!

holdenk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yikun commented Nov 18, 2021

Uh oh!

SparkQA commented Nov 18, 2021

Uh oh!

SparkQA commented Nov 18, 2021

Uh oh!

SparkQA commented Nov 18, 2021

Uh oh!

Yikun commented Nov 19, 2021

Uh oh!

dongjoon-hyun commented Dec 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Yikun commented Dec 1, 2021

Uh oh!

SparkQA commented Dec 1, 2021

Uh oh!

SparkQA commented Dec 1, 2021

Uh oh!

SparkQA commented Dec 1, 2021

Uh oh!

SparkQA commented Dec 16, 2021

Uh oh!

Yikun commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 16, 2021

Uh oh!

SparkQA commented Dec 16, 2021

Uh oh!

SparkQA commented Dec 16, 2021

Uh oh!

SparkQA commented Dec 16, 2021

Uh oh!

Yikun commented Dec 16, 2021

Uh oh!

SparkQA commented Dec 16, 2021

Uh oh!

Yikun commented Nov 15, 2021 •

edited

Loading

Yikun commented Nov 15, 2021 •

edited

Loading

Yikun commented Dec 16, 2021 •

edited

Loading

dongjoon-hyun Dec 16, 2021 •

edited

Loading

Yikun Dec 17, 2021 •

edited

Loading