KEP-1669 & KEP-1672 updates for v1.22 #2725

andrewsykim · 2021-05-12T15:05:40Z

#1669
#1672

keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md

keps/sig-network/1672-tracking-terminating-endpoints/README.md

thockin

I'm LGTM on content changes. PRR content is in progress

andrewsykim · 2021-05-12T17:29:21Z

/retest

andrewsykim · 2021-05-12T18:29:18Z

/assign @wojtek-t

for PRR review

wojtek-t · 2021-05-13T06:20:59Z

keps/prod-readiness/sig-network/1672.yaml

@@ -0,0 +1,5 @@
+kep-number: 1672


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...)

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.

1669 is dependent on 1672 for the terminating condition

But that's fine to have 1669 in alpha and 1672 alpha - let's please split them.

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.

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

Reverted to alpha, but I left the PRR changes since we'll need that for discussion later for beta. PTAL

Yeah, I'm just not sure there's time (modulo exception) to discuss :(

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.

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/1672-tracking-terminating-endpoints/README.md

keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md

wojtek-t · 2021-05-13T17:41:00Z

keps/prod-readiness/sig-network/1672.yaml

@@ -0,0 +1,5 @@
+kep-number: 1672


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

wojtek-t · 2021-05-13T18:03:18Z

keps/sig-network/1672-tracking-terminating-endpoints/README.md

+
+###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service?
+
+N/A


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).

keps/sig-network/1672-tracking-terminating-endpoints/README.md

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

thockin

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

wojtek-t · 2021-05-14T05:28:48Z

keps/prod-readiness/sig-network/1672.yaml

@@ -0,0 +1,5 @@
+kep-number: 1672


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.

wojtek-t · 2021-05-14T05:31:21Z

keps/sig-network/1672-tracking-terminating-endpoints/kep.yaml

+
+# The milestone at which this feature was, or is targeted to be, at each stage.
+milestone:
+ alpha: "v1.20"


nit: change to 1.22 ?

wojtek-t · 2021-05-14T05:33:41Z

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 :)
@andrewsykim @thockin @aojea

/lgtm
/approve

k8s-ci-robot · 2021-05-14T05:34:25Z

[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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-network/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 12, 2021

k8s-ci-robot requested review from dcbw and thockin May 12, 2021 15:05

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 12, 2021

thockin reviewed May 12, 2021

View reviewed changes

keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md Show resolved Hide resolved

thockin reviewed May 12, 2021

View reviewed changes

keps/sig-network/1672-tracking-terminating-endpoints/README.md Show resolved Hide resolved

thockin reviewed May 12, 2021

View reviewed changes

andrewsykim force-pushed the kep-1672 branch from 46a22c1 to 8fc56d4 Compare May 12, 2021 16:21

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 12, 2021

andrewsykim force-pushed the kep-1672 branch from 8fc56d4 to da063bf Compare May 12, 2021 16:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2021

andrewsykim force-pushed the kep-1672 branch from da063bf to bbe8178 Compare May 12, 2021 16:30

andrewsykim force-pushed the kep-1672 branch from bbe8178 to 2ca35a2 Compare May 12, 2021 18:09

andrewsykim changed the title ~~[WIP] KEP-1669 & KEP-1672 updates for v1.22~~ KEP-1669 & KEP-1672 updates for v1.22 May 12, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2021

k8s-ci-robot assigned wojtek-t May 12, 2021

wojtek-t reviewed May 13, 2021

View reviewed changes

wojtek-t assigned thockin May 13, 2021

aojea reviewed May 13, 2021

View reviewed changes

keps/sig-network/1669-graceful-termination-local-external-traffic-policy/README.md Outdated Show resolved Hide resolved

wojtek-t reviewed May 13, 2021

View reviewed changes

salaxander mentioned this pull request May 13, 2021

Proxy Terminating Endpoints #1669

Closed

andrewsykim force-pushed the kep-1672 branch 2 times, most recently from 9387262 to 1e501d3 Compare May 13, 2021 21:56

andrewsykim force-pushed the kep-1672 branch from 1e501d3 to 7454fd1 Compare May 13, 2021 22:00

kep-1669: update alpha milestones for v1.22

95c6f59

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

andrewsykim force-pushed the kep-1672 branch from 7454fd1 to 6ac5c9d Compare May 13, 2021 23:37

kep-1672: update beta milestones for v1.22

d248614

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

andrewsykim force-pushed the kep-1672 branch from 6ac5c9d to d248614 Compare May 13, 2021 23:39

thockin added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 13, 2021

thockin reviewed May 13, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2021

wojtek-t reviewed May 14, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2021

k8s-ci-robot merged commit 82b70b4 into kubernetes:master May 14, 2021

k8s-ci-robot added this to the v1.22 milestone May 14, 2021

JamesLaverack mentioned this pull request May 14, 2021

Tracking Terminating Endpoints #1672

Closed

palnabarun mentioned this pull request Jun 12, 2022

canonical kep number json field palnabarun/enhancements#1

Closed

palnabarun mentioned this pull request Jul 26, 2022

KEP Manifest Generator palnabarun/enhancements#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-1669 & KEP-1672 updates for v1.22 #2725

KEP-1669 & KEP-1672 updates for v1.22 #2725

andrewsykim commented May 12, 2021

thockin left a comment

andrewsykim commented May 12, 2021

andrewsykim commented May 12, 2021

wojtek-t May 13, 2021

wojtek-t May 13, 2021

andrewsykim May 13, 2021

wojtek-t May 13, 2021

andrewsykim May 13, 2021

andrewsykim May 13, 2021

andrewsykim May 13, 2021 •

edited

Loading

thockin May 13, 2021

andrewsykim May 13, 2021

wojtek-t May 14, 2021

wojtek-t May 13, 2021

wojtek-t May 13, 2021

thockin left a comment

wojtek-t May 14, 2021

wojtek-t May 14, 2021

wojtek-t commented May 14, 2021

k8s-ci-robot commented May 14, 2021


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

		N/A

KEP-1669 & KEP-1672 updates for v1.22 #2725

KEP-1669 & KEP-1672 updates for v1.22 #2725

Conversation

andrewsykim commented May 12, 2021

thockin left a comment

Choose a reason for hiding this comment

andrewsykim commented May 12, 2021

andrewsykim commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented May 14, 2021

k8s-ci-robot commented May 14, 2021

andrewsykim May 13, 2021 •

edited

Loading