Concurrency issues with the EvictPod method

[googs1025]

Does EvictPod need to be locked to control concurrency?

https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/evictions/evictions.go#L157

// pkg/descheduler/evictions/evictions.go
// EvictPod evicts a pod while exercising eviction limits.
// Returns true when the pod is evicted on the server side.
func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, opts EvictOptions) bool {
	var span trace.Span
	ctx, span = tracing.Tracer().Start(ctx, "EvictPod", trace.WithAttributes(attribute.String("podName", pod.Name), attribute.String("podNamespace", pod.Namespace), attribute.String("reason", opts.Reason), attribute.String("operation", tracing.EvictOperation)))
	defer span.End()
	...
        // Is there a need to lock here?
	if pod.Spec.NodeName != "" {
		pe.nodepodCount[pod.Spec.NodeName]++
	}
	pe.namespacePodCount[pod.Namespace]++

	if pe.metricsEnabled {
		metrics.PodsEvicted.With(map[string]string{"result": "success", "strategy": opts.StrategyName, "namespace": pod.Namespace, "node": pod.Spec.NodeName, "profile": opts.ProfileName}).Inc()
	}

	...
	return true
}

[googs1025]

I think that in a controller pattern, we should use locks to control concurrency.
If necessary, I will submit a pull request to make the modifications. If not needed, I will close this issue.

[damemi]

@googs1025 each plugin is run synchronously, along with each EvictPod request, so I don't think locks are necessary (unless you are referring to some sort of external lock implementation for running multiple descheduler instances?)

Are you currently hitting any concurrency issues with descheduler?

[googs1025]

@googs1025 each plugin is run synchronously, along with each EvictPod request, so I don't think locks are necessary (unless you are referring to some sort of external lock implementation for running multiple descheduler instances?)

Are you currently hitting any concurrency issues with descheduler?

thank you for reply！
No, I was just feeling a bit confused while reviewing the source code. I'm not sure if there is a multi-instance implementation, but typically, if there are multiple instances or concurrency involved, locks or the atomic package are commonly used.
The Evict method in the descheduler also follows the Kubernetes framework, right? If so, it should not involve any concurrency issues.

[damemi]

The Evict method in the descheduler also follows the Kubernetes framework, right? If so, it should not involve any concurrency issues.

Yes, this just calls the kubernetes API so we don't need to worry about concurrency there

[googs1025]

ok, I'll closed this issue.

[googs1025]

/close

[k8s-ci-robot]

@googs1025: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.