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

alexperez52 · 2021-07-22T19:39:46Z

Description

Follows the changes in #351

This PR addresses issue open-telemetry/wg-prometheus#60. This is part 2 of a 3 part update in which enhancements are presented to the OTel Operator to support the deployment of a Target Allocator. This update contains the logic for the Operator Managed target allocator image which will be used to perform target sharding for the Prometheus scrape targets and expose them to the collector(s) for consumption using http sd config.

The Design document for this part 2 can be found here:
Target Allocator Design Document

Type of change

New feature

Testing

Unit tests were added to test the least connection algorithm flow as well as updating and removing targets.
More testing details can be found on the doc mentioned above.

cc: @Raul9595 @alolita

alexperez52 · 2021-07-22T21:10:07Z

@dashpole @Aneurysm9 @alolita

alolita · 2021-07-26T20:24:09Z

@jpkrohling @Aneurysm9 can you please review. Look forward to any feedback you may have. ty!

jpkrohling · 2021-07-27T12:49:09Z

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

Aneurysm9 · 2021-07-26T23:14:42Z

cmd/otel-allocator/allocation/allocator.go

@@ -0,0 +1,128 @@
+package allocation


None of these packages are used outside of this program, correct? Can we move them to cmd/otel-allocator/internal/*? That way we can be confident their scope is contained and not worry about breaking any external packages that have depended on them.

This can be done separately and doesn't need to hold up this PR.

Has this been addressed?

cmd/otel-allocator/allocation/allocator.go

cmd/otel-allocator/config/config.go

cmd/otel-allocator/discovery/discovery.go

cmd/otel-allocator/main.go

jpkrohling · 2021-07-29T14:15:07Z

Similar to the other PR: I left a couple of comments in the design doc. Let me know if I should go ahead and review this, or if you want to settle the open points first.

Pull request has been modified.

alolita · 2021-08-10T05:32:56Z

@alexperez52 @Raul9595 can you rerun the tests to make sure the E2E tests are passing.

cmd/otel-allocator/allocation/allocator.go

cmd/otel-allocator/main.go

dashpole

lgtm overall. Mostly suggestions to make the code easier to read.

cmd/otel-allocator/allocation/allocator.go

cmd/otel-allocator/allocation/http.go

cmd/otel-allocator/allocation/allocator.go

Pull request has been modified.

jpkrohling

Sorry that I couldn't complete the review, but will make another pass later today or tomorrow.

jpkrohling · 2021-08-12T08:34:32Z

cmd/otel-allocator/allocation/allocator.go

@@ -0,0 +1,128 @@
+package allocation


Has this been addressed?

cmd/otel-allocator/allocation/allocator.go

cmd/otel-allocator/allocation/allocator_test.go

cmd/otel-allocator/allocation/allocator.go

cmd/otel-allocator/collector/collector.go

cmd/otel-allocator/collector/collector_test.go

Pull request has been modified.

jpkrohling · 2021-08-19T14:55:55Z

I'll review this again soon, but please, try to get a PR fixing the intermittent tests that the part 1 introduced.

alexperez52 · 2021-08-19T16:02:30Z

I'll review this again soon, but please, try to get a PR fixing the intermittent tests that the part 1 introduced.

Seems like the PR merged today solved the issue with the e2e tests

cmd/otel-allocator/Dockerfile

cmd/otel-allocator/allocation/allocator_test.go

jpkrohling · 2021-08-20T11:39:59Z

cmd/otel-allocator/allocation/allocator_test.go

+}
+
+// Tests that the delta in number of targets per collector is less than 15% of an even distribution
+func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) {


What advantages is this bringing in comparison to the test TestAddingAndRemovingTargets? Is this exercising a different code path than TestCollectorBalanceWhenAddingTargets when it comes to the balancing itself?

TestAddingAndRemovingTargets tests that targets are successfully added with the expected with the expected key
TestCollectorBalanceWhenAddingTargets tests the the balance on an easily divisible number for the purpose of even allocation
TestCollectorBalanceWhenAddingAndRemovingAtRandom uses a more random approach where targets are both added and removed while verifying that at each step there isnt above a 15% difference in workload from collector to collector

Not sure I get the value-add between the second and the third, but assuming there's enough value justifying the added complexity, isn't it covering the third case covering the second? Meaning, do we need the second? And the first?

From a straight coverage perspective, I think the third test will cover what the first two do as well. But they may still have value in enabling testing and reasoning about each capability somewhat more independently.

I'd like to take another pass at this allocation logic in the future, but I think for now it is a solid starting point.

jpkrohling · 2021-08-20T11:40:53Z

cmd/otel-allocator/allocation/allocator_test.go

+	}
+}
+
+func TestCollectorBalanceWhenCollectorsChanged(t *testing.T) {


Same as the previous comment: what's the difference between this and the previous test?

The current implementation will do a re allocation from scratch when collector information is modified so this test is for the purposes of verifying that targets were re allocated from scratch. Using a divisible ratio of targets to collectors, its verified that the targets are allocated evenly.

Sorry, I should have been clearer, but do we still need both tests? Isn't one of them covering what the other is doing?

cmd/otel-allocator/allocation/allocator_test.go

cmd/otel-allocator/go.mod

cmd/otel-allocator/main.go

jpkrohling · 2021-08-23T12:44:52Z

Ping me when this is ready for review again.

cmd/otel-allocator/collector/collector.go

Co-authored-by: Anthony Mirabella <[email protected]>

…rator into otel-allocator

jpkrohling · 2021-09-08T09:37:48Z

I understand that @Aneurysm9 will be maintaining this code, so, I'll wait for his review before doing a final round myself.

alolita · 2021-09-08T18:30:08Z

Hi @jpkrohling thanks for taking a look.

@Aneurysm9 can you do a final review and let's get this merged. Ty!!!

alolita · 2021-09-14T05:35:22Z

Ping @Aneurysm9 please review so that @jpkrohling can merge. Thx.

Aneurysm9

There are some things I'd like to refactor, particularly in the allocation package and the main() function, but I think this is a solid starting point and provides a usable, and useful, service.

@jpkrohling if you'd like to see that refactoring done before merging this I'd understand, but I also think it could be good to land it as it is to make the capability available and see what happens when people attempt to put it to use.

Aneurysm9 · 2021-09-15T04:55:11Z

cmd/otel-allocator/allocation/allocator_test.go

+}
+
+// Tests that the delta in number of targets per collector is less than 15% of an even distribution
+func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) {


From a straight coverage perspective, I think the third test will cover what the first two do as well. But they may still have value in enabling testing and reasoning about each capability somewhat more independently.

I'd like to take another pass at this allocation logic in the future, but I think for now it is a solid starting point.

jpkrohling · 2021-09-15T07:02:00Z

The e2e tests for Kubernetes 1.19 failed and I just restarted. I'll merge this as soon as the tests pass.

…ic) (open-telemetry#354) * Target Allocation server logic Co-Authored-By: Alexis Perez <[email protected]> Co-Authored-By: JBD <[email protected]> * Added cmd to indicate executable * Update cmd/otel-allocator/allocation/allocator.go Co-authored-by: Anthony Mirabella <[email protected]> * Update cmd/otel-allocator/allocation/allocator.go Co-authored-by: Anthony Mirabella <[email protected]> * Updated discovery manager, collector component and added testing file for collector.go Updated code to parse config using default Prometheus config and added testing file for collector component. * Removed unnecessary struct in config.go * Added load testing * Update cmd/otel-allocator/allocation/allocator.go Co-authored-by: Anthony Mirabella <[email protected]> * Update cmd/otel-allocator/allocation/allocator.go Co-authored-by: Anthony Mirabella <[email protected]> * Update cmd/otel-allocator/allocation/allocator.go Co-authored-by: Anthony Mirabella <[email protected]> * Update cmd/otel-allocator/allocation/allocator.go * Update cmd/otel-allocator/allocation/allocator.go * Removed nextCollector and modified locks * Updated collector.go to reflect new namespace * Refactored display map logic & updated locking convention * Updated container port * Change initialized empty collector to nil collector Co-authored-by: Anthony Mirabella <[email protected]> * Updated collector test logic * Updated allocation files * Updated allocation import in main.go * Updated collector & discovery files * Updated unit tallocator unit tests * Updated runWatch to prevent panic * Seperated http logic from allocator logic * Integrated logr * Updated collector test to use channels * Update use of logger and fix error messages * Update test files Co-authored-by: Rahul Varma <[email protected]> Co-authored-by: JBD <[email protected]> Co-authored-by: Anthony Mirabella <[email protected]> Co-authored-by: Rahul Varma <[email protected]>

alexperez52 requested review from a team and dmitryax July 22, 2021 19:39

Aneurysm9 reviewed Jul 28, 2021

View reviewed changes

Aneurysm9 previously approved these changes Aug 3, 2021

View reviewed changes

jpkrohling self-requested a review August 3, 2021 14:18

This was referenced Aug 5, 2021

Feature - Implemented exporter factory and added unit tests open-telemetry/opentelemetry-php#351

Merged

REQUEST: New membership for @alexperez52 open-telemetry/community#800

Closed

dashpole reviewed Aug 10, 2021

View reviewed changes

alexperez52 requested a review from dashpole August 11, 2021 14:07

dashpole approved these changes Aug 11, 2021

View reviewed changes

cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved

cmd/otel-allocator/allocation/http.go Outdated Show resolved Hide resolved

cmd/otel-allocator/allocation/http.go Outdated Show resolved Hide resolved

dashpole reviewed Aug 12, 2021

View reviewed changes

cmd/otel-allocator/allocation/http.go Outdated Show resolved Hide resolved

dashpole approved these changes Aug 12, 2021

View reviewed changes

Aneurysm9 previously approved these changes Aug 12, 2021

View reviewed changes

cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved

alexperez52 requested a review from Aneurysm9 August 13, 2021 14:57

Aneurysm9 previously approved these changes Aug 16, 2021

View reviewed changes

jpkrohling reviewed Aug 18, 2021

View reviewed changes

jpkrohling reviewed Aug 20, 2021

View reviewed changes

jpkrohling reviewed Aug 24, 2021

View reviewed changes

cmd/otel-allocator/collector/collector.go Outdated Show resolved Hide resolved

Rahul Varma and others added 13 commits August 27, 2021 00:04

Updated container port

a572659

Change initialized empty collector to nil collector

cf51157

Co-authored-by: Anthony Mirabella <[email protected]>

Updated collector test logic

286f9e9

Updated allocation files

725515d

Updated allocation import in main.go

8f03e47

Updated collector & discovery files

f5a7220

Updated unit tallocator unit tests

d638147

Updated runWatch to prevent panic

9db3956

Seperated http logic from allocator logic

d51b39b

Integrated logr

6bf5eda

Updated collector test to use channels

6d73fa2

Update use of logger and fix error messages

902a212

Merge branch 'main' of https://github.com/open-o11y/opentelemetry-ope…

f450e17

…rator into otel-allocator

alexperez52 force-pushed the otel-allocator branch from dde7f07 to f450e17 Compare August 27, 2021 04:07

Update test files

284ff54

alexperez52 mentioned this pull request Aug 27, 2021

Minimize mutex lock time in target allocator when updating collectors #414

Closed

alexperez52 requested a review from jpkrohling September 2, 2021 15:26

jpkrohling requested a review from Aneurysm9 September 8, 2021 09:37

Aneurysm9 approved these changes Sep 15, 2021

View reviewed changes

jpkrohling enabled auto-merge (squash) September 15, 2021 07:02

jpkrohling approved these changes Sep 15, 2021

View reviewed changes

jpkrohling merged commit acab291 into open-telemetry:main Sep 15, 2021

swiatekm mentioned this pull request Oct 23, 2024

[Otel-Operator] Target Allocator is not running as nonroot #3378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexperez52 commented Jul 22, 2021 •

edited

Loading

alexperez52 commented Jul 22, 2021

alolita commented Jul 26, 2021

jpkrohling commented Jul 27, 2021

Aneurysm9 Jul 26, 2021

jpkrohling Aug 12, 2021

jpkrohling commented Jul 29, 2021

alolita commented Aug 10, 2021

dashpole left a comment

jpkrohling left a comment

jpkrohling Aug 12, 2021

jpkrohling commented Aug 19, 2021

alexperez52 commented Aug 19, 2021

jpkrohling Aug 20, 2021

alexperez52 Aug 23, 2021

jpkrohling Aug 23, 2021

Aneurysm9 Sep 15, 2021

jpkrohling Aug 20, 2021

alexperez52 Aug 23, 2021

jpkrohling Aug 23, 2021

jpkrohling commented Aug 23, 2021

jpkrohling commented Sep 8, 2021

alolita commented Sep 8, 2021

alolita commented Sep 14, 2021

Aneurysm9 left a comment

Aneurysm9 Sep 15, 2021

jpkrohling commented Sep 15, 2021

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

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

Conversation

alexperez52 commented Jul 22, 2021 • edited Loading

Description

Type of change

Testing

alexperez52 commented Jul 22, 2021

alolita commented Jul 26, 2021

jpkrohling commented Jul 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Jul 29, 2021

alolita commented Aug 10, 2021

dashpole left a 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 19, 2021

alexperez52 commented Aug 19, 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

jpkrohling commented Aug 23, 2021

jpkrohling commented Sep 8, 2021

alolita commented Sep 8, 2021

alolita commented Sep 14, 2021

Aneurysm9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Sep 15, 2021

alexperez52 commented Jul 22, 2021 •

edited

Loading