KEP-1672: updates for v1.24 #3168
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
kind/kep
|
|
||
| # The milestone at which this feature was, or is targeted to be, at each stage. | ||
| milestone: | ||
| alpha: "v1.20" | ||
| beta: "v1.22" |
There was a problem hiding this comment.
my apologies -- seems like I never got around to coming back to update this KEP after some of the last minute changes that happened in v1.22
| @@ -164,6 +164,12 @@ E2E tests: | |||
| * `EndpointSliceTerminatingCondition` is enabled by default. | |||
| * Consensus on scalability implications resulting from additional EndpointSlice writes with approval from sig-scalability. | |||
|
|
|||
| #### GA | |||
There was a problem hiding this comment.
Adding what I think are the GA graduation criteria for this work right now so we have it in mind, but the feature is going to stay beta for v1.24
andrewsykim
changed the title
andrewsykim
changed the title
k8s-ci-robot
added
the
lgtm
|
/assign @wojtek-t |
| * E2E tests validating that terminating pods are properly reflected in EndpointSlice API | ||
| * Additional testing / validation on the scalability/performance impact, requiring approval from sig-scalability. | ||
| * All necessary metrics are in place to provide adequate observability and monitoring for this feature. | ||
|
|
There was a problem hiding this comment.
Couple comments (since I can't comment below);
L208: Were the tests added? If so, can you maybe link them?
L219: IIRC, we were discussing adding some metrics. Has that happened? If not, what is the plan?
[Relying on application-level metrics is not really where we would like to be for such important features like load serving.]
L223: Were the tests run? Can you summarize the findings?
L238: Were metrics added? If so, can you link their names/component exposing them?
There was a problem hiding this comment.
L208: Were the tests added? If so, can you maybe link them?
We have integration tests here as well as rest strategy unit tests here for validating the behavior of the feature gate. I'll link these in the KEP as well.
L219: IIRC, we were discussing adding some metrics. Has that happened? If not, what is the plan?
[Relying on application-level metrics is not really where we would like to be for such important features like load serving.]
I think we can add metrics for total # of endpoints by condition at the very least, let me update the KEP to include this.
L223: Were the tests run? Can you summarize the findings?
There was some manual testing done, will dig through some history to find the results.
L238: Were metrics added? If so, can you link their names/component exposing them?
(see my comment above re: metrics by condition)
There was a problem hiding this comment.
Btw -- we do have endpointslice metrics now that would be useful here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpointslice/metrics/metrics.go#L30-L53
So users would be able to see the amount of endpoint churn that happens due to this change during upgrade. I'll link these in the KEP too.
k8s-ci-robot
removed
the
lgtm
Signed-off-by: Andrew Sy Kim <[email protected]>
k8s-ci-robot
added
the
lgtm
|
I'm sorry - this is on me. Looking at it again now. |
There was a problem hiding this comment.
@andrewsykim - what are the plans for graduating this to GA?
/lgtm
/approve PRR
| #### GA | ||
|
|
||
| * E2E tests validating that terminating pods are properly reflected in EndpointSlice API. | ||
| * Ensure there are no performance/scalability regressions when enabling additional endpointslice writes for terminating endpoints. |
There was a problem hiding this comment.
Well - technically we know the regression exists. But we say that it's acceptable one. Please adjust the wording.
There was a problem hiding this comment.
But I guess we can tweak it when graduating the feature to GA - I don't want to block this PR on this simple comment now.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, thockin, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: Andrew Sy Kim [email protected]