chenyulin0719 commented on code in PR #848:
URL: https://github.com/apache/yunikorn-k8shim/pull/848#discussion_r1611634390
##########
pkg/admission/util.go:
##########
@@ -49,8 +50,10 @@ func updatePodLabel(pod *v1.Pod, namespace string,
generateUniqueAppIds bool, de
result[constants.LabelApplicationID] = generatedID
}
- // if existing label exist, it takes priority over everything else
- if _, ok := existingLabels[constants.LabelQueueName]; !ok {
+ labelQueueName := utils.GetPodLabelValue(pod, constants.LabelQueueName)
Review Comment:
The reasons I decided not to reuse GetQueueNameFromPod() are:
1. The default value handling (which you've provided a solution.)
2. We can only rewrite GetQueueNameFromPod(), but not for
GetApplicationIDFromPod() since it did too many things inside. (eg. scheduler
name/plugin mode check, generating default value.). My opinion is that if we
can't reuse both GetQueueNameFromPod() and GetApplicationIDFromPod() together
in UpdateLabels(), it might be cleaner not to reuse any of them. (Admission
Contoller handle it's own logic for retrieving information from pod.)
3. Another reason is in YUNIKORN-2631, we need to know "Where is the queue
value come?", to decide whether to add queue value to legacy label for
backward compatibility. I don't think add the AM specific logic to
GetQueueNameFromPod() is a good idea.
-
https://github.com/apache/yunikorn-k8shim/pull/843/files#diff-b9716f5c309aabd233802784b4b0b29ceed55c4432d6f58a6722837f224dc12eR73
The conclusion is, I prefer separate the pod information retrieval between
AM and Shim.
@chia7712 WDYT?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
