Race condition when using config store backed by config maps

[bobcatfish]

Expected Behavior

When I try to use a config store backed by a config map immediately after the process using it starts up, when the config map exists before the process starts, I'd like to be able to guarantee that I am reading the configured value.

I think we could do this if one of the following was true:

1. When the reconciler starts, it could wait for the config store to be initialized, i.e. wait for the goroutines that handle events to fire at least once (see TestInformedWatcher flaky  #1907 (comment) for a description of this code) before returning
2. Provide a way for code using this reconciler to wait for this state (e.g. maybe don't do (1) by default, but at least provide a way for a caller to do this, e.g. via the Reconciler object)
3. Export the config store so that I can look at the state

/kind bug

Actual Behavior

When I use a generated reconciler, which has an unexported configstore, if I try to back that config store with configmaps, there is a race condition where if I try to use that config store immediately after start up, the informed watcher's goroutines which read from the backing config maps and update the config store may not have fired yet, which means I might read something from the config store other than what I've configured.

Steps to Reproduce the Problem

• I believe TestInformedWatcher flaky  #1907 was actually catching this bug (explanation at TestInformedWatcher flaky  #1907 (comment))
• I repro-ed this on the Tekton side with these horrible hacks: bobcatfish/pipeline@1fbc388 (trying to more reliably reproduce Nightly unit test failure due to race condition when initializing config stores tektoncd/pipeline#2815)

Additional Info

• More detail from a Tekton perspective in Nightly unit test failure due to race condition when initializing config stores tektoncd/pipeline#2815 (comment)

[bobcatfish]

@dprotaso @mattmoor @runzexia do you have any thoughts on what the best approach is to take here? Are you open to the knative/pkg controller code doing something to wait and make sure the configstores are initialized correctly? (This seems like the best solution to me cuz this bug would impact all users of this lib)

(some other ideas listed above)

[imjasonh]

Friendly ping, I also noticed this in https://github.com/imjasonh/tektoncd-concurrency

[mattmoor]

Based on the discussion in the linked issue, I'd change this check to not be based on the informer cache, but based on a positive hand-off signal from the callback:

pkg/configmap/informed_watcher.go

Lines 175 to 176 in c5296cd

	if _, err := i.informer.Lister().ConfigMaps(i.Namespace).Get(k); err != nil {
	if _, ok := i.defaults[k]; ok && k8serrors.IsNotFound(err) {

I suspect we could do this just by instrumenting add here:

pkg/configmap/informed_watcher.go

Line 160 in c5296cd

AddFunc: i.addConfigMapEvent,

[bobcatfish]

@mattmoor sounds like it makes sense to resolve this in knative/pkg 👍

I'd be happy to make PR but I need a bit more detail about what you're envisioning (sounds like we'd be creating an interface where the callbacks can signal, maybe via a channel, that they're ready?) - lemme know if it makes more sense to schedule some time to discuss or if it makes sense for me to bring up in a knative WG

[mattmoor]

IDK that we need to change the interface, since we are responsible for calling each callback. I was just thinking about some internal bookkeeping to check that all of the callbacks had been called...? 🤷

I'm happy to chat, or we can throw it on an API WG agenda.

[bobcatfish]

@mattmoor I was gonna join a working group but I think I figured out what you meant - lemme know if #2036 is the kind of thing you had in mind :D ❤️