pbacsko commented on PR #1043:
URL: https://github.com/apache/yunikorn-k8shim/pull/1043#issuecomment-5102363726
I think we're getting there. This change is now logically correct, but we
can still improve it.
Suggestions:
1. Move `CycleState` deletion from `IsPodFitNode()` to scheduler cache's
`updatePod()`:
```
if utils.IsAssignedPod(pod) && !utils.IsPodTerminated(pod) {
// pod is bound to a node now: the scheduling cycle is over, so drop the
// cycle state populated during PreFilter (no-op if it was never set)
delete(cache.podsCycleState, key)
// assign to node
nodeInfo, ok := cache.nodesMap[pod.Spec.NodeName]
...
```
This is more robust and cleaner. It's an unambiguous lifecycle point, runs
under the write lock already held by `UpdatePod()`, consistent with the
existing `delete(cache.podsCycleState, key)` in the terminated branch.
2. Revert back to using read locks in `IsPodFitNode()`. The write lock is
only there for `DeleteCycleState()`. This serializes every predicate check
against informer-driven cache updates (node/pod add/update), which is a
throughput regression versus master (which used `LockForReads()`), and it's
inconsistent with `IsPodFitNodeViaPreemption()`, which correctly uses
`LockForReads()`.
If cycle-state deletion is moved to the pod-assignment path in updatePod
(see above), Filter no longer mutates the cache.
--
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]
