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
Graduate DefaultPodTopologySpread to beta #95631
Graduate DefaultPodTopologySpread to beta #95631
Conversation
/triage accepted |
/assign @Huang-Wei |
/retest |
LGTM. Please fix the CI failures. |
19da692
to
a1c75e1
Compare
And set to enabled by default Change-Id: Ie4cc4758c52492924cb0663450f2747908cb5882
a1c75e1
to
87c8349
Compare
Thanks @alculquicondor . /lgtm |
/assign @ahg-g |
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.
/lgtm
we should keep an eye on http://perf-dash.k8s.io/ to see how this will impact the scheduling latency.
@@ -390,38 +390,17 @@ func TestPluginArgsDefaults(t *testing.T) { | |||
MaxSkew: 2, | |||
}, | |||
}, | |||
// TODO(#94008): Make SystemDefaulting in v1beta2. |
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.
and this becomes an invalid configuration (constraints without defaulting type set)?
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.
Correct.
fwk, err := st.NewFramework( | ||
registeredPlugins, | ||
frameworkruntime.WithPodNominator(internalqueue.NewPodNominator()), | ||
frameworkruntime.WithSnapshotSharedLister(snapshot), | ||
frameworkruntime.WithInformerFactory(informerFactory), |
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.
why is this necessary now?
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.
Because when default spreading is enabled, we require the informers for Services and so on.
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, and we were not using selectorspread in the tests (which would have required this).
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.
/hold
I didn't stop to think about that. Let me check
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.
/hold cancel
There is a test for preemption when there is hard topology spreading, which now has constraints by default.
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.
Yes, the test that registers PodTopologySpread plugin now has a system default constraints, and it can pass only with a non-nil informerFactory::
if len(pl.defaultConstraints) != 0 { | |
if h.SharedInformerFactory() == nil { | |
return nil, fmt.Errorf("SharedInformerFactory is nil") | |
} |
@@ -302,7 +298,7 @@ profiles: | |||
{ | |||
Name: "PodTopologySpread", | |||
Args: &config.PodTopologySpreadArgs{ | |||
DefaultingType: config.ListDefaulting, |
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.
in retrospect, this looks strange, defaulting to listdefaulting without constraints.
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.
oh, or is that how we disable the default constraints?
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.
Correct. And that's the default if the feature gate is disabled.
/hold just so you respond to the comments. |
I also need an approval for pkg/features /hold cancel |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, alculquicondor 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 |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Graduate DefaultPodTopologySpread to beta and enable the feature gate by default.
Which issue(s) this PR fixes:
Fixes #94863
kubernetes/enhancements#1258
Special notes for your reviewer:
Implementation requirements are fulfilled
https://git.k8s.io/enhancements/keps/sig-scheduling/1258-default-pod-topology-spread#beta-v120
Pending integration and conformance tests
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: