wilfred-s commented on code in PR #1096:
URL: https://github.com/apache/yunikorn-core/pull/1096#discussion_r3619220457
##########
pkg/scheduler/objects/application.go:
##########
@@ -1085,8 +1085,29 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, allowPreemption
request.setHeadroomCheckPassed(sa.queuePath)
requiredNode := request.GetRequiredNode()
+
+ // run predicates for this pod before in hand and fetch
feasible nodes
+ feasibleNodes, predicatesResult :=
request.preAllocateConditions(true)
Review Comment:
We do not want to skip the `tryRequiredNode()` call. If the node has not
been reserved yet we need to do that. The trigger is in `tryRequiredNode()`.
This check could stop that reservation. If the predicates block the node the
node is still reserved for this required node pod, i.e. daemonset pod.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1448,7 +1487,24 @@ func (sa *Application) tryReservedAllocate(headRoom
*resources.Resource, nodeIte
}
}
// check allocation possibility
- // we don't care about predicate error messages here
+ skipReservedNode := false
+ feasibleNodes, predicatesResult :=
ask.preAllocateConditions(true)
+ if predicatesResult {
+ // Is this node suitable to run the pod?
+ if len(feasibleNodes) > 0 {
+ if _, ok := feasibleNodes[reserve.node.NodeID];
!ok {
+ skipReservedNode = true
+ }
+ }
+ } else {
+ skipReservedNode = true
+ }
+ if skipReservedNode {
+ getRateLimitedAppLog().Info("skipping reserved node as
it is not feasible to run the pod",
+ zap.String("allocationKey",
ask.GetAllocationKey()),
+ zap.String("reserved node",
reserve.node.NodeID))
+ continue
+ }
Review Comment:
This check is already run inside the `tryNode()` just below. This is not
required here and should be only run inside the `tryNodesNoReserve()`
##########
pkg/scheduler/objects/application.go:
##########
@@ -1302,16 +1322,24 @@ func (sa *Application)
tryPlaceholderAllocate(nodeIterator func() NodeIterator,
}
}
}
+
// cannot allocate if the iterator is not giving us any schedulable
nodes
iterator := nodeIterator()
if iterator == nil {
return nil
}
+
// we checked all placeholders and asks nothing worked as yet
// pick the first fit and try all nodes if that fails give up
var allocResult *AllocationResult
if phFit != nil && reqFit != nil {
resKey := reqFit.GetAllocationKey()
+
+ // run predicates for this pod before in hand and fetch
feasible nodes
+ feasibleNodes, predicatesResult :=
reqFit.preAllocateConditions(true)
+ if !predicatesResult {
+ return nil
+ }
Review Comment:
We run this check again in line 1364 if it passed here. We need to be
smarter about this and pick up the results from the first run we do within the
loop. We might never need to run this if we do not pass the earlier checks.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1085,8 +1085,29 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, allowPreemption
request.setHeadroomCheckPassed(sa.queuePath)
requiredNode := request.GetRequiredNode()
+
+ // run predicates for this pod before in hand and fetch
feasible nodes
+ feasibleNodes, predicatesResult :=
request.preAllocateConditions(true)
+
// does request have any constraint to run on specific node?
if requiredNode != "" {
Review Comment:
The only node that is possible if `requiredNode != ""` is the node set, no
reason to run any pre-filter or anything as the only node possible is already
known.
Run the predicate check for the generic case after we filter out required
node allocations.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1510,6 +1566,13 @@ func (sa *Application) tryPreemption(headRoom
*resources.Resource, preemptionDel
// This should never result in a reservation as the allocation is already
reserved
func (sa *Application) tryNodesNoReserve(ask *Allocation, iterator
NodeIterator, reservedNode string) *AllocationResult {
var allocResult *AllocationResult
+
+ // run predicates for this pod before in hand and fetch feasible nodes
+ feasibleNodes, predicatesResult := ask.preAllocateConditions(true)
+ if !predicatesResult {
+ return nil
+ }
Review Comment:
We need to be smarter about this and pick up the results from the first run
we do within the loop. [See previous
comment](https://github.com/apache/yunikorn-core/pull/1096/changes#r3619341858)
--
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]
