KEP-3094: Graduate NodeInclusionPolicy to Beta in v1.26 #3539

kerthcet · 2022-09-22T08:19:51Z

Signed-off-by: kerthcet [email protected]

One-line PR description: Graduate NodeInclusionPolicy to Beta

Issue link: Take taints/tolerations into consideration when calculating PodTopologySpread skew #3094

Other comments:

Signed-off-by: kerthcet <[email protected]>

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md

Signed-off-by: kerthcet <[email protected]>

alculquicondor · 2022-09-26T10:25:15Z

Still missing the kep.yaml update.

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2022-09-26T13:03:57Z

Update the stage to beta.

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md

alculquicondor · 2022-09-26T13:33:00Z

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md

 ###### What steps should be taken if SLOs are not being met to determine the problem?
-N/A
+
+Check the metrics to see whether it was caused by this feature, if so, disable the feature gate.


How do we determine that this feature is the problem?

To some extent, we can observe this via the Metric plugin_execution_duration_seconds{plugin="PodTopologySpread"}, if we see obviously execution time latency when enabling the feature-gate, then we may have some problems in PodTopologySpread plugin.

Signed-off-by: kerthcet <[email protected]>

alculquicondor

lgtm from sig-scheduling

alculquicondor · 2022-09-27T07:53:51Z

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md

-      ```
-    6. Both pods will be scheduled successfully.
+  - Details: We can only observe the behaviors based on pod scheduling results.
+  We can follow the steps described at [Upgrade / Downgrade Strategy](#upgrade--downgrade-strategy).


how is this related?
I would just remove this line

In case someone want to know the details about how to reproduce this. I'll remove this.

Signed-off-by: kerthcet <[email protected]>

alculquicondor · 2022-09-27T08:12:23Z

/approve
/hold for prr

kerthcet · 2022-09-28T03:16:16Z

cc @wojtek-t for PRR.

wojtek-t

Couple small nits - other than that LGTM.

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md

wojtek-t · 2022-09-28T07:41:09Z

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md

 are missing a bunch of machinery and tooling and can't do that now.
 -->

+Not yet, but it will be tested manually prior to upgrade following below steps:


The scenario LGTM - please update the PRR once you run it (fine to do this after feature freeze, but please do that before graduating the feature to beta in k/k).

Then let's merge this PR first for I may leave several days.. And I'll update this section after.

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2022-09-29T09:18:09Z

cc @wojtek-t for another round of review. Thanks.

wojtek-t · 2022-09-29T09:39:36Z

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md


 ###### Are there any tests for feature enablement/disablement?
-No, appropriate unit tests will be added for Alpha.
+Yes, both unit tests and integration tests are added.


Can you please link them here?

Updated @wojtek-t

Those aren't typical enablement/disablement tests - those test the feature when it is enabled/disabled.
The enablement/disablement tests change whether the feature is enabled/disabled in the middle of the test.

On the scheduler side this seems to be good enough, because in scheduler this is basically "in-memory" feature.
So maybe please add something like:

"In the scheduler, this is in-memory feature, so only tests checking both feature being enabled and disabled were added".

However, this KEP is also introducing an API change, so a test similar to this one would be useful on the registry side:
https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246-R282

Would you be able to add something like that?
I don't want to block this PR, so if you could add here that a strategy test will be added and add that to beta graduation criteria, I would be fine.

I add the test here kubernetes/kubernetes#112805, PTAL @wojtek-t

@kerthcet - I don't want to block Beta on it - can you please add here that such test will be added (and add that to beta graduation criteria) and I will approve the PRR then?

Updated the enablement tests again. PTAL.

@kerthcet the KEP freeze is tomorrow. Just update this PR to say that the tests will be added (linking to the PR is fine too).

Signed-off-by: kerthcet <[email protected]>

alculquicondor · 2022-10-06T07:10:14Z

/lgtm
/label tide/merge-method-squash

wojtek-t · 2022-10-06T07:13:52Z

/lgtm
/approve PRR

/hold cancel

k8s-ci-robot · 2022-10-06T07:14:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, kerthcet, 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

) * Graduate NodeInclusionPolicy to Beta Signed-off-by: kerthcet <[email protected]> * update PRR files Signed-off-by: kerthcet <[email protected]> * update the stage to beta Signed-off-by: kerthcet <[email protected]> * address comments Signed-off-by: kerthcet <[email protected]> * address comments Signed-off-by: kerthcet <[email protected]> * Make sure feature gate enable/disable tested Signed-off-by: kerthcet <[email protected]> * add feature gate enabled/disabled testcases Signed-off-by: kerthcet <[email protected]> * Add descriptions about feature enable/disable tests Signed-off-by: kerthcet <[email protected]> Signed-off-by: kerthcet <[email protected]>

Graduate NodeInclusionPolicy to Beta

6e88424

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 22, 2022

k8s-ci-robot requested review from Huang-Wei and alculquicondor September 22, 2022 08:19

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 22, 2022

kerthcet mentioned this pull request Sep 22, 2022

Take taints/tolerations into consideration when calculating PodTopologySpread skew #3094

Closed

12 tasks

kerthcet changed the title ~~Graduate NodeInclusionPolicy to Beta~~ KEP-3094: Graduate NodeInclusionPolicy to Beta Sep 22, 2022

kerthcet changed the title ~~KEP-3094: Graduate NodeInclusionPolicy to Beta~~ KEP-3094: Graduate NodeInclusionPolicy to Beta in v1.26 Sep 22, 2022

wojtek-t self-assigned this Sep 26, 2022

wojtek-t reviewed Sep 26, 2022

View reviewed changes

keps/sig-scheduling/3094-pod-topology-spread-considering-taints/README.md Show resolved Hide resolved

update PRR files

a71eb01

Signed-off-by: kerthcet <[email protected]>

update the stage to beta

af9efb2

Signed-off-by: kerthcet <[email protected]>

alculquicondor reviewed Sep 26, 2022

View reviewed changes

address comments

1fa269a

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 27, 2022

alculquicondor reviewed Sep 27, 2022

View reviewed changes

address comments

45bdcbd

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2022

wojtek-t reviewed Sep 28, 2022

View reviewed changes

Make sure feature gate enable/disable tested

ee84f38

Signed-off-by: kerthcet <[email protected]>

wojtek-t reviewed Sep 29, 2022

View reviewed changes

add feature gate enabled/disabled testcases

8a91b98

Signed-off-by: kerthcet <[email protected]>

Add descriptions about feature enable/disable tests

391e3dd

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 6, 2022

k8s-ci-robot assigned alculquicondor Oct 6, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2022

k8s-ci-robot merged commit eb13a25 into kubernetes:master Oct 6, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 6, 2022

kerthcet deleted the feat/graduate-nodeInclusionPoplicy-to-beta branch October 6, 2022 07:27

kerthcet mentioned this pull request Nov 4, 2022

Update nodeInclusionPolicy beta graduation progress #3645

Merged

KEP-3094: Graduate NodeInclusionPolicy to Beta in v1.26 #3539

KEP-3094: Graduate NodeInclusionPolicy to Beta in v1.26 #3539

Uh oh!

Conversation

kerthcet commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alculquicondor commented Sep 26, 2022

Uh oh!

kerthcet commented Sep 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerthcet Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alculquicondor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alculquicondor commented Sep 27, 2022

Uh oh!

kerthcet commented Sep 28, 2022

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerthcet commented Sep 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alculquicondor commented Oct 6, 2022

Uh oh!

wojtek-t commented Oct 6, 2022

Uh oh!

k8s-ci-robot commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kerthcet commented Sep 22, 2022 •

edited

Loading

kerthcet Sep 27, 2022 •

edited

Loading