wilfred-s commented on code in PR #1096:
URL: https://github.com/apache/yunikorn-core/pull/1096#discussion_r3584127731
##########
pkg/mock/predicate_plugin.go:
##########
@@ -61,9 +75,10 @@ func (f *PredicatePlugin) Predicates(args
*si.PredicatesArgs) error {
// mustFail will cause the predicate check to always fail
// nodes allows specifying which node to fail for which check using the nodeID:
// possible values: -1 fail reserve, 0 fail always, 1 fail alloc (defaults to
always)
-func NewPredicatePlugin(mustFail bool, nodes map[string]int) *PredicatePlugin {
+func NewPredicatePlugin(mustPreFilterFail bool, mustFilterFail bool, nodes
map[string]int) *PredicatePlugin {
Review Comment:
Comments do not line up with the signature anymore.
##########
pkg/mock/predicate_plugin.go:
##########
@@ -29,30 +29,44 @@ import (
type PredicatePlugin struct {
ResourceManagerCallback
- mustFail bool
- nodes map[string]int
+ mustPreFilterFail bool
+ mustFilterFail bool
+ nodes map[string]int
}
-func (f *PredicatePlugin) Predicates(args *si.PredicatesArgs) error {
- if f.mustFail {
- log.Log(log.Test).Info("fake predicate plugin fail: must fail
set")
- return fmt.Errorf("fake predicate plugin failed")
+func (f *PredicatePlugin) PredicatesPreFilter(args *si.PredicatesArgs)
(map[string]struct{}, error) {
+ feasibleNodes := make(map[string]struct{})
+ if f.mustPreFilterFail {
+ log.Log(log.Test).Info("fake predicate prefilter plugin fail:
must fail set")
+ return feasibleNodes, fmt.Errorf("fake predicate plugin failed")
}
- if fail, ok := f.nodes[args.NodeID]; ok {
- if args.Allocate && fail >= 0 {
- log.Log(log.Test).Info("fake predicate plugin node
allocate fail",
- zap.String("node", args.NodeID),
- zap.Int("fail mode", fail))
- return fmt.Errorf("fake predicate plugin failed")
- }
- if !args.Allocate && fail <= 0 {
- log.Log(log.Test).Info("fake predicate plugin node
reserve fail",
- zap.String("node", args.NodeID),
- zap.Int("fail mode", fail))
- return fmt.Errorf("fake predicate plugin failed")
+ for k, v := range f.nodes {
+ if args.Allocate {
+ if v > 0 && v < 200 {
Review Comment:
needs explanation as it is not clear what this does. is a tristate (-1,0,1)
not enough to handle what is needed?
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -557,7 +557,23 @@ func (p *Preemptor) tryNodes() (string, []*Allocation,
bool) {
// calculate victim list for each node
predicateChecks := make([]*si.PreemptionPredicatesArgs, 0)
victimsByNode := make(map[string][]*Allocation)
+
+ // run predicates for this pod before in hand and fetch feasible nodes
+ feasibleNodes, err := p.ask.preAllocateConditions(true)
Review Comment:
I think we need to do the same here as for the `tryPlaceholderAllocate()` we
only want to look at a single node. We do not care about all nodes in the
cluster. Need to think about this call.
##########
pkg/scheduler/partition_test.go:
##########
@@ -1803,6 +1803,7 @@ func TestRequiredNodeCancelOtherReservations(t
*testing.T) {
// the second one should be reserved as the 2nd node is not scheduling
result = partition.tryAllocate()
+ println(result.String())
Review Comment:
no print statements in production or test code unless specifically required
(as in the e2e tests)
##########
pkg/scheduler/objects/node.go:
##########
@@ -487,6 +488,23 @@ func (sn *Node) preAllocateConditions(ask *Allocation)
error {
// Checking pre-conditions in the shim for a reservation.
func (sn *Node) preReserveConditions(ask *Allocation) error {
+ // run predicates for this pod before in hand and fetch feasible nodes
Review Comment:
Since we run the pre-filter predicates earlier I do not think we need to
call it here.
`preReserveConditions()` is called from `tryPlaceholderAllocate()` and from
`tryNodes()`
Specially the case in the `tryNodes()` we reserve a node that has been found
in the iteration over all the nodes. If the node was not feasible it would not
have been chosen.
For the `tryPlaceholderAllocate()` case we might want to even special case
that call and just pass in a single node into the check to lighten the load.
--
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]
