[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #88: [YUNIKORN-54] admission-controller should keep generated ID tight

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

wilfred-s commented on a change in pull request #88: [YUNIKORN-54] admission-controller should keep generated ID tight
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/88#discussion_r398573588
 
 

 ##########
 File path: pkg/plugin/admissioncontrollers/webhook/admission_controller.go
 ##########
 @@ -138,25 +138,7 @@ func updateLabels(pod *v1.Pod, patch []patchOperation) []patchOperation {
 
 	if _, ok := existingLabels[common.SparkLabelAppID]; !ok {
 		if _, ok := existingLabels[common.LabelApplicationID]; !ok {
-			// if app id not exist, generate one
-			// the generated ID is using [PodNamespace]_[PodName]_[Timestamp] naming convention.
-			// some admission controllers have strict checks of the length/format of each labels,
-			// this convention keeps the name tidy and short.
-			podNamespace := "default"
-			if pod.Namespace != "" {
-				podNamespace = pod.Namespace
-			}
-
-			// pod's name can be generated, if name is not explicitly specified
-			// look for generateName instead
-			podName := "unknown"
-			if pod.Name != "" {
-				podName = pod.Name
-			} else if pod.GenerateName != "" {
-				podName = pod.GenerateName
-			}
-
-			generatedID := fmt.Sprintf("%s_%s_%d", podNamespace, podName, time.Now().Unix())
+			generatedID := fmt.Sprintf("__app_%d", time.Now().UnixNano())
 
 Review comment:
   I do not understand the change. We are going from using the pod name or generate name combined with a timestamp to just `app` and a timestamp. It would make tracking pods on the YuniKorn side really difficult.
   
   Can we not use a length limited label setup still leveraging the name or generate name?
   * remove the podNamespace (not making it more unique)
   * limit the size of podName to 50 char in the printed text
   * use a `_` as separator
   * use the millis since 1970 (13 characters) if seconds is not good enough
   ```
   	timestamp := strconv.FormatInt(time.Now().UnixNano(), 10)
   	generatedID := fmt.Sprintf("%.50s_%.13s", podName, timestamp)
   ```
   This will give a guaranteed maximum of 64 chars. We can even tweak that down if you think that is needed.

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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org