handle data-collection config map creation in autoscaler reconciler, enhance config map provider in the collector by RonFed · Pull Request #3522 · odigos-io/odigos

RonFed · 2025-09-18T15:21:01Z

This PR changes the node collector (data-collection) config map creation:

Following Combine odiglet datacollection - CORE-137 #3385 the config map was created by the helm/cli installation. This caused some issures when no sources were present and after helm upgrade the config map would get deleted. This PR move the responsibility of creating the no-op config map to the autoscaler which is already the component that is responsible for updating it.
If no sources are present, the data collection controller now creates the no-op config map.
The autoscaler deployment is set to be the owner of the config map - this guarantees that the config map will get deleted when odigos is un-installed - before this, the owner was the collectorsgroup - but if no sources are present there is no collectorsgroup object.

The cm config provider in the collector is enhanced by making it wait for the first time it retrieves the config - until the config map is present. The config map will now get created once the autoscaler starts and the data-collection reconciler is triggered upon start-up and the no-op config map is created. - This should not take a lot of time but in case it is, the config provider will wait until the config map will get created for the first time, and won't return an error resulting in the collector entering CrashloopBackoff.

To improve indication of this possible initial waiting step a startup probe is added to the data collection container - this start up probe will not return a healthy status until the first config is loaded into the collector.

…enhance config map provider in the collector

damemi

overall lgtm just some questions on things I didn't know about

damemi · 2025-09-18T20:35:56Z

            httpGet:
              path: /
              port: 13133
+          startupProbe:


what endpoint does this startup probe actually hit that tells it whether it's ready or not?

I guess it's the one from the healthcheckextension which becomes ready once the collector starts. like the livness and readiness above.

being setup here

damemi · 2025-09-18T20:44:40Z

 	processors := commonconf.FilterAndSortProcessorsByOrderHint(allProcessors, odigosv1.CollectorsGroupRoleNodeCollector)

-	desired, err := getDesiredConfigMap(sources, signals, processors, datacollection, scheme)
+	desired, err := getDesiredConfigMap(ctx, sources, signals, processors, datacollection, scheme, c)


this function SyncConfigMap is only called in reconcileNodeCollector which is called by the collectorsgroup controller, instrumentationconfig controller, and processor controller

how does it work then that this gets triggered on an initial startup where there aren't any of these, which seems like the no op configmap is meant for? or is the nop cm just for when there aren't destinations?

I remember we chatted about it at the office, wonder too how this get's created the first time eventually.

looking more into this the reconciler is defined with

Named("nodecollector-collectorsgroup"). For(&odigosv1.CollectorsGroup{}). Owns(&appsv1.DaemonSet{}). // in case the node-collector ds is deleted or modified for any reason, this will reconcile and recreate it Owns(&corev1.ConfigMap{}). // in case the configmap of the node-collector is deleted or modified for any reason, this will reconcile and recreate it // we assume everything in the collectorsgroup spec is the configuration for the collectors to generate. // thus, we need to monitor any change to the spec which is what the generation field is for. WithEventFilter( predicate.Or( predicate.And(&odigospredicate.OdigosCollectorsGroupNodePredicate, &predicate.GenerationChangedPredicate{}), predicate.And(&odigospredicate.OdigosCollectorsGroupClusterPredicate, &odigospredicate.ReceiverSignalsChangedPredicate{}), )).

looking at the logs what triggers this is the creation of the gateway collectors group. The ReceiverSignalsChangedPredicate return true on Create events - so that is the trigger.

We can make this more explicit and avoid potential future bugs if something in the collectors group logic changes.
I also thing the Owns(&appsv1.DaemonSet{}) is also not correct anymore after #3385 - but that is probably something better to do in a follow up PR.
WDYT? @blumamir @damemi

update: see 66d8aef

I added a controller that reacts only to create event of the autoscaler deployment - and it "Owns" the config map - so any modifications to the config - map will be reconciled as well - since now the cg is no longer the owner of the config map.

blumamir · 2025-09-18T21:56:38Z

-	}
-
-	setupLog.Info("starting manager")
-	if err := mgr.Start(ctx); err != nil {
-		setupLog.Error(err, "problem running manager")
-		os.Exit(1)
+		setupLog.Error(err, "autoscaler exited with error")
 	}
+	setupLog.V(0).Info("autoscaler exiting")


for error we will have 2 lines: exited -> exiting. should we write the 2nd log only on success?

blumamir · 2025-09-18T22:12:42Z


-	if err := ctrl.SetControllerReference(datacollection, &desired, scheme); err != nil {
+	autoscalerDeployment := appsv1.Deployment{}
+	err = c.Get(ctx, client.ObjectKey{Namespace: env.GetCurrentNamespace(), Name: k8sconsts.AutoScalerDeploymentName}, &autoscalerDeployment)


nit: not sure if supported, but do you think we can do it just once and store the deployment somewhere, and just use it here, without err and without needing to pass the client into this function?

since the deployment cannot change while this code runs :)

blumamir · 2025-09-18T22:15:03Z

 	processors := commonconf.FilterAndSortProcessorsByOrderHint(allProcessors, odigosv1.CollectorsGroupRoleNodeCollector)

-	desired, err := getDesiredConfigMap(sources, signals, processors, datacollection, scheme)
+	desired, err := getDesiredConfigMap(ctx, sources, signals, processors, datacollection, scheme, c)


I remember we chatted about it at the office, wonder too how this get's created the first time eventually.

blumamir · 2025-09-19T12:36:56Z

-			Scheme:           mgr.GetScheme(),
-			ImagePullSecrets: imagePullSecrets,
-			OdigosVersion:    odigosVersion,
+			nodeCollectorBaseReconciler: nodeCollectorBaseReconciler{


nit: now that it's all the same, we can create it just once and use the same instance for each controller in this file

blumamir · 2025-09-19T12:38:14Z

+	if b.autoscalerDeployment == nil {
+		// we only need to get the autoscaler deployment once since it can't change while this code is running
+		// (since we are running in the autoscaler pod)
+		autoscalerDeployment := &appsv1.Deployment{}
+		err := b.Client.Get(ctx, client.ObjectKey{Namespace: env.GetCurrentNamespace(), Name: k8sconsts.AutoScalerDeploymentName}, autoscalerDeployment)
+		if err != nil {
+			return err
+		}
+		b.autoscalerDeployment = autoscalerDeployment
+	}


nit: can we do it once in main maybe and the use the deployment as a parameter while setting up manager?

I was wondering the same thing but I think if the autoscaler deployment does change at all (status could change for example, something small in k8s) then we might end up having to retry. which is also not bad but I think it's just not perfect either option

… now created by the installation in Helm/CLI

… own the node collector config map

blumamir · 2025-09-20T01:24:08Z

+		WithEventFilter(
+			predicate.Or(
+				predicate.And(&odigospredicate.ObjectNamePredicate{AllowedObjectName: k8sconsts.AutoScalerDeploymentName}, &odigospredicate.CreationPredicate{}),
+				predicate.And(&odigospredicate.ObjectNamePredicate{AllowedObjectName: k8sconsts.OdigosNodeCollectorConfigMapName}, &odigospredicate.OnlyUpdatesPredicate{}),


how does it work with Own? is the event filter called with the NamespacedName of the Owned object?

yes from controller-runtime docs:

// WithEventFilter sets the event filters, to filter which create/update/delete/generic events eventually // trigger reconciliations. For example, filtering on whether the resource version has changed. // Given predicate is added for all watched objects and thus must be able to deal with the type // of all watched objects.

damemi · 2025-09-23T15:23:59Z

@@ -1,44 +0,0 @@
-{{/*


@RonFed I tried installing 1.5.0-rc1 from the helm chart locally (fresh install) and get this crashloop error in the odiglet data-collection pod:

$ kubectl logs pod/odiglet-nt5vh -c data-collection Error: failed to get config: cannot resolve the configuration: cannot retrieve the configuration: failed to get configmap: configmaps "odigos-data-collection" not found 2025/09/23 15:20:58 collector server run finished with error: failed to get config: cannot resolve the configuration: cannot retrieve the configuration: failed to get configmap: configmaps "odigos-data-collection" not found

this should probably resolve itself, right? Still looks a little bad

@damemi 1.5.0-rc1 does not include this change - so that is the behavior.
this should hopefully be part of rc2 or something - and these errors should go away - at max the collector will log that it is waiting for the config to get created we should not take a lot of time - until the autoscaler starts.

hm actually I think I deployed from main, but used the rc1 image, sorry for the false alarm

Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.3.0 to 2.4.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/releases">urllib3's releases</a>.</em></p> <blockquote> <h2>2.4.0</h2> <h2>🚀 urllib3 is fundraising for HTTP/2 support</h2> <p><a href="https://sethmlarson.dev/urllib3-is-fundraising-for-http2-support">urllib3 is raising ~$40,000 USD</a> to release HTTP/2 support and ensure long-term sustainable maintenance of the project after a sharp decline in financial support. If your company or organization uses Python and would benefit from HTTP/2 support in Requests, pip, cloud SDKs, and thousands of other projects <a href="https://opencollective.com/urllib3">please consider contributing financially</a> to ensure HTTP/2 support is developed sustainably and maintained for the long-haul.</p> <p>Thank you for your support.</p> <h1>Features</h1> <ul> <li>Applied PEP 639 by specifying the license fields in pyproject.toml. (<a href="https://redirect.github.com/urllib3/urllib3/issues/3522">#3522</a>)</li> <li>Updated exceptions to save and restore more properties during the pickle/serialization process. (<a href="https://redirect.github.com/urllib3/urllib3/issues/3567">#3567</a>)</li> <li>Added <code>verify_flags</code> option to <code>create_urllib3_context</code> with a default of <code>VERIFY_X509_PARTIAL_CHAIN</code> and <code>VERIFY_X509_STRICT</code> for Python 3.13+. (<a href="https://redirect.github.com/urllib3/urllib3/issues/3571">#3571</a>)</li> </ul> <h1>Bugfixes</h1> <ul> <li>Fixed a bug with partial reads of streaming data in Emscripten. (<a href="https://redirect.github.com/urllib3/urllib3/issues/3555">#3555</a>)</li> </ul> <h1>Misc</h1> <ul> <li>Switched to uv for installing development dependecies. (<a href="https://redirect.github.com/urllib3/urllib3/issues/3550">#3550</a>)</li> <li>Removed the <code>multiple.intoto.jsonl</code> asset from GitHub releases. Attestation of release files since v2.3.0 can be found on PyPI. (<a href="https://redirect.github.com/urllib3/urllib3/issues/3566">#3566</a>)</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/urllib3/urllib3/blob/main/CHANGES.rst">urllib3's changelog</a>.</em></p> <blockquote> <h1>2.4.0 (2025-04-10)</h1> <h2>Features</h2> <ul> <li>Applied PEP 639 by specifying the license fields in pyproject.toml. (<code>[odigos-io#3522](urllib3/urllib3#3522) <https://github.com/urllib3/urllib3/issues/3522></code>__)</li> <li>Updated exceptions to save and restore more properties during the pickle/serialization process. (<code>[odigos-io#3567](urllib3/urllib3#3567) <https://github.com/urllib3/urllib3/issues/3567></code>__)</li> <li>Added <code>verify_flags</code> option to <code>create_urllib3_context</code> with a default of <code>VERIFY_X509_PARTIAL_CHAIN</code> and <code>VERIFY_X509_STRICT</code> for Python 3.13+. (<code>[odigos-io#3571](urllib3/urllib3#3571) <https://github.com/urllib3/urllib3/issues/3571></code>__)</li> </ul> <h2>Bugfixes</h2> <ul> <li>Fixed a bug with partial reads of streaming data in Emscripten. (<code>[odigos-io#3555](urllib3/urllib3#3555) <https://github.com/urllib3/urllib3/issues/3555></code>__)</li> </ul> <h2>Misc</h2> <ul> <li>Switched to uv for installing development dependecies. (<code>[odigos-io#3550](urllib3/urllib3#3550) <https://github.com/urllib3/urllib3/issues/3550></code>__)</li> <li>Removed the <code>multiple.intoto.jsonl</code> asset from GitHub releases. Attestation of release files since v2.3.0 can be found on PyPI. (<code>[odigos-io#3566](urllib3/urllib3#3566) <https://github.com/urllib3/urllib3/issues/3566></code>__)</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/urllib3/urllib3/commit/a5ff7ac3bbb8659e2ec3ed41dd43889f06a7d7bc"><code>a5ff7ac</code></a> Release 2.4.0</li> <li><a href="https://github.com/urllib3/urllib3/commit/a135db29f72f828b0ef7314b856d19696a6f48ba"><code>a135db2</code></a> Upgrade memray and coverage to fix macOS tests (<a href="https://redirect.github.com/urllib3/urllib3/issues/3589">#3589</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/8f40e71612505a9985b0a58ad793cd84ec97614a"><code>8f40e71</code></a> Upgrade the publishing action to get correct licensing info on PyPI (<a href="https://redirect.github.com/urllib3/urllib3/issues/3585">#3585</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/3ff4e49ddf889554cf295b4a2e1189d066b60c71"><code>3ff4e49</code></a> Add a link to the 2024 annual report (<a href="https://redirect.github.com/urllib3/urllib3/issues/3586">#3586</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/75709c1dbd6770618f061fd0b8a6950c7741f17b"><code>75709c1</code></a> Set verify flags in <code>create_urllib3_context</code> (<a href="https://redirect.github.com/urllib3/urllib3/issues/3577">#3577</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/5c8f82a2f1c1b7a8360f0c84b5a88f25df070811"><code>5c8f82a</code></a> Bump astral-sh/setup-uv from 5.3.0 to 5.4.1 (<a href="https://redirect.github.com/urllib3/urllib3/issues/3580">#3580</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/42e90d894b30ef8b897708eb1cdfa24e83cf5067"><code>42e90d8</code></a> Bump actions/setup-python from 5.4.0 to 5.5.0 (<a href="https://redirect.github.com/urllib3/urllib3/issues/3579">#3579</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/3e8f2db735dcaced6a3b777aa1966f40c018af7c"><code>3e8f2db</code></a> Stop using Ubuntu 20.04 and 22.04 in CI (<a href="https://redirect.github.com/urllib3/urllib3/issues/3570">#3570</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/e29db82a6df5f9a9acbb6997899f93bda79bb61e"><code>e29db82</code></a> Update exceptions to have more of their attributes pickled (<a href="https://redirect.github.com/urllib3/urllib3/issues/3572">#3572</a>)</li> <li><a href="https://github.com/urllib3/urllib3/commit/f8a0c4360ad67aefd79317279ee90c72d5d18697"><code>f8a0c43</code></a> Add PyPy 3.11 to CI</li> <li>Additional commits viewable in <a href="https://github.com/urllib3/urllib3/compare/2.3.0...2.4.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=urllib3&package-manager=pip&previous-version=2.3.0&new-version=2.4.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ben Elferink <[email protected]>

handle data-collection config map creation in autoscaler reconciler, …

d40084d

…enhance config map provider in the collector

RonFed mentioned this pull request Sep 18, 2025

chore: fix re-upgrade without sources/destinations delete data collection cm #3517

Closed

13 tasks

RonFed added 2 commits September 18, 2025 18:23

fix ci compilation

d3aeaae

update ownerReference in asserts

497d6ef

RonFed requested review from blumamir, damemi and tamirdavid1 September 18, 2025 16:11

damemi approved these changes Sep 18, 2025

View reviewed changes

blumamir approved these changes Sep 18, 2025

View reviewed changes

RonFed added 2 commits September 19, 2025 14:08

code review adjustments

0b07ee6

refactor: nodecollector base reconciler as common struct

a75d408

blumamir reviewed Sep 19, 2025

View reviewed changes

RonFed added 3 commits September 19, 2025 16:16

remove unuesed feilds from nodeCollector controllers

55e1128

remove ownership of daemonset in nodeCollector controller since ds is…

ef30953

… now created by the installation in Helm/CLI

add autoscaler-deployment controller to react to one create event and…

66d8aef

… own the node collector config map

blumamir reviewed Sep 20, 2025

View reviewed changes

RonFed added 2 commits September 21, 2025 13:40

Merge branch 'main' into cm_provider_enhance

95c1023

Merge branch 'main' into cm_provider_enhance

7201de7

RonFed merged commit 8878f75 into odigos-io:main Sep 22, 2025
114 of 118 checks passed

RonFed deleted the cm_provider_enhance branch September 22, 2025 07:25

damemi reviewed Sep 23, 2025

View reviewed changes

natyharary mentioned this pull request Jan 6, 2026

CORE279: add StartupProbe to gateway #4028

Closed

Conversation

RonFed commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damemi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

damemi Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RonFed commented Sep 18, 2025 •

edited

Loading

damemi Sep 23, 2025 •

edited

Loading