Skip to content
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.

Sign up for GitHub

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

[WIP] Promote KEP-1672 to GA #2938

Closed
wants to merge 2 commits into from
Closed

[WIP] Promote KEP-1672 to GA #2938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aeaca6a
kep-1672: promote EndpointSliceTerminatingCondition feature to GA
andrewsykim Sep 2, 2021
53568f6
KEP-1672: update PRR with added tests
andrewsykim Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions keps/prod-readiness/sig-network/1672.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
kep-number: 1672
alpha:
approver: "@wojtek-t"
beta:
wojtek-t marked this conversation as resolved.
Show resolved Hide resolved
approver: "@wojtek-t"
stable:
approver: "@wojtek-t"
6 changes: 6 additions & 0 deletions keps/sig-network/1672-tracking-terminating-endpoints/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ EndpointSlice will continue to have the `terminating` and `serving` condition se

Yes, there will be strategy API unit tests validating if the new API field is allowed based on the feature gate.

Unit, integration and e2e tests that were as part of these PRs:
- https://github.com/kubernetes/kubernetes/pull/92968
- https://github.com/kubernetes/kubernetes/pull/103645
- https://github.com/kubernetes/kubernetes/pull/103621
- https://github.com/kubernetes/kubernetes/pull/103596

### Rollout, Upgrade and Rollback Planning

###### How can a rollout fail? Can it impact already running workloads?
Expand Down
6 changes: 4 additions & 2 deletions keps/sig-network/1672-tracking-terminating-endpoints/kep.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@ see-also:
replaces: []

# The target maturity stage in the current dev cycle for this KEP.
stage: alpha
stage: stable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fill in the PRR questionaire for this KEP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm there's no diff for PRR cause all the questions were answered in the last release, let me know if there's a specific question I missed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a diff - there are couple questions that weren't answered back then, including:

Are there any tests for feature enablement/disablement?

Have the tests been added? If so, please link them. If not - we shouldn't even go to beta...

What specific metrics should inform a rollback?

We were discussing adding a metric - has that happened?

Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?

Has that happened? Findings?

What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service?

Has the metric mentioned there been added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have the tests been added? If so, please link them. If not - we shouldn't even go to beta...

Oh yes, of course we added tests for feature enablement, do we want the PRR to link to every test case we added? The answer to the question says:

Yes, there will be strategy API unit tests validating if the new API field is allowed based on the feature gate.

Is that enough or do you want more details on the specific test cases (there's a lot)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add more details on metrics, I think there was some back and forth on the viability of per endpoint metrics due to cardinality. But maybe total endpoints by condition is acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that enough or do you want more details on the specific test cases (there's a lot)?

Please link them - no need to describe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added all the PRs that adds unit, integration or e2e tests for this feature under Are there any tests for feature enablement/disablement?


# The most recent milestone for which work toward delivery of this KEP has been
# done. This can be the current (upcoming) milestone, if it is being actively
# worked on.
latest-milestone: "v1.22"
latest-milestone: "v1.23"

# The milestone at which this feature was, or is targeted to be, at each stage.
milestone:
alpha: "v1.20"
beta: "v1.22"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we went Beta in 1.22 without:
(a) having this tracked appropriately by RT?
(b) having PRR approved?

This sounds very wrong to me...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a last-minute decision during a SIG Network call to include this feature as beta in v1.22 -- I guess we slipped the KEP updates though. Sorry, that's my bad :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no particular recollection of how this came to be, and certainly did not mean to bypass any process.

stable: "v1.23"

# The following PRR answers are required at alpha release
# List the feature gate name and the components for which it must be enabled
Expand Down