[GitHub] [pulsar] cckellogg commented on a change in pull request #4996: [k8s] convert to valid pod name part with k8s function runtime

[GitBox <gi...@apache.org>]

cckellogg commented on a change in pull request #4996: [k8s] convert to valid pod name part with k8s function runtime
URL: https://github.com/apache/pulsar/pull/4996#discussion_r316775810
 
 

 ##########
 File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/KubernetesRuntime.java
 ##########
 @@ -1011,8 +1011,12 @@ private static String createJobName(Function.FunctionDetails functionDetails) {
                 functionDetails.getName());
     }
 
+    private static String toValidPodName(String ori) {
+        return ori.toLowerCase().replaceAll("[^a-z0-9]", "-");
+    }
+
     private static String createJobName(String tenant, String namespace, String functionName) {
-        return "pf-" + tenant + "-" + namespace + "-" + functionName;
+        return "pf-" + toValidPodName(tenant) + "-" + toValidPodName(namespace) + "-" + toValidPodName(functionName);
 
 Review comment:
   To maintain backward compatibility a hash should only be added in certain cases. Also, the name needs to be predictable because there is a service that gets created and it's how the current implementation communicates with the function pods. 
   
   The logic could be something like this
   
   ```
   private static String createJobName(String tenant, String namespace, String functionName) {
     final String jobName = "pf-" + tenant + "-" + namespace + "-" + functionName;
     final String convertedJobName = toValidPodName(jobName);
     // we have a valid name based on the tenant/namespace/function
     if (jobName.equals.(convertedJobName) {
        return jobName;
     }
     // the fqn of the function contains characters not accept in kubernetes
     // maybe add a hash here to avoid potential naming collisions 
     // whatever gets added to avoid the collision it needs to always be the same for that fqn
     return convertedJobName
   }
   ```
   
   Also, can we add some unit tests for this we don't want to break things for users if they update their cluster.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services