[KEP-3022] Write the production readiness requirements to graduate to beta #3338
There are no files selected for viewing
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,5 @@ | |
| kep-number: 3022 | ||
| alpha: | ||
| approver: "@wojtek-t" | ||
| beta: | ||
| approver: "@wojtek-t" | ||
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| # KEP-3022: min domains in Pod Topology Spread | ||
|
|
||
| <!-- toc --> | ||
| - [Release Signoff Checklist](#release-signoff-checklist) | ||
| - [Summary](#summary) | ||
| - [Motivation](#motivation) | ||
|
|
@@ -14,6 +13,10 @@ | |
| - [Implementation details](#implementation-details) | ||
| - [How user stories are addressed](#how-user-stories-are-addressed) | ||
| - [Test Plan](#test-plan) | ||
| - [Prerequisite testing updates](#prerequisite-testing-updates) | ||
| - [Unit tests](#unit-tests) | ||
| - [Integration tests](#integration-tests) | ||
| - [e2e tests](#e2e-tests) | ||
| - [Graduation Criteria](#graduation-criteria) | ||
| - [Alpha (v1.24):](#alpha-v124) | ||
| - [Beta (v1.25):](#beta-v125) | ||
|
|
@@ -198,6 +201,13 @@ With the flow, this deployment will be spread over at least 5 Nodes while protec | |
|
|
||
| ### Test Plan | ||
|
|
||
|
|
||
| [x] I/we understand the owners of the involved components may require updates to | ||
| existing tests to make this code solid enough prior to committing the changes necessary | ||
| to implement this enhancement. | ||
|
|
||
| ##### Prerequisite testing updates | ||
|
|
||
| To ensure this feature to be rolled out in high quality. Following tests are mandatory: | ||
|
|
||
| - **Unit Tests**: All core changes must be covered by unit tests. | ||
|
|
@@ -207,6 +217,52 @@ To ensure this feature to be rolled out in high quality. Following tests are man | |
| using this feature, but it shouldn't impose penalty to users who are not using | ||
| this feature. We will verify it by designing some benchmark tests. | ||
|
|
||
| ##### Unit tests | ||
|
|
||
| <!-- | ||
| In principle every added code should have complete unit test coverage, so providing | ||
| the exact set of tests will not bring additional value. | ||
| However, if complete unit test coverage is not possible, explain the reason of it | ||
| together with explanation why this is acceptable. | ||
| --> | ||
|
|
||
| <!-- | ||
| Additionally, for Alpha try to enumerate the core package you will be touching | ||
| to implement this enhancement and provide the current unit coverage for those | ||
| in the form of: | ||
| - <package>: <date> - <current test coverage> | ||
| The data can be easily read from: | ||
| https://testgrid.k8s.io/sig-testing-canaries#ci-kubernetes-coverage-unit | ||
| This can inform certain test coverage improvements that we want to do before | ||
| extending the production code to implement this enhancement. | ||
| --> | ||
|
|
||
| - `k8s.io/kubernetes/pkg/scheduler/framework/plugins/podtopologyspread`: `2020-06-17` - `86%` | ||
|
There was a problem hiding this comment. also mention the pod validation packages |
||
|
|
||
| ##### Integration tests | ||
|
|
||
| <!-- | ||
| This question should be filled when targeting a release. | ||
| For Alpha, describe what tests will be added to ensure proper quality of the enhancement. | ||
| For Beta and GA, add links to added tests together with links to k8s-triage for those tests: | ||
| https://storage.googleapis.com/k8s-triage/index.html | ||
| --> | ||
|
|
||
| test: https://github.com/kubernetes/kubernetes/blob/19c8a2127177b43effb9bfe1de272d6f08ea56e8/test/integration/scheduler/filters/filters_test.go#L1060 | ||
| k8s-triage: https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling&test=TestPodTopologySpreadFilter | ||
|
|
||
| ##### e2e tests | ||
|
|
||
| <!-- | ||
| This question should be filled when targeting a release. | ||
| For Alpha, describe what tests will be added to ensure proper quality of the enhancement. | ||
| For Beta and GA, add links to added tests together with links to k8s-triage for those tests: | ||
| https://storage.googleapis.com/k8s-triage/index.html | ||
| We expect no non-infra related flakes in the last month as a GA graduation criteria. | ||
| --> | ||
|
|
||
| N/A | ||
|
There was a problem hiding this comment. explain why: possible explanations is that there are no new API endpoints and that the feature doesn't interact with other components, so E2E doesn't add extra value to integration tests. |
||
|
|
||
| ### Graduation Criteria | ||
|
|
||
| #### Alpha (v1.24): | ||
|
|
@@ -292,13 +348,23 @@ rollout. Similarly, consider large clusters and how enablement/disablement | |
| will rollout across nodes. | ||
| --> | ||
|
|
||
| It shouldn't impact already running workloads. It's an opt-in feature, | ||
|
There was a problem hiding this comment. Will review tomorrow - in the meantime, if you're targeting beta, please update the kep.yaml and corresponding prr file. There was a problem hiding this comment. There are new requirements for the Test Plan. Please check the updated template. |
||
| and users need to set `pod.spec.topologySpreadConstraints.minDomains` field to use this feature. | ||
|
|
||
| When this feature is disabled by the feature flag, the already created Pod's `pod.spec.topologySpreadConstraints.minDomains` field is preserved, | ||
| but, the newly created Pod's `pod.spec.topologySpreadConstraints.minDomains` field is silently dropped. | ||
|
|
||
|
|
||
| ###### What specific metrics should inform a rollback? | ||
|
|
||
| <!-- | ||
| What signals should users be paying attention to when the feature is young | ||
| that might indicate a serious problem? | ||
| --> | ||
|
|
||
| - A spike on metric `schedule_attempts_total{result="error|unschedulable"}` when pods using this feature are added. | ||
| - A spike on metric `plugin_execution_duration_seconds{plugin="PodTopologySpread"}` or `scheduling_algorithm_duration_seconds` when pods using this feature are added. | ||
|
|
||
| ###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested? | ||
|
|
||
| <!-- | ||
|
|
@@ -307,12 +373,30 @@ Longer term, we may want to require automated upgrade/rollback tests, but we | |
| are missing a bunch of machinery and tooling and can't do that now. | ||
| --> | ||
|
|
||
| Yes. The behavior is changed as expected. | ||
|
There was a problem hiding this comment. Describe the manual testing that was done :) There was a problem hiding this comment. I'm ok with this manual testing. Although, a test that most closely follows the question is when you actually do an upgrade. So you start with an apiserver in version 1.24 (with feature disabled) and then upgrade to 1.25 with feature enabled and back. There was a problem hiding this comment. Okay, I'll do another manual test. |
||
|
|
||
| Test scenario: | ||
| 1. start kube-apiserver where `MinDomains` feature is enabled. | ||
| 2. create three nodes and pods spread across nodes as 2/2/1 | ||
| 3. create new Pod that has a TopologySpreadConstraints: maxSkew is 1, topologyKey is `kubernetes.io/hostname`, and minDomains is 4 (larger than the number of domains (= 3)). | ||
| 4. the Pod created in (3) isn't scheduled because of `MinDomain`. | ||
| 5. delete the Pod created in (3). | ||
| 6. recreate kube-apiserver where `MinDomains` feature is disabled. | ||
| 7. create the same Pod as (3). | ||
| 8. the Pod created in (7) is scheduled because `MinDomain` is disabled. | ||
| 9. delete the Pod created in (7). | ||
| 10. recreate kube-apiserver where `MinDomains` feature is enabled. | ||
| 11. create the same Pod as (3). | ||
| 12. the Pod created in (11) isn't scheduled because of `MinDomain`. | ||
|
|
||
| ###### Is the rollout accompanied by any deprecations and/or removals of features, APIs, fields of API types, flags, etc.? | ||
|
|
||
| <!-- | ||
| Even if applying deprecation policies, they may still surprise some users. | ||
| --> | ||
|
|
||
| No. | ||
|
|
||
| ### Monitoring Requirements | ||
|
|
||
| <!-- | ||
|
|
@@ -327,6 +411,8 @@ checking if there are objects with field X set) may be a last resort. Avoid | |
| logs or events for this purpose. | ||
| --> | ||
|
|
||
| The operator can query pods with `pod.spec.topologySpreadConstraints.minDomains` field set. | ||
|
There was a problem hiding this comment. I wonder if we could extend kubernetes/kubernetes#107556 to produce a metric. But I wouldn't block beta graduation to this. There was a problem hiding this comment. We definitely need kubernetes/kubernetes#107556...
So, what metrics do you imagine like? There was a problem hiding this comment. can you create an issue so we can discuss there? But it would be something about which filter plugins influenced a pod scheduling decision. One counter increment for each plugin. There was a problem hiding this comment. Sure, will create that. There was a problem hiding this comment. Opened: kubernetes/kubernetes#110643 |
||
|
|
||
| ###### How can someone using this feature know that it is working for their instance? | ||
|
|
||
| <!-- | ||
|
|
@@ -338,13 +424,13 @@ and operation of this feature. | |
| Recall that end users cannot usually observe component logs or access metrics. | ||
| --> | ||
|
|
||
| - [x] Other (treat as last resort) | ||
| - Details: | ||
| The feature MinDomains in Pod Topology Sprad plugin doesn't cause any logs, any events, any pod status updates. | ||
| If a Pod using `pod.spec.topologySpreadConstraints.minDomains` was successfully assigned a Node, | ||
| nodeName will be updated. | ||
| And if not, `PodScheduled` condition will be false and an event will be recorded with a detailed message | ||
| describing the reason including the failed filters. (Pod Topology Spread plugin could be one of them.) | ||
|
|
||
| ###### What are the reasonable SLOs (Service Level Objectives) for the enhancement? | ||
|
|
||
|
|
@@ -363,18 +449,18 @@ These goals will help you determine what you need to measure (SLIs) in the next | |
| question. | ||
| --> | ||
|
|
||
| - Metric `plugin_execution_duration_seconds{plugin="PodTopologySpread"}` <= 100ms on 90-percentile. | ||
|
|
||
| ###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service? | ||
|
|
||
| <!-- | ||
| Pick one more of these and delete the rest. | ||
| --> | ||
|
|
||
| - [x] Metrics | ||
| - Component exposing the metric: kube-scheduler | ||
| - Metric name: `plugin_execution_duration_seconds{plugin="PodTopologySpread"}` | ||
| - Metric name: `schedule_attempts_total{result="error|unschedulable"}` | ||
|
|
||
| ###### Are there any missing metrics that would be useful to have to improve observability of this feature? | ||
|
|
||
|
|
@@ -383,6 +469,8 @@ Describe the metrics themselves and the reasons why they weren't added (e.g., co | |
| implementation difficulties, etc.). | ||
| --> | ||
|
|
||
| Yes. It would be useful if we could see more details related to scheduler's decisions in metrics. | ||
|
There was a problem hiding this comment. Link to the issue. Maybe be more specific about decisions. In this case, we would like to know which Filters had impact on the scheduling of the pod. |
||
|
|
||
| ### Dependencies | ||
|
|
||
| <!-- | ||
|
|
@@ -406,6 +494,8 @@ and creating new ones, as well as about cluster-level services (e.g. DNS): | |
| - Impact of its degraded performance or high-error rates on the feature: | ||
| --> | ||
|
|
||
| No. | ||
|
|
||
| ### Scalability | ||
|
|
||
| ###### Will enabling / using this feature result in any new API calls? | ||
|
|
@@ -466,8 +556,13 @@ details). For now, we leave it here. | |
|
|
||
| ###### How does this feature react if the API server and/or etcd is unavailable? | ||
|
|
||
| The feature isn't affected because Pod Topology Spread plugin doesn't communicate with kube-apiserver or etcd | ||
| during Filter phase. | ||
|
|
||
| ###### What are other known failure modes? | ||
|
|
||
| N/A | ||
|
|
||
| <!-- | ||
| For each of them, fill in the following information by copying the below template: | ||
| - [Failure mode brief description] | ||
|
|
@@ -483,6 +578,12 @@ For each of them, fill in the following information by copying the below templat | |
|
|
||
| ###### What steps should be taken if SLOs are not being met to determine the problem? | ||
|
|
||
| - Check `plugin_execution_duration_seconds{plugin="PodTopologySpread"}` to see if latency increased. | ||
| - In this case, the metrics showes literally the feature is slow. | ||
| - You should stop using `MinDomains` in your Pods and may need to disable `MinDomains` feature by feature flag `MinDomainsInPodTopologySpread`. | ||
| - Check `schedule_attempts_total{result="error|unschedulable"}` to see if the number of attempts increased. | ||
|
There was a problem hiding this comment. What should I do if I see problems in either of the metrics? |
||
| - In this case, your use of `MinDomains` may be incorrect or not appropriate for your cluster. | ||
|
|
||
| ## Implementation History | ||
|
|
||
| <!-- | ||
|
|
@@ -496,6 +597,11 @@ Major milestones might include: | |
| - when the KEP was retired or superseded | ||
| --> | ||
|
|
||
| - 2021-11-02: Initial KEP sent for review | ||
| - 2022-01-14: Initial KEP is merged. | ||
| - 2022-03-16: The implementation PRs are merged. | ||
| - 2022-05-03: The MinDomain feature is released as alpha feature with Kubernetes v1.24 release. | ||
|
|
||
| ## Drawbacks | ||
|
|
||
| <!-- | ||
|
|
||
This file contains 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
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.
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.
Wojtek is out, please change to me.
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.
Sure, thanks for taking over! 🙏