Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix target allocator readiness check #2883

Merged

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Apr 20, 2024

Description:
The target allocator readiness check is broken in two ways:

  • The target allocator doesn't do an initial load of targets from Prometheus CRs. So if there aren't any CRs, no targets are generated.
  • If there are no scrape configs, but there is an empty scrape configs array, targets are loaded from these, and the target allocator becomes ready even though it shouldn't be.

This PR fixes both of these issues by:

  • Loading targets from Prometheus CRs at startup.
  • Not loading targets from scrape configs if there aren't any.
  • For good measure, the target allocator config now doesn't even have the prometheus configuration section if there aren't any scrape targets.

Link to tracking Issue(s): #2903

Testing:
I changed an integration test with only Prometheus CR enabled to run without any actual CRs created initially.

@swiatekm swiatekm changed the title Fix/load initial servicemonitors Fix target allocator readiness check Apr 20, 2024
@swiatekm swiatekm force-pushed the fix/load-initial-servicemonitors branch 2 times, most recently from 2b2a8f2 to 45bb65d Compare April 27, 2024 17:27
@swiatekm swiatekm marked this pull request as ready for review April 27, 2024 17:27
@swiatekm swiatekm requested review from a team April 27, 2024 17:27
@swiatekm swiatekm force-pushed the fix/load-initial-servicemonitors branch from 45bb65d to 661d079 Compare April 29, 2024 15:46
setupLog.Error(err, "Can't load initial Prometheus configuration from Prometheus CRs")
os.Exit(1)
}
loadErr = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourcePrometheusCR, promConfig.ScrapeConfigs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done below in the rungroup which should be run once when it's first loaded. Do we need to also do it out here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. it's fine to do the load config here, but couldn't we just set it as cfg.PromConfig and let the rungroup handle the call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't in general, because cfg.PromConfig might not be empty. I could move it into the rungroup which established the Prometheus CR Watcher, if you think that'd be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calls look similar, but they actually refer to different event sources.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see that now, yes. I wonder if we should just send a message to the events channel to force an initial refresh rather than making new calls here?

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swiatekm-sumo and I discussed a different approach where we just send the event for the channel which for now is going to make the config loading fail silently to a user which seems not ideal.

@swiatekm swiatekm merged commit dc5dda9 into open-telemetry:main May 1, 2024
31 checks passed
@swiatekm swiatekm deleted the fix/load-initial-servicemonitors branch May 1, 2024 10:25
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Load initial Prometheus CR config at startup

* Fix target allocator readiness check
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Load initial Prometheus CR config at startup

* Fix target allocator readiness check
rubenvp8510 pushed a commit to rubenvp8510/opentelemetry-operator that referenced this pull request May 7, 2024
* Load initial Prometheus CR config at startup

* Fix target allocator readiness check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Target allocator never becomes ready if only Prometheus CRs are enabled, and there aren't any defined
3 participants