Skip to content
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

Fixing bugs related to Endpoint Slices #82289

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@robscott
Copy link
Member

commented Sep 3, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
This should fix a bug that could break masters when the EndpointSlice feature gate was enabled. This was all tied to how the apiserver creates and manages it's own services and endpoints (or in this case endpoint slices). Consumers of endpoint slices also need to know about the corresponding service. Previously we were trying to set an owner reference here for this purpose, but that came with potential downsides and increased complexity. This PR changes behavior of the apiserver endpointslice integration to set the service name label instead of owner references, and simplifies consumer logic to reference that (both are set by the EndpointSlice controller).

Which issue(s) this PR fixes:
Should help with #82174.

Special notes for your reviewer:
This PR includes a bit of cleanup as well. Where both corev1 and v1 were accidentally used as imports, the code has been cleaned up to use whichever one required less changes. Additionally, serviceNameLabel had been set as a const in a couple packages. This change meant it would be required in yet another package, so an equivalent has been added to core/v1/well_known_labels.

Does this PR introduce a user-facing change?:

NONE

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

/cc @freehan

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

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

@freehan

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

/ok-to-test

@alejandrox1
Copy link
Contributor

left a comment

/priority critical-urgent
/milestone v1.16

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Sep 3, 2019

@alejandrox1

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

flake on presubmit
/test pull-kubernetes-e2e-gce-device-plugin-gpu

@fejta-bot

This comment has been minimized.

Copy link

commented Sep 3, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@robscott

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

/retest

@lachie83

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@robscott It looks like this is ready for review. Do you have someone that can complete the to review and get this ready to merge?

@alejandrox1

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

/assign @MrHohn @bowei
could you please take a look?

@MrHohn

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

I'm sure @freehan will help review this :)
/assign @freehan

@robscott

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

/retest

pkg/proxy/ipvs/proxier.go Outdated Show resolved Hide resolved
pkg/controller/endpointslice/utils.go Show resolved Hide resolved
@@ -103,8 +97,7 @@ func (adapter *EndpointsAdapter) ensureEndpointSliceFromEndpoints(namespace stri
func endpointSliceFromEndpoints(endpoints *corev1.Endpoints) *discovery.EndpointSlice {
endpointSlice := &discovery.EndpointSlice{}
endpointSlice.Name = endpoints.Name
endpointSlice.Labels = map[string]string{serviceNameLabel: endpoints.Name}
endpointSlice.OwnerReferences = []metav1.OwnerReference{{Kind: "Service", Name: endpoints.Name}}

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 4, 2019

Member

I'm missing context for the original design that called for setting ownerreferences. Can @thockin or @bowei weigh in on whether this makes sense (here and in all the places that switched)?

If the garbage collector is no longer removing endpoint slices when a service is deleted, is there a controller loop that makes that happen?

This comment has been minimized.

Copy link
@robscott

robscott Sep 4, 2019

Author Member

There were two reasons for omitting owner references here. First, to properly include them would require significant restructuring of code in this package that I was hoping to avoid here for an alpha feature. The corresponding service may or may not exist at this point as both the service and endpoint(slice) are managed independently here. There's just a simple loop that ensures that both the service and endpoint(slice) resources exist to reference the apiserver.

Second, the select few resources managed by apiserver like this were never meant to be garbage collected. Since the apiserver is constantly trying to keep these resources in existence, it would actually potentially create more work to have an owner refererence here. If someone were to delete the kubernetes service this creates, and the corresponding endpointslice was deleted, it would result in unnecessary cycles for kube-proxy.

I don't know, open to ideas here, this is definitely a strange use case. I was really trying to minimize the scale of changes to this package, but open to another approach.

This comment has been minimized.

Copy link
@robscott

robscott Sep 4, 2019

Author Member

I should add that the reason the owner reference was originally included here is simply that endpointslicecache in kube-proxy derived the service name from owner references instead of from the service name label. This PR switches that to derive the service name from the label instead, an approach that seems simpler and cleaner.

This comment has been minimized.

Copy link
@freehan

freehan Sep 4, 2019

Member

The only downside for not setting ownerRef here is that when the default/kubernetes service is deleted. Its Endpointslice will be not be removed. I think this downside is acceptable compared to refactoring of existing battle proven code since default/kubernetes service should always exists during the cluster's life cycle.

This comment has been minimized.

Copy link
@bowei

bowei Sep 4, 2019

Member

I view this situation as the case where a user is manually creating an Endpoints object rather than the Endpoints(Slice)Controller (user in this case being the API server). Given that, it is up to the creator of the EndpointSlice object to managed its lifetime. ownerRef is used by the EndpointPointSlice controller to manage lifetimes -- those slices not created by the controller will have to manage lifetimes in their own way.

Also, pragmatically, these objects are basically ensured to always exist while the API server is up, so the life cycle is somewhat moot.

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 5, 2019

Member

when the default/kubernetes service is deleted. Its Endpointslice will be not be removed

if that happens, when it gets recreated, does the controller autocorrect any extant EndpointSlice objects as needed with the new endpoint data?

This comment has been minimized.

Copy link
@robscott

robscott Sep 5, 2019

Author Member

Yep, the endpoint reconcilers are responsible for this and run consistently to ensure that endpoints (and optionally endpoint slices) exist for this. They are not dependent on any kind of watch/change event for the reconcile loop to run. Start sets up an async runner, which includes RunKubernetesService, which loops indefinitely in a NonSlidingUntil to call UpdateKubernetesService which ensures the existence and accuracy of the corresponding service first, followed by endpoints (and endpoint slices if the feature gate is enabled). There's a bit more to it than that, but the gist of it is that this sync logic will run as long as apiserver does, and it does not depend on a change to a specific resource to trigger. This is likely not an ideal flow, but it feels out of scope to change it here.

pkg/proxy/endpointslicecache.go Show resolved Hide resolved

@robscott robscott force-pushed the robscott:endpointslice-fixes branch 3 times, most recently from 715493e to 45ed997 Sep 4, 2019

Fixing bugs related to Endpoint Slices
This should fix a bug that could break masters when the EndpointSlice
feature gate was enabled. This was all tied to how the apiserver creates
and manages it's own services and endpoints (or in this case endpoint
slices). Consumers of endpoint slices also need to know about the
corresponding service. Previously we were trying to set an owner
reference here for this purpose, but that came with potential downsides
and increased complexity. This commit changes behavior of the apiserver
endpointslice integration to set the service name label instead of owner
references, and simplifies consumer logic to reference that (both are
set by the EndpointSlice controller).

Additionally, this should fix a bug with the EndpointSlice GenerateName
value that had previously been set with a "." as a suffix.

@robscott robscott force-pushed the robscott:endpointslice-fixes branch from 45ed997 to 8f9483d Sep 4, 2019

endpointslicecontroller "k8s.io/kubernetes/pkg/controller/endpointslice"
)

func startEndpointSliceController(ctx ControllerContext) (http.Handler, bool, error) {
if !ctx.AvailableResources[schema.GroupVersionResource{Group: "discovery", Version: "v1alpha1", Resource: "endpointslices"}] {
if !ctx.AvailableResources[discoveryv1alpha1.SchemeGroupVersion.WithResource("endpointslices")] {

This comment has been minimized.

Copy link
@khenidak

khenidak Sep 4, 2019

Contributor

is this an error condition? What would the user do in a case where they expect endpoint slices (say configured proxy to switch to EndpointSlice)?

This comment has been minimized.

Copy link
@robscott

robscott Sep 4, 2019

Author Member

This could theoretically occur if the --controllers flag on kube-controller-manager was set to include endpointslice but the api group was not enabled. There are likely other edge cases I'm not thinking of. This just follows a pattern seen in a number of other startController methods, such as the batch controller startup code.

@lachie83

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@bowei - I believe this PR is awaiting your comments. Can you please comment at your earliest convenience as this PR potentially fixes a 1.16 release-blocking job so time is of the upmost importance as we are less than 2 weeks out from release. If we don't think we can't get this merged within the next 48 hours I need to know so we can discuss reverting the commits for the enhancement. I just wanted to let you know where we stand as part of the 1.16 release. Thanks

@bowei

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

/lgtm (see comment above)

@lachie83

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Thanks @bowei. We need that lgtm on a separate line in order for the bot to pick it up and label correctly.

@robscott

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

@liggitt Thanks for your reviews on this PR! It's ready for another look if you have time today.

@bowei

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 4, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lachie83

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

/test pull-kubernetes-node-e2e

1 similar comment
@robscott

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

/test pull-kubernetes-node-e2e

@fejta-bot

This comment has been minimized.

Copy link

commented Sep 5, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Sep 5, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

commented Sep 5, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 61ecdba into kubernetes:master Sep 5, 2019

24 checks passed

cla/linuxfoundation robscott authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
if !ok || serviceName == "" {
err = fmt.Errorf("No %s label set on endpoint slice: %s", discovery.LabelServiceName, endpointSlice.Name)
} else if endpointSlice.Namespace == "" || endpointSlice.Name == "" {
err = fmt.Errorf("Expected EndpointSlice name and namespace to be set: %v", endpointSlice)

This comment has been minimized.

Copy link
@tedyu

tedyu Sep 5, 2019

Contributor

Should nil be returned as the first component of return value (types.NamespacedName) in this case ?

This comment has been minimized.

Copy link
@tedyu

tedyu Sep 5, 2019

Contributor

See #82389

@robscott robscott referenced this pull request Sep 12, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.