onUpdate is called on re-list even if resourceVersion is unchaged

[akram]

Describe the bug

When a re-list happens, an onUpdate event is received even if the object resourceVersion didn't change.
5.10.1

Fabric8 Kubernetes Client version

5.10.1@latest

Steps to reproduce

create a test having a relist to a quite short version
wait for the relist to happen
see that an onUpdate is triggered for even a not changed object

Expected behavior

the onUpdate should not be triggered if resourceVersion is unchanged

Runtime

OpenShift

Kubernetes API Server version

1.22.3@latest

Environment

Amazon

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

[shawkins]

Internally this is processed as a resync event, so this should only happen if the listener is due for resync events.

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/cache/ProcessorStore.java

Line 97 in 12af747

this.processor.distribute(new ProcessorListener.UpdateNotification<>(old, newValue), same);

[akram]

this is on 6.0.0-SNAPSHOT @shawkins . I don't remember that it was that way on 5.x
In any case, would it make sense to consider filtering the updates notifications if the versions are equal?

[shawkins]

I don't remember that it was that way on 5.x

It's been this way since #3318

In any case, would it make sense to consider filtering the updates notifications if the versions are equal?

From what I recall not filtering matches the behavior of the go client.

If you have resync disabled, then you won't see events like this.

[akram]

In our project, we are ending up filtering the event, as disabling resync was not also an option. Because, we are fearing to loose some events at some point. So, having a regular resync makes things clearer for us.

What do you suggest @shawkins ?
should we maintain this filtering on unchanged objects?
Or disable the resync and find a way to re-sync manually at some points?

I would be interested in knowing how users of go client are handling this. any feedback?

[shawkins]

should we maintain this filtering on unchanged objects?

All resync events have unchanged resourceVersions - if you are filtering them then you have effectively disabled resync.

[shawkins]

It's worth mentioning that in 5.11+ relists are much less frequent as the underlying watch will use bookmarks.

[akram]

should we maintain this filtering on unchanged objects?

All resync events have unchanged resourceVersions - if you are filtering them then you have effectively disabled resync.

unless we really missed an event. In this case, a resync will send us the update and we will compare the resourceVersion of our local object with the one received on update.

We will give it a try that way then. And, we will also try a version with resync disabled, but that's more impacts.

And, we can probably close the issue then, and I may ask to re-open if I got additional arguments for the discussion.

Thanks @shawkins

[shawkins]

unless we really missed an event. In this case, a resync will send us the update and we will compare the resourceVersion of our local object with the one received on update.

Can you clarify which local object you are referring to?

What I'm saying is that on all resync events when onUpdate is called oldObj and newObj will be the same.

[akram]

we are putting some watched objects (ConfigMap) in our local cache (basically a Map). When we received an update event for an object, we were replacing it (delete/insert) in our local cache.

[shawkins]

How does the local cache differ from the informer cache/store?

[akram]

it was there for historical reason, even before we migrated to informers. We have objects coming from file, and others for which the ConfigMap.
If we change the ones from disk, we reload those coming from ConfigMap . If the ones from ConfigMap are modified (label removed or added), we also update the disk representation.

That's our internal sauce. We cannot change it easily without risking another set of regressions.

[shawkins]

Here's the relevant commit from the go client kubernetes/client-go@b775e00

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.

The implication here is that their handling of the Replace event happens regardless of whether the Handlers use resync. So our handling already differs. At the time that we process the event we can discern if it's an update or a sync. I'll walk back my earlier assessment and say that it seems fine to take this one step further and to just not emit these in the sync case.

That's our internal sauce. We cannot change it easily without risking another set of regressions.

Understandable. I'm always curious to see how resync is being used as there's a lot of somewhat fragile logic dedicated to it and it's a blunt instrument - at it's core it's simply a timed job that iterates over the cache values and calls onUpdate. I'd like to better understand how it might be improved - such as only re-syncing for handler calls that through an exception (relates to #3968)

[shawkins]

@akram is this something you can do the pr for?

[akram]

@shawkins it seems good to have the discussion about it. And, these small implementations differences or behaviour are often the causes of bugs :-)
Our use case seems really similar to what #3968 does. Except that instead of having a database, we have a file, that we also read on startup, and for which we wait for updates events that we consider to be a higher source of truth.

Regarding working out a PR right now, I don't have a lot of time this week, and I could have a look only by the end of next week. And, also, currently, we are using a 5.x version, which may differ from 6.x, so, my PR would probably only target 5.x.

If, you think that you could be more efficient in writing a PR for 6.x, feel free to go ahead, I can then focus on migrating our plugin to 6.x and wait for the 6.0.0 Final.

Also, the question of backward compatibility is important here. For users who already "exploit" the existing behaviour, they may be impacted. So, should we introduce a compatibility flag? and what should be the default?

[shawkins]

Also, the question of backward compatibility is important here. For users who already "exploit" the existing behaviour, they may be impacted. So, should we introduce a compatibility flag? and what should be the default?

I would not introduce a flag. These events are dependent upon a relist, which there is no user way of triggering and will be very rare with 5.11+. These events also don't represent a change, so it would be odd for a user to be dependent upon them - at worst it's an infrequent off period sync event. So this is really a narrow change, which after reviewing the work in the go client, I'm fine with diverging further.