Skip to content

DRA scheduler: implement filter timeout #132033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pohly
Copy link
Contributor

@pohly pohly commented May 30, 2025

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:

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 k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 30, 2025
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot 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 k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. wg/device-management Categorizes an issue or PR as relevant to WG Device Management. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 30, 2025
@@ -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())
Copy link
Contributor Author

@pohly pohly May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pohly force-pushed the dra-scheduler-filter-timeout branch from 1d4d178 to f1aec04 Compare May 30, 2025 16:05
@k8s-ci-robot 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
Copy link
Contributor Author

pohly commented May 30, 2025

/retest

Some known flakes, timeouts.

@pohly pohly force-pushed the dra-scheduler-filter-timeout branch from f1aec04 to dc1bb36 Compare June 2, 2025 08:48
@k8s-ci-robot 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
@pohly
Copy link
Contributor Author

pohly commented Jun 30, 2025

/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
Copy link
Contributor

@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
Copy link
Contributor Author

pohly commented Jun 30, 2025

/label api-review

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added and rebased to address conflicts with other Filter interface changes - please re-add LGTM?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2025
pohly added 3 commits July 2, 2025 09:29
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.
@pohly pohly force-pushed the dra-scheduler-filter-timeout branch from 2436ee7 to 796b64e Compare July 2, 2025 07:29
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2025
@k8s-ci-robot
Copy link
Contributor

[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 ask for approval from ahg-g, msau42. 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:

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

pohly added a commit to pohly/kubernetes that referenced this pull request Jul 2, 2025
want: want{
filter: perNodeResult{
workerNode.Name: {
status: framework.NewStatus(framework.UnschedulableAndUnresolvable, `asked by caller to stop allocating devices: test canceling Filter`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case you're not already on top of it, it looks like #132087 also came with some more subtle conflicts when these were moved:

Suggested change
status: framework.NewStatus(framework.UnschedulableAndUnresolvable, `asked by caller to stop allocating devices: test canceling Filter`),
status: fwk.NewStatus(fwk.UnschedulableAndUnresolvable, `asked by caller to stop allocating devices: test canceling Filter`),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had fixed the merge conflicts, but hadn't noticed that I need to update some code without conflicts. Should be fixed now.

pohly added 4 commits July 2, 2025 17:06
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.
@pohly pohly force-pushed the dra-scheduler-filter-timeout branch from 796b64e to 8c70ff3 Compare July 2, 2025 15:08
pohly added a commit to pohly/kubernetes that referenced this pull request Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. wg/device-management Categorizes an issue or PR as relevant to WG Device Management.
Projects
Status: No status
Status: 👀 In review
Status: Archive-it
Development

Successfully merging this pull request may close these issues.

9 participants