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

[hehuiyuan]

What changes were proposed in this pull request?

The regex of Pod is ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*.
The regex of Servive is ^[a-z]([-a-z0-9]*[a-z0-9])?.
The regex of Secret is ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-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.

[vanzin]

Instead of adding more ad-hoc tests, I think it's better to just check if the preferred name is valid using com.google.common.net.InternetDomainName.isValid. It seems to do a better job than the checks that were put in this code originally.

[hehuiyuan]

Instead of adding more ad-hoc tests, I think it's better to just check if the preferred name is valid using com.google.common.net.InternetDomainName.isValid. It seems to do a better job than the checks that were put in this code originally.

.replaceAll("^-", "") ,we can use the code to avoid that the first character of all kubernetes resources(Pod ,Service,Secret,etc) name is '-'.
Then Character.isLetter(preferredServiceName.charAt(0) is used to checking the Service name

[SparkQA]

Test build #4671 has finished for PR 24219 at commit 6af6557.

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

[skonto]

There are some public utility methods that could be used offered by the fabric8io client for this purprose:
https://github.com/fabric8io/kubernetes-client/blob/v4.2.2/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/KubernetesResourceUtil.java
For example io.fabric8.kubernetes.client.utils.KubernetesResourceUtil.{sanitizeName(),isValidName()} methods etc.
This way code is cleaner and we dont need to maintain changes as K8s evolves.

[hehuiyuan]

There are some public utility methods that could be used offered by the fabric8io client for this purprose:
https://github.com/fabric8io/kubernetes-client/blob/v4.2.2/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/KubernetesResourceUtil.java
For example io.fabric8.kubernetes.client.utils.KubernetesResourceUtil.{sanitizeName(),isValidName()} methods etc.
This way code is cleaner and we dont need to maintain changes as K8s evolves.

Hi, that sounds good. This class supports many methods. But I don't found the method that processes the first character of resourceName is - and checks the name of Service in KubernetesResourceUtil .

Service is different from the others.

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

[skonto]

sanitize() method does not fix the dash issue as a first character I agree, btw this happened before because of truncation, but the sanitize method could be improved or re-use the logic for the executor pod names here and create one re-usable method. Pod names for example can be set in different ways eg via spark.kubernetes.driver.pod.name, scenario currently not covered. Code should be refactored isolating the logic in new methods like getPodName, getServiceName and so on, so validation and sanitization/truncation can happen in one place for the final string. I general I dont think these details should be managed by user code, they should be offered by the client (we could create a PR against fabric8io if makes sense but anyway).

[hehuiyuan]

sanitize() method does not fix the dash issue as a first character I agree, btw this happened before because of truncation, but the sanitize method could be improved or re-use the logic for the executor pod names here and create one re-usable method. Pod names for example can be set in different ways eg via spark.kubernetes.driver.pod.name, scenario currently not covered. Code should be refactored isolating the logic in new methods like getPodName, getServiceName and so on, so validation and sanitization/truncation can happen in one place for the final string. I general I dont think these details should be managed by user code, they should be offered by the client (we could create a PR against fabric8io if makes sense but anyway).

That sounds good.

[hehuiyuan]

service name is a-z ，which is not a-z0-9

…

---Original--- From: "Stavros Kontopoulos"<notifications@github.com> Date: Tue, May 14, 2019 17:06 PM To: "apache/spark"<spark@noreply.github.com>; Cc: "Author"<author@noreply.github.com>;"hehuiyuan"<471627698@qq.com>; Subject: Re: [apache/spark] [SPARK-27258][K8S]Deal with the k8s resource names that don't match their own regular expression (#24219) isValidName() method uses "a-z0-9?" which is in compliance with the k8s codebase for DNS-1123 labels and works for service names. Sanitize does not fix the dash issue as a first character I agree, btw this happened before because of truncation, but the sanitize method could be improved or re-use the logic for the executor pod names here. So its not just dash. I would refactor the code and use only one method to sanitize resourceNamePrefix because resourceNamePrefix can be used by the executor pods for the hostname thing anyway. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[skonto]

@hehuiyuan I removed that comment :) , yes indeed no digits.

[hehuiyuan]

@hehuiyuan I removed that comment :) , yes indeed no digits.

Hi, I have extracted the methods to get the name of the resources for Service / Pod / Secret / ConfigMap etc. But this PR doesn't found my branch.

[skonto]

@hehuiyuan any progress on this one?

[hehuiyuan]

@hehuiyuan any progress on this one?

Is this your meaning ?

[skonto]

I mean are all the comments addressed? I think vanzin has one above. @srowen should this be merged?

[srowen]

support -> supported.
You could just say "or is not a valid service name"

[hehuiyuan]

OK! But there are some conflicts at present.

[hehuiyuan]

@srowen Hi, i have a question that is how to deal with the prefix of user-defined pod.

It handles the default prefix of podName in this PR .
Is there any need for further treatment or it's necessary to explain it for KUBERNETES_EXECUTOR_POD_NAME_PREFIX and KUBERNETES_DRIVER_POD_NAME in the doc.

overridevalresourceNamePrefix:String= {
get(KUBERNETES_EXECUTOR_POD_NAME_PREFIX).getOrElse(
KubernetesConf.getResourceNamePrefix(appName))
}
privatevaldriverPodName= conf
.get(KUBERNETES_DRIVER_POD_NAME)
.getOrElse(s"${conf.resourceNamePrefix}-driver")

[srowen]

That I don't know ... @skonto?