KEP-3022: graduate Pod Topology Spread MinDomains to stable #4407

sanposhiho · 2024-01-15T14:06:00Z

One-line PR description: graduate Pod Topology Spread MinDomains to stable

Issue link: Min domains in PodTopologySpread #3022

Other comments:

sanposhiho · 2024-01-15T14:06:33Z

sanposhiho · 2024-01-15T14:07:12Z

keps/prod-readiness/sig-scheduling/3022.yaml

+  approver: "@johnbelamaric"
+stable:
+  approver: "@wojtek-t"


@wojtek-t Hi, I just assigned it to you, but please give it to another person if needed.

soltysh

#prr-shadow
Several comments/suggestions from prr-shadow:

Release Signoff Checklist needs to be updated to reflect reality
Update tests coverage in unit tests, links in integration.
Update graduation criteria, currently only alpha are checked and filled, do we addressed all the problems for beta? What are the criteria for GA?
In Are there any missing metrics that would be useful to have to improve observability of this feature? you're pointing to metrics to see which plugins affect to scheduler's decisions in Filter/Score phase kubernetes#110643 which seems like it was solved, can you update the answer based on the current state of code?
Update the implementation history

sftim · 2024-01-26T01:42:22Z

keps/sig-scheduling/3022-min-domains-in-pod-topology-spread/kep.yaml

 milestone:
  alpha: "v1.24"
  beta: "v1.25"
+  stable: "v1.30"
 disable-supported: true


Do we still support disabling it?

sanposhiho · 2024-01-28T03:14:16Z

@soltysh @sftim Updated based on your suggestions, thanks.

alculquicondor

/approve
/lgtm

We did good progress with the new metric :D

wojtek-t

Two small comments from the PRR.

wojtek-t · 2024-02-02T13:13:19Z

keps/sig-scheduling/3022-min-domains-in-pod-topology-spread/README.md

+
+#### GA (v1.30):
+
+- [x] No particular issue is reported to this feature for a certain length of time.


Can't comment on unchanged lines, so commenting here:

"Are there any tests for feature enablement/disablement?"

Please fill in. In particular, this comment from the KEP template is relevant:

Additionally, for features that are introducing a new API field, unit tests that are exercising the `switch` of feature gate itself (what happens if I disable a feature gate after having objects written with the new field) are also critical. You can take a look at one potential example of such test in: https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246-R282

Actually, we missed such test and we only did the manual testing for switching the feature gate.
I created the PR to add the integration test in kubernetes/kubernetes#123115. However if we will remove the feature gate when reaching GA, the PR is no longer needed.

Please update with the text like this then [agree it's too late now...]

@wojtek-t Fixed 🙏

wojtek-t · 2024-02-02T13:14:23Z

keps/sig-scheduling/3022-min-domains-in-pod-topology-spread/README.md

-Issue: https://github.com/kubernetes/kubernetes/issues/110643
+It was lacking the way to see which filter plugin affected Pod's scheduling results in metrics
+(https://github.com/kubernetes/kubernetes/issues/110643),
+and we've got `plugin_evaluation_total`.


It's not a lacking metric anymore. Let' move it to "How can someone using this feature know that it is working for their instance?" question - as it actually helps with that.
Please also better describe this metric.

sanposhiho · 2024-02-04T04:44:56Z

@alculquicondor

Rethinking about the comment from @sftim.
#4407 (comment)

Do we normally keep the feature gate for a few releases even after the feature reached GA? I checked other feature gates but some of them are remaining even though they're already GA, I'm not sure those are just forgotten to be removed, or intentionally kept for a few releases to keep support of the disablement.

wojtek-t

One minor nit.

wojtek-t · 2024-02-05T10:08:22Z

keps/sig-scheduling/3022-min-domains-in-pod-topology-spread/README.md

+
+#### GA (v1.30):
+
+- [x] No particular issue is reported to this feature for a certain length of time.


Please update with the text like this then [agree it's too late now...]

alculquicondor · 2024-02-05T15:27:10Z

Do we normally keep the feature gate for a few releases even after the feature reached GA? I checked other feature gates but some of them are remaining even though they're already GA, I'm not sure those are just forgotten to be removed, or intentionally kept for a few releases to keep support of the disablement.

Yes, we do keep the feature gate for a couple of releases, so that someone that a manifest that has MyFeature=true continues to run. But we ignore the feature gate in code.

It is common that we forget to remove the gate.

sanposhiho · 2024-02-06T12:38:34Z

Got it, so we keep the feature gate not to break users' configuration but not support the disablement actually.

wojtek-t · 2024-02-07T07:22:32Z

/lgtm
/approve PRR

k8s-ci-robot · 2024-02-07T07:22:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanposhiho, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-scheduling/OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

KEP-3022: graduate Pod Topology Spread MinDomains to stable

3025bc9

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 15, 2024

k8s-ci-robot requested a review from Huang-Wei January 15, 2024 14:06

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jan 15, 2024

k8s-ci-robot requested a review from jeremyrickard January 15, 2024 14:06

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 15, 2024

k8s-ci-robot requested a review from alculquicondor January 15, 2024 14:06

sanposhiho commented Jan 15, 2024

View reviewed changes

soltysh reviewed Jan 23, 2024

View reviewed changes

sftim reviewed Jan 26, 2024

View reviewed changes

fix: update based on the reviews

cd58b39

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 28, 2024

update lint in integration tests

2df15e4

update toc

e8be17a

alculquicondor reviewed Feb 1, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Feb 1, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2024

wojtek-t reviewed Feb 2, 2024

View reviewed changes

sanposhiho mentioned this pull request Feb 4, 2024

add the integration test for the MinDomain feature gate switching kubernetes/kubernetes#123115

Closed

address the review

ca70daf

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2024

wojtek-t reviewed Feb 5, 2024

View reviewed changes

pnbrown mentioned this pull request Feb 5, 2024

Min domains in PodTopologySpread #3022

Closed

12 tasks

fix based on the review

5fd5087

k8s-ci-robot assigned wojtek-t Feb 7, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024

k8s-ci-robot merged commit fd4dc6c into kubernetes:master Feb 7, 2024
4 checks passed

k8s-ci-robot added this to the v1.30 milestone Feb 7, 2024

sanposhiho mentioned this pull request Feb 24, 2024

graduate MinDomainsInPodTopologySpread to stable kubernetes/kubernetes#123481

Merged

soltysh mentioned this pull request Apr 3, 2024

Add soltysh to prod-readiness-approvers #4566

Merged

sanposhiho mentioned this pull request Apr 23, 2024

Remove MinDomainsInPodTopologySpread feature gate kubernetes/kubernetes#124460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3022: graduate Pod Topology Spread MinDomains to stable #4407

KEP-3022: graduate Pod Topology Spread MinDomains to stable #4407

sanposhiho commented Jan 15, 2024

sanposhiho commented Jan 15, 2024

sanposhiho Jan 15, 2024

soltysh left a comment

sftim Jan 26, 2024

sanposhiho commented Jan 28, 2024

alculquicondor left a comment

wojtek-t left a comment

wojtek-t Feb 2, 2024

sanposhiho Feb 4, 2024

wojtek-t Feb 5, 2024

sanposhiho Feb 6, 2024

wojtek-t Feb 2, 2024

sanposhiho Feb 4, 2024

sanposhiho commented Feb 4, 2024 •

edited

Loading

wojtek-t left a comment

wojtek-t Feb 5, 2024

alculquicondor commented Feb 5, 2024

sanposhiho commented Feb 6, 2024

wojtek-t commented Feb 7, 2024

k8s-ci-robot commented Feb 7, 2024


		#### GA (v1.30):

		- [x] No particular issue is reported to this feature for a certain length of time.

KEP-3022: graduate Pod Topology Spread MinDomains to stable #4407

KEP-3022: graduate Pod Topology Spread MinDomains to stable #4407

Conversation

sanposhiho commented Jan 15, 2024

sanposhiho commented Jan 15, 2024

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Jan 28, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Feb 4, 2024 • edited Loading

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Feb 5, 2024

sanposhiho commented Feb 6, 2024

wojtek-t commented Feb 7, 2024

k8s-ci-robot commented Feb 7, 2024

sanposhiho commented Feb 4, 2024 •

edited

Loading