[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #258: Shim side changes for YUNIKORN-657

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

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



##########
File path: pkg/shim/scheduler.go
##########
@@ -234,7 +234,17 @@ func (ss *KubernetesShim) GetSchedulerState() string {
 func (ss *KubernetesShim) handle(se events.SchedulerEvent) error {
 	ss.lock.Lock()
 	defer ss.lock.Unlock()
-	err := ss.stateMachine.Event(string(se.GetEvent()))
+	var err error
+	if len(se.GetArgs()) >= 1 {
+		eventArgs := make([]string, 2)
+		if err = events.GetEventArgsAsStrings(eventArgs, se.GetArgs()); err != nil {
+			log.Logger().Error("fail to parse event arg", zap.Error(err))
+		}
+		message := eventArgs[1]
+		err = ss.stateMachine.Event(string(se.GetEvent()), message)
+	} else {
+		err = ss.stateMachine.Event(string(se.GetEvent()))
+	}
 	if err != nil && err.Error() == "no transition" {

Review comment:
       Can we use a more general form? The interface looks like
   
   ```
   type SchedulerEvent interface {
     GetEvent() SchedulerEventType
     GetArgs() []interface{}
   }
   ```
   
   Can we leverage the args here:
   
   ```
   err = ss.stateMachine.Event(string(se.GetEvent()), se.GetArgs())
   ```

##########
File path: pkg/common/utils/utils.go
##########
@@ -58,6 +58,10 @@ func NeedRecovery(pod *v1.Pod) (bool, error) {
 		return true, nil
 	}
 
+	if pod.Status.Phase == v1.PodFailed && pod.Status.Reason == constants.ApplicationRejectedFailure || pod.Status.Reason == constants.ApplicationInsufficientResourcesFailure {
+		return false, nil
+	}

Review comment:
       Can we simply return false here when pod phase is Failed? 
   Do we have to check the reason?
   

##########
File path: pkg/client/kubeclient.go
##########
@@ -118,3 +118,28 @@ func (nc SchedulerKubeClient) Delete(pod *v1.Pod) error {
 	}
 	return nil
 }
+
+func (nc SchedulerKubeClient) Get(podNamespace string, podName string) (*v1.Pod, error) {
+	pod, err := nc.clientSet.CoreV1().Pods(podNamespace).Get(podName, apis.GetOptions{})
+	if err != nil {
+		log.Logger().Warn("failed to get pod",
+			zap.String("namespace", pod.Namespace),
+			zap.String("podName", pod.Name),
+			zap.Error(err))
+		return nil, err
+	}
+	return pod, nil
+}
+
+func (nc SchedulerKubeClient) Update(pod *v1.Pod) (*v1.Pod, error) {
+	var updatedPod *v1.Pod
+	var err error
+	if updatedPod, err = nc.clientSet.CoreV1().Pods(pod.Namespace).UpdateStatus(pod); err != nil {
+		log.Logger().Warn("failed to update pod",
+			zap.String("namespace", pod.Namespace),
+			zap.String("podName", pod.Name),
+			zap.Error(err))
+		return pod, err
+	}
+	return updatedPod, nil
+}

Review comment:
       The function is called "Update" but the actual impl is "UpdateStatus".
   I think we should either re-implement this with "nc.clientSet.CoreV1().Pods(pod.Namespace).Update()" or change this function name to "UpdateStatus" to avoid mis-usage.




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