Move interfaces: Handle and Plugin and related types from kubernetes/kubernetes to staging repo kube-scheduler #133172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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-sigs/prow repository. |
5f3231b to
5742f25
Compare
3bbc46b to
11d7d44
Compare
5a73985 to
7c1266e
Compare
c1af824 to
d261ce9
Compare
|
/retest |
macsko
reviewed
Jul 31, 2025
Comment on lines
752
to
784
| // APICacher defines methods that send API calls through the scheduler's cache | ||
| // before they are executed asynchronously by the APIDispatcher. | ||
| // This ensures the scheduler's internal state is updated optimistically, | ||
| // reflecting the intended outcome of the call. | ||
| // This methods should be used only if the SchedulerAsyncAPICalls feature gate is enabled. | ||
| type APICacher interface { | ||
| // PatchPodStatus sends a patch request for a Pod's status. | ||
| // The patch could be first applied to the cached Pod object and then the API call is executed asynchronously. | ||
| // It returns a channel that can be used to wait for the call's completion. | ||
| PatchPodStatus(pod *v1.Pod, condition *v1.PodCondition, nominatingInfo *NominatingInfo) (<-chan error, error) | ||
|
|
||
| // BindPod sends a binding request. The binding could be first applied to the cached Pod object | ||
| // and then the API call is executed asynchronously. | ||
| // It returns a channel that can be used to wait for the call's completion. | ||
| BindPod(binding *v1.Binding) (<-chan error, error) | ||
|
|
||
| // WaitOnFinish blocks until the result of an API call is sent to the given onFinish channel | ||
| // (returned by methods BindPod or PreemptPod). | ||
| // | ||
| // It returns the error received from the channel. | ||
| // It also returns nil if the call was skipped or overwritten, | ||
| // as these are considered successful lifecycle outcomes. | ||
| // Direct onFinish channel read can be used to access these results. | ||
| WaitOnFinish(ctx context.Context, onFinish <-chan error) error | ||
| } | ||
|
|
||
| // APICallImplementations define constructors for each APICall that is used by the scheduler internally. | ||
| type APICallImplementations[T, K APICall] struct { | ||
| // PodStatusPatch is a constructor used to create APICall object for pod status patch. | ||
| PodStatusPatch func(pod *v1.Pod, condition *v1.PodCondition, nominatingInfo *NominatingInfo) T | ||
| // PodBinding is a constructor used to create APICall object for pod binding. | ||
| PodBinding func(binding *v1.Binding) K | ||
| } |
There was a problem hiding this comment.
Could you move these two into a new file within src/k8s.io/kube-scheduler/framework? We could name it api_calls.go (src/k8s.io/kube-scheduler/framework/api_calls.go.
Btw, it might be worth to also move the pkg/scheduler/framework/api_calls/... to the src/k8s.io/kube-scheduler/framework/api_calls/....
There was a problem hiding this comment.
Could you move these two into a new file within
src/k8s.io/kube-scheduler/framework? We could name itapi_calls.go(src/k8s.io/kube-scheduler/framework/api_calls.go.
Done, thanks
Btw, it might be worth to also move the
pkg/scheduler/framework/api_calls/...to thesrc/k8s.io/kube-scheduler/framework/api_calls/....
How about doing this in another PR?
There was a problem hiding this comment.
How about doing this in another PR?
Sounds good
macsko
reviewed
Jul 31, 2025
| @@ -29,7 +29,6 @@ import ( | |||
| "k8s.io/kubernetes/pkg/scheduler/apis/config" | |||
| "k8s.io/kubernetes/pkg/scheduler/apis/config/validation" | |||
| "k8s.io/kubernetes/pkg/scheduler/framework" | |||
There was a problem hiding this comment.
You might think of moving GetAffinityTerms and GetPodAntiAffinityTerms to the staging as well. It would remove the framework import from this file.
There was a problem hiding this comment.
This could be done in another PR
There was a problem hiding this comment.
Let's do this in the next PR, this one's already too big
There was a problem hiding this comment.
This should definitely be fixed, thanks. I'll list the changes to do in the following PRs, seems that there will be a few
There was a problem hiding this comment.
I listed actions mentioned in this review here: https://docs.google.com/document/d/1c2qfNAL9M-lturrWUHZWzvZyiUV2dlm4dEw8JheLl0k/edit?tab=t.0#heading=h.e2fbypdh6m0f
|
/retest pull-kubernetes-unit |
|
@ania-borowiec: The The following commands are available to trigger optional jobs: Use In response to this:
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. |
|
/test pull-kubernetes-unit |
macsko
reviewed
Jul 31, 2025
| @@ -32,6 +33,8 @@ type Parallelizer struct { | |||
| parallelism int | |||
| } | |||
|
|
|||
| var _ fwk.Parallelizer = &Parallelizer{} | |||
There was a problem hiding this comment.
Adding this for Parallelizer is not needed. Such conformance with the interface is already checked by the code compiler.
There was a problem hiding this comment.
Actually I added it for readability - to explain that it's supposed to implement fwk.Parallelizer interface. On a second thought, though, it's probably better to include this in a comment instead.
|
|
||
| // Parallelizer holds the parallelism for scheduler. | ||
| type Parallelizer interface { | ||
| Until(ctx context.Context, pieces int, doWorkPiece workqueue.DoWorkPieceFunc, operation string) |
| APICacher() APICacher | ||
| } | ||
|
|
||
| // Parallelizer holds the parallelism for scheduler. |
There was a problem hiding this comment.
I'm not sure if this comment well describes the Parallelizer interface
pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go
Outdated
Show resolved
Hide resolved
staging/publishing/rules.yaml
Outdated
There was a problem hiding this comment.
Because according to verify-XXX scripts kube-scheduler needs to be defined AFTER its dependencies
There was a problem hiding this comment.
But why release-1.31/2/3 are moved as well?
macsko
reviewed
Aug 1, 2025
staging/publishing/rules.yaml
Outdated
There was a problem hiding this comment.
But why release-1.31/2/3 are moved as well?
|
/test pull-kubernetes-verify |
…kubernetes to staging repo kube-scheduler
e456e7f to
9b4a91d
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ania-borowiec, macsko 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 |
|
/assign @sanposhiho |
|
/retest |
|
PR needs rebase. 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This is the last part of a larger change that moves interfaces and type and func defiinitions to the staging repo "k8s.io/kube-scheduler", to allow users for importing scheduler framework interfaces without importing k/k repo.
This PR moves types:
Handle,
Plugin,
PreEnqueuePlugin, QueueSortPlugin, EnqueueExtensions, PreFilterExtensions, PreFilterPlugin, FilterPlugin, PostFilterPlugin, PreScorePlugin, ScorePlugin, ReservePlugin, PreBindPlugin, PostBindPlugin, PermitPlugin, BindPlugin,
PodActivator, PodNominator, PluginsRunner,
LessFunc, ScoreExtensions, NodeToStatusReader, NodeScoreList, NodeScore, NodePluginScores, PluginScore, NominatingMode, NominatingInfo, WaitingPod, PreFilterResult, PostFilterResult,
Extender,
NodeInfoLister, StorageInfoLister, SharedLister, ResourceSliceLister, DeviceClassLister, ResourceClaimTracker, SharedDRAManager
from k/k to the staging repo.
The PR also extracts interface Parallelizer to staging repo, leaving struct Parallelizer as internal implementation in k/k.
Which issue(s) this PR is related to:
Part of #89930
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: