[SPARK-29233][K8S] Add regex expression checks for executorEnv… #25920

merrily01 · 2019-09-24T19:09:23Z

What changes were proposed in this pull request?

In kubernetes, there are some naming regular expression requirements and restrictions on environment variable names, such as:

In kubernetes version release-1.7 and earlier, the naming rules of pod environment variable names should meet the requirements of regular expressions: [A-Za-z_] [A-Za-z0-9_]*
In kubernetes version release-1.8 and later, the naming rules of pod environment variable names should meet the requirements of regular expressions: [-. _ A-ZA-Z][-. _ A-ZA-Z0-9].*

However, in spark on k8s mode, spark should add restrictions on environmental variable names when creating executorEnv.

In addition, we need to use regular expressions adapted to the high version of k8s to increase the restrictions on the names of environmental variables.

Otherwise, the pod will not be created properly and the spark application will be suspended.

To solve the problem above, a regular validation to executorEnv is added and committed.

Why are the changes needed?

If no validation rules are added, the environment variable names that don't meet the requirements will cause the pod to not be created properly and the application will be suspended.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add unit tests and manually run.

dongjoon-hyun

Hi, @merrily01 . Thank you for making this PR. Can we have additional integration test for the following?

Otherwise, it will lead to the problem that the pod can not be created normally and the tasks will be suspended.

dongjoon-hyun

BTW, did you run this PR locally, @merrily01 ? This doesn't compile yet.

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

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

merrily01 · 2019-09-25T13:04:17Z

@srowen @dongjoon-hyun
Hi~ Thank you for your quick review.
I'm a little confused.
For the Spark community, in the case of different checking rules for low and high versions of k8s, should I base my checking rules on high versions or on stricter rules?
Should I take the high version of k8s as my personal feeling?

If so, I might need to modify the rules and code.

srowen

What versions would work, not work, with or without this change?

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

merrily01 · 2019-09-25T18:49:16Z

What versions would work, not work, with or without this change?

@srowen

In kubernetes version release-1.7 and earlier, the naming rules of pod environment variable names should meet the requirements of regular expressions: [A-Za-z_] [A-Za-z0-9_]*
In kubernetes version release-1.8 and later, the naming rules of pod environment variable names should meet the requirements of regular expressions: [-. _ A-ZA-Z][-. _ A-ZA-Z0-9].*

This change was submitted in commit 604dfb3 of k8s release-1.8 version.

This current code will take effect for k8s release-1.8 and later versions.

srowen · 2019-09-25T19:04:41Z

How about just using a more lenient regex then, that would allow both? this is more of a preemptive check than something that must exactly match?

merrily01 · 2019-09-25T19:20:51Z

That's a good idea！I'll try it later.
Thank you very much for your careful review.(^__^) @srowen

dongjoon-hyun · 2019-09-25T22:32:22Z

ok to test

SparkQA · 2019-09-25T22:49:19Z

Test build #111370 has finished for PR 25920 at commit 562b367.

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

SparkQA · 2019-09-25T23:26:28Z

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

SparkQA · 2019-09-25T23:42:17Z

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

merrily01 · 2019-09-26T05:21:50Z

Hey~ @srowen

I was so sleepy last night that my mind was a little unclear. After my careful consideration, I have something to tell you:

This validation is necessary, otherwise an executor environment variable name that does not conform to the specification will lead to pod creation errors.
As you know, this check rule is different in high and low versions of k8s.(The low version is stricter than the high version)
That means there will be no problem for low to high versions of k8s, but in the case of high to low versions，k8s itself may also have this problem.
Compatibility can be achieved with lower version regex (Stricter), but this is contrary to the original intention of the high version of k8s to make this change.
I prefer the validation here to be consistent with the high version behavior of k8s, rather than considering this as a compatibility issue.
What do you think if I declare it in the notes and log message?For example, as follows：

SparkQA · 2019-09-26T11:09:03Z

Test build #111421 has finished for PR 25920 at commit 81a30fc.

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

merrily01 · 2019-09-26T15:21:59Z

Wonder why it failed？@AmplabJenkins

srowen · 2019-09-26T15:27:27Z

Unless there's an easy way to know what version of K8S will be in use, I think you have to keep the looser check here. It may let through invalid configs, and something will fail, but you get a failure either way. This change just tries to bring it forward.

AmplabJenkins is a bot. Click the link to see why the build failed. It may be unrelated though: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/16460/console

merrily01 · 2019-09-27T21:51:42Z

Sorry for misoperation and reopen. Now using a looser check，could you please kindly review? i m so sorry . @srowen @dongjoon-hyun

srowen

Looks OK pending tests

SparkQA · 2019-09-28T15:38:51Z

Test build #4888 has finished for PR 25920 at commit c64fbd2.

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

merrily01 · 2019-09-28T21:09:32Z

Thank you so much @srowen and @dongjoon-hyun please recheck ，i have added addition test and it can be tested and compiled. thanks a lot !

srowen · 2019-09-29T14:57:09Z

I'm going to try retriggering the K8S tests by closing and reopening

dongjoon-hyun · 2019-09-29T22:33:03Z

Retest this please.

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

dongjoon-hyun · 2019-09-29T22:42:52Z

...anagers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesConfSuite.scala

+  private val EXECUTOR_ENV_VARS = Map(
+    "spark.executorEnv.1executorEnvVars1/var1" -> "executorEnvVars1",
+    "spark.executorEnv.executorEnvVars2*var2" -> "executorEnvVars2",
+    "spark.executorEnv.executorEnvVars3_var3" -> "executorEnvVars3")


~~Shall we move this into the test case? Do we have a plan to reuse this in another test case?~~
Never mind. This existing one looks good, too.

BTW, please add another test case like "spark.executorEnv.4executorEnvVars4/var4" -> "executorEnvVars4" in order to show clearly that this is only key-constraint.

@dongjoon-hyun "spark.executorEnv.4executorEnvVars4/var4" -> "executorEnvVars4" is the same as "spark.executorEnv.1executorEnvVars1/var1" -> "executorEnvVars1" . BTW, I added two test cases, which I think will prove that this is only key-constraint.

dongjoon-hyun

Thanks, @merrily01 . Look fine except a few minor comments.

SparkQA · 2019-09-29T22:53:07Z

Test build #111577 has finished for PR 25920 at commit c64fbd2.

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

SparkQA · 2019-09-29T23:08:10Z

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

SparkQA · 2019-09-29T23:24:13Z

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

srowen · 2019-10-03T06:01:05Z

@merrily01 do you want to make these last updates per above?

merrily01 · 2019-10-03T11:32:09Z

@merrily01 do you want to make these last updates per above?
Yes， I will update later today. I am on the way to England.

@srowen @dongjoon-hyun done , thanks a lot !

… in K8S mode In kubernetes, there are some naming regular expression requirements and restrictions on environment variable names, such as: - In kubernetes version release-1.7 and earlier, the naming rules of pod environment variable names should meet the requirements of regular expressions: [[A-Za-z_] [A-Za-z0-9_]*](https://github.com/kubernetes/kubernetes/blob/release-1.7/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L169) - In kubernetes version release-1.8 and later, the naming rules of pod environment variable names should meet the requirements of regular expressions: [[-. _ A-ZA-Z][-. _ A-ZA-Z0-9].*](https://github.com/kubernetes/kubernetes/blob/release-1.8/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L305) However, in spark on k8s mode, spark should add restrictions on environmental variable names when creating executorEnv. In addition, we need to use regular expressions adapted to the high version of k8s to increase the restrictions on the names of environmental variables. Otherwise, the pod will not be created properly and the spark application will be suspended. To solve the problem above, a regular validation to executorEnv is added and committed. Unit tests have been added.

SparkQA · 2019-10-04T14:59:06Z

Test build #4891 has finished for PR 25920 at commit 799d87f.

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

srowen · 2019-10-06T14:41:25Z

Merged to master

merrily01 · 2019-10-12T03:30:59Z

Forget to say thank you.
Thanks a lot @srowen @dongjoon-hyun

dongjoon-hyun reviewed Sep 24, 2019

View reviewed changes

dongjoon-hyun requested changes Sep 24, 2019

View reviewed changes

merrily01 force-pushed the SPARK-29233 branch 2 times, most recently from 1371532 to ca729a4 Compare September 25, 2019 10:43

srowen requested changes Sep 25, 2019

View reviewed changes

dongjoon-hyun added the KUBERNETES label Sep 25, 2019

merrily01 force-pushed the SPARK-29233 branch from ca729a4 to ddeffb6 Compare September 25, 2019 12:32

srowen requested changes Sep 25, 2019

View reviewed changes

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala Show resolved Hide resolved

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

merrily01 force-pushed the SPARK-29233 branch from ddeffb6 to 2200fdd Compare September 25, 2019 18:48

merrily01 force-pushed the SPARK-29233 branch from 2200fdd to 562b367 Compare September 25, 2019 18:52

merrily01 force-pushed the SPARK-29233 branch from 562b367 to 81a30fc Compare September 26, 2019 10:49

merrily01 closed this Sep 27, 2019

merrily01 deleted the SPARK-29233 branch September 27, 2019 21:23

merrily01 reopened this Sep 27, 2019

merrily01 force-pushed the SPARK-29233 branch from 81a30fc to c64fbd2 Compare September 27, 2019 21:43

srowen approved these changes Sep 28, 2019

View reviewed changes

merrily01 requested a review from dongjoon-hyun September 28, 2019 21:03

srowen closed this Sep 29, 2019

srowen reopened this Sep 29, 2019

dongjoon-hyun reviewed Sep 29, 2019

View reviewed changes

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

dongjoon-hyun reviewed Sep 29, 2019

View reviewed changes

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

dongjoon-hyun reviewed Sep 29, 2019

View reviewed changes

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

dongjoon-hyun reviewed Sep 29, 2019

View reviewed changes

dongjoon-hyun requested changes Sep 29, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-29233][KUBERNETES] Add regex expression checks for executorEnv…~~ [SPARK-29233][K8S] Add regex expression checks for executorEnv… Sep 29, 2019

merrily01 force-pushed the SPARK-29233 branch from d839309 to 6858480 Compare October 3, 2019 21:58

merrily01 force-pushed the SPARK-29233 branch from dc90ab0 to 799d87f Compare October 3, 2019 23:43

srowen approved these changes Oct 4, 2019

View reviewed changes

merrily01 requested a review from dongjoon-hyun October 4, 2019 22:40

srowen closed this in 77510c6 Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29233][K8S] Add regex expression checks for executorEnv… #25920

[SPARK-29233][K8S] Add regex expression checks for executorEnv… #25920

merrily01 commented Sep 24, 2019 •

edited

dongjoon-hyun left a comment

dongjoon-hyun left a comment

merrily01 commented Sep 25, 2019

srowen left a comment

merrily01 commented Sep 25, 2019

srowen commented Sep 25, 2019

merrily01 commented Sep 25, 2019

dongjoon-hyun commented Sep 25, 2019

SparkQA commented Sep 25, 2019

SparkQA commented Sep 25, 2019

SparkQA commented Sep 25, 2019

merrily01 commented Sep 26, 2019 •

edited

SparkQA commented Sep 26, 2019

merrily01 commented Sep 26, 2019

srowen commented Sep 26, 2019

merrily01 commented Sep 27, 2019

srowen left a comment

SparkQA commented Sep 28, 2019

merrily01 commented Sep 28, 2019 •

edited

srowen commented Sep 29, 2019

dongjoon-hyun commented Sep 29, 2019

dongjoon-hyun Sep 29, 2019 •

edited

dongjoon-hyun Sep 29, 2019

merrily01 Oct 4, 2019

dongjoon-hyun left a comment

SparkQA commented Sep 29, 2019

SparkQA commented Sep 29, 2019

SparkQA commented Sep 29, 2019

srowen commented Oct 3, 2019

merrily01 commented Oct 3, 2019 •

edited

SparkQA commented Oct 4, 2019

srowen commented Oct 6, 2019

merrily01 commented Oct 12, 2019

[SPARK-29233][K8S] Add regex expression checks for executorEnv… #25920

[SPARK-29233][K8S] Add regex expression checks for executorEnv… #25920

Conversation

merrily01 commented Sep 24, 2019 • edited

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?

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

merrily01 commented Sep 25, 2019

srowen left a comment

Choose a reason for hiding this comment

merrily01 commented Sep 25, 2019

srowen commented Sep 25, 2019

merrily01 commented Sep 25, 2019

dongjoon-hyun commented Sep 25, 2019

SparkQA commented Sep 25, 2019

SparkQA commented Sep 25, 2019

SparkQA commented Sep 25, 2019

merrily01 commented Sep 26, 2019 • edited

SparkQA commented Sep 26, 2019

merrily01 commented Sep 26, 2019

srowen commented Sep 26, 2019

merrily01 commented Sep 27, 2019

srowen left a comment

Choose a reason for hiding this comment

SparkQA commented Sep 28, 2019

merrily01 commented Sep 28, 2019 • edited

srowen commented Sep 29, 2019

dongjoon-hyun commented Sep 29, 2019

dongjoon-hyun Sep 29, 2019 • edited

Choose a reason for hiding this comment

dongjoon-hyun Sep 29, 2019

Choose a reason for hiding this comment

merrily01 Oct 4, 2019

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Sep 29, 2019

SparkQA commented Sep 29, 2019

SparkQA commented Sep 29, 2019

srowen commented Oct 3, 2019

merrily01 commented Oct 3, 2019 • edited

SparkQA commented Oct 4, 2019

srowen commented Oct 6, 2019

merrily01 commented Oct 12, 2019

merrily01 commented Sep 24, 2019 •

edited

merrily01 commented Sep 26, 2019 •

edited

merrily01 commented Sep 28, 2019 •

edited

dongjoon-hyun Sep 29, 2019 •

edited

merrily01 commented Oct 3, 2019 •

edited