-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
KEP-1669 & KEP-1672 updates for v1.22 #2725
Conversation
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
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.
I'm LGTM on content changes. PRR content is in progress
/retest |
/assign @wojtek-t for PRR review |
@@ -0,0 +1,5 @@ | |||
kep-number: 1672 |
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.
Can you please split those two into separate PRs? I can easily imagine one being ready for approval and the other not (given that you opened the PR day before deadline...)
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.
After reviewing it below - it's even more important. I think 1669 is doable (it's very close to be ready), but 1672 is definitely at high risk.
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.
1669 is dependent on 1672 for the terminating condition
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.
But that's fine to have 1669 in alpha and 1672 alpha - let's please split them.
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.
If you're proposing wekeep 1672 in alpha for v1.22, that's fine with me. But I don't want to split the PR because the context between the two is important.
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.
OK, even easier, I will remove the beta milestone now and leave it as alpha, we can discuss in a follow-up if we want beta in v1.22
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.
Reverted to alpha, but I left the PRR changes since we'll need that for discussion later for beta. PTAL
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.
Yeah, I'm just not sure there's time (modulo exception) to discuss :(
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.
No problem, my fault for updating my KEPs so last minute ;)
For what it's worth, I think we addressed most of @wojtek-t 's comments regarding metrics and implications on SLO, so I'm curious what the remaining concerns are.
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.
OK - metrics are not strictly needed for alpha - so we have more time to discuss before beta next cycle.
Let me quickly go over it now.
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,5 @@ | |||
kep-number: 1672 |
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.
I'm saying that I can imagine one of them being ready for approval and the other not, so keeping in a single PR you're risking not having anything (e.g. two exceptions requests later instead of one, etc...)
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Outdated
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md
Show resolved
Hide resolved
|
||
###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service? | ||
|
||
N/A |
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.
That covers total endpoints across all EndpointSlices, that seems reasonable to me. Thoughts @wojtek-t ?
Yes - aggregations are definitely enough for me. Please do.
I admit some ignorance of metrics compat - is it allowable to take a metric like EndpointsDesired which has no labels, and add new labels?
Yes - we're doing that pretty often.
In this example wouldn't we end up double counting endpoints?
That's actually great point...
We would need to make them non-overlapping.
And going back to the question about usefulness of those metrics - I think that having visibility into them can be fairly useful "spot check" if something bad was happening (e.g. suddenly number of terminating suddenty grow up 2x or sth).
9387262
to
1e501d3
Compare
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
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.
I'm LGTM on both, and I think we can commit to exploring better metrics in k-p in general.
/lgtm
/approve
I suspect @wojtek-t is offline (I hope so!!), so hail-mary at @johnbelamaric
@@ -0,0 +1,5 @@ | |||
kep-number: 1672 |
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.
OK - metrics are not strictly needed for alpha - so we have more time to discuss before beta next cycle.
Let me quickly go over it now.
|
||
# The milestone at which this feature was, or is targeted to be, at each stage. | ||
milestone: | ||
alpha: "v1.20" |
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.
nit: change to 1.22 ?
OK - given that metrics etc. sections are not required for Alpha and we reverted the KEP to target alpha, I'm fine with this and approving it [still almost ~1.5h to feature freeze IIUC] But please have a discussion about metrics sooner than 2 days before next feature freeze :) /lgtm |
[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 |
#1669
#1672