Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DRA: scheduler event handlers via assume cache #124595

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

DRA: scheduler event handlers via assume cache #124595

wants to merge 4 commits into from

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Apr 28, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Events that make pods scheduleable were triggered by the informer cache, not the assume cache. For "claim was deallocated", this led to a small, unlikely race if a pod got scheduled and stopped so quickly that the informer cache didn't ever see the "claim is allocated" state. The event handler now reacts to changes in the assume cache because that cache is guaranteed to receive the "claim is allocated" state which cause some pod to not get scheduled, because by definition the cache must have listed some other claim as using resources needed for that pod.

Which issue(s) this PR fixes:

Fixes ##123698

Does this PR introduce a user-facing change?

DRA: fix some small, unlikely race condition during pod scheduling

/assign @kerthcet

Do you have time to review?

/cc @towca

This is related to the work that you are doing for the cluster autoscaler.

@pohly
scheduler: add FIFO queue
1dc55af 
This is a basic implementation of a first-in-first-out queue with unbounded
size. It's useful for cases where a channel with fixed size might deadlock.

The caller is responsible for locking.
@pohly
ktesting: add Step
c6f2a55 
Step simplifies using WithStep because it creates a local scope where the same
tCtx variable is the one with the step name.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Apr 28, 2024
@k8s-ci-robot k8s-ci-robot requested a review from towca April 28, 2024 12:53
@k8s-ci-robot k8s-ci-robot added 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. labels Apr 28, 2024
@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/test-infra 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 Apr 28, 2024
@pohly pohly changed the title DRA: schedule event handlers via assume cache DRA: scheduler event handlers via assume cache Apr 28, 2024
@k8s-ci-robot k8s-ci-robot added area/test 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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 28, 2024
@pohly pohly force-pushed the dra-scheduler-assume-cache-eventhandlers branch from 3cc6fe2 to 7d9abd5 Compare April 29, 2024 06:35
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please ask for approval from kerthcet. For more information see the Kubernetes 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
scheduler: AddEventHandler for assume cache
75dd31d 
This enables using the assume cache for cluster events.
@pohly pohly force-pushed the dra-scheduler-assume-cache-eventhandlers branch from 7d9abd5 to 2d66ba2 Compare April 29, 2024 08:59
@pohly
Copy link
Contributor Author

pohly commented Apr 29, 2024

/retest

@bart0sh bart0sh added this to Triage in SIG Node PR Triage Apr 29, 2024
@pohly
scheduler: central ResourceClaim assume cache
0b0e8e3 
This enables connecting the event handler for ResourceClaim to the assume
cache, which addresses a theoretic race condition.

It may also be useful for implementing the autoscaler support, because now
the autoscaler can modify the content of the cache.
@pohly pohly force-pushed the dra-scheduler-assume-cache-eventhandlers branch from 2d66ba2 to 0b0e8e3 Compare April 29, 2024 12:43
@bart0sh bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage May 8, 2024
@haircommander haircommander moved this from Triage to Archive-it in SIG Node CI/Test Board May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@towca towca Awaiting requested review from towca

Assignees

@kerthcet kerthcet

Labels
area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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.
Projects
SIG Node CI/Test Board
Archive-it
SIG Node PR Triage
Needs Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pohly @k8s-ci-robot @kerthcet