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.

Sign up for GitHub

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

Jump to bottom

Updating EndpointSliceMirroring controller to wait for cache to be updated #99756

Merged
merged 1 commit into from Mar 4, 2021
Merged

Updating EndpointSliceMirroring controller to wait for cache to be updated #99756

merged 1 commit into from Mar 4, 2021

Conversation

robscott
Copy link
Member

@robscott robscott commented Mar 4, 2021

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

This matches the recent updates to the EndpointSliceTracker for the EndpointSlice controller in #99345 that accomplished the same thing.

Does this PR introduce a user-facing change?

EndpointSliceMirroring controller is now less likely to emit FailedToUpdateEndpointSlices events.

/sig network
/triage accepted
/priority important-soon
/cc @aojea @swetharepakula
/assign @wojtek-t

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Mar 4, 2021
@k8s-ci-robot k8s-ci-robot requested a review from aojea March 4, 2021 03:58
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Mar 4, 2021
@k8s-ci-robot
Copy link
Contributor

@robscott: GitHub didn't allow me to request PR reviews from the following users: swetharepakula.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

This matches the recent updates to the EndpointSliceTracker for the EndpointSlice controller in #99345 that accomplished the same thing.

Does this PR introduce a user-facing change?

EndpointSliceMirroring controller is now less likely to emit FailedToUpdateEndpointSlices events.

/sig network
/triage accepted
/priority important-soon
/cc @aojea @swetharepakula
/assign @wojtek-t

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.

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Mar 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2021
@robscott robscott force-pushed the endpointslicemirroring-tracker branch from d98282f to 8445e61 Compare March 4, 2021 05:16
@aojea
Copy link
Member

aojea commented Mar 4, 2021

/hold
feel free to unhold if I'm wrong :), but I think that this is missing

	if c.endpointSliceTracker.StaleSlices(service, endpointSlices) {
		return &StaleInformerCache{"EndpointSlice informer cache is out of date"}

in the syncEndpoints() function in line 318 of the pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go
https://github.com/kubernetes/kubernetes/pull/99756/files#diff-bef128c89253bdf82a5fb567a2cf3b639f3aafd24fd293932491d443ec543f12L314-L318

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2021
@robscott
Updating EndpointSliceMirroring controller to wait for cache to be up…
06db357 
…dated

This matches the recent updates to the EndpointSliceTracker for the
EndpointSlice controller in kubernetes#99345 that accomplished the same thing.
@robscott robscott force-pushed the endpointslicemirroring-tracker branch from 8445e61 to 06db357 Compare March 4, 2021 17:17
robscott
robscott commented Mar 4, 2021
View reviewed changes
pkg/controller/endpointslicemirroring/endpointslicemirroring_controller.go
Comment on lines +462 to +472
// EndpointSlice generation does not change when labels change. Although the
// controller will never change LabelServiceName, users might. This check
// ensures that we handle changes to this label.
svcName := endpointSlice.Labels[discovery.LabelServiceName]
prevSvcName := prevEndpointSlice.Labels[discovery.LabelServiceName]
if svcName != prevSvcName {
klog.Warningf("%s label changed from %s to %s for %s", discovery.LabelServiceName, prevSvcName, svcName, endpointSlice.Name)
c.queueEndpointsForEndpointSlice(endpointSlice)
c.queueEndpointsForEndpointSlice(prevEndpointSlice)
return
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Although #99750 is about to merge, leaving this in makes it easier to backport this PR independently. I'll plan to remove this logic later here and in #99345.

@robscott
Copy link
Member Author

robscott commented Mar 4, 2021

Thanks for catching that @aojea! I double checked everything and think this should have all the same logic now, let me know if I'm missing anything.

@aojea
Copy link
Member

aojea commented Mar 4, 2021

/hold cancel
/lgtm
@robscott you mentioned several times before, but I realise now that we should merge this code with the slices controller one or this will be impossible to handle.
I know 1 or 2 persons that are willing to help with this, if you want we can open a HelpNeeded issue

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2021
@robscott
Copy link
Member Author

robscott commented Mar 4, 2021

@aojea you're completely right. I've had a PR open for quite awhile that does this, I just tend to remember right before code freeze each year and other PRs end up having higher priority. I'll get that PR cleaned up after this one merges.

@aojea
Copy link
Member

aojea commented Mar 4, 2021

@aojea you're completely right. I've had a PR open for quite awhile that does this, I just tend to remember right before code freeze each year and other PRs end up having higher priority. I'll get that PR cleaned up after this one merges.

@JornShen is this something that you want to give it a shot? ^^^

@robscott
Copy link
Member Author

robscott commented Mar 4, 2021

To clarify, I'm very open to help on this one, that PR is quite old and adds pretty minimal value, if someone has time to take this on that would be awesome. @aojea thanks for helping find someone that can take this on!

@robscott
Copy link
Member Author

robscott commented Mar 4, 2021

/retest

1 similar comment
@robscott
Copy link
Member Author

robscott commented Mar 4, 2021

/retest

@robscott
Copy link
Member Author

robscott commented Mar 4, 2021 •
edited

Test flakes were:

  • Kubernetes e2e suite: [sig-api-machinery] Watchers should receive events on concurrent watches in same order [Conformance]
  • Kubernetes e2e suite: [sig-storage] Downward API volume should provide container's cpu request [NodeConformance] [Conformance]

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/99756/pull-kubernetes-e2e-kind/1367550175085596672

/retest

@k8s-ci-robot k8s-ci-robot merged commit fde2814 into kubernetes:master Mar 4, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 4, 2021
@JornShen
Copy link
Member

JornShen commented Mar 5, 2021

@aojea you're completely right. I've had a PR open for quite awhile that does this, I just tend to remember right before code freeze each year and other PRs end up having higher priority. I'll get that PR cleaned up after this one merges.

@JornShen is this something that you want to give it a shot? ^^^

yeah, I can help do it
:)

@JornShen JornShen mentioned this pull request Mar 9, 2021
Merged
This was referenced Mar 9, 2021
Closed
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Mar 12, 2021
@k8s-ci-robot
Merge pull request #100144 from robscott/automated-cherry-pick-of-#99…
8e3ec7d 
…756-release-1.19

Automated cherry pick of #99756: Updating EndpointSliceMirroring controller to wait for cache
k8s-ci-robot added a commit that referenced this pull request Mar 13, 2021
@k8s-ci-robot
Merge pull request #100143 from robscott/automated-cherry-pick-of-#99…
989d96d 
…756-release-1.20

Automated cherry pick of #99756: Updating EndpointSliceMirroring controller to wait for cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea Awaiting requested review from aojea

Assignees

@aojea aojea

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. 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. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@robscott @k8s-ci-robot @aojea @JornShen @wojtek-t