[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #285: [YUNIKORN-758] Ignore unschedulable nodes for asks with certain tags

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

yangwwei commented on a change in pull request #285:
URL: https://github.com/apache/incubator-yunikorn-core/pull/285#discussion_r691782892



##########
File path: pkg/common/utils_test.go
##########
@@ -116,3 +118,18 @@ func TestConvertSITimeout(t *testing.T) {
 		})
 	}
 }
+
+func TestGetIgnoreUnschedulable(t *testing.T) {

Review comment:
       can we cover the case where the tag value is not true or false?

##########
File path: pkg/scheduler/objects/node.go
##########
@@ -358,20 +358,16 @@ func (sn *Node) preConditions(allocID string, allocate bool) bool {
 // Check if the node should be considered as a possible node to allocate on.
 //
 // This is a lock free call. No updates are made this only performs a pre allocate checks
-func (sn *Node) preAllocateCheck(res *resources.Resource, resKey string, preemptionPhase bool) error {
-	// shortcut if a node is not schedulable
-	if !sn.IsSchedulable() {
-		log.Logger().Debug("node is unschedulable",
-			zap.String("nodeID", sn.NodeID))
-		return fmt.Errorf("pre alloc check, node is unschedulable: %s", sn.NodeID)
-	}
-	// cannot allocate zero or negative resource
-	if !resources.StrictlyGreaterThanZero(res) {
-		log.Logger().Debug("pre alloc check: requested resource is zero",
-			zap.String("nodeID", sn.NodeID))
-		return fmt.Errorf("pre alloc check: requested resource is zero: %s", sn.NodeID)
+func (sn *Node) preAllocateCheck(res *resources.Resource, resKey string, preemptionPhase, ignoreUnschedulable bool) error {
+	// skip schedulable check if ignoreUnschedulable is true
+	if !ignoreUnschedulable {
+		// shortcut if a node is not schedulable
+		if !sn.IsSchedulable() {
+			log.Logger().Debug("node is unschedulable",
+				zap.String("nodeID", sn.NodeID))
+			return fmt.Errorf("pre alloc check, node is unschedulable: %s", sn.NodeID)
+		}
 	}

Review comment:
       move into one if ?
   ```
   if !sn.IsSchedulable() && !ignoreUnschedulable {
      ...
   }
   ```

##########
File path: pkg/scheduler/objects/node.go
##########
@@ -380,6 +376,12 @@ func (sn *Node) preAllocateCheck(res *resources.Resource, resKey string, preempt
 			return fmt.Errorf("pre alloc check: node %s reserved for different app or ask: %s", sn.NodeID, resKey)
 		}
 	}
+	// cannot allocate zero or negative resource
+	if !resources.StrictlyGreaterThanZero(res) {
+		log.Logger().Debug("pre alloc check: requested resource is zero",
+			zap.String("nodeID", sn.NodeID))
+		return fmt.Errorf("pre alloc check: requested resource is zero: %s", sn.NodeID)
+	}

Review comment:
       why do we need to move these few lines?

##########
File path: pkg/common/utils.go
##########
@@ -113,3 +114,19 @@ func ConvertSITimeout(millis int64) time.Duration {
 	}
 	return time.Duration(result)
 }
+
+func GetIgnoreUnschedulable(tag map[string]string) bool {
+	var ignore bool
+	var err error
+	if ignoreUnschedulable, ok := tag[interfaceCommon.DomainYuniKorn+interfaceCommon.KeyIgnoreUnschedulable]; !ok {
+		// if there isn't ignoreUnschedulable tag, set it to false
+		ignore = false
+	} else {
+		ignore, err = strconv.ParseBool(ignoreUnschedulable)
+		if err != nil {
+			log.Logger().Warn("Failed to convert allocationTag ignoreUnschedulable from string to bool")
+			ignore = false
+		}
+	}
+	return ignore
+}

Review comment:
       this can be simplified to: 
   
   ```
   if ignoreUnschedulable, ok := tag[xxx]; ok {
     // tag found
    if  ignore, err = strconv.ParseBool(ignoreUnschedulable); err == nil {
       // parse successful
       return ignore
     }
   }
   
   // every other cases, return false (default)
   return false
   ```




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org