[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #269: YUNIKORN-679: Add gangSchedulingStyle scheduling parameter

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

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



##########
File path: pkg/cache/application.go
##########
@@ -241,6 +252,12 @@ func (app *Application) setOwnReferences(ref []metav1.OwnerReference) {
 	app.placeholderOwnerReferences = ref
 }
 
+func (app *Application) setSchedulingStyle(schedulingStyle string) {
+	app.lock.RLock()
+	defer app.lock.RUnlock()

Review comment:
       this should take the write lock

##########
File path: pkg/appmgmt/interfaces/amprotocol.go
##########
@@ -81,6 +81,7 @@ type ApplicationMetadata struct {
 	TaskGroups              []v1alpha1.TaskGroup
 	PlaceholderTimeoutInSec int64
 	OwnerReferences         []metav1.OwnerReference
+	SchedulingStyle         string

Review comment:
       change this to the struct `SchedulingParameters` and add `placeholderTimeout` and `gangSchedulingStyle` into the single struct. So it will be easier to extend as well.

##########
File path: pkg/common/utils/gang_utils.go
##########
@@ -123,24 +125,53 @@ func GetTaskGroupsFromAnnotation(pod *v1.Pod) ([]v1alpha1.TaskGroup, error) {
 	return taskGroups, nil
 }
 
-func GetPlaceholderTimeoutParam(pod *v1.Pod) (int64, error) {
+func GetSchedulingPolicyParam(pod *v1.Pod) map[string]interface{} {
+	schedulingPolicyParams := make(map[string]interface{})
+	timeout := int64(0)
+	var timeoutErr = fmt.Errorf("")
+	var styleErr = fmt.Errorf("")
+	style := constants.SchedulingPolicyStyleParamDefault
 	param, ok := pod.Annotations[constants.AnnotationSchedulingPolicyParam]
 	if !ok {
-		return 0, nil
+		schedulingPolicyParams["placeholderTimeout"] = timeout
+		schedulingPolicyParams["placeholderTimeoutErr"] = timeoutErr
+		schedulingPolicyParams["schedulingStyle"] = style
+		schedulingPolicyParams["schedulingStyleErr"] = styleErr

Review comment:
       please do not add `placeholderTimeoutErr` and `schedulingStyleErr` into the return map, which makes the return value pretty odd. for easier access, we can return a struct such as:
   
   ```
   type schedulingParameters struct {
     placeholderTimeout int64
     gangSchedulingStyle string
   }
   ```

##########
File path: pkg/cache/application.go
##########
@@ -599,6 +618,19 @@ func (app *Application) handleReleaseAppAllocationEvent(event *fsm.Event) {
 			}
 		}
 	}
+	app.postAppReleased()
+}
+
+func (app *Application) postAppReleased() {
+	canRunApplication := true
+	for _, task := range app.taskMap {
+		if !utils.IsPodTerminated(task.GetTaskPod()) {
+			canRunApplication = false
+		}

Review comment:
       when the shim deletes the placeholder pod, the pod will be gone, I don’t think this is a safe way to check.
   instead of doing this check in the shim side, can we leverage the core side app state?
   IIUC, the core side has the check when all the placeholders are removed, then moves the app to the Running state. In that case, can we simply rely on that, if the app state from the core transits to the Running state, and the shim is not in Running state yet, we trigger a RunApplication event.




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