[SPARK-27258][K8S]Deal with the k8s resource names that don't match their own regular expression #24839

hehuiyuan · 2019-06-11T07:18:36Z

What changes were proposed in this pull request?

The regex of Pod is ^a-z0-9?(.a-z0-9?).
The regex of Servive is ^a-z?.
The regex of Secret is ^a-z0-9?(.a-z0-9?).

If --name=-min, the first character is '-' ,which does not satisfy the regex for Pod ,Service,Secret.
if --name=1min, the first character is digit, which does not satisfy the regex for Service.

Previous Conflict PR

…r ServiceName

hehuiyuan · 2019-06-11T07:20:13Z

@srowen @skonto Hi, let us solve this problem.
Can this be merged now?

skonto · 2019-06-11T10:24:23Z

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

-      s"too long (must be <= $MAX_SERVICE_NAME_LENGTH characters). Falling back to use " +
-      s"$shorterServiceName as the driver service's name.")
+      s"too long (must be <= $MAX_SERVICE_NAME_LENGTH characters) or is not a valid service name." +
+      s"Falling back to use $shorterServiceName as the driver service's name.")


it should be falling back to some default name. It is not necessarily the shorter if the preferred is invalid.

This is only a method for dealing with the invalid service name. If the name is invalid and we don't deal with it, the job only has a driver pod.

I am referring to the warning message could be made more specific and also the name of the variable shorterServiceName . Since it may be invalid and not shorter in some cases I would replace shorterServiceName with defaultServiceName. Anyway this is nit.

Oh，I got it!

skonto · 2019-06-11T10:35:02Z

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

  private val preferredServiceName = s"${kubernetesConf.resourceNamePrefix}$DRIVER_SVC_POSTFIX"
-  private val resolvedServiceName = if (preferredServiceName.length <= MAX_SERVICE_NAME_LENGTH) {
+  private val resolvedServiceName = if (preferredServiceName.length <= MAX_SERVICE_NAME_LENGTH
+    && Character.isLetter(preferredServiceName.charAt(0))) {


Is the svc name valid in all cases according to com.google.common.net.InternetDomainName.isValid as mentioned in the other PR? It seems that the svc name which is up to 63 chars and has no dots in it matches a domain label (https://www.ietf.org/rfc/rfc3490.txt) so that method will cover the validation.

Is the svc name valid in all cases according to com.google.common.net.InternetDomainName.isValid as mentioned in the other PR? As it is now it seems valid according to the spec: https://en.wikipedia.org/wiki/Domain_name.

Now, fabric8 API has some limitations，

public class Service implements HasMetadata { @NotNull @JsonProperty("apiVersion") private String apiVersion = "v1"; @NotNull @JsonProperty("kind") private String kind = "Service"; @JsonProperty("metadata") @Valid @CheckObjectMeta( regexp = "^[a-z]([-a-z0-9]*[a-z0-9])?$", max = 63 )

@CheckObjectMeta( regexp = "^[a-z]([-a-z0-9]*[a-z0-9])?$", max = 63 )

Yeah I saw that when digging into it. It should be ok from what I read: https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/ "Assume a Service named foo in the Kubernetes namespace bar. A Pod running in namespace bar can look up this service by simply doing a DNS query for foo. A Pod running in namespace quux can look up this service by doing a DNS query for foo.bar." so it matches the domain label, that is why there are these limitations. @liyinan926 can you verify this?

I don't understand the purpose of Character.isLetter here, we've already filtered this down to only allow [a-z0-9]

skonto · 2019-06-11T10:57:45Z

LGTM wrt validation part.

hehuiyuan · 2019-06-14T04:56:00Z

LGTM wrt validation part.

Hi , how is progress now?

In addition, is it planned to provide hostNetwork and dnsPolicy for PodSpec?

srowen · 2019-06-14T12:27:49Z

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

      .replaceAll("\\.", "-")
      .replaceAll("[^a-z0-9\\-]", "")
      .replaceAll("-+", "-")
+      .replaceAll("^-", "")


Is this still needed then, if you're simply validating and replacing the name if it starts with "-"?

Is this still needed then, if you're simply validating and replacing the name if it starts with "-"?

The '^-' is used for all kubernetes resources. If the resource name starts with "-",we replace it with "" ,which can run normally .

The service name that starts with 0-9 is replace.

If we need to match the fabric8 regex bellow, shouldn't this be .replaceAll("^[^a-z]", "") to replace any leading/first non alpha character?

srowen

I don't know enough to evaluate; @skonto does this look reasonable at this point?

hehuiyuan · 2019-06-21T03:07:59Z

I don't know enough to evaluate; @skonto does this look reasonable at this point?

@srowen @skonto ?

SparkQA · 2019-07-17T13:29:52Z

Test build #4823 has finished for PR 24839 at commit 7ed72cb.

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

hehuiyuan · 2019-07-18T07:14:38Z

？

holdenk · 2019-09-17T01:16:55Z

Jenkins ok to test

holdenk

Thanks for working on this, I've got a question about two of the checks I'd like to understand more.

holdenk · 2019-09-17T01:24:14Z

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

  private val preferredServiceName = s"${kubernetesConf.resourceNamePrefix}$DRIVER_SVC_POSTFIX"
-  private val resolvedServiceName = if (preferredServiceName.length <= MAX_SERVICE_NAME_LENGTH) {
+  private val resolvedServiceName = if (preferredServiceName.length <= MAX_SERVICE_NAME_LENGTH
+    && Character.isLetter(preferredServiceName.charAt(0))) {


I don't understand the purpose of Character.isLetter here, we've already filtered this down to only allow [a-z0-9]

holdenk · 2019-09-17T01:29:53Z

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

      .replaceAll("\\.", "-")
      .replaceAll("[^a-z0-9\\-]", "")
      .replaceAll("-+", "-")
+      .replaceAll("^-", "")


If we need to match the fabric8 regex bellow, shouldn't this be .replaceAll("^[^a-z]", "") to replace any leading/first non alpha character?

SparkQA · 2019-09-17T01:36:04Z

Test build #110712 has finished for PR 24839 at commit 7ed72cb.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-09-17T01:38:18Z

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

hehuiyuan · 2019-09-17T01:47:26Z

@holdenk Hi,
#24839 (comment)
this resourceNamePrefix is used for Pod and Secret and Service resource . But only Service resource is not used when the first character is digit.

#24839 (comment)

Character.isLetter is only used for Service .

SparkQA · 2019-09-17T01:54:48Z

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

holdenk · 2019-09-20T16:33:59Z

Hi @hehuiyuan I don't completely understand your response. Let me write my understanding and you can point out where we're not matching up:

So the resourceNamePrefix is (with the change) gauranteed to start with something in [a-z0-9].
The point of the Character.isLetter is to filter out anything that starts with a number for service because only service has that requirement. While Character.isLetter accepts more than just [a-z] the other things it accepts has already been filtered out so in practice this is the same as doing a regex check for ^[a-z].*

If that's the case maybe just add a comment about that, because it takes a bit of tracing through the code to build up that context.

hehuiyuan · 2019-09-27T10:47:33Z

Hi @hehuiyuan I don't completely understand your response. Let me write my understanding and you can point out where we're not matching up:

So the resourceNamePrefix is (with the change) gauranteed to start with something in [a-z0-9].
The point of the Character.isLetter is to filter out anything that starts with a number for service because only service has that requirement. While Character.isLetter accepts more than just [a-z] the other things it accepts has already been filtered out so in practice this is the same as doing a regex check for ^[a-z].*

If that's the case maybe just add a comment about that, because it takes a bit of tracing through the code to build up that context.

Hi, it is ok. All resources (Pod /Service / Secret ..) will start with the first character is [a-z].

The changes I've made are more detailed.

First, all resources (Pod \Service ..) is not supported that the first character of name is -.
Then the first character of Pod's name can be digit.

github-actions · 2020-01-06T00:07:14Z

We're closing this PR because it hasn't been updated in a while.
This isn't a judgement on the merit of the PR in any way. It's just
a way of keeping the PR queue manageable.

If you'd like to revive this PR, please reopen it!

deal with the first character that is '-' for PodName and is digit fo…

7ed72cb

…r ServiceName

skonto reviewed Jun 11, 2019

View reviewed changes

dongjoon-hyun added the IMPROVEMENT label Jun 12, 2019

srowen reviewed Jun 14, 2019

View reviewed changes

dongjoon-hyun added KUBERNETES and removed IMPROVEMENT labels Jun 14, 2019

srowen reviewed Jun 19, 2019

View reviewed changes

rvesse approved these changes Jul 17, 2019

View reviewed changes

holdenk reviewed Sep 17, 2019

View reviewed changes

github-actions bot added the Stale label Jan 6, 2020

srowen closed this Jan 6, 2020

[SPARK-27258][K8S]Deal with the k8s resource names that don't match their own regular expression #24839

[SPARK-27258][K8S]Deal with the k8s resource names that don't match their own regular expression #24839

Uh oh!

Conversation

hehuiyuan commented Jun 11, 2019

What changes were proposed in this pull request?

Uh oh!

hehuiyuan commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jun 11, 2019 • 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!

hehuiyuan Jun 11, 2019 • 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!

skonto Jun 11, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

skonto Jun 11, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

skonto commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hehuiyuan commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hehuiyuan Jun 15, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

hehuiyuan commented Jun 21, 2019

Uh oh!

SparkQA commented Jul 17, 2019

Uh oh!

hehuiyuan commented Jul 18, 2019

Uh oh!

holdenk commented Sep 17, 2019

Uh oh!

holdenk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

hehuiyuan commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

holdenk commented Sep 20, 2019

Uh oh!

hehuiyuan commented Sep 27, 2019

hehuiyuan commented Jun 11, 2019 •

edited

Loading

skonto Jun 11, 2019 •

edited

Loading

hehuiyuan Jun 11, 2019 •

edited

Loading

skonto Jun 11, 2019 •

edited

Loading

skonto Jun 11, 2019 •

edited

Loading

skonto commented Jun 11, 2019 •

edited

Loading

hehuiyuan commented Jun 14, 2019 •

edited

Loading

hehuiyuan Jun 15, 2019 •

edited

Loading

hehuiyuan commented Sep 17, 2019 •

edited

Loading