[targetallocator] PrometheusOperator CRD MVC #653

secustor · 2021-12-28T21:50:58Z

Context: #383

Currently implemented:

fetch CRDs ServiceMonitor and Podmonitor
generate PromConfigs from monitor instances
update on creation, deletion and change of instances

TBD:

clean up server shutdown
add flag to make PrometheusCR watcher optional
add option to control watched namespaces
add option to apply a label selector for monitors

Needs discussion:

splitting of application configuration and promconfig in the tracked config map

cc @Aneurysm9

Aneurysm9 · 2021-12-28T23:29:26Z

😻

…sing kubernetes_sd outside of cluster

secustor · 2021-12-31T17:08:52Z

Splitting this here of as else the PR would get to big.

jpkrohling · 2022-01-04T08:21:03Z

@pavolloffay, @VineethReddy02, is one of you interested in reviewing this?

secustor · 2022-01-18T23:26:32Z

Any chance we get this reviewed by a maintainer?

I would like to propose that we use the operator/collector, but we heavely lean on PrometheusCRDs to configure our scrape targets.

VineethReddy02 · 2022-01-19T06:37:52Z

I took a quick pass through the PR. Looks good to me in the first pass. Can we add e2e tests for the same?

secustor · 2022-01-28T17:16:54Z

No, the Collectors would have to be redeployed when CRs would be changed.

That would be definitely the cleanest solution.

I agree, I'm going to create an issue if this PR is accepted.

jpkrohling · 2022-02-03T12:57:56Z

@pavolloffay, @VineethReddy02 you've made a couple of comments to this PR, do you think this is ready to be merged?

VineethReddy02

LGTM

pavolloffay · 2022-02-22T12:35:38Z

apis/v1alpha1/opentelemetrycollector_types.go

@@ -120,11 +120,21 @@ type OpenTelemetryTargetAllocator struct {
 	// +optional
 	Enabled bool `json:"enabled,omitempty"`

+	// PrometheusCR defines the configuration for the retrieval of PrometheusOperator CRDs ( servicemonitor.monitoring.coreos.com/v1 and podmonitor.monitoring.coreos.com/v1 )  retrieval.


question as before, from which namespaces are these objects queried?

I have added a line clearing this up.

pavolloffay · 2022-02-22T12:39:23Z

cmd/otel-allocator/README.md

+Watches the Prometheus service discovery for new targets and sets targets to the Allocator 
+
+### Allocator
+Shards the received targets based on the discovered Collector instances


based on the discovered

Is this correct? Strictly speaking the TA does not discover collectors. The collectors should be known from the collector CR.

It does watch the associated stateful set to observe scaling events and reallocate targets as necessary when the size of the set of collectors changes.

Currently the TargetAllocator watches the API for pods with the expected labels.
Because of this I have described it as discovered, would you prefer watch?

pavolloffay · 2022-02-22T12:40:36Z

cmd/otel-allocator/README.md

+### Allocator
+Shards the received targets based on the discovered Collector instances
+
+### Collector


Is this OTEL collector? Maybe we should find a different name if it is not.

The headlines are referring to the packages. The collector packages watches for new collector instances. I have rearranged it a bit to clear this up.

pavolloffay · 2022-02-22T12:45:41Z

cmd/otel-allocator/watcher/promOperator.go

+
+	"github.com/go-kit/log"
+	"github.com/go-logr/logr"
+	allocatorconfig "github.com/otel-allocator/config"


is this import from this repository? Then it should be moved to a 3rd group. Why doesn't it have the full path?

Yes, this is referencing the local module.

Why this module name has been chosen, I can not say.

I think this import path is incorrect and should be changed. Probably best to change it in a separate PR, though.

pavolloffay · 2022-02-22T12:47:41Z

cmd/otel-allocator/README.md

+
+# Design
+
+If the Allocator is activated, all Prometheus configurations will be transferred in a separate ConfigMap which get in


all Prometheus configurations

Is it OTEL p8s receiver configuration or as well p8s CRs?

This applies to the configuration of the prometheus receiver in the collector configuration. The Prometheus operator CRs are observed separately after the target allocator has been started, at which point it further modifies the service discovery configuration it is using.

This is only referencing the OTEL p8s configuration.

pavolloffay · 2022-02-22T12:49:13Z

cmd/otel-allocator/README.md

+turn mounted to the Allocator.    
+This configuration will be resolved to target configurations and then split across all OpenTelemetryCollector instances.
+
+TargetAllocators exposes the results as [HTTP_SD endpoints](https://prometheus.io/docs/prometheus/latest/http_sd/)


How does the OTEL colletor get the final config (resolved targets)?

Does the OTEL operator configure the collector to use the http_sd to get the targets?

Yes, the OTEL operator rewrites the collector prometheus receiver config to use http_sd for all configured jobs, pointing at the target allocator instance for that collector.

There is still a gap, that @secustor has proposed to close with open-telemetry/opentelemetry-collector-contrib#8055. This addresses the fact that the Prometheus operator CRs can cause new jobs to be exposed at the target allocator but the collector configuration is static and does not currently have a mechanism for discovering those jobs. The target allocator exposes a job list resource that should suffice to discover new/ended jobs.

Yes, that's correct. The operator configures the collector to use http_sd endpoints which are provided by the TargetAllocator.

Currently the Operator defines the p8s jobs which the collector should query, therefore no p8s operator CR defined jobs are added to the collector. Later on the the collector should query the jobs directly from the TargetAllocator.
See following issues:

operator: Use /job endpoint instead of generated promconfig #713

collector: [prometheusreceiver] allow to pull Prometheus jobs from /job endpoint of the OpenTelemetryOperator-TargetAllocator opentelemetry-collector-contrib#7944

pavolloffay · 2022-02-22T12:49:46Z

Sorry for super long delay in reviewing this. I had to take some time to lear about target allocator.

codeboten

@pavolloffay it appears your comments have been addressed, can you confirm whether there are still outstanding items to address?

jaronoff97

Love the approach here, one non-blocking question. I see you're adding the changes to the end-to-end tests, would it be possible to add a test involving a prometheus CRD to gut check that the expected configuration gets generated?

jaronoff97 · 2022-04-06T17:16:32Z

cmd/otel-allocator/config/config.go

+	PromCRWatcherConf  PrometheusCRWatcherConfig
+}
+
+func Load(file string) (Config, error) {
 	var cfg Config
 	if err := unmarshal(&cfg, file); err != nil {


question: It seems previously the allocator would load a default config file if no file was provided, it seems that was because we would always pass in an empty string in main.go. Should that functionality remain in case there any consumers of this function?

Are you referring to users of the TargetAllocator or Devs which import this package?

In the first case the behavior has not changed. It is still loading the same default file as before. This is implement using pflags now. https://github.com/open-telemetry/opentelemetry-operator/pull/653/files#diff-d292758014333553bcd82dfdf75743ef82b0b32f6414e2dc345777fc342e4480R77

secustor · 2022-04-06T22:24:15Z

see you're adding the changes to the end-to-end tests, would it be possible to add a test involving a prometheus CRD to gut check that the expected configuration gets generated?

I don't think it is possible right now to test API responses with KUTTL.

pavolloffay

I am approving this PR, however I am still not familiar with the target-allocator and it is not area where I focus.

I am fine merging this if there will be people maintaining it.

# Conflicts: # cmd/otel-allocator/Dockerfile # cmd/otel-allocator/go.sum

secustor · 2022-04-15T17:06:41Z

@pavolloffay Can you merge this? All checks are successful.

pavolloffay · 2022-04-22T07:31:41Z

Merging based on the support discussion in https://cloud-native.slack.com/archives/C033BJ8BASU/p1650372898413199.

@secustor @Aneurysm9 (AWS) are willing to support the target-allocator.

* feat(target-allocator): allow custom config file path * feat(target-allocator): move CLI config options to config package * feat(target-allocator): allow running outside of cluster for debugging * introduce meta watcher * add event source * fix: log panics * fix: race condition * fixup! fix: log panics * feat: implement promCR retrieval * feat: functioning * refactor: some cleanup * feat(target-allocator): escape job names in query parameters * feat(target-allocator): make prometheusCR lookup optional and allow using kubernetes_sd outside of cluster * refactor(target-allocator): improve memory usage and comments * chore(target-allocator): update PromOperator and Kubernetes deps * refactor(target-allocator): use exclude instead of replace directive * ci: add Makefile targets for target allocator * tests: add kuttl tests for PrometheusCR feature of target allocator * docs(targetAllocator): add README.md * fixup CRD docs * fix(Makefile): add missing PHONY tags * implement change requests * go mod tidy and fix linting

secustor added 11 commits December 28, 2021 00:49

feat(target-allocator): allow custom config file path

4a72ffb

feat(target-allocator): move CLI config options to config package

626d002

feat(target-allocator): allow running outside of cluster for debugging

b71d1f2

introduce meta watcher

727493b

add event source

f6a1996

fix: log panics

f12336d

fix: race condition

d5c90ea

fixup! fix: log panics

24275d0

feat: implement promCR retrieval

0da5fa8

feat: functioning

8a29f22

refactor: some cleanup

077c0c4

secustor changed the title ~~[targetallocator] Implement prometheus crd MVC~~ [targetallocator] PrometheusOperator CRD MVC Dec 28, 2021

secustor added 3 commits December 31, 2021 15:48

feat(target-allocator): escape job names in query parameters

78161df

feat(target-allocator): make prometheusCR lookup optional and allow u…

28fc0c9

…sing kubernetes_sd outside of cluster

refactor(target-allocator): improve memory usage and comments

bb21889

secustor marked this pull request as ready for review December 31, 2021 17:07

secustor requested review from a team and owais December 31, 2021 17:07

secustor added 2 commits January 2, 2022 13:57

chore(target-allocator): update PromOperator and Kubernetes deps

c18ac42

refactor(target-allocator): use exclude instead of replace directive

acc07c5

Aneurysm9 approved these changes Jan 3, 2022

View reviewed changes

secustor mentioned this pull request Jan 11, 2022

Will opentelemetry-operator support prometheus CRDs, like ServiceMonitor #383

Closed

secustor added 2 commits January 19, 2022 22:07

ci: add Makefile targets for target allocator

8b7d8bf

tests: add kuttl tests for PrometheusCR feature of target allocator

1b249ca

VineethReddy02 approved these changes Feb 5, 2022

View reviewed changes

secustor and others added 2 commits February 9, 2022 21:57

Merge branch 'main' into implement_prometheus_crd

e23a2c5

fix(Makefile): add missing PHONY tags

0a429a7

secustor mentioned this pull request Feb 11, 2022

[receiver/prometheusoperator] Add base structure open-telemetry/opentelemetry-collector-contrib#6344

Closed

secustor requested a review from pavolloffay February 15, 2022 08:44

secustor mentioned this pull request Feb 16, 2022

Use /job endpoint instead of generated promconfig #713

Closed

pavolloffay reviewed Feb 22, 2022

View reviewed changes

implement change requests

92a55d5

secustor requested a review from pavolloffay February 23, 2022 08:11

templarfelix approved these changes Mar 4, 2022

View reviewed changes

codeboten reviewed Mar 17, 2022

View reviewed changes

jaronoff97 approved these changes Apr 6, 2022

View reviewed changes

Merge branch 'main' into implement_prometheus_crd

96cea47

pavolloffay approved these changes Apr 7, 2022

View reviewed changes

secustor added 2 commits April 9, 2022 16:59

Merge branch 'main' into implement_prometheus_crd

45a0e84

# Conflicts: # cmd/otel-allocator/Dockerfile # cmd/otel-allocator/go.sum

go mod tidy and fix linting

7d3f036

pavolloffay merged commit a84a44c into open-telemetry:main Apr 22, 2022

secustor deleted the implement_prometheus_crd branch April 22, 2022 07:46

jaronoff97 mentioned this pull request Apr 22, 2022

Allow the target allocator to use a custom serviceaccount #834

Closed

secustor mentioned this pull request Jul 18, 2022

Implement prometheus receiver target allocator open-telemetry/opentelemetry-collector-contrib#12586

Merged

jaronoff97 mentioned this pull request Jul 26, 2022

REQUEST: New membership for @jaronoff97 open-telemetry/community#1120

Closed

6 tasks

secustor mentioned this pull request Aug 2, 2022

REQUEST: New membership for secustor open-telemetry/community#1123

Closed

6 tasks

benoittoulme mentioned this pull request Mar 6, 2023

Support ServiceMonitor CRD signalfx/splunk-otel-collector-chart#689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[targetallocator] PrometheusOperator CRD MVC #653

[targetallocator] PrometheusOperator CRD MVC #653

secustor commented Dec 28, 2021 •

edited

Loading

Aneurysm9 commented Dec 28, 2021

secustor commented Dec 31, 2021

jpkrohling commented Jan 4, 2022

secustor commented Jan 18, 2022

VineethReddy02 commented Jan 19, 2022

secustor commented Jan 28, 2022

jpkrohling commented Feb 3, 2022

VineethReddy02 left a comment

pavolloffay Feb 22, 2022

secustor Feb 22, 2022

pavolloffay Feb 22, 2022

Aneurysm9 Feb 22, 2022

secustor Feb 22, 2022

pavolloffay Feb 22, 2022

secustor Feb 22, 2022

pavolloffay Feb 22, 2022

secustor Feb 22, 2022

Aneurysm9 Feb 22, 2022

pavolloffay Feb 22, 2022

Aneurysm9 Feb 22, 2022

secustor Feb 22, 2022

pavolloffay Feb 22, 2022

Aneurysm9 Feb 22, 2022

secustor Feb 22, 2022

pavolloffay commented Feb 22, 2022

codeboten left a comment

jaronoff97 left a comment

jaronoff97 Apr 6, 2022

secustor Apr 6, 2022

secustor commented Apr 6, 2022 •

edited

Loading

pavolloffay left a comment

secustor commented Apr 15, 2022

pavolloffay commented Apr 22, 2022


		# Design

		If the Allocator is activated, all Prometheus configurations will be transferred in a separate ConfigMap which get in

[targetallocator] PrometheusOperator CRD MVC #653

[targetallocator] PrometheusOperator CRD MVC #653

Conversation

secustor commented Dec 28, 2021 • edited Loading

Aneurysm9 commented Dec 28, 2021

secustor commented Dec 31, 2021

jpkrohling commented Jan 4, 2022

secustor commented Jan 18, 2022

VineethReddy02 commented Jan 19, 2022

secustor commented Jan 28, 2022

jpkrohling commented Feb 3, 2022

VineethReddy02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Feb 22, 2022

codeboten left a comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

secustor commented Apr 6, 2022 • edited Loading

pavolloffay left a comment

Choose a reason for hiding this comment

secustor commented Apr 15, 2022

pavolloffay commented Apr 22, 2022

secustor commented Dec 28, 2021 •

edited

Loading

secustor commented Apr 6, 2022 •

edited

Loading