DRA scheduler: implement filter timeout #132033

pohly · 2025-05-30T15:56:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The intent is to catch abnormal runtimes with the generously large default timeout of 10 seconds, as discussed here:

DRA: Allocation of devices can be slow in some situations #131730 (comment)
KEP 4381: DRA structured parameters: updates, promotion to GA enhancements#5333 (comment)

Which issue(s) this PR fixes:

Related-to: #131730 (comment), kubernetes/enhancements#4381

Special notes for your reviewer:

We have to set up a context with the configured timeout (optional!), then ensure that both CEL evaluation and the allocation logic itself properly returns the context error. The scheduler plugin then can convert that into "unschedulable".

Does this PR introduce a user-facing change?

DRA: the scheduler plugin now prevents abnormal filter runtimes by timing out after 10 seconds. This is configurable via the plugin configuration's `FilterTimeout`. Setting it to zero disables the timeout and restores the behavior of Kubernetes <= 1.33.

k8s-ci-robot · 2025-05-30T15:57:00Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

pohly · 2025-05-30T15:58:17Z

staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go

@@ -682,6 +685,10 @@ func lookupAttribute(device *draapi.BasicDevice, deviceID DeviceID, attributeNam
 // This allows the logic for subrequests to call allocateOne with the same
 // device index without causing infinite recursion.
 func (alloc *allocator) allocateOne(r deviceIndices, allocateSubRequest bool) (bool, error) {
+	if alloc.ctx.Err() != nil {
+		return false, fmt.Errorf("filter operation aborted: %w", alloc.ctx.Err())


TODO:

benchmark this additional if check

decide whether we should add a separate feature gate for it (KEP 4381: DRA structured parameters: updates, promotion to GA enhancements#5333 (comment))

I found no relevant performance impact of this additional if check.

What I did find was that our instructions for running scheduler_perf didn't show how to use benchstat 😁 I had to re-discover how to do that. I've included one commit with updated instructions.

@sanposhiho: I also included one commit with the enhancements for Filter cancellation (use context.CancelCause, documentation in the interface).

This PR is now ready for merging.

/assign @sanposhiho @macsko

pohly · 2025-05-30T18:21:38Z

/retest

Some known flakes, timeouts.

macsko · 2025-06-26T13:16:36Z

pkg/scheduler/apis/config/types_pluginargs.go

+	// usage of DRA. However, slow filtering can slow down Pod scheduling
+	// also for Pods not using DRA. Administators can reduce the timeout
+	// after checking the
+	// `scheduler_framework_extension_point_duration_seconds` metrics.


Relying on this metric would give the e2e filtering duration (all plugins for all the nodes). You could add that the value could be adjusted based on the scheduler_plugin_execution_duration_seconds metric (specific plugin for one node).

Good point. Then admins should look at scheduler_plugin_execution_duration_seconds first because that is what the timeout is applied to, with scheduler_framework_extension_point_duration_seconds providing some additional insights. Updated accordingly.

// after checking the // `scheduler_plugin_execution_duration_seconds` metrics. // That tracks the time spend in each Filter operation. // There's also `scheduler_framework_extension_point_duration_seconds` // which tracks the duration of filtering overall.

macsko · 2025-06-26T13:17:57Z

pkg/scheduler/framework/interface.go

@@ -559,6 +559,14 @@ type FilterPlugin interface {
 	// For example, during preemption, we may pass a copy of the original
 	// nodeInfo object that has some pods removed from it to evaluate the
 	// possibility of preempting them to schedule the target pod.
+	//
+	// Plugins are encourage to check the context for cancellation.


Suggested change

// Plugins are encourage to check the context for cancellation.

// Plugins are encouraged to check the context for cancellation.

macsko · 2025-06-26T13:27:09Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go

+		case errors.Is(err, context.DeadlineExceeded):
+			return statusUnschedulable(logger, "timed out trying to allocate devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()))
+		case ctx.Err() != nil:
+			return statusUnschedulable(logger, "asked by caller to stop allocate devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()), "cause", context.Cause(ctx))


Suggested change

return statusUnschedulable(logger, "asked by caller to stop allocate devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()), "cause", context.Cause(ctx))

return statusUnschedulable(logger, "asked by caller to stop allocating devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()), "cause", context.Cause(ctx))

macsko · 2025-06-26T13:59:17Z

pkg/scheduler/apis/config/v1/defaults_test.go

+						PluginConfig: append(append(slices.Clone(pluginConfigs[0:1]), configv1.PluginConfig{
+							Name: "DynamicResources",
+							Args: runtime.RawExtension{Object: &configv1.DynamicResourcesArgs{
+								TypeMeta: metav1.TypeMeta{
+									Kind:       "DynamicResourcesArgs",
+									APIVersion: "kubescheduler.config.k8s.io/v1",
+								},
+								FilterTimeout: &metav1.Duration{Duration: 10 * time.Second},
+							}},
+						}), pluginConfigs[1:]...),


Maybe this way will be simpler:

Suggested change

PluginConfig: append(append(slices.Clone(pluginConfigs[0:1]), configv1.PluginConfig{

Name: "DynamicResources",

Args: runtime.RawExtension{Object: &configv1.DynamicResourcesArgs{

TypeMeta: metav1.TypeMeta{

Kind: "DynamicResourcesArgs",

APIVersion: "kubescheduler.config.k8s.io/v1",

},

FilterTimeout: &metav1.Duration{Duration: 10 * time.Second},

}},

}), pluginConfigs[1:]...),

PluginConfig: append([]configv1.PluginConfig{pluginConfigs[0], configv1.PluginConfig{

Name: "DynamicResources",

Args: runtime.RawExtension{Object: &configv1.DynamicResourcesArgs{

TypeMeta: metav1.TypeMeta{

Kind: "DynamicResourcesArgs",

APIVersion: "kubescheduler.config.k8s.io/v1",

},

FilterTimeout: &metav1.Duration{Duration: 10 * time.Second},

}},

}}, pluginConfigs[1:]...),

Yes. Thanks!

macsko · 2025-06-26T14:12:58Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources_test.go

+			// This variant uses the normal test objects to avoid excessive runtime.
+			// It could theoretically pass even though the 1 ns limit is enforced,
+			// but that's unlikely.


Move this comment to the "timeout" test case?

"timeout" uses unusual ("large") test data to increase the likelihood that processing them runs into the timeout.

It's really this case here which falls back to the normal test data (input + output) again.

Perhaps the "theoretically pass" part is misleading. I changed it to:

// It could theoretically pass even though the 1 ns limit is enforced // although it shouldn't be (which then would be a false positive), // but that's unlikely.

macsko · 2025-06-26T14:16:12Z

staging/src/k8s.io/dynamic-resource-allocation/cel/compile.go

@@ -260,6 +266,10 @@ func (c CompilationResult) DeviceMatches(ctx context.Context, input Device) (boo

 	result, details, err := c.Program.ContextEval(ctx, variables)
 	if err != nil {
+		// CEL does not wrap the context error. We have to deduce why it failed.
+		if strings.Contains(err.Error(), "operation interrupted") && ctx.Err() != nil {
+			return false, details, fmt.Errorf("%w: %w", err, ctx.Err())


Shouldn't it use context.Cause similarly to allocator?

Yes, that's indeed better. I try to train myself to use it, but sometimes forget 😅

macsko · 2025-06-26T14:20:15Z

staging/src/k8s.io/dynamic-resource-allocation/cel/compile_test.go

+			case "cancel":
+				c, cancel := context.WithCancel(ctx)
+				defer cancel()
+				ctx = c


Does this test case make sense if we defer the cancel?

No - good catch! This was meant to start the evaluation with an already canceled context. Damn muscle memory... or cut-and-paste.

sanposhiho

I know the linter is faling, but leaving
/approve
from me anyway because I'm going to be unavailable next week.

leave /lgtm to @macsko. Though I believe it needs api-review anyway to get merged

macsko · 2025-06-27T07:22:23Z

staging/src/k8s.io/dynamic-resource-allocation/structured/allocator_test.go

+				ctx = c
+			case "cancel":
+				c, cancel := context.WithCancel(ctx)
+				defer cancel()


Remove defer from here as well.

macsko · 2025-06-27T07:24:49Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go

+		case errors.Is(err, context.DeadlineExceeded):
+			return statusUnschedulable(logger, "timed out trying to allocate devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()))
+		case ctx.Err() != nil:
+			return statusUnschedulable(logger, "asked by caller to stop allocating devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()), "cause", context.Cause(ctx))


This behavior is a bit inconsistent with the Filter's interface comment, where you mentioned that context.Cause(ctx) is passed to the status message. Here, you would only log the cause .

True. Let's do as suggested and include the cause. I also added a test case. I need to rebase, so here's the incremental diff before the rebase for your review: https://github.com/kubernetes/kubernetes/compare/66b9406a7dd4de8a82f1c11769e3379f5c1b51cf..62bc3360c20b1dd54206d4e0a6414a8fb5618ec8

With benchstat it's easy to do before/after comparisons, but the section for running benchmark didn't mention it at all and didn't work as shown there: - benchmark results must be printed (FULL_LOG) - timeout might have been too short (KUBE_TIMEOUT) - only "short" benchmarks ran (SHORT) - klog log output must be redirected (ARTIFACTS)

When using context.CancelCause in the scheduler and context.Cause in plugins, the status returned by plugins is more informative than just "context canceled". Context cancellation itself is not new, but many plugin authors probably weren't aware of it because it wasn't documented.

The only option is the filter timeout. The implementation of it follows in a separate commit.

The intent is to catch abnormal runtimes with the generously large default timeout of 10 seconds. We have to set up a context with the configured timeout (optional!), then ensure that both CEL evaluation and the allocation logic itself properly returns the context error. The scheduler plugin then can convert that into "unschedulable". The allocator and thus Filter now also check for context cancellation by the scheduler. This happens when enough nodes have been found.

It's unclear why k8s.io/kubernetes/pkg/apis/resource/install needs to be imported explicitly. Having the apiserver and scheduler ready to be started ensures that all APIs are available.

This covers disabling the feature via the configuration, failing to schedule because of timeouts for all nodes, and retrying after ResourceSlice changes with partial success (timeout for one node, success for the other). While at it, some helper code gets improved.

The DRASchedulerFilterTimeout feature gate simplifies disabling the timeout because setting a feature gate is often easier than modifying the scheduler configuration with a zero timeout value. The timeout and feature gate are new. The gate starts as beta and enabled by default, which is consistent with the "smaller changes with low enough risk that still may need to be disabled..." guideline.

macsko · 2025-06-30T07:52:17Z

/lgtm
/approve

k8s-ci-robot · 2025-06-30T07:52:25Z

LGTM label has been added.

Git tree hash: 37b3ada6bf1fc58c23e3ac7b3a62790a656c58c5

k8s-ci-robot · 2025-06-30T07:52:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: macsko, pohly, sanposhiho
Once this PR has been reviewed and has the lgtm label, please assign ahg-g, msau42 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/features/OWNERS
pkg/generated/openapi/OWNERS
~~pkg/scheduler/OWNERS~~ [macsko,sanposhiho]
~~staging/src/k8s.io/dynamic-resource-allocation/OWNERS~~ [pohly]
staging/src/k8s.io/kube-scheduler/config/OWNERS
~~test/OWNERS~~ [pohly]
test/compatibility_lifecycle/reference/OWNERS

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

pohly · 2025-06-30T09:29:01Z

/retest

pohly · 2025-06-30T09:32:07Z

/label api/review
/assign @ahg-g @msau42

This changes the scheduler config (implies API review) and adds a feature gate which acts as emergency hatch if the new functionality causes problems (unlikely, so goes straight to beta with on-by-default).

k8s-ci-robot · 2025-06-30T09:32:14Z

@pohly: The label(s) /label api/review cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, ci-short, ci-extended, ci-full, official-cve-feed. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label api/review
/assign @ahg-g @msau42

This changes the scheduler config (implies API review) and adds a feature gate which acts as emergency hatch if the new functionality causes problems (unlikely, so goes straight to beta with on-by-default).

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-sigs/prow repository.

pohly · 2025-06-30T09:33:02Z

/label api-review

nojnhuh · 2025-06-30T14:43:12Z

test/integration/scheduler_perf/README.md

 ```

+The output can used for `benchstat` to summarize results or to do before/after


Could we add a link?

Suggested change

The output can used for `benchstat` to summarize results or to do before/after

The output can used for [`benchstat`](https://pkg.go.dev/golang.org/x/perf/cmd/benchstat) to summarize results or to do before/after

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 30, 2025

k8s-ci-robot requested review from bart0sh and derekwaynecarr May 30, 2025 15:57

github-project-automation bot added this to SIG Node: code and documentation PRs and Dynamic Resource Allocation May 30, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs May 30, 2025

github-project-automation bot moved this to 🆕 New in Dynamic Resource Allocation May 30, 2025

pohly commented May 30, 2025

View reviewed changes

pohly force-pushed the dra-scheduler-filter-timeout branch from 1d4d178 to f1aec04 Compare May 30, 2025 16:05

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2025

pohly mentioned this pull request May 30, 2025

KEP 4381: DRA structured parameters: updates, promotion to GA kubernetes/enhancements#5333

Merged

pohly force-pushed the dra-scheduler-filter-timeout branch from f1aec04 to dc1bb36 Compare June 2, 2025 08:48

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/code-generation and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 2, 2025

macsko reviewed Jun 26, 2025

View reviewed changes

pohly force-pushed the dra-scheduler-filter-timeout branch from aa2c617 to fd7197f Compare June 26, 2025 15:15

sanposhiho approved these changes Jun 26, 2025

View reviewed changes

pohly force-pushed the dra-scheduler-filter-timeout branch from fd7197f to 66b9406 Compare June 27, 2025 07:16

macsko reviewed Jun 27, 2025

View reviewed changes

pohly added a commit to pohly/kubernetes that referenced this pull request Jun 27, 2025

merge kubernetes#132033 dra-scheduler-filter-timeout

fa211cf

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2025

pohly force-pushed the dra-scheduler-filter-timeout branch from 66b9406 to 62bc336 Compare June 28, 2025 15:35

pohly added 7 commits June 28, 2025 17:37

DRA scheduler: add plugin configuration

9837ed2

The only option is the filter timeout. The implementation of it follows in a separate commit.

DRA integration: remove unnecessary anonymous import

7fbb909

It's unclear why k8s.io/kubernetes/pkg/apis/resource/install needs to be imported explicitly. Having the apiserver and scheduler ready to be started ensures that all APIs are available.

pohly force-pushed the dra-scheduler-filter-timeout branch from 62bc336 to 2436ee7 Compare June 28, 2025 15:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2025

k8s-ci-robot assigned ahg-g and msau42 Jun 30, 2025

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jun 30, 2025

github-project-automation bot added this to API Reviews Jun 30, 2025

nojnhuh reviewed Jun 30, 2025

View reviewed changes

	// Plugins are encourage to check the context for cancellation.
	// Plugins are encouraged to check the context for cancellation.

	return statusUnschedulable(logger, "asked by caller to stop allocate devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()), "cause", context.Cause(ctx))
	return statusUnschedulable(logger, "asked by caller to stop allocating devices", "pod", klog.KObj(pod), "node", klog.KObj(node), "resourceclaims", klog.KObjSlice(state.allocator.ClaimsToAllocate()), "cause", context.Cause(ctx))

		```

		The output can used for `benchstat` to summarize results or to do before/after

	The output can used for `benchstat` to summarize results or to do before/after
	The output can used for [`benchstat`](https://pkg.go.dev/golang.org/x/perf/cmd/benchstat) to summarize results or to do before/after

DRA scheduler: implement filter timeout #132033

Are you sure you want to change the base?

DRA scheduler: implement filter timeout #132033

Conversation

pohly commented May 30, 2025

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!

k8s-ci-robot commented May 30, 2025

Uh oh!

pohly May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sanposhiho left a comment

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!

macsko commented Jun 30, 2025

Uh oh!

k8s-ci-robot commented Jun 30, 2025

Uh oh!

k8s-ci-robot commented Jun 30, 2025

Uh oh!

pohly commented Jun 30, 2025

Uh oh!

pohly commented Jun 30, 2025

Uh oh!

k8s-ci-robot commented Jun 30, 2025

Uh oh!

pohly commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly May 30, 2025 •

edited

Loading

pohly Jun 26, 2025 •

edited

Loading