Wait until stores are filled before processing service account token events

[liggitt]

Since the service account tokens controller makes use of two stores, getting update events from the initial list operations caused misses if the other store wasn't populated yet. This adds a gate to prevent that.

[liggitt]

@lavalamp PTAL. If there's a better way to coordinate two stores, I'd be happy to switch to it

[lavalamp]

Yeah-- I know there's a general need to tell when a store has been initially populated.

This particular solution is not one I'd like to see copied everywhere (you know people will do that if it shows up in the codebase somewhere). (Also--I'm pretty sure the race detector is going to flag this.)

I have two ideas:

1. Add plumbing to expose the last sync version; testing if it's not empty should do the trick. https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/client/cache/reflector.go#L148
2. Add a HasSynced() bool method to Store which returns false until Replace() is called, and then afterwards returns true.

[liggitt]

I combined your ideas and created HasSynced() on Controller... the warnings about thread-safety on LastSyncResourceVersion() made me leery of exposing an exact version for people to rely on. PTAL

[liggitt]

You win the data race prediction:

WARNING: DATA RACE
Read by goroutine 247:
  github.com/GoogleCloudPlatform/kubernetes/pkg/controller/framework.(*Controller).HasSynced()

[liggitt]

Guarded the lastSyncResourceVersion field with a RWMutex to prevent data races, but didn't pursue synchronizing with the underlying store.

[lavalamp]

nit: // These are here so tests can inject a 'return true'.

[lavalamp]

LGTM, but you should fix the (still remaining) data race.

[derekwaynecarr]

I thought I was the only one writing controllers that needed to work against more than one store :-)

Sent from my iPhone

On May 20, 2015, at 5:59 PM, Daniel Smith notifications@github.com wrote:

LGTM, but you should fix the (still remaining) data race.

—
Reply to this email directly or view it on GitHub.

[liggitt]

needed a mutex around the reflector nil check in HasSynced(). Added and testing comment added

[liggitt]

Travis flaked on TestPersistentVolumeClaimBinder, but passed the data race checks

[lavalamp]

LGTM