refactoring informer logic for type safety and to refine the role of stores

[shawkins]

Description

While looking at changes for #2993, #2995, and another bug that has not been logged here. It appears there are some clarifications that could be made to improve the readability and the safety of the informer code. The central change is that Store should be more type specific and have a narrower interface - I realize this differs from the go implementation and this does change the public interface of SharedIndexInformer.getIndexer because the Store will no longer have replace, resync, or isPopulated. If it's a problem to make such a change it can of course be refined (previous code, all no-ops, or throw not implemented exceptions). In general it seems problematic that SharedIndexInformer.getIndexer returns as broad of an interface as it does - for example in DefaultSharedIndexInformer.addIndexers you can't add an indexer once running, but if you call SharedIndexInformer.getIndexer.addIndexers you can.

If you'd prefer another interface such as ResyncableStore vs direct usage of the DeltaFIFO in ResyncRunnable, Reflector, and ReflectorWatcher - that can be done as well, it just didn't seem needed given the single implementation of DeltaFIFO. I do see several implementations of Store types in go.

This also moves the responsibility for the lastResourceVersion to the DeltaFIFO - having it updatable outside the DeltaFIFO lock is dangerous. With the code pre #2812 it appears to allow for timing issues in threads updating the DeltaFIFO/lastResourceVersion:

1. resyncExecutor thread performs scheduled relist - sees state s1, sets the last resourceVersion to s1
2. watch thread via ReflectorWatcher - sees an update to state s2, calls DeltaFIFO.update and sets last resourceVersion to s2
3. resyncExecutor thread continues - calls DeltaFIFO.replace with state s1, adds a syncronization event to state s1 after the update event

The event generated from 2 will be an update showing s1 as the old state and s2 as the new
The event generated from 3 will be an update showing s2 as the old state and s1 as the new

A reconnect won't fix that as the last resourceVersion is s2.

In theory though on the next relist there will be a synchronization and event to s2 (or whatever the latest state is). However with the pre #2812 code any uncaught exception from relist will inhibit the job from running again.

Granted this should no longer happen after #2812 because the scheduled relist has been removed, but it is still something that should be guarded against.

@rohanKanojia or others let me know if you are open to these types of changes, I can refine this PR as needed.

Type of change

• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[centos-ci]

Can one of the admins verify this patch?

[sonarcloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

27.3% Coverage
0.0% Duplication

[shawkins]

I'll rebase this after 5.3.1 - it appears that quite a few changes will make it into that release. And some of this is already being addressed by things like #3031

[manusa]

I'll rebase this after 5.3.1

Hi @shawkins, 5.3.1 was just released

[shawkins]

@manusa thank you. I think the way forward will be to split this into several prs. I've started with #3048 - that incorporates some the changes from here.

After that, can we look at #3029

Then I will include the changes that move the last resource version responsibility onto the store.

Finally I'll open a pr that refines the Store interface.

[shawkins]

I'll now replace this pr with one that refines the store interface. This will be for after the deltafifo removal #3061 and ideally after a cleanup of the threading concerns for informers: shawkins#2