[SPARK-37372][K8S] Removing redundant label addition #34646

Yikun · 2021-11-18T12:53:57Z

What changes were proposed in this pull request?

Remove redundant Pod label addtions in driver and executor.

Why are the changes needed?

These labels are already included by conf.labels as preset labels, we don't need do a extra addition.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

UT passed:
Especially:

spark/resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesConfSuite.scala

Lines 157 to 164 in a3886ba

    
           assert(conf.labels === Map( 
        
             SPARK_VERSION_LABEL -> SPARK_VERSION, 
        
             SPARK_EXECUTOR_ID_LABEL -> EXECUTOR_ID, 
        
             SPARK_APP_ID_LABEL -> KubernetesTestConf.APP_ID, 
        
             SPARK_APP_NAME_LABEL -> KubernetesConf.getAppNameLabel(conf.appName), 
        
             SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE, 
        
             SPARK_RESOURCE_PROFILE_ID_LABEL -> DEFAULT_RESOURCE_PROFILE_ID.toString) ++ CUSTOM_LABELS) 
        
           assert(conf.annotations === CUSTOM_ANNOTATIONS)

SparkQA · 2021-11-18T13:13:44Z

Test build #145390 has finished for PR 34646 at commit eb04493.

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

SparkQA · 2021-11-18T14:29:58Z

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

SparkQA · 2021-11-18T15:30:37Z

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

dongjoon-hyun

nit. editition -> addition in the PR title and description, @Yikun ?

...s/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala

dongjoon-hyun

This PR aims to more than a simple removing. We had better make the PR clear about what it's doing in the code.

Yikun · 2021-11-19T06:19:24Z

@dongjoon-hyun Thanks for you patient review, and I rename the title to Removing redundant label addition and refactoring related test case, and update the PR description.

Yikun · 2021-11-29T07:52:44Z

@dongjoon-hyun Would you mind taking a look again? Or I misundertanded your suggestion, it's not enough to update the PR message, I should split this PR to 2 PRs:

Remove the redundant edition, then passed new added the ut "assert(executor.pod.getMetadata.getLabels === conf.labels.asJava)", and existing conf.label ut.
then rename CUSTOM_DRIVER_LABELS?

Or maybe there were some misleading, I clarify in #34646 (comment) , anyway, much thanks for your review.

Yikun · 2021-12-10T08:48:13Z

@dongjoon-hyun Friendly ping...Thanks

Run SparkR on simple dataframe.R example *** FAILED ***
Test decommissioning with dynamic allocation & shuffle cleanups *** FAILED ***

Unrelated failed.

Yikun · 2022-01-14T09:04:52Z

I separate the test refactoring in #35209 to make PR clear.

Yikun · 2022-01-14T09:10:49Z

...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStep.scala

-        .addToLabels(
-          SPARK_APP_NAME_LABEL,
-          KubernetesConf.getAppNameLabel(kubernetesConf.appName)
-        )


FYI, these labels are ready included in KubernetesExecutorConf.labels:

spark/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala

Lines 158 to 165 in 068d53b

override def labels: Map[String, String] = {

val presetLabels = Map(

SPARK_VERSION_LABEL -> SPARK_VERSION,

SPARK_EXECUTOR_ID_LABEL -> executorId,

SPARK_APP_ID_LABEL -> appId,

SPARK_APP_NAME_LABEL -> KubernetesConf.getAppNameLabel(appName),

SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE,

SPARK_RESOURCE_PROFILE_ID_LABEL -> resourceProfileId.toString)

This also addresses the comment in https://github.com/apache/spark/pull/33508/files#r682842336

Yikun · 2022-01-14T09:12:02Z

...rnetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala

      .editOrNewMetadata()
        .withName(driverPodName)
        .addToLabels(conf.labels.asJava)
-        .addToLabels(SPARK_APP_NAME_LABEL, KubernetesConf.getAppNameLabel(conf.appName))


FYI, this label are ready included by KubernetesDriverConf.labels:

spark/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala

Lines 96 to 103 in 068d53b

override def labels: Map[String, String] = {

val presetLabels = Map(

SPARK_VERSION_LABEL -> SPARK_VERSION,

SPARK_APP_ID_LABEL -> appId,

SPARK_APP_NAME_LABEL -> KubernetesConf.getAppNameLabel(appName),

SPARK_ROLE_LABEL -> SPARK_POD_DRIVER_ROLE)

val driverCustomLabels = KubernetesUtils.parsePrefixedKeyValuePairs(

sparkConf, KUBERNETES_DRIVER_LABEL_PREFIX)

dongjoon-hyun

+1, LGTM. Thank you for simplification, @Yikun .
Merged to master for Apache Spark 3.3.

Yikun · 2022-01-15T02:25:16Z

@dongjoon-hyun Thanks!

### What changes were proposed in this pull request? Remove redundant Pod label addtions in driver and executor. ### Why are the changes needed? These labels are already included by conf.labels as preset labels, we don't need do a extra addition. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? UT passed: Especially: https://github.com/apache/spark/blob/a3886ba976469bef0dfafc3da8686a53c5a59d95/resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesConfSuite.scala#L157-L164 Closes apache#34646 from Yikun/SPARK-labels-improve. Authored-by: Yikun Jiang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…r/ExecutorFeatureStepSuite ### What changes were proposed in this pull request? - Rename DRIVER_LABELS to CUSTOM_DRIVER_LABELS, LABELS to CUSTOM_EXECUTORS_LABELS, make their names more clear. - Refactoring on preset labels and add preset labels test. ### Why are the changes needed? There are two type Pod label in current implementations: [preset label](https://github.com/apache/spark/blob/068d53bd5d89c96bf0cdb05d3ec7f2f023cf3875/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala#L158-L165) set by spark and custom label set by user, but there are some mix up in testcase, so this PR just fix it. Also see realted: #34646 (comment) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #35209 from Yikun/SPARK-37908. Authored-by: Yikun Jiang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Yikun mentioned this pull request Nov 18, 2021

[SPARK-36058][K8S] Add support for statefulset APIs in K8s #33508

Closed

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

Yikun marked this pull request as ready for review November 18, 2021 13:39

dongjoon-hyun reviewed Nov 19, 2021

View reviewed changes

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

dongjoon-hyun requested changes Nov 19, 2021

View reviewed changes

Yikun changed the title ~~[SPARK-37372][K8S] Remove redundant Pod label edition~~ [SPARK-37372][K8S] Removing redundant label addition and refactoring related test case Nov 19, 2021

Yikun requested a review from dongjoon-hyun November 19, 2021 07:53

Remove redundant label addition

526b359

Yikun force-pushed the SPARK-labels-improve branch from ad5341d to 526b359 Compare January 14, 2022 08:50

Yikun mentioned this pull request Jan 14, 2022

[SPARK-37908][K8S][TESTS] Refactoring on pod label test in BasicDriver/ExecutorFeatureStepSuite #35209

Closed

Yikun changed the title ~~[SPARK-37372][K8S] Removing redundant label addition and refactoring related test case~~ [SPARK-37372][K8S] Removing redundant label addition Jan 14, 2022

Yikun marked this pull request as draft January 14, 2022 09:07

Yikun commented Jan 14, 2022

View reviewed changes

Yikun marked this pull request as ready for review January 14, 2022 10:24

dongjoon-hyun approved these changes Jan 14, 2022

View reviewed changes

dongjoon-hyun closed this in 6f908de Jan 14, 2022

	assert(conf.labels === Map(
	SPARK_VERSION_LABEL -> SPARK_VERSION,
	SPARK_EXECUTOR_ID_LABEL -> EXECUTOR_ID,
	SPARK_APP_ID_LABEL -> KubernetesTestConf.APP_ID,
	SPARK_APP_NAME_LABEL -> KubernetesConf.getAppNameLabel(conf.appName),
	SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE,
	SPARK_RESOURCE_PROFILE_ID_LABEL -> DEFAULT_RESOURCE_PROFILE_ID.toString) ++ CUSTOM_LABELS)
	assert(conf.annotations === CUSTOM_ANNOTATIONS)

	override def labels: Map[String, String] = {
	val presetLabels = Map(
	SPARK_VERSION_LABEL -> SPARK_VERSION,
	SPARK_EXECUTOR_ID_LABEL -> executorId,
	SPARK_APP_ID_LABEL -> appId,
	SPARK_APP_NAME_LABEL -> KubernetesConf.getAppNameLabel(appName),
	SPARK_ROLE_LABEL -> SPARK_POD_EXECUTOR_ROLE,
	SPARK_RESOURCE_PROFILE_ID_LABEL -> resourceProfileId.toString)

[SPARK-37372][K8S] Removing redundant label addition #34646

[SPARK-37372][K8S] Removing redundant label addition #34646

Uh oh!

Conversation

Yikun commented Nov 18, 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!

SparkQA commented Nov 18, 2021

Uh oh!

SparkQA commented Nov 18, 2021

Uh oh!

SparkQA commented Nov 18, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Yikun commented Jan 14, 2022

Uh oh!

Yikun Jan 14, 2022 • 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!

Yikun Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Yikun commented Jan 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Yikun commented Nov 18, 2021 •

edited

Loading

Yikun commented Nov 19, 2021 •

edited

Loading

Yikun commented Nov 29, 2021 •

edited

Loading

Yikun commented Dec 10, 2021 •

edited

Loading

Yikun Jan 14, 2022 •

edited

Loading