scheduler CC: add v1beta2 API, deprecate plugins #99597
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
There was a problem hiding this comment.
Just to double-check, I did a diff of all files in v1beta1 and v1beta2 and here's a list of all differences minus v1beta1/v1beta2 differences: http://ix.io/2RjG/diff
|
/sig scheduling |
k8s-ci-robot
added
the
sig/scheduling
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
added
size/XXL
k8s-ci-robot
removed
the
needs-rebase
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
I've squashed some commits and added a new commit. The second commit in this PR adds the infrastructure to deprecate plugins in CC; in v1beta2, we want to deprecate NodeLabel and ServiceAffinity. The second commit was a bit tricky from a API point-of-view, so please lmk if I'm doing something wrong. cc @alculquicondor and @ahg-g for the scheduler re the 2nd commit. |
|
I know we're past the 1.21 code freeze, so we'll be targetting v1beta2 in 1.22 (with possibly more changes than there are in this PR) |
adtac
changed the title
k8s-ci-robot
added
the
release-note
sounds good /milestone v1.22 |
adtac
force-pushed
the
v1b2
branch
2 times, most recently
from
5a6d237
to
197054e
Compare
| // because the field will be cleared later by API machinery during | ||
| // conversion. To work around that, we set the API version field of the | ||
| // internal object here to the default version (latest). We need this for | ||
| // later when we need to make decisions based on the external version. |
There was a problem hiding this comment.
IIUC, we set this in two places:
- When generating the default options
- After loading config from file
And the places we're using this field after setting it is:
validateKubeSchedulerProfileLogOrWriteConfigscheduler.New
All other places should be getting the version from frameworkOptions.componentConfigVersion right?
Can we add this information as a comment in the internal types.go?
There was a problem hiding this comment.
That is correct. I've added a comment in the internal KubeSchedulerConfiguration type definition.
| // conversion. To work around that, we set the API version field of the | ||
| // internal object here to the version we read from file. We need this for | ||
| // later when we need to make decisions based on the external version. | ||
| cfgObj.TypeMeta.APIVersion = gvk.GroupVersion().String() |
There was a problem hiding this comment.
do you have a pointer to the decisions made based on this field? it's pretty strange for the external version source to change the interpretation of the converted internal config
There was a problem hiding this comment.
if the only decision made is a pass/fail validation decision, that would be helpful to clarify here. we explicitly don't want the internal configuration getting interpreted differently based on version
There was a problem hiding this comment.
There are two places, the validation and the plugin config defaulting logic inside the framework. The former will continue to exist, the latter will be removed as a followup to this PR (#102211).
There was a problem hiding this comment.
*three places. We also use this in --write-config-to. I've added a comment in the internal KubeSchedulerConfiguration with references to exactly what places use this field.
| gvk = v1beta1.SchemeGroupVersion.WithKind(name + "Args") | ||
| case v1beta2.SchemeGroupVersion.String(): | ||
| gvk = v1beta2.SchemeGroupVersion.WithKind(name + "Args") | ||
| default: |
There was a problem hiding this comment.
should we default to this even if componentConfigVersion is non-empty and is an unknown version, or only if it is ""?
There was a problem hiding this comment.
The original code was doing it regardless of componentConfigVersion, I think this is fine as is. In any case, this defaulting logic will be removed in a followup PR as part of #102211
pkg/scheduler/framework/plugins/serviceaffinity/service_affinity.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/nodepreferavoidpods/node_prefer_avoid_pods.go
Outdated
Show resolved
Hide resolved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adtac, alculquicondor, liggitt 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 |
k8s-ci-robot
added
the
approved
|
/hold pls squash :) |
k8s-ci-robot
added
the
do-not-merge/hold
Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/retest |
What type of PR is this?
/kind api-change
What this PR does / why we need it:
Adds scheduler config v1beta2 API. This PR builds on top of #95308.
xref: #95446 - things completed in this PR:
Which issue(s) this PR fixes:
Related to #94008
Special notes for your reviewer:
Commits in @hprateek43's PR are preserved as-is. See everything after the 3rd commit for the changes I've made on top.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
cc @hprateek43 @alculquicondor @ahg-g @liggitt