-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-27258][K8S]Deal with the k8s resource names that don't match their own regular expression #24219
Conversation
...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverServiceFeatureStep.scala
Outdated
Show resolved
Hide resolved
...etes/core/src/main/scala/org/apache/spark/deploy/k8s/features/DriverServiceFeatureStep.scala
Outdated
Show resolved
Hide resolved
Instead of adding more ad-hoc tests, I think it's better to just check if the preferred name is valid using |
...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
Show resolved
Hide resolved
|
Test build #4671 has finished for PR 24219 at commit
|
There are some public utility methods that could be used offered by the fabric8io client for this purprose: |
Hi, that sounds good. This class supports many methods. But I don't found the method that processes the first character of resourceName is Service is different from the others.
|
|
That sounds good. |
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.
|
@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. |
@hehuiyuan any progress on this one? |
Is this your meaning ? |
I mean are all the comments addressed? I think vanzin has one above. @srowen should this be merged? |
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) " + | ||
s"or the first character of $preferredServiceName is not letter which is not support." + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
support -> supported.
You could just say "or is not a valid service name"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! But there are some conflicts at present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
override val resourceNamePrefix: String = {
get(KUBERNETES_EXECUTOR_POD_NAME_PREFIX).getOrElse(
KubernetesConf.getResourceNamePrefix(appName))
}
private val driverPodName = conf
.get(KUBERNETES_DRIVER_POD_NAME)
.getOrElse(s"${conf.resourceNamePrefix}-driver")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That I don't know ... @skonto?
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.