wilfred-s commented on code in PR #1043:
URL: https://github.com/apache/yunikorn-k8shim/pull/1043#discussion_r3801462735
##########
pkg/cache/external/scheduler_cache.go:
##########
@@ -577,6 +590,24 @@ func (cache *SchedulerCache) updatePVCRefCounts(node
*framework.NodeInfo, remove
}
}
+func (cache *SchedulerCache) GetCycleState(pod *v1.Pod) *framework.CycleState {
+ return cache.podsCycleState[string(pod.UID)]
+}
+
+func (cache *SchedulerCache) UpdateCycleState(pod *v1.Pod, cycleState
*framework.CycleState) {
+ if pod == nil {
+ return
+ }
+ cache.podsCycleState[string(pod.UID)] = cycleState
+}
Review Comment:
We should not have an update, we create a CycleState, and set it in the
cache. We retrieve it and then it is removed. The next time we need a clean new
cycle state.
The updates are made directly in the cycle state object always using that
same reference
##########
pkg/cache/context.go:
##########
@@ -692,6 +699,38 @@ func (ctx *Context) EventsToRegister(queueingHintFn
fwk.QueueingHintFn) []fwk.Cl
return ctx.predManager.EventsToRegister(queueingHintFn)
}
+// PreFilter evaluates given prefilter based predicates based on current
context
+func (ctx *Context) PreFilter(name string, allocate bool)
*si.PreFilterPredicatesResponse {
+ ctx.lock.RLock()
+ defer ctx.lock.RUnlock()
+ pod := ctx.schedulerCache.GetPod(name)
+ if pod == nil {
+ log.Log(log.ShimContext).Error("failed running PreFilter
plugin",
+ zap.String("pod", name),
+ zap.Error(ErrorPodNotFound))
+ return &si.PreFilterPredicatesResponse{
+ FeasibleNodes: make(map[string]*si.Empty),
+ Success: false,
+ }
+ }
+ // if pod exists in cache, try to run predicates
+ // need to lock cache here as predicates need a stable view into the
cache
+ ctx.schedulerCache.LockForWrites()
+ defer ctx.schedulerCache.UnlockForWrites()
+ feasibleNodes, cycleState, err := ctx.predManager.PreFilter(pod,
allocate)
+ if err == nil {
+ ctx.schedulerCache.UpdateCycleState(pod, cycleState)
Review Comment:
Not `UpdateCycleState` should be `StoreCycleState`
##########
pkg/cache/context.go:
##########
@@ -692,6 +699,38 @@ func (ctx *Context) EventsToRegister(queueingHintFn
fwk.QueueingHintFn) []fwk.Cl
return ctx.predManager.EventsToRegister(queueingHintFn)
}
+// PreFilter evaluates given prefilter based predicates based on current
context
+func (ctx *Context) PreFilter(name string, allocate bool)
*si.PreFilterPredicatesResponse {
Review Comment:
Here we can and must create a new cycle state. This cycle state can be
stored for later retrieval
##########
pkg/cache/external/scheduler_cache.go:
##########
@@ -209,6 +219,7 @@ func (cache *SchedulerCache) removeNode(node *v1.Node)
(*v1.Node, []*v1.Pod) {
key := string(pod.UID)
delete(cache.assignedPods, key)
delete(cache.assumedPods, key)
+ delete(cache.podsCycleState, key)
Review Comment:
There should never be a cycle state that needs a cleanup based on a node
removal. The scheduling cycle cleans up its state and if a pod gets removed
that handles all other cases. A pod running on a node MUST not have a cycle
state left.
If the assume or bind fail the cleanup should be automatic.
--
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]
