Fix endpoints status out-of-sync when the pod state changes rapidly#125675
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
| e.podsSynced = podInformer.Informer().HasSynced | ||
|
|
||
| endpointsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
| UpdateFunc: func(old, cur interface{}) { |
There was a problem hiding this comment.
I'm trying to find the issue where this option was discussed and discarded several releases ago, @thockin @robscott do you remember?
This fixes the issue because it process its own update, we are already doing it on other controllers, but it means we are going to do double the work now for each endpoint update
There was a problem hiding this comment.
I found this change would break a conformance test: "should test the lifecycle of an Endpoint".
However, the conformance test itself seems contradictory with some logic of endpoints controller:
- The conformance test expects an Endpoint created without a Service to exist
- The
checkLeftoverEndpointsfunction of endpoint controller triggers syncing all Endpoints except for those having leader election annotation and will cause Endpoints without Services to be deleted:kubernetes/pkg/controller/endpoint/endpoints_controller.go
Lines 561 to 567 in 921fb0b
The conformance test gives an impression that an user created Endpoints (without a Service) can exist but it will be deleted once kube-controller-manager restarts or fails over. Subscribing endpoints update events extends the cleanup logic of checkLeftoverEndpoints in a sense.
There was a problem hiding this comment.
It seems the conformance test would fail as long as the controller watches endpoints update events.
I'm thinking another fix which doesn't affect the conformance test and could avoid doubling the work: we track the stale resource versions of endpoints after each successful endpoints update, use it to determine whether the endpoints in cache is stale when syncing it next time, and return error if it's stale and retry. (The reason why we don't track generation like endpointslice controller is endpoints doesn't have a generation.)
The 2nd commit implements the above fix.
2a51e4f to
ef15f35
Compare
|
/retest |
1 similar comment
|
/retest |
|
@tnqn sorry for being very picky but everytime we change something here there are always unexpected edge cases, I think that adding an integration test that reproduces the problem, we can do multiple transitions in parallel if necessary to increase the chances of getting an stale cache entry, will give us more confidence |
|
/test pull-kubernetes-integration |
@aojea no problem, I will add one. |
ef15f35 to
e90da46
Compare
ca449a4 to
baffa09
Compare
|
/lgtm |
|
Thanks @tnqn! /hold cancel |
|
Actually I guess @aojea said Friday, but I think we likely have enough reviewers that have signed off on this now, will defer to him. /hold |
/hold cancel There are 3 additional reviews, more than enough 😄 |
…-upstream-release-1.29 Automated cherry pick of #125675: Fix endpoints status out-of-sync when the pod state changes
…-upstream-release-1.28 Automated cherry pick of #125675: Fix endpoints status out-of-sync when the pod state changes
…-upstream-release-1.30 Automated cherry pick of #125675: Fix endpoints status out-of-sync when the pod state changes
|
For visibility, backports of this regressed 1.30.3+, 1.29.7+, and 1.28.12+ (see #127370). Make sure we stick to the criteria for backports. Backporting changes which are not regression fixes or critical bugfixes as described in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md#what-kind-of-prs-are-good-for-cherry-picks should be avoided. Every backport carries risk, and in this case, we made patch releases worse with a backport. |
…20.6' into 'tke/v1.20.6' (merge request !1682) Automated cherry pick of kubernetes#1681: Fix endpoints status out-of-sync when the pod state changes 功能:endpoints 更新不一致问题修复 社区 PR:kubernetes#125675 Cherry pick of kubernetes#1681 on tke/v1.20.6. kubernetes#1681: Fix endpoints status out-of-sync when the pod state changes.
…24.4' into 'tke/v1.24.4' (merge request !1680) Automated cherry pick of kubernetes#1679: Fix endpoints status out-of-sync when the pod state changes 功能:endpoints 更新不一致问题修复 社区 PR:kubernetes#125675 Cherry pick of kubernetes#1679 on tke/v1.24.4. kubernetes#1679: Fix endpoints status out-of-sync when the pod state changes.
…22.5' into 'tke/v1.22.5' (merge request !1681) Automated cherry pick of kubernetes#1680: Fix endpoints status out-of-sync when the pod state changes 功能:endpoints 更新不一致问题修复 社区 PR:kubernetes#125675 Cherry pick of kubernetes#1680 on tke/v1.22.5. kubernetes#1680: Fix endpoints status out-of-sync when the pod state changes.
…26.1' into 'tke/v1.26.1' (merge request !1679) Automated cherry pick of kubernetes#1666: Fix endpoints status out-of-sync when the pod state changes 功能:endpoints 更新不一致问题修复 社区 PR:kubernetes#125675 Cherry pick of kubernetes#1666 on tke/v1.26.1. kubernetes#1666: Fix endpoints status out-of-sync when the pod state changes.
…1.30.0' into 'tke/v1.30.0' (merge request !1665) Automated cherry pick of kubernetes#125675: Fix endpoints status out-of-sync when the pod state changes 功能:endpoints 更新不一致问题修复 社区 PR:kubernetes#125675 Cherry pick of kubernetes#125675 on tke/v1.30.0. kubernetes#125675: Fix endpoints status out-of-sync when the pod state changes.
…28.3' into 'tke/v1.28.3' (merge request !1666) Automated cherry pick of kubernetes#1665: Fix endpoints status out-of-sync when the pod state changes 功能:endpoints 更新不一致问题修复 社区 PR:kubernetes#125675 Cherry pick of kubernetes#1665 on tke/v1.28.3. kubernetes#1665: Fix endpoints status out-of-sync when the pod state changes.
What type of PR is this?
/kind bug
What this PR does / why we need it:
When Pod state changes rapidly, endpoints controller may use outdated informer cache to sync Service. If the outdated endpoints appear to be expected by the controller, it skips updating it.
The commit fixes it by checking if endpoints informer cache is outdated when processing a service. If the endpoints is stale, it returns an error and retries later.
Which issue(s) this PR fixes:
Fixes #125638
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: