Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

hehuiyuan
Copy link

@hehuiyuan hehuiyuan commented Mar 26, 2019

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.

@hehuiyuan hehuiyuan changed the title [SPARK-27258][K8S] The first character of Service's name is digit,which does not match regular expression [a-z]([-a-z0-9]*[a-z0-9])?) [SPARK-27258][K8S] If the first character of Service's name is digit,which does not match regular expression ^[a-z]([-a-z0-9]*[a-z0-9])?),this situation needs to be dealt Mar 26, 2019
@hehuiyuan hehuiyuan changed the title [SPARK-27258][K8S] If the first character of Service's name is digit,which does not match regular expression ^[a-z]([-a-z0-9]*[a-z0-9])?),this situation needs to be dealt [SPARK-27258][K8S] If the first character of Service's name is not a-z,which does not match regular expression ^[a-z]([-a-z0-9]*[a-z0-9])?),this situation needs to be dealt Mar 26, 2019
@hehuiyuan hehuiyuan changed the title [SPARK-27258][K8S] If the first character of Service's name is not a-z,which does not match regular expression ^[a-z]([-a-z0-9]*[a-z0-9])?),this situation needs to be dealt [SPARK-27258][K8S]Deal with the Service name that does not match regular expression ^[a-z]([-a-z0-9]*[a-z0-9])?) Mar 26, 2019
@vanzin
Copy link
Contributor

vanzin commented Mar 26, 2019

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 hehuiyuan changed the title [SPARK-27258][K8S]Deal with the Service name that does not match regular expression ^[a-z]([-a-z0-9]*[a-z0-9])?) [SPARK-27258][K8S]Deal with the k8s resource names that don't match their own regular expression Mar 28, 2019
@hehuiyuan
Copy link
Author

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
Copy link

SparkQA commented Mar 31, 2019

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
Copy link
Contributor

skonto commented May 13, 2019

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
Copy link
Author

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
Copy link
Contributor

skonto commented May 14, 2019

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
Copy link
Author

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
Copy link
Author

hehuiyuan commented May 14, 2019 via email

@skonto
Copy link
Contributor

skonto commented May 14, 2019

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

@hehuiyuan
Copy link
Author

hehuiyuan commented May 16, 2019

@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
Copy link
Contributor

skonto commented Jun 3, 2019

@hehuiyuan any progress on this one?

@hehuiyuan
Copy link
Author

hehuiyuan commented Jun 4, 2019

@hehuiyuan any progress on this one?

resourcename
standardpod

Is this your meaning ?

@skonto
Copy link
Contributor

skonto commented Jun 7, 2019

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." +
Copy link
Member

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"

Copy link
Author

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.

Copy link
Author

@hehuiyuan hehuiyuan Jun 10, 2019

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")

Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants