yangwwei commented on a change in pull request #365:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r804375007
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -255,26 +246,25 @@ func (cache *SchedulerCache) UpdatePod(oldPod, newPod
*v1.Pod) error {
defer cache.lock.Unlock()
currState, ok := cache.podsMap[key]
- switch {
- // An assumed pod won't have Update/Remove event. It needs to have Add
event
- // before Update event, in which case the state would change from
Assumed to Added.
- case ok && !cache.isAssumedPod(key):
- if currState.Spec.NodeName != newPod.Spec.NodeName {
- log.Logger().Error("pod updated on a different node
than previously added to", zap.String("pod", key))
- log.Logger().Error("scheduler cache is corrupted and
can badly affect scheduling decisions")
- }
- if err = cache.updatePod(oldPod, newPod); err != nil {
- return err
+ if ok {
+ // pod exists and is assumed
+ if !cache.isAssumedPod(key) && currState.Spec.NodeName !=
newPod.Spec.NodeName {
Review comment:
Refer to
[cache.go#L516-L523](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L516-L523),
the default side, it only updates the pod when the pod is not assumed. But
our logic is updating the pod no matter if it is assumed or not.
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -217,30 +217,21 @@ func (cache *SchedulerCache) AddPod(pod *v1.Pod) error {
defer cache.lock.Unlock()
currState, ok := cache.podsMap[key]
- switch {
- case ok && cache.isAssumedPod(key):
- if currState.Spec.NodeName != pod.Spec.NodeName {
- // The pod was added to a different node than it was
assumed to.
- log.Logger().Warn("inconsistent pod location",
- zap.String("assumedLocation",
pod.Spec.NodeName),
- zap.String("actualLocation",
currState.Spec.NodeName))
-
- // Clean this up.
- err = cache.removePod(currState)
- if err != nil {
- log.Logger().Debug("node not in cache",
- zap.Error(err))
+ if ok {
+ // pod exists
+ if cache.isAssumedPod(key) {
Review comment:
Refer to
[cache.go#L481-492](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L481-L492),
there is a slight difference:
in the default scheduler side, it only updates the pod when the
pod.sepc.NodeName changes and the pod is assumed. In our code, we do the update
no matter if the pod is assumed or not.
Another thing, when the pod node name is unchanged, and the pod is assumed,
it deletes the pod from the cache.
[cache.go#489](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L489).
We no longer have this logic anywhere.
--
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: reviews-unsubscr...@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
