From 8358ab168aaecd813d7e41dc05ec553c557b3ff4 Mon Sep 17 00:00:00 2001 From: Ugur Turkarslan <52721344+utr1903@users.noreply.github.com> Date: Fri, 22 Dec 2023 16:13:56 +0100 Subject: [PATCH 1/4] Implement test for adding new collector after completed initial allocation (#1) * Rename target creator helper function * Add test for adding new collector --- .../allocation/allocatortest.go | 15 ++++- .../allocation/consistent_hashing_test.go | 8 +-- .../allocation/least_weighted_test.go | 63 +++++++++++++++++-- .../allocation/strategy_test.go | 4 +- cmd/otel-allocator/server/bench_test.go | 2 +- 5 files changed, 78 insertions(+), 14 deletions(-) diff --git a/cmd/otel-allocator/allocation/allocatortest.go b/cmd/otel-allocator/allocation/allocatortest.go index 88312a80a5..f50c1aaacd 100644 --- a/cmd/otel-allocator/allocation/allocatortest.go +++ b/cmd/otel-allocator/allocation/allocatortest.go @@ -23,6 +23,19 @@ import ( "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/target" ) +func MakeNNewTargetsWithoutPreAssigningCollectors(n int, startingIndex int) map[string]*target.Item { + toReturn := map[string]*target.Item{} + for i := startingIndex; i < n+startingIndex; i++ { + label := model.LabelSet{ + "i": model.LabelValue(strconv.Itoa(i)), + "total": model.LabelValue(strconv.Itoa(n + startingIndex)), + } + newTarget := target.NewItem(fmt.Sprintf("test-job-%d", i), fmt.Sprintf("test-url-%d", i), label, "") + toReturn[newTarget.Hash()] = newTarget + } + return toReturn +} + func colIndex(index, numCols int) int { if numCols == 0 { return -1 @@ -30,7 +43,7 @@ func colIndex(index, numCols int) int { return index % numCols } -func MakeNNewTargets(n int, numCollectors int, startingIndex int) map[string]*target.Item { +func MakeNNewTargetsWithPreAssigningCollectors(n int, numCollectors int, startingIndex int) map[string]*target.Item { toReturn := map[string]*target.Item{} for i := startingIndex; i < n+startingIndex; i++ { collector := fmt.Sprintf("collector-%d", colIndex(i, numCollectors)) diff --git a/cmd/otel-allocator/allocation/consistent_hashing_test.go b/cmd/otel-allocator/allocation/consistent_hashing_test.go index bbd4295202..3f9c4a4040 100644 --- a/cmd/otel-allocator/allocation/consistent_hashing_test.go +++ b/cmd/otel-allocator/allocation/consistent_hashing_test.go @@ -24,7 +24,7 @@ func TestCanSetSingleTarget(t *testing.T) { cols := MakeNCollectors(3, 0) c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargets(1, 3, 0)) + c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(1, 3, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, 1) for _, item := range actualTargetItems { @@ -40,7 +40,7 @@ func TestRelativelyEvenDistribution(t *testing.T) { expectedDelta := (expectedPerCollector * 1.5) - expectedPerCollector c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargets(numItems, 0, 0)) + c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(numItems, 0, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, numItems) actualCollectors := c.Collectors() @@ -54,7 +54,7 @@ func TestFullReallocation(t *testing.T) { cols := MakeNCollectors(10, 0) c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargets(10000, 10, 0)) + c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(10000, 10, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, 10000) actualCollectors := c.Collectors() @@ -79,7 +79,7 @@ func TestNumRemapped(t *testing.T) { cols := MakeNCollectors(numInitialCols, 0) c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargets(numItems, numInitialCols, 0)) + c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(numItems, numInitialCols, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, numItems) actualCollectors := c.Collectors() diff --git a/cmd/otel-allocator/allocation/least_weighted_test.go b/cmd/otel-allocator/allocation/least_weighted_test.go index 417c0e5ed3..8acde5c9e7 100644 --- a/cmd/otel-allocator/allocation/least_weighted_test.go +++ b/cmd/otel-allocator/allocation/least_weighted_test.go @@ -15,6 +15,7 @@ package allocation import ( + "fmt" "math" "math/rand" "testing" @@ -50,7 +51,7 @@ func TestAddingAndRemovingTargets(t *testing.T) { cols := MakeNCollectors(3, 0) s.SetCollectors(cols) - initTargets := MakeNNewTargets(6, 3, 0) + initTargets := MakeNNewTargetsWithPreAssigningCollectors(6, 3, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -60,7 +61,7 @@ func TestAddingAndRemovingTargets(t *testing.T) { assert.Len(t, s.TargetItems(), expectedTargetLen) // prepare second round of targets - tar := MakeNNewTargets(4, 3, 0) + tar := MakeNNewTargetsWithPreAssigningCollectors(4, 3, 0) // test that fewer targets are found - removed s.SetTargets(tar) @@ -125,7 +126,7 @@ func TestNoCollectorReassignment(t *testing.T) { for _, i := range cols { assert.NotNil(t, s.Collectors()[i.Name]) } - initTargets := MakeNNewTargets(6, 3, 0) + initTargets := MakeNNewTargetsWithPreAssigningCollectors(6, 3, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -144,6 +145,56 @@ func TestNoCollectorReassignment(t *testing.T) { } +// Tests that the newly added collector instance does not get assigned any target when the targets remain the same +func TestNoAssignmentToNewCollector(t *testing.T) { + s, _ := New("least-weighted", logger) + + // instantiate only 1 collector + cols := MakeNCollectors(1, 0) + s.SetCollectors(cols) + + expectedColLen := len(cols) + assert.Len(t, s.Collectors(), expectedColLen) + + for _, i := range cols { + assert.NotNil(t, s.Collectors()[i.Name]) + } + + initialColsBeforeAddingNewCol := s.Collectors() + initTargets := MakeNNewTargetsWithoutPreAssigningCollectors(6, 0) + + // test that targets and collectors are added properly + s.SetTargets(initTargets) + + // verify + expectedTargetLen := len(initTargets) + targetItems := s.TargetItems() + assert.Len(t, targetItems, expectedTargetLen) + + // add another collector + newColName := fmt.Sprintf("collector-%d", len(cols)) + cols[newColName] = &Collector{ + Name: newColName, + NumTargets: 0, + } + s.SetCollectors(cols) + + // targets shall not change + newTargetItems := s.TargetItems() + assert.Equal(t, targetItems, newTargetItems) + + // initial collectors still should have the same targets + for colName, col := range s.Collectors() { + if colName != newColName { + assert.Equal(t, initialColsBeforeAddingNewCol[colName], col) + } + } + + // new collector should have no targets + newCollector := s.Collectors()[newColName] + assert.Equal(t, newCollector.NumTargets, 0) +} + func TestSmartCollectorReassignment(t *testing.T) { t.Skip("This test is flaky and fails frequently, see issue 1291") s, _ := New("least-weighted", logger) @@ -157,7 +208,7 @@ func TestSmartCollectorReassignment(t *testing.T) { for _, i := range cols { assert.NotNil(t, s.Collectors()[i.Name]) } - initTargets := MakeNNewTargets(6, 0, 0) + initTargets := MakeNNewTargetsWithPreAssigningCollectors(6, 0, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -202,7 +253,7 @@ func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) { cols := MakeNCollectors(3, 0) s.SetCollectors(cols) - targets := MakeNNewTargets(27, 3, 0) + targets := MakeNNewTargetsWithPreAssigningCollectors(27, 3, 0) s.SetTargets(targets) // Divisor needed to get 15% @@ -241,7 +292,7 @@ func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) { assert.InDelta(t, i.NumTargets, count, math.Round(percent)) } // adding targets at 'random' - for _, item := range MakeNNewTargets(13, 3, 100) { + for _, item := range MakeNNewTargetsWithPreAssigningCollectors(13, 3, 100) { targets[item.Hash()] = item } s.SetTargets(targets) diff --git a/cmd/otel-allocator/allocation/strategy_test.go b/cmd/otel-allocator/allocation/strategy_test.go index c12529d8d8..4ccd2cb099 100644 --- a/cmd/otel-allocator/allocation/strategy_test.go +++ b/cmd/otel-allocator/allocation/strategy_test.go @@ -44,7 +44,7 @@ func BenchmarkGetAllTargetsByCollectorAndJob(b *testing.B) { b.Fail() } cols := MakeNCollectors(v.numCollectors, 0) - jobs := MakeNNewTargets(v.numJobs, v.numCollectors, 0) + jobs := MakeNNewTargetsWithPreAssigningCollectors(v.numJobs, v.numCollectors, 0) a.SetCollectors(cols) a.SetTargets(jobs) b.Run(fmt.Sprintf("%s_num_cols_%d_num_jobs_%d", s, v.numCollectors, v.numJobs), func(b *testing.B) { @@ -76,7 +76,7 @@ func Benchmark_Setting(b *testing.B) { for _, v := range table { a, _ := New(s, logger) cols := MakeNCollectors(v.numCollectors, 0) - targets := MakeNNewTargets(v.numTargets, v.numCollectors, 0) + targets := MakeNNewTargetsWithPreAssigningCollectors(v.numTargets, v.numCollectors, 0) b.Run(fmt.Sprintf("%s_num_cols_%d_num_jobs_%d", s, v.numCollectors, v.numTargets), func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { diff --git a/cmd/otel-allocator/server/bench_test.go b/cmd/otel-allocator/server/bench_test.go index 8fcea90b0e..a9bb7a5528 100644 --- a/cmd/otel-allocator/server/bench_test.go +++ b/cmd/otel-allocator/server/bench_test.go @@ -50,7 +50,7 @@ func BenchmarkServerTargetsHandler(b *testing.B) { for _, v := range table { a, _ := allocation.New(allocatorName, logger) cols := allocation.MakeNCollectors(v.numCollectors, 0) - targets := allocation.MakeNNewTargets(v.numJobs, v.numCollectors, 0) + targets := allocation.MakeNNewTargetsWithPreAssigningCollectors(v.numJobs, v.numCollectors, 0) listenAddr := ":8080" a.SetCollectors(cols) a.SetTargets(targets) From 93aa71373d963b1ecfb0a0a89c4e9b0e3cfc9190 Mon Sep 17 00:00:00 2001 From: uturkarslan Date: Sat, 23 Dec 2023 10:20:41 +0100 Subject: [PATCH 2/4] Add dot to test comment to fix lint --- cmd/otel-allocator/allocation/least_weighted_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/otel-allocator/allocation/least_weighted_test.go b/cmd/otel-allocator/allocation/least_weighted_test.go index 8acde5c9e7..79221b98df 100644 --- a/cmd/otel-allocator/allocation/least_weighted_test.go +++ b/cmd/otel-allocator/allocation/least_weighted_test.go @@ -145,7 +145,7 @@ func TestNoCollectorReassignment(t *testing.T) { } -// Tests that the newly added collector instance does not get assigned any target when the targets remain the same +// Tests that the newly added collector instance does not get assigned any target when the targets remain the same. func TestNoAssignmentToNewCollector(t *testing.T) { s, _ := New("least-weighted", logger) From fa5c0c54d10e6d07ad2c8a624992e90d2a4eb86d Mon Sep 17 00:00:00 2001 From: uturkarslan Date: Tue, 2 Jan 2024 17:00:19 +0100 Subject: [PATCH 3/4] Revert target creator helper function --- cmd/otel-allocator/allocation/allocatortest.go | 15 +-------------- .../allocation/consistent_hashing_test.go | 8 ++++---- .../allocation/least_weighted_test.go | 14 +++++++------- cmd/otel-allocator/allocation/strategy_test.go | 4 ++-- cmd/otel-allocator/server/bench_test.go | 2 +- 5 files changed, 15 insertions(+), 28 deletions(-) diff --git a/cmd/otel-allocator/allocation/allocatortest.go b/cmd/otel-allocator/allocation/allocatortest.go index f50c1aaacd..88312a80a5 100644 --- a/cmd/otel-allocator/allocation/allocatortest.go +++ b/cmd/otel-allocator/allocation/allocatortest.go @@ -23,19 +23,6 @@ import ( "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/target" ) -func MakeNNewTargetsWithoutPreAssigningCollectors(n int, startingIndex int) map[string]*target.Item { - toReturn := map[string]*target.Item{} - for i := startingIndex; i < n+startingIndex; i++ { - label := model.LabelSet{ - "i": model.LabelValue(strconv.Itoa(i)), - "total": model.LabelValue(strconv.Itoa(n + startingIndex)), - } - newTarget := target.NewItem(fmt.Sprintf("test-job-%d", i), fmt.Sprintf("test-url-%d", i), label, "") - toReturn[newTarget.Hash()] = newTarget - } - return toReturn -} - func colIndex(index, numCols int) int { if numCols == 0 { return -1 @@ -43,7 +30,7 @@ func colIndex(index, numCols int) int { return index % numCols } -func MakeNNewTargetsWithPreAssigningCollectors(n int, numCollectors int, startingIndex int) map[string]*target.Item { +func MakeNNewTargets(n int, numCollectors int, startingIndex int) map[string]*target.Item { toReturn := map[string]*target.Item{} for i := startingIndex; i < n+startingIndex; i++ { collector := fmt.Sprintf("collector-%d", colIndex(i, numCollectors)) diff --git a/cmd/otel-allocator/allocation/consistent_hashing_test.go b/cmd/otel-allocator/allocation/consistent_hashing_test.go index 3f9c4a4040..bbd4295202 100644 --- a/cmd/otel-allocator/allocation/consistent_hashing_test.go +++ b/cmd/otel-allocator/allocation/consistent_hashing_test.go @@ -24,7 +24,7 @@ func TestCanSetSingleTarget(t *testing.T) { cols := MakeNCollectors(3, 0) c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(1, 3, 0)) + c.SetTargets(MakeNNewTargets(1, 3, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, 1) for _, item := range actualTargetItems { @@ -40,7 +40,7 @@ func TestRelativelyEvenDistribution(t *testing.T) { expectedDelta := (expectedPerCollector * 1.5) - expectedPerCollector c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(numItems, 0, 0)) + c.SetTargets(MakeNNewTargets(numItems, 0, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, numItems) actualCollectors := c.Collectors() @@ -54,7 +54,7 @@ func TestFullReallocation(t *testing.T) { cols := MakeNCollectors(10, 0) c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(10000, 10, 0)) + c.SetTargets(MakeNNewTargets(10000, 10, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, 10000) actualCollectors := c.Collectors() @@ -79,7 +79,7 @@ func TestNumRemapped(t *testing.T) { cols := MakeNCollectors(numInitialCols, 0) c := newConsistentHashingAllocator(logger) c.SetCollectors(cols) - c.SetTargets(MakeNNewTargetsWithPreAssigningCollectors(numItems, numInitialCols, 0)) + c.SetTargets(MakeNNewTargets(numItems, numInitialCols, 0)) actualTargetItems := c.TargetItems() assert.Len(t, actualTargetItems, numItems) actualCollectors := c.Collectors() diff --git a/cmd/otel-allocator/allocation/least_weighted_test.go b/cmd/otel-allocator/allocation/least_weighted_test.go index 79221b98df..c8c379e4a2 100644 --- a/cmd/otel-allocator/allocation/least_weighted_test.go +++ b/cmd/otel-allocator/allocation/least_weighted_test.go @@ -51,7 +51,7 @@ func TestAddingAndRemovingTargets(t *testing.T) { cols := MakeNCollectors(3, 0) s.SetCollectors(cols) - initTargets := MakeNNewTargetsWithPreAssigningCollectors(6, 3, 0) + initTargets := MakeNNewTargets(6, 3, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -61,7 +61,7 @@ func TestAddingAndRemovingTargets(t *testing.T) { assert.Len(t, s.TargetItems(), expectedTargetLen) // prepare second round of targets - tar := MakeNNewTargetsWithPreAssigningCollectors(4, 3, 0) + tar := MakeNNewTargets(4, 3, 0) // test that fewer targets are found - removed s.SetTargets(tar) @@ -126,7 +126,7 @@ func TestNoCollectorReassignment(t *testing.T) { for _, i := range cols { assert.NotNil(t, s.Collectors()[i.Name]) } - initTargets := MakeNNewTargetsWithPreAssigningCollectors(6, 3, 0) + initTargets := MakeNNewTargets(6, 3, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -161,7 +161,7 @@ func TestNoAssignmentToNewCollector(t *testing.T) { } initialColsBeforeAddingNewCol := s.Collectors() - initTargets := MakeNNewTargetsWithoutPreAssigningCollectors(6, 0) + initTargets := MakeNNewTargets(6, 0, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -208,7 +208,7 @@ func TestSmartCollectorReassignment(t *testing.T) { for _, i := range cols { assert.NotNil(t, s.Collectors()[i.Name]) } - initTargets := MakeNNewTargetsWithPreAssigningCollectors(6, 0, 0) + initTargets := MakeNNewTargets(6, 0, 0) // test that targets and collectors are added properly s.SetTargets(initTargets) @@ -253,7 +253,7 @@ func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) { cols := MakeNCollectors(3, 0) s.SetCollectors(cols) - targets := MakeNNewTargetsWithPreAssigningCollectors(27, 3, 0) + targets := MakeNNewTargets(27, 3, 0) s.SetTargets(targets) // Divisor needed to get 15% @@ -292,7 +292,7 @@ func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) { assert.InDelta(t, i.NumTargets, count, math.Round(percent)) } // adding targets at 'random' - for _, item := range MakeNNewTargetsWithPreAssigningCollectors(13, 3, 100) { + for _, item := range MakeNNewTargets(13, 3, 100) { targets[item.Hash()] = item } s.SetTargets(targets) diff --git a/cmd/otel-allocator/allocation/strategy_test.go b/cmd/otel-allocator/allocation/strategy_test.go index 4ccd2cb099..c12529d8d8 100644 --- a/cmd/otel-allocator/allocation/strategy_test.go +++ b/cmd/otel-allocator/allocation/strategy_test.go @@ -44,7 +44,7 @@ func BenchmarkGetAllTargetsByCollectorAndJob(b *testing.B) { b.Fail() } cols := MakeNCollectors(v.numCollectors, 0) - jobs := MakeNNewTargetsWithPreAssigningCollectors(v.numJobs, v.numCollectors, 0) + jobs := MakeNNewTargets(v.numJobs, v.numCollectors, 0) a.SetCollectors(cols) a.SetTargets(jobs) b.Run(fmt.Sprintf("%s_num_cols_%d_num_jobs_%d", s, v.numCollectors, v.numJobs), func(b *testing.B) { @@ -76,7 +76,7 @@ func Benchmark_Setting(b *testing.B) { for _, v := range table { a, _ := New(s, logger) cols := MakeNCollectors(v.numCollectors, 0) - targets := MakeNNewTargetsWithPreAssigningCollectors(v.numTargets, v.numCollectors, 0) + targets := MakeNNewTargets(v.numTargets, v.numCollectors, 0) b.Run(fmt.Sprintf("%s_num_cols_%d_num_jobs_%d", s, v.numCollectors, v.numTargets), func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { diff --git a/cmd/otel-allocator/server/bench_test.go b/cmd/otel-allocator/server/bench_test.go index a9bb7a5528..8fcea90b0e 100644 --- a/cmd/otel-allocator/server/bench_test.go +++ b/cmd/otel-allocator/server/bench_test.go @@ -50,7 +50,7 @@ func BenchmarkServerTargetsHandler(b *testing.B) { for _, v := range table { a, _ := allocation.New(allocatorName, logger) cols := allocation.MakeNCollectors(v.numCollectors, 0) - targets := allocation.MakeNNewTargetsWithPreAssigningCollectors(v.numJobs, v.numCollectors, 0) + targets := allocation.MakeNNewTargets(v.numJobs, v.numCollectors, 0) listenAddr := ":8080" a.SetCollectors(cols) a.SetTargets(targets) From 4d3208b518a67fc01e416cc4d5f7a05b2b3b7e25 Mon Sep 17 00:00:00 2001 From: uturkarslan Date: Tue, 2 Jan 2024 17:00:55 +0100 Subject: [PATCH 4/4] Remove unused collector label to avoid confusion --- cmd/otel-allocator/allocation/allocatortest.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/otel-allocator/allocation/allocatortest.go b/cmd/otel-allocator/allocation/allocatortest.go index 88312a80a5..74dc8a44e1 100644 --- a/cmd/otel-allocator/allocation/allocatortest.go +++ b/cmd/otel-allocator/allocation/allocatortest.go @@ -35,9 +35,8 @@ func MakeNNewTargets(n int, numCollectors int, startingIndex int) map[string]*ta for i := startingIndex; i < n+startingIndex; i++ { collector := fmt.Sprintf("collector-%d", colIndex(i, numCollectors)) label := model.LabelSet{ - "collector": model.LabelValue(collector), - "i": model.LabelValue(strconv.Itoa(i)), - "total": model.LabelValue(strconv.Itoa(n + startingIndex)), + "i": model.LabelValue(strconv.Itoa(i)), + "total": model.LabelValue(strconv.Itoa(n + startingIndex)), } newTarget := target.NewItem(fmt.Sprintf("test-job-%d", i), fmt.Sprintf("test-url-%d", i), label, collector) toReturn[newTarget.Hash()] = newTarget