Reload Collectors without restart when ConfigMap changes

[damemi]

Kubernetes automatically updates mounted configmaps in a pod (see https://kubernetes.io/docs/tutorials/configuration/updating-configuration-via-a-configmap/). But, the workload needs to watch for the update itself. So in our case, if the collector knows to watch for file changes, it can automatically signal a dynamic config reload without restarting the collector.

This forks the default fileprovider in our collectors to create a new odigosfileprovider that uses fsnotify to watch for updates to the collector ConfigMap. When the ConfigMap is updated, the new fileprovider will signal the collector to hot-reload its config. Technically, we watch for fsnotify.Remove events, because the projected configmap data is a symlink, not an actual copy of the configmap (meaning that watching fsnotify.Write doesn't trigger any update).

This means we don't need to restart the collector deployments or daemonsets for basic config updates, so those controllers have been updated to no longer update deployments when just the configmap has changed. They can of course still be manually redeployed with kubectl.

In my manual testing, it took about 1 minute for the change to be reflected, which is due to the default kubelet sync period (https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#mounted-configmaps-are-updated-automatically). Not sure how we could test for this automatically but working on that

[notion-workspace]

Research: Dynamic collector configuration instead of static ConfigMap

[blumamir]

Thanks mike, added few comments.

I wonder if we should now revisit this mechanism:

odigos/autoscaler/controllers/datacollection/daemonset.go

Line 49 in 89aa4d0

func (dm *DelayManager) RunSyncDaemonSetWithDelayAndSkipNewCalls(delay time.Duration, retries int, dests *odigosv1.DestinationList,

It was added to prevent a burst of restarts to the node collectors when sources a reconciled in a batch one-by-one. we can do it also as a followup PR. If we can remove it, we will have odigos pipeline starting 5 seconds earlier.

[blumamir]

According to the provider docs:

// Provider is an interface that helps to retrieve a config map and watch for any
// changes to the config map. Implementations may load the config from a file,
// a database or any other source.
//
// The typical usage is the following:
//
//	r, err := provider.Retrieve("file:/path/to/config")
//	// Use r.Map; wait for watcher to be called.
//	r.Close()
//	r, err = provider.Retrieve("file:/path/to/config")
//	// Use r.Map; wait for watcher to be called.
//	r.Close()
//	// repeat retrieve/wait/close cycle until it is time to shut down the Collector process.
//	// ...
//	provider.Shutdown()

Seems the Retrieve function may be called once per file update which can accumulate over time and Add many watchers on the same file.

Do we need to remove this watch after invoking the wf callback to ensure we are not leaking resources?

[damemi]

I think we don't need to worry about adding multiple watches on the same file from the same provider, the fsnotify docs say: A path can only be watched once; watching it more than once is a no-op and will not return an error.

But you are right that this code will leak goroutines, so I added some handling for that with a boolean to track if a watcher loop is already running (so we don't start another one)

[RonFed]

can we use sync.Once for this pattern? (instead of boolean and mutex)

[damemi]

I'm not sure sync.Once is what we want. I think that will only let the routine be called once, ever, for this instance of the provider. What I'm doing here considers the case where the watcher closes (for whatever reason) and ends the goroutine, then Retrieve is called again on the same Provider. In that case, we should start up the watcher again. sync.Once won't start it a 2nd time

[RonFed]

Why are those needed?

[damemi]

See #1903 (comment)

[RonFed]

I think we can remove them now that we have a provider and we don't use them.

[damemi]

We probably will still want the env provider at least, others I think can be removed

[RonFed]

Looks great, left a few non-blocking comments (except maybe the concurrent access to the running flag in the provider)

[RonFed]

Can we remove the configData argument from this function?

[damemi]

that's what I did, since we don't want a change in the configdata to trigger a new rollout, we don't need to hash the config data anymore. is that what you were asking?

[RonFed]

I was referring to the function signature of syncDeployment:

func syncDeployment(dests *odigosv1.DestinationList, gateway *odigosv1.CollectorsGroup, configData string,
	ctx context.Context, c client.Client, scheme *runtime.Scheme, imagePullSecrets []string, odigosVersion string) (*appsv1.Deployment, error)

[damemi]

Ah, updated in 6d53ab0

[RonFed]

I think there is possible non-safe concurrent access to this flag since this is set inside a goroutine, and being read from the outer Retrieve

[damemi]

updated this to lock around the variable. this feels a little like "communicating by sharing memory" but I'm not sure of a better way to do what we want here

[blumamir]

If this code is invoked from a flow which isn't Shutdown, this call will block indefinitely, right?

[damemi]

good point, I switched this back to using a waitgroup which I think will fix that