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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
