Graduate EndpointSlice API to GA #99662

swetharepakula · 2021-03-02T23:22:32Z

removes discovery v1alpha1 API
removes EndpointSlice Topology field from GA API
adds per Endpoint Zone field in GA API

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Graduates the EndpointSlice API to GA and removes the v1alpha1 API.

Does this PR introduce a user-facing change?

EndpointSlice API is now GA. The EndpointSlice topology field has been removed from the GA API and will be replaced by a new per Endpoint Zone field. If the topology field was previously used, it will be converted into an annotation in the v1 Resource. The discovery.k8s.io/v1alpha1 API is removed.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: EndpointSlice Enhancement

/cc @aojea @andrewsykim @liggitt @robscott @wojtek-t
/assign @thockin

k8s-ci-robot · 2021-03-02T23:22:40Z

Hi @swetharepakula. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

robscott · 2021-03-02T23:26:20Z

Thanks for the work on this!

/ok-to-test
/sig network
/triage accepted
/priority important-soon

swetharepakula · 2021-03-05T01:57:39Z

I have made an update to PR with the new approach outlined in the above comment. It is ready for review again. Thank you all for all the feedback and help!

pkg/apis/discovery/validation/validation.go

thockin · 2021-03-05T07:00:35Z

pkg/registry/discovery/endpointslice/strategy_test.go

+		},
+	}
+
+	for _, tc := range testcases {


Great testcases

thockin

This looks pretty solid to me

pkg/apis/discovery/install/install.go

test/integration/etcd/data.go

pkg/apis/discovery/types.go

pkg/controlplane/storageversionhashdata/data.go

pkg/registry/discovery/endpointslice/strategy.go

liggitt · 2021-03-05T07:14:57Z

pkg/registry/discovery/endpointslice/strategy.go

+		}
+
+		requestGV := schema.GroupVersion{Group: info.APIGroup, Version: info.APIVersion}
+		if requestGV == discoveryv1.SchemeGroupVersion {


suggest checking our version and returning early for v1beta1 specifically. we'd want to drop deprecated topology the same way in future versions without needing to revisit this code.

func dropTopologyOnV1(ctx context.Context, oldEPS, newEPS *discovery.EndpointSlice) { if info, ok := genericapirequest.RequestInfoFrom(ctx); ok { // ... return early if v1beta1 } ... check deepequal, drop, etc...

that has a nice property of forcing us to look at this when v1b1 goes away

updated so we check for v1beta1 requests and return early.

liggitt · 2021-03-05T07:18:21Z

pkg/registry/discovery/endpointslice/strategy.go

+				if ep.NodeName != nil {
+					if ep.DeprecatedTopology == nil {
+						ep.DeprecatedTopology = make(map[string]string, 1)
+					}
+					ep.DeprecatedTopology[corev1.LabelHostname] = *ep.NodeName
+				}


why isn't this limited to endpointListUpdated as well? this means a v1 writer touching only metadata fields (like the garbage collector or something) would cause nodeName to get propagated into deprecatedTopology. is that what we want?

edit: I guess if we drop the hostname topology label in internal→v1 conversion as described in #99662 (comment), we have to restore it in v1→internal conversion

I think we need to have nodeName propagated in deprecatedTopology, otherwise older kube-proxies would not be able to read nodeName information since we don't do anything special in the conversion in regards to the hostname topology label. When we remove v1beta1, we could then remove this logic to stop populating topology and eventually deprecatedTopology will be an unused field.

I think we need to have nodeName propagated in deprecatedTopology, otherwise older kube-proxies would not be able to read nodeName information since we don't do anything special in the conversion in regards to the hostname topology label.

When endpoints are being modified via v1, I agree. If the endpoint was written via v1beta1, doing this unconditionally (not just when endpoints were modified via v1) means we'll copy nodeName into topology

We still have the version check, but I see your point that it makes sense only to do this when the v1 client is writing to Endpoints

pkg/apis/discovery/v1beta1/conversion.go

liggitt · 2021-03-05T07:37:04Z

I agree this looks close. The only open question I have is whether we want deprecatedTopology[hostname] served in v1 when it matches nodeName.

* Removes discovery v1alpha1 API * Replaces per Endpoint Topology with a read only DeprecatedTopology in GA API * Adds per Endpoint Zone field in GA API

thockin

YAY!

/lgtm
/approve

k8s-ci-robot · 2021-03-05T21:16:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: swetharepakula, thockin

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:

~~api/OWNERS~~ [thockin]
~~cmd/kube-apiserver/OWNERS~~ [thockin]
~~hack/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/controlplane/OWNERS~~ [thockin]
~~pkg/registry/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/client-go/OWNERS~~ [thockin]
~~test/integration/apiserver/OWNERS~~ [thockin]
~~test/integration/etcd/OWNERS~~ [thockin]
~~vendor/OWNERS~~ [thockin]

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

aojea · 2021-03-05T22:03:03Z

Impressive 👏

liggitt · 2021-03-06T19:48:57Z

/test pull-kubernetes-dependencies
/test pull-kubernetes-unit

aojea · 2021-03-06T20:18:57Z

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/99662/pull-kubernetes-unit/1368287507921244160

https://storage.googleapis.com/k8s-gubernator/triage/index.html?date=2021-03-06&pr=1&text=etcd%20event%20received%20with%20PrevKv%3Dnil

=== RUN   TestWatch
I0306 20:06:14.051962   69532 client.go:360] parsed scheme: "endpoint"
I0306 20:06:14.052118   69532 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:65802040492353799000  <nil> 0 <nil>}]
I0306 20:06:14.059865   69532 client.go:360] parsed scheme: "endpoint"
I0306 20:06:14.060215   69532 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:65802040492353799000  <nil> 0 <nil>}]
I0306 20:06:14.066951   69532 client.go:360] parsed scheme: "endpoint"
I0306 20:06:14.067180   69532 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:65802040492353799000  <nil> 0 <nil>}]
E0306 20:06:17.870930   69532 watcher.go:218] watch chan error: etcd event received with PrevKv=nil (key="/registry/ingresses/test/foo-ingress", modRevision=3, type=DELETE)
    resttest.go:1559: unexpected result from result channel
--- FAIL: TestWatch (4.07s)

/retest

liggitt · 2021-03-06T21:03:53Z

/retest

robscott · 2021-03-06T21:45:02Z

/retest

liggitt · 2021-03-06T23:07:05Z

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/99662/pull-kubernetes-unit/1368295059291639808

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/99662/pull-kubernetes-unit/1368306368418680832

https://storage.googleapis.com/k8s-gubernator/triage/index.html?date=2021-03-06&pr=1&test=TestAdmit

=== RUN   TestAdmit/match_dry_run_side_effects_None
W0306 20:42:50.473031   82007 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
W0306 20:42:50.473220   82007 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
    plugin_test.go:187: 
        	Error Trace:	plugin_test.go:187
        	Error:      	Not equal: 
        	            	expected: map[string]string{"allow/key1":"value1", "mutation.webhook.admission.k8s.io/round_0_index_0":"{\"configuration\":\"test-webhooks\",\"webhook\":\"allow\",\"mutated\":false}"}
        	            	actual  : map[string]string(nil)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,5 +1,2 @@
        	            	-(map[string]string) (len=2) {
        	            	- (string) (len=10) "allow/key1": (string) (len=6) "value1",
        	            	- (string) (len=49) "mutation.webhook.admission.k8s.io/round_0_index_0": (string) (len=67) "{\"configuration\":\"test-webhooks\",\"webhook\":\"allow\",\"mutated\":false}"
        	            	-}
        	            	+(map[string]string) <nil>
        	            	 
        	Test:       	TestAdmit/match_dry_run_side_effects_None
        	Messages:   	match dry run side effects None: annotations not set as expected.
    --- FAIL: TestAdmit/match_dry_run_side_effects_None (0.02s)

liggitt · 2021-03-06T23:07:53Z

(the unit test flakes don't look related at all, just capturing them for referencing in flake issues)

k8s-ci-robot assigned thockin Mar 2, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 2, 2021

k8s-ci-robot requested review from andrewsykim, aojea, liggitt, robscott and wojtek-t March 2, 2021 23:22

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 2, 2021

swetharepakula mentioned this pull request Mar 2, 2021

EndpointSlice API kubernetes/enhancements#752

Closed

swetharepakula force-pushed the eps-ga-api-changes branch from 4529b0c to 0c0feaa Compare March 2, 2021 23:41

k8s-ci-robot added the area/test label Mar 2, 2021

thockin reviewed Mar 5, 2021

View reviewed changes

pkg/apis/discovery/validation/validation.go Show resolved Hide resolved

thockin reviewed Mar 5, 2021

View reviewed changes

pkg/registry/discovery/endpointslice/strategy_test.go

},

}

for _, tc := range testcases {

Copy link

Member

thockin Mar 5, 2021

Choose a reason for hiding this comment

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

Great testcases

thockin reviewed Mar 5, 2021

View reviewed changes

liggitt reviewed Mar 5, 2021

View reviewed changes

robscott mentioned this pull request Mar 5, 2021

Adding support for Topology Aware Hints #99522

Merged

Graduate EndpointSlice API to GA

a9891b4

* Removes discovery v1alpha1 API * Replaces per Endpoint Topology with a read only DeprecatedTopology in GA API * Adds per Endpoint Zone field in GA API

swetharepakula force-pushed the eps-ga-api-changes branch from 244dddd to a9891b4 Compare March 5, 2021 20:03

thockin reviewed Mar 5, 2021

View reviewed changes

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

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

liggitt mentioned this pull request Mar 5, 2021

Add Etcd compatibility test #99873

Merged

swetharepakula mentioned this pull request Mar 6, 2021

Update controlplane to use EndpointSlice V1 API #99879

Merged

liggitt mentioned this pull request Mar 6, 2021

Add discovery and flowcontrol compatibility API data #99871

Merged

k8s-ci-robot merged commit 36244f3 into kubernetes:master Mar 6, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 6, 2021

robscott mentioned this pull request Mar 6, 2021

Graduate EndpointSlice Controllers to GA #99870

Merged

liggitt mentioned this pull request Mar 7, 2021

[Flaky] k8s.io/apiserver/pkg/admission/plugin/webhook/mutating: TestAdmit #99911

Closed

swetharepakula deleted the eps-ga-api-changes branch March 7, 2021 04:13

swetharepakula mentioned this pull request Mar 15, 2021

EndpointSlice API GA Doc Updates for 1.21 kubernetes/website#27070

Merged

aojea mentioned this pull request May 26, 2022

LoadBalancer&NodePort service cannot access after upgrade from 1.20 to 1.22 #110208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graduate EndpointSlice API to GA #99662

Graduate EndpointSlice API to GA #99662

swetharepakula commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

robscott commented Mar 2, 2021

swetharepakula commented Mar 5, 2021

thockin Mar 5, 2021

thockin left a comment

liggitt Mar 5, 2021

thockin Mar 5, 2021

swetharepakula Mar 5, 2021

liggitt Mar 5, 2021 •

edited

swetharepakula Mar 5, 2021

liggitt Mar 5, 2021

swetharepakula Mar 5, 2021

liggitt commented Mar 5, 2021

thockin left a comment

k8s-ci-robot commented Mar 5, 2021

aojea commented Mar 5, 2021

liggitt commented Mar 6, 2021

aojea commented Mar 6, 2021 •

edited by liggitt

liggitt commented Mar 6, 2021

robscott commented Mar 6, 2021

liggitt commented Mar 6, 2021 •

edited

liggitt commented Mar 6, 2021

Graduate EndpointSlice API to GA #99662

Graduate EndpointSlice API to GA #99662

Conversation

swetharepakula commented Mar 2, 2021

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Mar 2, 2021

robscott commented Mar 2, 2021

swetharepakula commented Mar 5, 2021

thockin Mar 5, 2021

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

liggitt Mar 5, 2021

Choose a reason for hiding this comment

thockin Mar 5, 2021

Choose a reason for hiding this comment

swetharepakula Mar 5, 2021

Choose a reason for hiding this comment

liggitt Mar 5, 2021 • edited

Choose a reason for hiding this comment

swetharepakula Mar 5, 2021

Choose a reason for hiding this comment

liggitt Mar 5, 2021

Choose a reason for hiding this comment

swetharepakula Mar 5, 2021

Choose a reason for hiding this comment

liggitt commented Mar 5, 2021

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 5, 2021

aojea commented Mar 5, 2021

liggitt commented Mar 6, 2021

aojea commented Mar 6, 2021 • edited by liggitt

liggitt commented Mar 6, 2021

robscott commented Mar 6, 2021

liggitt commented Mar 6, 2021 • edited

liggitt commented Mar 6, 2021

liggitt Mar 5, 2021 •

edited

aojea commented Mar 6, 2021 •

edited by liggitt

liggitt commented Mar 6, 2021 •

edited