Short circuit volume checker if the pod is not requesting any volumes

[bsalamat]

What type of PR is this?
/kind cleanup

This is an optimization.

What this PR does / why we need it:
Volume checker predicate function turns out to take about 5% of the scheduler execution time in our benchmarks while pods in those tests do not request any volumes. This PR short circuits the logic for pods that do not need any volumes.

Before:

BenchmarkScheduling/5000Nodes/1000Pods-12         	   10000	   9140564 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	114.688s

After:

pkg: k8s.io/kubernetes/test/integration/scheduler_perf
BenchmarkScheduling/5000Nodes/1000Pods-12         	   10000	   8541605 ns/op
PASS
ok  	k8s.io/kubernetes/test/integration/scheduler_perf	108.582s

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

/sig scheduling
/assign @msau42 @cofyc

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/scheduler/OWNERS [bsalamat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[msau42]

This might help the benchmark but I'm not sure if it will help in reality because there is an admission controller that injects a secret volume into every pod.

This predicate should be a no-op if the pod does not use any PVCs. @cofyc can you help check if there's anything we can fix in FindPodVolumes to return early?

[cofyc]

FindPodVolumes will return early if pod has no PVCs, see

kubernetes/pkg/controller/volume/persistentvolume/scheduler_binder.go

Lines 154 to 178 in 5323aed

	// FindPodVolumes caches the matching PVs and PVCs to provision per node in podBindingCache.
	// This method intentionally takes in a *v1.Node object instead of using volumebinder.nodeInformer.
	// That's necessary because some operations will need to pass in to the predicate fake node objects.
	func (b *volumeBinder) FindPodVolumes(pod *v1.Pod, node *v1.Node) (unboundVolumesSatisfied, boundVolumesSatisfied bool, err error) {
	podName := getPodName(pod)
	// Warning: Below log needs high verbosity as it can be printed several times (#60933).
	klog.V(5).Infof("FindPodVolumes for pod %q, node %q", podName, node.Name)
	// Initialize to true for pods that don't have volumes
	unboundVolumesSatisfied = true
	boundVolumesSatisfied = true
	start := time.Now()
	defer func() {
	VolumeSchedulingStageLatency.WithLabelValues("predicate").Observe(time.Since(start).Seconds())
	if err != nil {
	VolumeSchedulingStageFailed.WithLabelValues("predicate").Inc()
	}
	}()
	if !podHasClaims(pod) {
	// Fast path
	return unboundVolumesSatisfied, boundVolumesSatisfied, nil
	}

How about skip VolumeSchedulingStageLatency logic and klog.V in FindPodVolumes too? This will make predicate a real no-op if the pod does not use any PVCs.

[bsalamat]

Thanks, @msau42 and @cofyc!
We definitely need to fix this issue. The current predicate is unacceptably slow for pods that do not request volumes.
FindPodVolumes gets pod name, reads time which often needs a system call, and increments Prometheus metrics even when a pod does not need any volume.
The predicate itself (which calls FindPodVolumes) also allocates an array for failure reasons and processes the output of FindPodVolumes for all pods whether they request a volume or not. The predicate itself must have a check to bypass all this and return immediately when a pod does not need a volume. I changed the PR to check PVCs and return immediately if there is no PVCs.
PTAL.

[cofyc]

LGTM
It makes sense to skip FindPodVolumes in predicate function when it knows there is no need to find pod volumes. Prometheus metrics in volume binder will record latency only for pods which needs volume scheduling, I think this is acceptable and good for performance.
btw, TestSchedulerWithVolumeBinding fails, I guess we need to pass a pod with PVCs to test VolumeBinder now

[bsalamat]

Thanks, @cofyc!
I added a PVC to the pod and the client to make the test happy.

[ravisantoshgudimetla]

Thanks for the PR @bsalamat. LGTM. I will wait for @cofyc to give a final lgtm.

[ravisantoshgudimetla]

I think adding a log statement with level 5 or above would here would help in debugging.

[cofyc]

/lgtm