Scheduler: remove direct import to /pkg/features #90111
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gavinfish 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 @alculquicondor |
|
|
||
| import "k8s.io/component-base/featuregate" | ||
|
|
||
| // NOTE: constants in this file are copied from pkg/features package to avoid external scheduler dependency |
There was a problem hiding this comment.
scoping feature gates per component seems like a natural progression.
during this process there is the potential for default state of FGs (enabled for >= beta) to drift between the core (pkg/features) and the component that forked the FG. interesting how do you plan to handle that?
@kubernetes/sig-architecture-pr-reviews
There was a problem hiding this comment.
I am not sure what is the common practices for other components. I think the worst situation may be synchronizing it before each release?
There was a problem hiding this comment.
from my perspective, FGs that target a single component should only be known by this component instead of residing in a shared pool.
on the other hand feature gates that apply to multiple components may have a separate lifecycle. so SomeFeatureGate can be Alpha in the ComponentA and Beta at ComponentB. if SomeFeatureGate graduates to Beta in both that's great. but maybe the overall feature should be considered the lowest version between ComponentA-B.
There was a problem hiding this comment.
I don't think that replicating these definitions is a good idea. All but two of these apply to multiple components and their definitions will drift.
if SomeFeatureGate graduates to Beta in both that's great. but maybe the overall feature should be considered the lowest version between ComponentA-B.
that's really confusing for a cluster admin... if there are different aspects that need enabling differently per component, those are probably distinct gates (xref EndpointSlice and EndpointSliceProxying)
An alternative to the scheduler using a global feature map would be to inject the feature states depended on by the scheduler in the scheduler.New() call, so only the cmd/kube-scheduler/app package would depend on the feature gates
There was a problem hiding this comment.
Only BalanceAttachedNodeVolumes and ResourceLimitsPriorityFunction(one IT also depends on this) FGs are only used in scheduler.
/cc @Huang-Wei @damemi @ahg-g @alculquicondor WDYT about handling FG in staging scheduler?
There was a problem hiding this comment.
I also think this is a bad idea. Feature Gates have always been k8s-wide. So it's not a pattern that can be changed lightly.
Certainly, there are features that involve a number of components and features that don't. But we still release all k8s binaries together, so it doesn't make much sense to treat them separately at this point.
Maybe FG should be part of k8s.io/component-base instead.
There was a problem hiding this comment.
I have created an issue to track this at #90136.
| @@ -279,7 +279,7 @@ func New(client clientset.Interface, | |||
| bindTimeoutSeconds: options.bindTimeoutSeconds, | |||
| podInitialBackoffSeconds: options.podInitialBackoffSeconds, | |||
| podMaxBackoffSeconds: options.podMaxBackoffSeconds, | |||
| enableNonPreempting: utilfeature.DefaultFeatureGate.Enabled(kubefeatures.NonPreemptingPriority), | |||
| enableNonPreempting: utilfeature.DefaultFeatureGate.Enabled(schedutil.NonPreemptingPriority), | |||
There was a problem hiding this comment.
instead of importing utilfeature "k8s.io/apiserver/pkg/util/feature" which contains this:
https://github.com/kubernetes/apiserver/blob/master/pkg/util/feature/feature_gate.go
individual components should just define their local "DefaultFeatureGate"
There was a problem hiding this comment.
This PR is only for /pkg/features, so I will make another PR to take care of /pkg/util/feature.
8d32e03 to
306749f
Compare
|
@gavinfish: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
@gavinfish: 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/test-infra repository. |
|
/hold |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. 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/test-infra repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Scheduler: remove direct import to /pkg/features
Which issue(s) this PR fixes:
Ref #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.: