Fix volume binding e2e testing #4483

JesseStutler · 2025-07-24T14:07:58Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

In #4430, we removed these lines:

volcano/pkg/scheduler/capabilities/volumebinding/binder.go

Lines 956 to 958 in 0a8cbd6

    
           if !b.capacityCheckEnabled { 
        
           	return true, nil 
        
           }

, which is important when EnableCSIStorage server option is false. Now when we want to create Dynamic provision pod, the scheduler will panic at this line:

volcano/pkg/scheduler/capabilities/volumebinding/binder.go

Line 982 in 0a8cbd6

capacities, err := b.csiStorageCapacityLister.List(labels.Everything())

Besides, current volume binding e2e testing incorrectly use default-scheduler to schedue pods requesting PVCs. These pods should use volcano to schedule, therefore I add test/e2e/util/scheduler_injector.go, it can modify the schedulerName of the pod non-invasively without copying the k8s source code. And I add a new Dynamic Provision e2e testing case

Which issue(s) this PR fixes:

Fixes #4480

Special notes for your reviewer:

Does this PR introduce a user-facing change?

gemini-code-assist

Summary of Changes

Hello @JesseStutler, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical panic within the Volcano scheduler's volume binding logic and significantly improves the robustness and accuracy of its end-to-end (e2e) testing for volume-related functionalities. It ensures that e2e tests correctly interact with the Volcano scheduler and expands test coverage to include dynamic volume provisioning scenarios.

Highlights

Fixes Volume Binding Panic: Implemented a safeguard in the volume binder's hasEnoughCapacity function to prevent a panic that occurred when CSI storage capacity checking was disabled. The fix ensures the function returns early if capacityCheckEnabled is false, avoiding nil pointer dereferences.
Enhances E2E Test Scheduler Injection: Introduced a new utility (test/e2e/util/scheduler_injector.go) that wraps the Kubernetes clientset. This utility automatically injects the Volcano scheduler name into pod specifications during e2e test execution, ensuring that all pods created by the tests are correctly scheduled by Volcano.
Adds New Dynamic Provisioning E2E Test: A new end-to-end test case has been added under the 'Dynamic Provisioning' suite to validate the successful provisioning of storage using the Kind cluster's standard storage class (rancher.io/local-path) when scheduled by Volcano.
Configurable Ignored Provisioners: Extended the Volcano scheduler's configuration to allow specifying storage provisioners that should be ignored. This is exposed via Helm chart values and utilized in e2e test scripts (e.g., ignoring kubernetes.io/no-provisioner) to provide better control over the test environment.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

JesseStutler · 2025-07-24T14:09:20Z

/cc @Monokaix @ouyangshengjia @wangyang0616

volcano-sh-bot · 2025-07-24T14:09:27Z

@JesseStutler: GitHub didn't allow me to request PR reviews from the following users: ouyangshengjia.

Note that only volcano-sh members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Monokaix @ouyangshengjia @wangyang0616

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.

gemini-code-assist

Code Review

This pull request addresses a panic in the volume binder and improves the e2e test suite for volume binding. The fix for the panic is direct and correct, and the introduction of the schedulerInjectingClientset is a clean, non-invasive method to ensure pods in tests are scheduled by Volcano. The new dynamic provisioning test case is a valuable addition. The suggestion improves the robustness of the new Helm chart configuration.

installer/helm/chart/volcano/templates/scheduler.yaml

Monokaix · 2025-07-25T01:30:22Z

Makefile


 e2e-test-schedulingbase: images
-	E2E_TYPE=SCHEDULINGBASE ./hack/run-e2e-kind.sh
+	E2E_TYPE=SCHEDULINGBASE IGNORED_PROVISIONERS="kubernetes.io/no-provisioner" ./hack/run-e2e-kind.sh


Why ignore this one?

Because in volume binding e2e testing, setupStorageClass uses the no-provisioner storageclass to set up:
https://github.com/JesseStutler/volcano/blob/3aa260cf38ed112a1764473a316e2f280ae4571c/test/e2e/schedulingbase/volume_binding.go#L451-L462
If I don't set ignored-provisioners here,

volcano/pkg/scheduler/cache/event_handlers.go

Lines 87 to 98 in 36a46e2

func (sc *SchedulerCache) addPodCSIVolumesToTask(pi *schedulingapi.TaskInfo) error {

volumes, err := sc.getPodCSIVolumes(pi.Pod)

if err != nil {

klog.Errorf("got pod csi attachment persistent volumes count error: %s", err.Error())

return err

}

for key, count := range volumes {

pi.Resreq.AddScalar(key, float64(count))

}

return nil

}

will add an attachment resource for pods requesting PVCs, then overused validation in proportion plugin will reject the pod to be scheduled because the default queue didn't set attachment resource quota.

We can add a comment here.

Signed-off-by: JesseStutler <[email protected]>

Monokaix · 2025-07-26T02:08:49Z

test/e2e/schedulingbase/volume_binding.go

 		})
 	})
+
+	f.Context("Dynamic Provisioning", f.WithSlow(), func() {


Is this a dedicated case just for volcano?

yes, since we do not have a cloud vendor offering PVs, I use kind's "standard" storageclass to provide PVs

Monokaix · 2025-07-26T02:10:41Z

test/e2e/util/scheduler_injector.go

+}
+
+func (p *schedulerInjectingPodInterface) Create(ctx context.Context, pod *v1core.Pod, opts metav1.CreateOptions) (*v1core.Pod, error) {
+	if pod.Spec.SchedulerName == "" {


What if pods already set a schedulerName?

Then I think these pods should be scheduled by other schedulers, such as kube-scheduler, but in our use cases there are no cases specify schedulerName to default-scheduler, it should be fine. And currently scheduler_injector is only used in volume_binding e2e testing cases

The schedulerName field is set by kube-scheduler's e2e test, right?
If so, the schedulerName will be set to default-scheduler.

No in volume binding e2e testing, the pod functions related to creation (copied from the kubernetes repo) do not set the schedulerName. My injector is to inject the schedulerName before issuing the pod creation request. If the schedulerName is set to volcano, the kube-apiserver will not set the default value for this pod.

We can't determine the behavior of k8s e2e, so it's better to rewrite this field directly.

OK. Updated

Monokaix · 2025-07-26T02:13:04Z

test/e2e/schedulingbase/volume_binding.go

 	)

 	ginkgo.BeforeEach(func(ctx context.Context) {
+		f.ClientSet = e2eutil.NewSchedulerInjectingClientset(f.ClientSet, e2eutil.SchedulerName)


How about set a parameter --scheduler-name="volcano,defaule-scheduler" to let volcano schedule all pods directly?

I think it may cause kube-scheduler and volcano to compete for scheduling pods and cause strange behavior, set the schedulerName directly will make sure everything is correct and scheduled by volcano

1. Set pod schedulerName to volcano 2. Add ignored-provisioner for volume binding e2e testing 3. Add dynamic provision e2e testing case Signed-off-by: JesseStutler <[email protected]>

Monokaix · 2025-07-30T01:36:29Z

/approve

volcano-sh-bot · 2025-07-30T01:36:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

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:

~~OWNERS~~ [Monokaix]

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

hwdef · 2025-08-01T06:31:34Z

/lgtm

volcano-sh-bot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 24, 2025

volcano-sh-bot requested review from hzxuzhonghu, lowang-bh and shinytang6 July 24, 2025 14:08

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2025

gemini-code-assist bot reviewed Jul 24, 2025

View reviewed changes

volcano-sh-bot requested review from Monokaix and wangyang0616 July 24, 2025 14:09

gemini-code-assist bot reviewed Jul 24, 2025

View reviewed changes

installer/helm/chart/volcano/templates/scheduler.yaml Show resolved Hide resolved

Monokaix reviewed Jul 25, 2025

View reviewed changes

JesseStutler mentioned this pull request Jul 25, 2025

fix: Volcano scheduler panic when scheduling Pods with delayed binding PVCs #4484

Merged

Add volume binding feature gates

b853339

Signed-off-by: JesseStutler <[email protected]>

JesseStutler force-pushed the fix_volume_e2e branch from 089c483 to 54e6e75 Compare July 25, 2025 02:57

JesseStutler requested a review from Monokaix July 25, 2025 07:04

Monokaix reviewed Jul 26, 2025

View reviewed changes

JesseStutler force-pushed the fix_volume_e2e branch from 54e6e75 to 2f89810 Compare July 26, 2025 07:14

JesseStutler changed the title ~~Fix volume binding panic and volume binding e2e testing~~ Fix volume binding e2e testing Jul 26, 2025

JesseStutler requested a review from Monokaix July 28, 2025 06:52

JesseStutler force-pushed the fix_volume_e2e branch from 2f89810 to 1b72e9a Compare July 28, 2025 09:17

Fix volume binding e2e testing:

1fb9d13

1. Set pod schedulerName to volcano 2. Add ignored-provisioner for volume binding e2e testing 3. Add dynamic provision e2e testing case Signed-off-by: JesseStutler <[email protected]>

JesseStutler force-pushed the fix_volume_e2e branch from 1b72e9a to 1fb9d13 Compare July 28, 2025 09:34

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2025

volcano-sh-bot assigned hwdef Aug 1, 2025

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2025

volcano-sh-bot merged commit 90a6735 into volcano-sh:master Aug 1, 2025
18 checks passed

	func (sc SchedulerCache) addPodCSIVolumesToTask(pi schedulingapi.TaskInfo) error {
	volumes, err := sc.getPodCSIVolumes(pi.Pod)
	if err != nil {
	klog.Errorf("got pod csi attachment persistent volumes count error: %s", err.Error())
	return err
	}

	for key, count := range volumes {
	pi.Resreq.AddScalar(key, float64(count))
	}
	return nil
	}

Fix volume binding e2e testing #4483

Fix volume binding e2e testing #4483

Uh oh!

Conversation

JesseStutler commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

JesseStutler commented Jul 24, 2025

Uh oh!

volcano-sh-bot commented Jul 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Monokaix commented Jul 30, 2025

Uh oh!

volcano-sh-bot commented Jul 30, 2025

Uh oh!

hwdef commented Aug 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JesseStutler commented Jul 24, 2025 •

edited

Loading