Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

So I think this is a bit closer now, I've updated the logic to have a max delay instead of a max number of retries and I've added some admittedly less than ideal logic that will ensure that a single failure doesn't doom a Service to slow syncs forever. I've been testing this with kind and e2e clusters to try to understand the behavior of an informer and it's been frustratingly inconsistent. Sometimes adds or updates are taking significantly longer than my 3 second max delay to show up. At this point that is purely anecdotal and I don't have sufficient data/evidence to back that up. Will work on trying to figure out exactly what is going on here, but would certainly appreciate a second set of eyes on this in case I'm missing something obvious or even just if this direction doesn't actually make sense.

I talked a bit with @wojtek-t this morning and he suggested trying a larger master. I've moved this up to a n2-standard-4 and only scaling up a 100 pod deployment. Here's a copy of the kube-controller-manager logs from that (using the debug logging adding in this PR). The good news is that this approach generally seems to work. The bad news is that even with a larger master I'm still missing cache updates. The key lines in these logs are TRACKER UPDATE (controller has added/updated an EndpointSlice and the tracker has tracked that update) and CACHE UPDATED FOR SLICE (controller has received an EndpointSlice add/update event).

There are some examples where this works exactly as expected:
https://gist.github.com/robscott/df1832cd9a6d5a69adeccfa2830edf9a#file-kube-controller-manager-log-L427-L434
https://gist.github.com/robscott/df1832cd9a6d5a69adeccfa2830edf9a#file-kube-controller-manager-log-L488-L505

There are also other examples where the cache still takes >3s to be updated though:
~3.2s: https://gist.github.com/robscott/df1832cd9a6d5a69adeccfa2830edf9a#file-kube-controller-manager-log-L451-L489
~5s: https://gist.github.com/robscott/df1832cd9a6d5a69adeccfa2830edf9a#file-kube-controller-manager-log-L506-L526

Although this code is still quite messy, this is still the best solution I can think of. Unless @liggitt or @wojtek-t have better ideas I'll try to get this cleaned up and add some tests so this can actually be ready for review.

Added some additional test coverage, going to move this out of draft mode so I can get full e2e tests on this. There are some EndpointSlice controller tests added by @mborsz around Pod batching that this breaks, so at least those will fail. Those are heavily dependent on timing and this is breaking that. I think there's a way to get that those to work with this, just haven't figured it out yet.

I think this PR is ready for an initial round of review. I ended up updating the pod batch timing tests to essentially bypass this functionality by setting a maxCacheDelay of 0 seconds. Unfortunately given the nature of those tests being heavily dependent on timing and this introducing some inconsistency to the timing of when syncService() will be called, any non-zero value for maxCacheDelay could have introduced flakes here.

As much as I would appreciate review on this PR now, I know that I still need to add test coverage for MarkServiceCacheUpdated and ServiceCacheUpdatedSince. Additionally, if this new approach makes sense we should likely apply the same updates to the EndpointSliceMirroring controller. At that point my work towards moving this out to a separate package that can be referenced by both controllers becomes relevant. Adding a new package does seem like a bit of a stretch for a bug fix though.

With that said, my tests have shown that this does fix #93374 and the related bugs. I know this is not an ideal solution though, so if anyone has other ideas I'm very open to them.

/assign @liggitt @freehan @wojtek-t

/priority critical-urgent

Talked with @bowei this morning about this some more and he pointed out that this specific PR is more of an optimization than a correction. With that said, I think I'm OK with leaving this specific PR/optimization problem for later and making the following smaller fixes:

• Update the flaky test to consider duplicate EndpointSlices valid.
• Update the docs to make it very clear that consumers of the EndpointSlice API will have to do deduplication (as kube-proxy already does).
• Update the controller error handling to return immediately if a create, update, or delete errors (in that order).

I'm hoping to have PRs for all of the above ready later today.

/priority important-longterm

/remove-priority critical-urgent

/milestone v1.20

explicitly holding until we decide if we want to go with this approach

@robscott: PR needs rebase.

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

Thanks for the reminder on this one @thockin! This specific approach is likely not ideal, but it does seem like we'll need some optimizations here. At present, we're doing a lot of unnecessary work due to the controller operating with an out of date cache. Although certainly not elegant, I believe even this PR would represent an improvement over the status quo. I think @lavalamp's suggestion above to try to use/implement something resembling a write-through cache could be helpful.

Fundamentally I don't think we want to move away from the informer cache as the source of truth here, so we just need to find a way reduce the odds that we'll run a sync with before that cache has caught up. Those odds are currently rather high during a rolling update, and I don't think it would take a very complex solution to provide a significant improvement here.

/remove-lifecycle stale

A cheap hack is to at least make actions deterministic enough that they conflict

@robscott if the endpoint slice name is deterministic, will this be solved?

kubectl get endpointslices -o wide
NAME         ADDRESSTYPE   PORTS     ENDPOINTS    AGE
test-001   IPv4          <unset>   <unset>      19d
test-002   IPv4          <unset>   <unset>      19d

Unfortunately I don't think the name itself is sufficient. If I'm remembering the problem correctly, we're also seeing a large number of these warnings from the controller attempting to update a stale copy of an EndpointSlice.

Created a new PR that takes a bit of a simpler approach here: #99345. Closing this one in favor of that.

/close

@robscott: Closed this PR.