Race condition on startup when using configuring controller via configmaps

[bobcatfish]

Expected Behavior

When a user configures the Pipeline controller's behavior via configmaps, e.g. disabling the affinity assistant or enabling custom tasks, the controller should always respect these configured values.

When the controller tries to use these configured values, it should always reflect the settings in the config maps if they exist.

Actual Behavior

We are using an object provided by knative/pkg called configStore to access feature flags (configuration provided by users via config maps). It used to be that we loaded these values once on startup but in #2637 we switched over to using these config store objects so we could hot reload changes made to the config maps the while the controller is running.

1. Controller is started via main
2. main calls knative's sharedmain.MainWithConfig
3. MainWithConfig starts the config watchers
4. cmw.Start is InformedWatcher.Start
5. Start calls registerCallbackAndStartInformer
6. That calls sharedInformerFactory.Start
7. Ultimately this is where the interesting bit happens, all the informers are run in separate go routines:
   

   pipeline/pkg/client/resource/informers/externalversions/factory.go

   Lines 115 to 120 in 0fddf2e

   	for informerType, informer := range f.informers {
   	if !f.startedInformers[informerType] {
   	go informer.Run(stopCh)
   	f.startedInformers[informerType] = true
   	}
   	}

The gorutine started in (7) (client-go cache controller's Run I think?) is what (waves hands) ultimately leads to reading the configured values from the config maps and storing them such that when the tekton controller tries to read these values (via FromContextOrDefaults) the idea is that it will see the values populated by the goroutine.

The problem is that there is no synchronization so sometimes the controller tries to read those values before the goroutines have populated them.

This causes tests of this functionality to sometimes fail, but more importantly, there is a chance (a very small chance) this could happen to users.

Steps to Reproduce the Problem

0. Configure a feature via config map
1. Start the controller
2. Immediately try to start a taskrun that uses the feature you have configured
3. Some tiny eensy teensy percentage of the time, you might see that the configured value isn't used and the controller uses its hard coded default instead

(3) would happen SO rarely that you might never be able to reproduce it - see #2815 (comment) on how to reproduce this via the integration tests

Additional Info

This is the bug in knative/pkg describing the problem and requesting it to be fixed or a way to work around it: knative/pkg#1960

I think this is the route cause of flakey failures of:

• TestReconcile_ExplicitDefaultSA (Nightly unit test failure due to race condition when initializing config stores #2815)
• TestReconcile_FeatureFlags ([flaky] TestReconcile_FeatureFlags Unit test failed in pipeline nightly job #3587)
• TestReconcile_FeatureFlags/disable-working-dir-overwrite (TestReconcile_FeatureFlags/disable-working-dir-overwrite unit test seems flaky #3351)

What to do??

While we definitely want to fix this issue, I think that letting our tests continue to flake because of this issue is reducing the quality of the signal we're getting from our tests. Now that we know this is a problem, we don't need the tests to keep failing the way they do.

But because the details of this are hidden in unexported knative/pkg code I'm not sure how to work around it. Some ideas, all terrible:

1. Disable the impacted tests <-- not great b/c then if something ELSE around this functionality breaks, we won't know
2. Retry the impacted tests <-- probably better than (1) but still not a guarantee they won't flake
3. Add an arbitrary sleep into the impacted tests between when they instantiate the controller and when they use the value <-- not guaranteed to fix the problem but since chances of it happening are so small, it might be enough
4. Do the same as (3) but do it in the actual controller <-- pretty awful, and would make controller start up take longer, but might "fix" the actual problem for users too

[bobcatfish]

Okay knative/pkg#2036 is an attempt to fix this! If it's accepted, we should see these races fixed when we next update knative/pkg :D 🙏

[shinji62]

We are still seeing this issue with 0.24.x, if we do a restart of the controller it will create an affinity assitant even if false in the configmap, and we got this at every restart (~150 pipelinerun running a day) .

Wondering why we don't make sure that we read the configmap before the controller can start working.

[vdemeester]

@shinji62 sounds like a bump indeed 🤔 can you create an new issue for this ?