Ensure EndpointSlice exist if Endpoint is found #84421

tnqn · 2019-10-27T16:50:44Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
The EndpointSlice for masters was not created after enabling EndpointSlice feature on a pre-existing cluster. This was because the Endpoint object had been created and ReconcileEndpoints would skip creating or updating it after EndpointSlice feature is enabled.

This patch ensures EndpointSlice is consistent with Endpoints after the reconciler reconciles Endpoints even if Endpoints is unchanged. It also avoids an update if the desired EndpointSlice matches the existing one.

Which issue(s) this PR fixes:

Fixes #84419

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix the bug that EndpointSlice for masters wasn't created after enabling EndpointSlice feature on a pre-existing cluster.

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

k8s-ci-robot · 2019-10-27T16:50:51Z

Hi @tnqn. 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.

tnqn · 2019-10-27T16:56:36Z

/assign @robscott

robscott · 2019-10-27T21:52:17Z

Hey @tnqn, thanks for catching this! Looks like a good fix on first glance, will try to take a closer look at this tomorrow.
/ok-to-test

robscott · 2019-10-27T21:53:14Z

/priority important-soon

tnqn · 2019-10-28T02:58:15Z

@robscott thanks for reviewing it.
/retest

robscott

Hey @tnqn, this looks great, just have a suggestion on how to simplify the tests you're adding. I spun up an e2e cluster with this branch and the upgrade worked as expected simply by toggling the EndpointSlice feature flag on the apiserver manifest. Thanks for the fix!

robscott · 2019-10-28T21:38:45Z

pkg/master/reconcilers/endpointsadapter_test.go

@@ -98,6 +108,28 @@ func TestEndpointsAdapterGet(t *testing.T) {
 			if !apiequality.Semantic.DeepEqual(endpoints, testCase.expectedEndpoints) {
 				t.Errorf("Expected endpoints: %v, got: %v", testCase.expectedEndpoints, endpoints)
 			}
+
+			epSliceList, err := client.DiscoveryV1alpha1().EndpointSlices(testCase.namespaceParam).List(metav1.ListOptions{})


All of this test logic can actually be simplified because the EndpointSlice created here will always have a consistent name (nameParam here, but generally just kubernetes). I think it would make sense to test based on the existence of that EndpointSlice specifically with a Get call instead of a List.

Makes sense. I have updated, PTAL.

Hey @tnqn, this looks great, I think it could be simplified one more step to just use the deepEqual whether or not the expected value was nil. Something like what's done above for Endpoints: https://github.com/kubernetes/kubernetes/blob/master/pkg/master/reconcilers/endpointsadapter_test.go#L262-L264

Sure, done.

tnqn · 2019-10-29T03:50:09Z

@robscott thanks for the suggestion!

robscott · 2019-10-29T05:17:29Z

Thanks for catching this @tnqn! This fix works well for me.

/lgtm

robscott · 2019-10-29T05:23:58Z

@liggitt are you able to take a look here? I think you helped review/approve my initial PR here back in the day. This looks like a good fix that should help anyone upgrading to a cluster with EndpointSlices enabled.

/assign @liggitt

pkg/master/reconcilers/endpointsadapter.go

liggitt · 2019-10-29T20:52:44Z

This was because the Endpoint object had been created and ReconcileEndpoints would skip creating or updating it after EndpointSlice feature is enabled.

Can you clarify this point? Does the endpoints controller not reconcile the kube-system/kubernetes Endpoints object to an EndpointsSlice object? Or is the issue that the masters must bootstrap their EndpointsSlice object just like their Endpoints object in order to make sure the API server is reachable from wherever the kube-controller-manager is running?

tnqn · 2019-10-30T07:58:34Z

This was because the Endpoint object had been created and ReconcileEndpoints would skip creating or updating it after EndpointSlice feature is enabled.

Can you clarify this point? Does the endpoints controller not reconcile the kube-system/kubernetes Endpoints object to an EndpointsSlice object?
Or is the issue that the masters must bootstrap their EndpointsSlice object just like their Endpoints object in order to make sure the API server is reachable from wherever the kube-controller-manager is running?

The endpoints controller is not in charge of the default/kubernetes Endpoints and EndpointSlice:

kubernetes/pkg/controller/endpointslice/endpointslice_controller.go

Lines 260 to 264 in 8d8a068

    
           if service.Spec.Selector == nil { 
        
           	// services without a selector receive no endpoint slices from this controller; 
        
           	// these services will receive endpoint slices that are created out-of-band via the REST API. 
        
           	return nil 
        
           }

It's the EndpointReconciler (leaseEndpointReconciler by default) in masters reconciling its Endpoints and EndpointSlice:

kubernetes/pkg/master/reconcilers/lease.go

Line 162 in 8d8a068

    
           func (r *leaseEndpointReconciler) doReconcile(serviceName string, endpointPorts []corev1.EndpointPort, reconcilePorts bool) error {

I think it's the same issue you mentioned that EndpointSlices for masters must be created by themselves to make kube-controller-manager reach them.

tnqn · 2019-10-30T08:00:56Z

/retest

tnqn · 2019-10-31T11:41:55Z

/retest

pkg/master/reconcilers/endpointsadapter.go

The EndpointSlice for masters was not created after enabling EndpointSlice feature on a pre-existing cluster. This was because the Endpoint object had been created and ReconcileEndpoints would skip creating or updating it after EndpointSlice feature is enabled. This patch ensures EndpointSlice is consistent with Endpoints after the reconciler reconciles Endpoints even if Endpoints is unchanged. It also avoids an update if the desired EndpointSlice matches the existing one.

liggitt · 2019-10-31T15:15:31Z

/approve

@robscott has final lgtm

k8s-ci-robot · 2019-10-31T15:16:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tnqn

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:

~~pkg/master/OWNERS~~ [liggitt]

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

tnqn · 2019-10-31T15:27:04Z

Thanks! @liggitt

/retest

robscott · 2019-10-31T17:00:16Z

Thanks for all your work on this @tnqn!

/lgtm

robscott · 2019-10-31T19:33:12Z

/retest

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 27, 2019

k8s-ci-robot requested review from justinsb and yujuhong October 27, 2019 16:52

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 27, 2019

tnqn force-pushed the missing-endpointslice branch from 689e236 to c42069c Compare October 27, 2019 16:55

k8s-ci-robot assigned robscott Oct 27, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 27, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 27, 2019

robscott reviewed Oct 28, 2019

View reviewed changes

tnqn force-pushed the missing-endpointslice branch from c42069c to 8fe9681 Compare October 29, 2019 03:38

tnqn force-pushed the missing-endpointslice branch from 8fe9681 to 1610e78 Compare October 29, 2019 05:00

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2019

k8s-ci-robot assigned liggitt Oct 29, 2019

liggitt reviewed Oct 29, 2019

View reviewed changes

pkg/master/reconcilers/endpointsadapter.go Outdated Show resolved Hide resolved

tnqn force-pushed the missing-endpointslice branch from 1610e78 to 264a333 Compare October 30, 2019 07:18

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 30, 2019

tnqn force-pushed the missing-endpointslice branch from 264a333 to 7294b31 Compare October 31, 2019 11:24

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 31, 2019

tnqn force-pushed the missing-endpointslice branch 2 times, most recently from 6933823 to f3041fa Compare October 31, 2019 12:02

liggitt reviewed Oct 31, 2019

View reviewed changes

pkg/master/reconcilers/endpointsadapter.go Outdated Show resolved Hide resolved

liggitt reviewed Oct 31, 2019

View reviewed changes

pkg/master/reconcilers/endpointsadapter.go Outdated Show resolved Hide resolved

tnqn force-pushed the missing-endpointslice branch from f3041fa to 92ceb16 Compare October 31, 2019 14:30

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2019

k8s-ci-robot merged commit 4002e4c into kubernetes:master Oct 31, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure EndpointSlice exist if Endpoint is found #84421

Ensure EndpointSlice exist if Endpoint is found #84421

tnqn commented Oct 27, 2019 •

edited

Loading

k8s-ci-robot commented Oct 27, 2019

tnqn commented Oct 27, 2019

robscott commented Oct 27, 2019

robscott commented Oct 27, 2019

tnqn commented Oct 28, 2019

robscott left a comment

robscott Oct 28, 2019

tnqn Oct 29, 2019

robscott Oct 29, 2019

tnqn Oct 29, 2019

tnqn commented Oct 29, 2019

robscott commented Oct 29, 2019 •

edited

Loading

robscott commented Oct 29, 2019

liggitt commented Oct 29, 2019 •

edited

Loading

tnqn commented Oct 30, 2019

tnqn commented Oct 30, 2019

tnqn commented Oct 31, 2019

liggitt commented Oct 31, 2019

k8s-ci-robot commented Oct 31, 2019

tnqn commented Oct 31, 2019

robscott commented Oct 31, 2019

robscott commented Oct 31, 2019

Ensure EndpointSlice exist if Endpoint is found #84421

Ensure EndpointSlice exist if Endpoint is found #84421

Conversation

tnqn commented Oct 27, 2019 • edited Loading

k8s-ci-robot commented Oct 27, 2019

tnqn commented Oct 27, 2019

robscott commented Oct 27, 2019

robscott commented Oct 27, 2019

tnqn commented Oct 28, 2019

robscott left a comment

Choose a reason for hiding this comment

robscott Oct 28, 2019

Choose a reason for hiding this comment

tnqn Oct 29, 2019

Choose a reason for hiding this comment

robscott Oct 29, 2019

Choose a reason for hiding this comment

tnqn Oct 29, 2019

Choose a reason for hiding this comment

tnqn commented Oct 29, 2019

robscott commented Oct 29, 2019 • edited Loading

robscott commented Oct 29, 2019

liggitt commented Oct 29, 2019 • edited Loading

tnqn commented Oct 30, 2019

tnqn commented Oct 30, 2019

tnqn commented Oct 31, 2019

liggitt commented Oct 31, 2019

k8s-ci-robot commented Oct 31, 2019

tnqn commented Oct 31, 2019

robscott commented Oct 31, 2019

robscott commented Oct 31, 2019

tnqn commented Oct 27, 2019 •

edited

Loading

robscott commented Oct 29, 2019 •

edited

Loading

liggitt commented Oct 29, 2019 •

edited

Loading