[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #231: [YUNIKORN-460] Handle placeholder timeout

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

yangwwei commented on a change in pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#discussion_r584462823



##########
File path: pkg/common/utils/gang_utils.go
##########
@@ -117,6 +118,28 @@ func GetTaskGroupsFromAnnotation(pod *v1.Pod) ([]v1alpha1.TaskGroup, error) {
 	return taskGroups, nil
 }
 
+func GetPlaceholderTimeoutParam(pod *v1.Pod) (int64, error) {
+	param, ok := pod.Annotations[constants.AnnotationSchedulingPolicyParam]
+	if !ok {
+		return 0, fmt.Errorf("no scheduling policy parameters defined for the pod")
+	}
+	params := strings.Split(param, constants.SchedulingPolicyParamDelimiter)
+	for _, p := range params {
+		if strings.HasPrefix(p, constants.SchedulingPolicyTimeoutParam) {
+			timeoutParam := strings.Split(p, "=")
+			if len(timeoutParam) != 2 {
+				return 0, fmt.Errorf("unable to parse timeout value from annotation")
+			}
+			timeout, err := strconv.ParseInt(timeoutParam[1], 10, 64)
+			if err != nil {
+				return 0, fmt.Errorf("failed to parse timeout value: %s", timeoutParam[1])
+			}
+			return timeout, nil
+		}
+	}

Review comment:
       Can we do the split first and then find the exact match for SchedulingPolicyTimeoutParam?
   Since the parameters could be arbitrary key-value pairs, just in case there could be other parameters that starts with "timeout".

##########
File path: pkg/common/constants/constants.go
##########
@@ -50,11 +50,14 @@ const SchedulerName = "yunikorn"
 
 // Application crd
 const AppManagerHandlerName = "yunikorn-app"
-const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
-const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
-const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
 
 // Gang scheduling
 const PlaceholderContainerImage = "k8s.gcr.io/pause"
 const PlaceholderContainerName = "pause"
 const PlaceholderPodRestartPolicy = "Never"
+const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
+const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
+const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
+const AnnotationSchedulingPolicyParam = "yunikorn.apache.org/schedulingPolicyParameters"
+const SchedulingPolicyTimeoutParam = "timeout"

Review comment:
       We need to do a second think about the name.
   "timeout" is not very descriptive to me and may cause confusion.
   Maybe we can explicitly call it "reservationTimeout"

##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -139,6 +150,11 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
 				log.Logger().Error("failed to delete application", zap.Error(err))
 			}
 		} else {
+			if updated.State == events.States().Application.Killed {
+				//TODO: implement the killed event
+				ev := cache.NewFailApplicationEvent(updated.ApplicationID)
+				dispatcher.Dispatch(ev)
+			}

Review comment:
       @kingamarton how can we support a "Kill" application event?
   I am not fully understanding what will happen behind this.




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