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
scheduler: framework: initialize indexers in scheduler core with non-nil map #110663
Conversation
@fromanirh: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
/sig scheduling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
thanks, I think it is reasonable to allow adding indexers. Can a test be added to verify that this actually fixes it? |
thanks for the comment. Good point, I'll think about a suitable test. |
5e00c5e
to
13a5a78
Compare
13a5a78
to
535e91c
Compare
@fromanirh Thanks for bringing up this.
No particular reason. Historically, the core scheduler doesn't leverage the indexer so it's never initialized. Please note that we need to add a caveat in the code, as well as in this PR to claim that duplicated (by the same key) indexer registries can cause conflicts: kubernetes/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go Lines 229 to 248 in a57c140
|
535e91c
to
19324f0
Compare
/retest |
This is the only comment non explicitely addressed (added unit tests kinda do, however). What could be the best way to convey this information? |
In the release note, document it like "Scheduler framework's shared PodInformer is now initialized with an indexer. Note that only non-conflict indexers are allowed to be added." And also add it as a comment in the code. |
19324f0
to
8b4a6ae
Compare
Done, thanks! |
@Huang-Wei just asking is this fix suitable for backporting down to 1.23? I think it is small and safe, with high enough ROI. |
/retest |
We only backport critical bug fixes :( |
For the changelog, please can we write this from the cluster operator's point of view? For example, explain that we've fixed a crash that happened with some kinds of scheduling plugin. |
I'll review the changelog, but this change is expected to have no impact from the cluster operator's PoV, but from scheduler plugin developer PoV: without this change, scheduler plugins could not use indexers, so they had to use workarounds. But this is expected to affect efficiency - not stability - of the plugins |
There's another outcome: a plugin built to assume that this bug is fixed / enhancement is made will not work in an earlier Kubernetes version. Maybe we should consider a backport? |
Plugins are generally versioned and released in lockstep with k8s; I don't think this is a candidate for backport |
Using a nil map to initialize the pod indexers will cause runtime failure when trying to add indexers in scheduler plugin. We use a empty map to enable scheduler plugins to add their indexers. Signed-off-by: Francesco Romani <fromani@redhat.com>
8b4a6ae
to
b4e015b
Compare
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fromanirh, Huang-Wei 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Allow scheduler plugins to add their indexers, if they want to.
Which issue(s) this PR fixes:
Fixes #110660
Special notes for your reviewer:
Not sure it was intentional for the scheduler framework to prevent plugins to add indexers. Please check the linked issue for more context.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: