Target Allocator implementation (Part 1 - OTEL Operator Enhancements) #351

rsvarma95 · 2021-07-21T13:54:35Z

Description

This PR addresses issue open-telemetry/prometheus-interoperability-spec#60. This is Part 1 of a 3 part update in which enhancements are presented to the OTel Operator to support the deployment and reconciliation support for a target allocator resource. This update serves as an addition to StatefulSet support in which the target allocator resource can utilize multiple collector instances to split the Prometheus targets among them evenly.
Part 2 will contain the internal target allocator logic which will live as an image and used by the OTel operator.
Part 3 will have the logic to modify the ConfigMap to allow the collectors to find the scrape targets using http_sd_config.
The Design document for Part 1 can be found here: Target Allocator Design Document

Type of change

New feature & Enhancement

Testing

Implemented Unit tests and KUTTL end to end testing for assuring that deployment, reconciliation & target allocator internal logic are working as expected.
A more detailed implementation/testing document for Part 1 of the update is atatched below: Implementation Details

@alexperez52

alolita · 2021-07-21T15:11:04Z

Pl review @dashpole @Aneurysm9 ty!

alolita · 2021-07-26T20:21:32Z

@jpkrohling @Aneurysm9 - please review. Look forward to any comments you may have. ty!

jpkrohling · 2021-07-27T12:48:48Z

I'm planning on reviewing this by the end of this week.

jpkrohling · 2021-07-29T14:13:40Z

I left a few comments in the design doc. Let me know if you still want me to review this, or whether we should clarify the points there first.

rsvarma95 · 2021-07-29T14:55:29Z

Hi @jpkrohling, we have tried to address most of your concerns in the document. Please let us know if you feel more clarification is required.

alolita · 2021-07-29T23:55:22Z

Hi @jpkrohling thanks for your detailed review and comments on the design doc. The v3 design is what we have implemented currently. If you're good with the design, please review the code else we're happy to walk you through any grey areas / questions you may have.

jpkrohling · 2021-07-30T11:03:39Z

I'm mostly happy with the design, there's just one point to clarify or adapt, related to how changes are handled by the allocator. I think the configmap might not be necessary, neither a restart of the allocator process.

Aneurysm9 · 2021-08-02T23:49:09Z

internal/config/main.go

@@ -70,15 +74,22 @@ func New(opts ...Option) Config {
 		o.collectorImage = fmt.Sprintf("otel/opentelemetry-collector:%s", o.version.OpenTelemetryCollector)
 	}

+	if len(o.targetAllocatorImage) == 0 {
+		// will be replcaed with target allocator image once approved and available
+		o.targetAllocatorImage = "raul9595/otel-loadbalancer:0.0.1"


This needs to be updated to an appropriate image.

make sure to fetch to the default TA image version from versions.txt file similar to how we fetch the latest version of otel-collector image. This helps us to easily manage version upgrades of TA.

jpkrohling · 2021-08-03T10:22:26Z

config/rbac/role.yaml

+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - rolebindings


Why do you need to create role bindings?

The main reason to create a rolebinding is to allow the target allocator pod to view the collector pods in the same namespace. I am basically automating the below command. If I dont create a appropriate rolebinding, it will not be able to query for the collector pods. The logic of this part is in Part2.

kubectl create clusterrolebinding default-view --clusterrole=view --serviceaccount={namespace}:default

Reference link

The operator should definitely not be able to create cluster roles/bindings by default, as it would require the operator to have cluster-admin permissions. There should be a separate role/binding YAML file that users can opt in to use, giving the operator the extra permissions. At runtime, as part of the "auto detect" package, the operator can then detect if it has been given this extra permission and self-adjust, enabling/disabling this feature.

Ok this makes sense. I would remove that part then and add a check instead just to ensure the appropriate permissions are granted. This would remove the role/rolebinding files.

Instead of having it in the auto detect during which we do not have access to the request namespace while initializing the auto detect, would it be fine to create a helper method in targetallocator/reconcile which would use the k8s client to check the service account for appropriate permissions and return an error if permissions are not available?

Another option is when the rest.inClusterConfig() is called inside the logic (Part of PR2), we can just return an error.

Got it. The operator then should indeed provision the role/service account/binding. Are you sure it requires cluster roles, though?

Actually role and role bindings are enough but for the operator to create a role with the appropriate permissions, the operator needs to have these permissions thmeselves. Thus, the operator needs to have a cluster role defined since the collector namespaces are dynamic.

One solution can be just to let the user create the role and rolebindings as defined in the kuttl tests (latest update).

config/rbac/role.yaml

controllers/targetallocator_controller.go

pkg/targetallocator/reconcile/role.go

pkg/targetallocator/reconcile/role_test.go

pkg/targetallocator/reconcile/rolebinding.go

jpkrohling · 2021-08-03T12:00:29Z

tests/e2e/loadbalancer-features/00-assert.yaml

+      volumes:
+        - configMap:
+            items:
+            - key: targetallocator.yaml


Could you please add an assertion with the contents of the configmap?

One reason I have not included the configmap contents is because it creates some of the the content dynamically (from OTEL collector namespace). This is still not supported in KUTTL and I am not sure how to proceed with this.

And the contents of the configmap are being tested in the Go unit tests. Would it be sufficient to just check if it is created (as being checked in the latest update)?

jpkrohling · 2021-08-03T14:04:44Z

tests/e2e/smoke-loadbalancer/00-assert.yaml

@@ -0,0 +1,8 @@
+apiVersion: apps/v1
+kind: Deployment


Shouldn't it be a statefulset?

The OpenTelemetryCollector target allocation pod would be spawned when the CR mode is StatefulSet but the target allocation kind would be a Deployment. This KUTTL test is for testing the target allocation deployment alone.

Could you add extra assert files, outlining what's expected in terms of deployments/statefulsets/configmaps?

Have added more assert cases. Please let me know if these look fine.

jpkrohling · 2021-08-09T11:41:43Z

The e2e tests failed probably due to intermittent issues with the cert-manager, and have been restarted.

VineethReddy02

LGTM!

@Raul9595 before merging this PR make sure to update the default TA load balancer image. :)

VineethReddy02 · 2021-08-10T13:31:41Z

internal/config/main.go

@@ -70,15 +74,22 @@ func New(opts ...Option) Config {
 		o.collectorImage = fmt.Sprintf("otel/opentelemetry-collector:%s", o.version.OpenTelemetryCollector)
 	}

+	if len(o.targetAllocatorImage) == 0 {
+		// will be replcaed with target allocator image once approved and available
+		o.targetAllocatorImage = "raul9595/otel-loadbalancer:0.0.1"


make sure to fetch to the default TA image version from versions.txt file similar to how we fetch the latest version of otel-collector image. This helps us to easily manage version upgrades of TA.

pkg/collector/reconcile/configmap.go

pkg/collector/reconcile/service.go

pkg/collector/reconcile/suite_test.go

Pull request has been modified.

internal/config/main.go

api/v1alpha1/opentelemetrycollector_webhook.go

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Pull request has been modified.

jpkrohling

LGTM. There are a couple of minor changes that would be needed, but they can be addressed in a follow-up PR. If you prefer to do that, please create an issue tracking those changes and I'll merge this.

I'll also give a few hours for other folks to review (@VineethReddy02?), in case they are interested. If I don't hear any objections, I'll merge this today.

api/v1alpha1/opentelemetrycollector_types.go

pkg/collector/reconcile/configmap.go

jpkrohling · 2021-08-12T08:30:38Z

pkg/collector/reconcile/deployment_test.go

@@ -55,6 +116,54 @@ func TestExpectedDeployments(t *testing.T) {
 		assert.Equal(t, int32(2), *actual.Spec.Replicas)
 	})

+	t.Run("should not update target allocator deployment when the config is updated", func(t *testing.T) {


Could you please also change, say, the number of replicas? This is to demonstrate that you thought about the case where users are changing the deployment manually and you are OK with that.

Pull request has been modified.

jpkrohling · 2021-08-13T15:10:01Z

I'm really excited about this change! Thank you for your persistence and patience, @Raul9595!

This PR is set to auto-merge on green. I restarted the failing tests and will check it again later today.

…open-telemetry#351) Co-authored-by: Alexis Perez <perzmen@amazon.com> Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

alexperez52 and others added 13 commits July 8, 2021 15:56

load balancer deployment implementation & controller & e2e tests

130e69d

Load balancer crd update, reconcile logic and e2e tests

2835c16

Reset last commit and only uploading changes to main.go

6f81007

Updated bundle and api with new autogenerated resources

31f97fe

Updated code with lint fixes

47c5fe8

Updated code to include minor fixes

8ef4cde

Update helper.go to add header

c73470e

Updated comment and bundles

7482e13

Updated lb reconcile helper function and its invocations

729d4ea

Updated naming scheme and changed CR config

2dcf8a1

Updated bundle files

a0d3f34

Minor changes

492749e

Lint and default file fixes

6a72424

rsvarma95 requested review from a team and tigrannajaryan July 21, 2021 13:54

jpkrohling self-requested a review July 21, 2021 14:30

alexperez52 mentioned this pull request Jul 22, 2021

Target Allocator implementation (Part 2 - Target Allocator Image logic) #354

Merged

rsvarma95 and others added 3 commits July 30, 2021 00:30

Added rolebinding to automate manual setting

8657668

Update opentelemetry-operator.clusterserviceversion.yaml

0528856

Merge branch 'main' into load-balancer-implementation

1a921cb

Aneurysm9 reviewed Aug 3, 2021

View reviewed changes

jpkrohling reviewed Aug 3, 2021

View reviewed changes

rsvarma95 requested a review from jpkrohling August 9, 2021 11:27

jpkrohling previously approved these changes Aug 9, 2021

View reviewed changes

VineethReddy02 previously approved these changes Aug 10, 2021

View reviewed changes

dashpole reviewed Aug 10, 2021

View reviewed changes

pkg/collector/reconcile/configmap.go Outdated Show resolved Hide resolved

pkg/collector/reconcile/service.go Outdated Show resolved Hide resolved

pkg/collector/reconcile/service.go Outdated Show resolved Hide resolved

pkg/collector/reconcile/suite_test.go Outdated Show resolved Hide resolved

rsvarma95 mentioned this pull request Aug 10, 2021

Target Allocator implementation (Part 3 - Collector Enhancement for Prometheus Config Update) #389

Merged

Added version management & Minor fixes

a7764b9

rsvarma95 requested review from VineethReddy02, dashpole and jpkrohling August 11, 2021 03:31

jpkrohling reviewed Aug 11, 2021

View reviewed changes

internal/config/main.go Outdated Show resolved Hide resolved

Rahul Varma added 2 commits August 11, 2021 07:16

Updated docker image to quay.io image

33cb042

Added validation in webhook to validate prometheus config

3d37148

Aneurysm9 reviewed Aug 11, 2021

View reviewed changes

api/v1alpha1/opentelemetrycollector_webhook.go Outdated Show resolved Hide resolved

Update api/v1alpha1/opentelemetrycollector_webhook.go

a496825

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Aneurysm9 previously approved these changes Aug 11, 2021

View reviewed changes

dashpole approved these changes Aug 11, 2021

View reviewed changes

jpkrohling previously approved these changes Aug 12, 2021

View reviewed changes

Updated struct name & minor changes

98acb2f

rsvarma95 requested a review from jpkrohling August 12, 2021 12:45

jpkrohling approved these changes Aug 13, 2021

View reviewed changes

jpkrohling enabled auto-merge (squash) August 13, 2021 15:08

jpkrohling merged commit f7b7b93 into open-telemetry:main Aug 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target Allocator implementation (Part 1 - OTEL Operator Enhancements) #351

Target Allocator implementation (Part 1 - OTEL Operator Enhancements) #351

rsvarma95 commented Jul 21, 2021 •

edited

Loading

alolita commented Jul 21, 2021

alolita commented Jul 26, 2021

jpkrohling commented Jul 27, 2021

jpkrohling commented Jul 29, 2021

rsvarma95 commented Jul 29, 2021

alolita commented Jul 29, 2021

jpkrohling commented Jul 30, 2021

Aneurysm9 Aug 2, 2021

VineethReddy02 Aug 10, 2021

jpkrohling Aug 3, 2021

rsvarma95 Aug 3, 2021

jpkrohling Aug 3, 2021

rsvarma95 Aug 3, 2021

rsvarma95 Aug 3, 2021

jpkrohling Aug 4, 2021

rsvarma95 Aug 4, 2021

jpkrohling Aug 3, 2021

rsvarma95 Aug 3, 2021

rsvarma95 Aug 3, 2021 •

edited

Loading

jpkrohling Aug 3, 2021

rsvarma95 Aug 3, 2021

jpkrohling Aug 3, 2021

rsvarma95 Aug 3, 2021

jpkrohling commented Aug 9, 2021

VineethReddy02 left a comment

VineethReddy02 Aug 10, 2021

jpkrohling left a comment

jpkrohling Aug 12, 2021

jpkrohling commented Aug 13, 2021

Target Allocator implementation (Part 1 - OTEL Operator Enhancements) #351

Target Allocator implementation (Part 1 - OTEL Operator Enhancements) #351

Conversation

rsvarma95 commented Jul 21, 2021 • edited Loading

Description

Type of change

Testing

alolita commented Jul 21, 2021

alolita commented Jul 26, 2021

jpkrohling commented Jul 27, 2021

jpkrohling commented Jul 29, 2021

rsvarma95 commented Jul 29, 2021

alolita commented Jul 29, 2021

jpkrohling commented Jul 30, 2021

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

rsvarma95 Aug 3, 2021 • edited Loading

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

jpkrohling commented Aug 9, 2021

VineethReddy02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Aug 13, 2021

rsvarma95 commented Jul 21, 2021 •

edited

Loading

rsvarma95 Aug 3, 2021 •

edited

Loading