informers: Don't treat relist same as sync #86015

squeed · 2019-12-06T21:44:54Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

Background:

Before this change, DeltaFIFO emits the Sync DeltaType on Resync() and Replace(). Seperately, the SharedInformer will only pass that event on to handlers that have a ResyncInterval and are due for Resync. This can cause updates to be lost if an object changes as part of the Replace(), as it may be incorrectly discarded if the handler does not want a Resync.

What this change does:

Creates a new DeltaType, Replaced, which is emitted by DeltaFIFO on Replace(). For backwards compatability concerns, the old behavior of always emitting Sync is preserved unless explicity overridden.

As a result, if an object changes (or is added) on Replace(), now all SharedInformer handlers will get a correct Add() or Update() notification.

One additional side-effect is that handlers which do not ever want Resyncs will now see them for all objects that have not changed during the Replace.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Shared informers are now more reliable in the face of network disruption.

/cc @sttts
/cc @liggitt

squeed · 2019-12-06T21:48:39Z

Many thanks to @sttts, who helped me understand exactly where to look in the SharedInformer when faced with an "impossible" sequence of log lines.

liggitt · 2019-12-06T23:29:41Z

Separately, there is a commit that elides re-List updates if RV and UID hasn't changed.

Can that be split to a separate PR? That's separate from the bug fix, correct? We've previously decided against teaching informers about uids, would like to consider that separately and not backport that.

staging/src/k8s.io/client-go/tools/cache/testing/fake_controller_source.go

staging/src/k8s.io/client-go/tools/cache/shared_informer.go

squeed · 2019-12-09T07:47:27Z

Can that be split to a separate PR? That's separate from the bug fix, correct? We've previously decided against teaching informers about uids, would like to consider that separately and not backport that.

It's not separate to this bug fix; @sttts suggested that I split it out since it's not strictly necessary. There are two points for consideration:

The larger bugfix is also changing existing behavior. Assume a handler without ResyncInterval doesn't want to get a Sync-style update (e.g. Update(A1, A1)) and only wants edges. However, if the client internally decides to do a re-List, then the handler will still see no-op updates. Now, I don't think there's any kind of contract that says you'll never get no-op updates, but it it was coded in to some of the tests.
As to UIDs, I wasn't sure of the guarantees for ResourceVersion across delete-and-recreate. It's not explicitly mentioned in the documentation. If a new object can't have the same ResourceVersion (currently impossible with etcd, of course), then we can drop the UID.

Additionally, the elision only takes place for Replace events - so it's not a large change.

liggitt · 2019-12-09T14:10:40Z

2. If a new object can't have the same ResourceVersion (currently impossible with etcd, of course), then we can drop the UID.

resourceVersion is unique per resource type (it has to be, since we use it for list requests across resource instances), so it cannot repeat on a delete/recreate

liggitt · 2019-12-09T14:11:28Z

It's not separate to this bug fix; @sttts suggested that I split it out since it's not strictly necessary.

We should split it to a separate PR, as it needs more discussion, and even if accepted, I would not expect to backport that part.

squeed · 2019-12-09T14:12:00Z

resourceVersion is unique per resource type (it has to be, since we use it for list requests across resource instances), so it cannot repeat on a delete/recreate

Ack. Removed UID check; doesn't affect correctness.

liggitt · 2019-12-09T14:12:07Z

fyi @smarterclayton @lavalamp

staging/src/k8s.io/client-go/tools/cache/controller.go

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

squeed · 2019-12-09T15:37:33Z

We should split it to a separate PR, as it needs more discussion, and even if accepted, I would not expect to backport that part.

Understood. The open question to me is: is it OK to send resync updates to handlers that don't want them? If so, then we don't need the last commit. Otherwise we do.

lavalamp · 2019-12-09T19:54:54Z

I think it is probably good to make the distinction, but this will be disruptive to users, potentially very disruptive.

Therefore, I think you need to wire an option or provide a 2nd constructor or something, so that existing code can continue to work the same with no changes.

squeed · 2019-12-09T21:24:24Z

I think it is probably good to make the distinction, but this will be disruptive to users, potentially very disruptive.

Therefore, I think you need to wire an option or provide a 2nd constructor or something, so that existing code can continue to work the same with no changes.

@lavalamp I don't quite follow - what sort of option were you thinking of?

To be clear here, this PR doesn't disable resyncs or change the standard informer behavior at all. It just ensures we don't miss updates due to a re-List. AFAICT, the question is whether or not we should expose the re-List due to disruption to the handlers.

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

lavalamp · 2019-12-09T21:31:46Z

doesn't disable resyncs or change the standard informer behavior at all

There can be direct clients of this.

The fact that we can make corresponding changes in the informer to preserve its behavior makes your life much easier though.

squeed · 2019-12-09T22:36:44Z

As an alternative to adding a new Delta type, we could "fix" this in the SharedInformer by cheating a bit, and treating Sync as Updated if the object differs from the store.

liggitt · 2020-01-20T17:08:17Z

/lgtm
/approve

liggitt · 2020-01-20T17:09:16Z

unit test failure is legitimate:

$ go test ./vendor/k8s.io/client-go/tools/cache
# k8s.io/client-go/tools/cache [k8s.io/client-go/tools/cache.test]
staging/src/k8s.io/client-go/tools/cache/delta_fifo_test.go:294:3: undefined: keyLookupFunc
staging/src/k8s.io/client-go/tools/cache/delta_fifo_test.go:410:17: undefined: keyLookupFunc
staging/src/k8s.io/client-go/tools/cache/delta_fifo_test.go:440:17: undefined: keyLookupFunc

liggitt · 2020-01-20T17:09:22Z

/lgtm cancel

squeed · 2020-01-20T17:11:29Z

whoops, fixing

squeed · 2020-01-20T20:25:17Z

/retest

liggitt · 2020-01-21T15:26:35Z

/retest

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

lavalamp · 2020-01-21T17:15:16Z

Overall, great change-- I hate to ask for something but the name is unchangeable once we cut a client-go release, so...

Background: Before this change, DeltaFIFO emits the Sync DeltaType on Resync() and Replace(). Seperately, the SharedInformer will only pass that event on to handlers that have a ResyncInterval and are due for Resync. This can cause updates to be lost if an object changes as part of the Replace(), as it may be incorrectly discarded if the handler does not want a Resync. What this change does: Creates a new DeltaType, Replaced, which is emitted by DeltaFIFO on Replace(). For backwards compatability concerns, the old behavior of always emitting Sync is preserved unless explicity overridden. As a result, if an object changes (or is added) on Replace(), now all SharedInformer handlers will get a correct Add() or Update() notification. One additional side-effect is that handlers which do not ever want Resyncs will now see them for all objects that have not changed during the Replace.

squeed · 2020-01-23T10:26:27Z

OK, rename done.

liggitt · 2020-01-23T13:48:31Z

/lgtm
/approve
/retest

k8s-ci-robot · 2020-01-23T13:49:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~staging/src/k8s.io/client-go/tools/cache/OWNERS~~ [liggitt]

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

fejta-bot · 2020-01-23T17:37:01Z

/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 · 2020-01-23T20:45:50Z

/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 · 2020-01-24T01:39:49Z

/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.

k8s-ci-robot requested review from liggitt and sttts December 6, 2019 21:44

squeed force-pushed the informer-missing-updates branch from e77bb56 to c3567d0 Compare December 6, 2019 21:47

abhat reviewed Dec 9, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/testing/fake_controller_source.go Outdated Show resolved Hide resolved

abhat reviewed Dec 9, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/shared_informer.go Outdated Show resolved Hide resolved

squeed force-pushed the informer-missing-updates branch from c3567d0 to c174e22 Compare December 9, 2019 10:51

liggitt reviewed Dec 9, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/controller.go Show resolved Hide resolved

liggitt reviewed Dec 9, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go Outdated Show resolved Hide resolved

lavalamp reviewed Dec 9, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go Show resolved Hide resolved

lavalamp reviewed Dec 9, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go Outdated Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2020

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2020

squeed force-pushed the informer-missing-updates branch from 0279ec0 to c889d9d Compare January 20, 2020 17:29

lavalamp reviewed Jan 21, 2020

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go Outdated Show resolved Hide resolved

squeed force-pushed the informer-missing-updates branch from c889d9d to ca1eeb9 Compare January 23, 2020 10:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2020

k8s-ci-robot merged commit 9f09913 into kubernetes:master Jan 24, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 24, 2020

This was referenced Feb 8, 2020

Treat replaced events that didn't change resourceVersion as resync events #87957

Merged

[flaky test] Garbage collector timeout tests #87668

Closed

Lower ssa auto-enablement to 10% #88019

Merged

aojea mentioned this pull request Feb 21, 2020

[Flaky Test] [sig-network] Services should be able to switch session affinity for service with type clusterIP [LinuxOnly] #86370

Closed

liggitt mentioned this pull request Jul 28, 2020

Webhook REST clients cache invalid and possibly not expiring #93409

Closed

MikeSpreitzer mentioned this pull request Nov 30, 2020

Replace type of Delta not mentioned in comment on DeltaFIFO #96942

Closed

ingvagabund mentioned this pull request Mar 18, 2021

Replace deprecated NewDeltaFIFO with NewDeltaFIFOWithOptions #100355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

informers: Don't treat relist same as sync #86015

informers: Don't treat relist same as sync #86015

squeed commented Dec 6, 2019 •

edited

squeed commented Dec 6, 2019

liggitt commented Dec 6, 2019

squeed commented Dec 9, 2019 •

edited

liggitt commented Dec 9, 2019

liggitt commented Dec 9, 2019 •

edited

squeed commented Dec 9, 2019

liggitt commented Dec 9, 2019

squeed commented Dec 9, 2019

lavalamp commented Dec 9, 2019

squeed commented Dec 9, 2019

lavalamp commented Dec 9, 2019

squeed commented Dec 9, 2019

liggitt commented Jan 20, 2020

liggitt commented Jan 20, 2020

liggitt commented Jan 20, 2020

squeed commented Jan 20, 2020

squeed commented Jan 20, 2020

liggitt commented Jan 21, 2020

lavalamp commented Jan 21, 2020

squeed commented Jan 23, 2020

liggitt commented Jan 23, 2020

k8s-ci-robot commented Jan 23, 2020

fejta-bot commented Jan 23, 2020

fejta-bot commented Jan 23, 2020

fejta-bot commented Jan 24, 2020

informers: Don't treat relist same as sync #86015

informers: Don't treat relist same as sync #86015

Conversation

squeed commented Dec 6, 2019 • edited

Background:

What this change does:

squeed commented Dec 6, 2019

liggitt commented Dec 6, 2019

squeed commented Dec 9, 2019 • edited

liggitt commented Dec 9, 2019

liggitt commented Dec 9, 2019 • edited

squeed commented Dec 9, 2019

liggitt commented Dec 9, 2019

squeed commented Dec 9, 2019

lavalamp commented Dec 9, 2019

squeed commented Dec 9, 2019

lavalamp commented Dec 9, 2019

squeed commented Dec 9, 2019

liggitt commented Jan 20, 2020

liggitt commented Jan 20, 2020

liggitt commented Jan 20, 2020

squeed commented Jan 20, 2020

squeed commented Jan 20, 2020

liggitt commented Jan 21, 2020

lavalamp commented Jan 21, 2020

squeed commented Jan 23, 2020

liggitt commented Jan 23, 2020

k8s-ci-robot commented Jan 23, 2020

fejta-bot commented Jan 23, 2020

fejta-bot commented Jan 23, 2020

fejta-bot commented Jan 24, 2020

squeed commented Dec 6, 2019 •

edited

squeed commented Dec 9, 2019 •

edited

liggitt commented Dec 9, 2019 •

edited