Merge branch 'jv-flaky-balancer-test' into 'master'

Try to resolve flaky TestRemoval balancer test

Closes #1510

See merge request gitlab-org/gitaly!1094

3 files changed, 21 insertions, 18 deletions

diff --git a/changelogs/unreleased/jv-flaky-balancer-test.yml b/changelogs/unreleased/jv-flaky-balancer-test.yml
new file mode 100644
index 000000000..001f50cc1
--- /dev/null
+++ b/changelogs/unreleased/jv-flaky-balancer-test.yml
@@ -0,0 +1,5 @@
+---
+title: Try to resolve flaky TestRemoval balancer test
+merge_request: 1094
+author:
+type: fixed
diff --git a/internal/rubyserver/balancer/balancer.go b/internal/rubyserver/balancer/balancer.go
index 4ba136244..e978c29e9 100644
--- a/internal/rubyserver/balancer/balancer.go
+++ b/internal/rubyserver/balancer/balancer.go
@@ -75,8 +75,9 @@ type builder struct {
 	addressUpdates chan addressUpdate
 	configUpdate   chan config
 
-	// for testing only
-	testingRestart chan struct{}
+	// testingTriggerRestart is for testing only. It causes b.monitor(...) to
+	// re-execute.
+	testingTriggerRestart chan struct{}
 }
 
 // ConfigureBuilder changes the configuration of the global balancer
@@ -100,11 +101,11 @@ func ConfigureBuilder(numAddrs int, removeDelay time.Duration) {
 
 func newBuilder() *builder {
 	b := &builder{
-		addAddress:     make(chan string),
-		removeAddress:  make(chan addressRemoval),
-		addressUpdates: make(chan addressUpdate),
-		configUpdate:   make(chan config),
-		testingRestart: make(chan struct{}),
+		addAddress:            make(chan string),
+		removeAddress:         make(chan addressRemoval),
+		addressUpdates:        make(chan addressUpdate),
+		configUpdate:          make(chan config),
+		testingTriggerRestart: make(chan struct{}),
 	}
 	go b.monitor()
 
@@ -129,7 +130,11 @@ func (b *builder) monitor() {
 	p := newPool()
 	notify := make(chan struct{})
 	cfg := <-b.configUpdate
-	lastRemoval := time.Now()
+
+	// At this point, there has been no previous removal command yet, so the
+	// "last removal" is undefined. We want it to default to "long enough
+	// ago".
+	lastRemoval := time.Now().Add(-1 * time.Hour)
 
 	// This channel is intentionally nil so that our 'select' below won't
 	// send messages to it. We do this to prevent sending out invalid (empty)
@@ -170,7 +175,7 @@ func (b *builder) monitor() {
 			notify = broadcast(notify)
 		case cfg = <-b.configUpdate:
 			// We have received a config update
-		case <-b.testingRestart:
+		case <-b.testingTriggerRestart:
 			go b.monitor()
 			b.configUpdate <- cfg
 			return
diff --git a/internal/rubyserver/balancer/balancer_test.go b/internal/rubyserver/balancer/balancer_test.go
index 8fdcb0a09..4038bc815 100644
--- a/internal/rubyserver/balancer/balancer_test.go
+++ b/internal/rubyserver/balancer/balancer_test.go
@@ -155,13 +155,7 @@ func TestRemovals(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			lbBuilder.testingRestart <- struct{}{}
-			bootSleep := 2 * removeDelay
-			if bootSleep == 0 {
-				bootSleep = 2 * time.Millisecond
-			}
-
-			time.Sleep(bootSleep) // wait for lastRemoval in monitor goroutine to be long enough ago
+			lbBuilder.testingTriggerRestart <- struct{}{}
 
 			for i, a := range tc.actions {
 				if a.add != "" {
@@ -227,8 +221,7 @@ func (tcc *testClientConn) ConfigUpdates() []string {
 func configureBuilderTest(numAddrs int) []string {
 	delay := 1 * time.Millisecond
 	ConfigureBuilder(numAddrs, delay)
-	lbBuilder.testingRestart <- struct{}{}
-	time.Sleep(2 * delay)
+	lbBuilder.testingTriggerRestart <- struct{}{}
 
 	var addrs []string
 	for i := 0; i < numAddrs; i++ {