tests(debug server): fix race condition (openservicemesh#2017)

Currently, the tests that dynamically start/stop the debug HTTP server
rely on a `sync.WaitGroup` that signals when a mocked debug server has
been started or stopped. `TestConfigureDebugServerErr` was failing
occasionally because `configureDebugServer` wasn't guaranteed to finish
stopping the server before the test's `sync.WaitGroup` signalled it was
done.

This change removes the `sync.WaitGroup` from the tests and instead adds
a new channel parameter to `configureDebugServer` that will emit errors
(or nil if successful) whenever the server is started or stopped. Then,
the tests can be sure that `configureDebugServer` has finished before
checking any values.

cmd/osm-controller/osm-controller.go

@@ -267,27 +267,36 @@ func main() {
 		debugServer:        httpserver.NewDebugHTTPServer(debugConfig, constants.DebugPort),
 	}
 
-	go c.configureDebugServer(cfg)
-
-	// Wait for exit handler signal
-	<-stop
+	errs := make(chan error)
+	go c.configureDebugServer(cfg, errs)
+
+	done := false
+	for !done {
+		select {
+		case <-stop:
+			done = true
+		case err := <-errs:
+			if err != nil {
+				log.Error().Err(err)
+			}
+		}
+	}
 
 	log.Info().Msg("Goodbye!")
 }
 
-func (c *controller) configureDebugServer(cfg configurator.Configurator) {
+func (c *controller) configureDebugServer(cfg configurator.Configurator, errs chan<- error) {
 	//GetAnnouncementsChannel will check ConfigMap every 3 * time.Second
 	var mutex = &sync.Mutex{}
 	for range cfg.GetAnnouncementsChannel() {
 		if c.debugServerRunning && !cfg.IsDebugServerEnabled() {
 			mutex.Lock()
 			err := c.debugServer.Stop()
-			if err != nil {
-				log.Error().Err(err).Msg("Unable to stop debug server")
-			} else {
+			if err == nil {
 				c.debugServer = nil
 			}
 			c.debugServerRunning = false
+			errs <- errors.Wrap(err, "unable to stop debug server")
 			mutex.Unlock()
 		} else if !c.debugServerRunning && cfg.IsDebugServerEnabled() {
 			mutex.Lock()
@@ -296,6 +305,7 @@ func (c *controller) configureDebugServer(cfg configurator.Configurator) {
 			}
 			c.debugServer.Start()
 			c.debugServerRunning = true
+			errs <- nil
 			mutex.Unlock()
 		}
 	}

cmd/osm-controller/osm-controller_test.go

@@ -4,8 +4,8 @@ import (
 	"context"
 	"net/http"
 	"strconv"
-	"sync"
 	"testing"
+	"time"
 
 	"github.com/golang/mock/gomock"
 	"github.com/pkg/errors"
@@ -40,16 +40,16 @@ func TestConfigureDebugServerStart(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	var wg sync.WaitGroup
 
-	fakeDebugServer := FakeDebugServer{0, 0, nil, &wg}
+	fakeDebugServer := FakeDebugServer{0, 0, nil}
 	con := &controller{
 		debugServerRunning: false,
 		debugComponents:    mockDebugConfig(mockCtrl),
 		debugServer:        &fakeDebugServer,
 	}
-	wg.Add(1)
-	go con.configureDebugServer(cfg)
+
+	errs := make(chan error)
+	go con.configureDebugServer(cfg, errs)
 
 	updatedConfigMap := v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
@@ -64,8 +64,8 @@ func TestConfigureDebugServerStart(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	wg.Wait()
 
+	getErrorOrTimeout(assert, errs, 1*time.Second)
 	close(stop)
 	assert.Equal(1, fakeDebugServer.startCount)
 	assert.Equal(0, fakeDebugServer.stopCount)
@@ -84,17 +84,16 @@ func TestConfigureDebugServerStop(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	var wg sync.WaitGroup
 
-	fakeDebugServer := FakeDebugServer{0, 0, nil, &wg}
+	fakeDebugServer := FakeDebugServer{0, 0, nil}
 	con := &controller{
 		debugServerRunning: true,
 		debugComponents:    mockDebugConfig(mockCtrl),
 		debugServer:        &fakeDebugServer,
 	}
-	wg.Add(1)
 
-	go con.configureDebugServer(cfg)
+	errs := make(chan error)
+	go con.configureDebugServer(cfg, errs)
 
 	updatedConfigMap := v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
@@ -109,8 +108,8 @@ func TestConfigureDebugServerStop(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	wg.Wait()
 
+	getErrorOrTimeout(assert, errs, 1*time.Second)
 	close(stop)
 	assert.Equal(0, fakeDebugServer.startCount)
 	assert.Equal(1, fakeDebugServer.stopCount)
@@ -129,16 +128,15 @@ func TestConfigureDebugServerErr(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	var wg sync.WaitGroup
 
-	fakeDebugServer := FakeDebugServer{0, 0, errors.Errorf("Debug server error"), &wg}
+	fakeDebugServer := FakeDebugServer{0, 0, errors.Errorf("Debug server error")}
 	con := &controller{
 		debugServerRunning: true,
 		debugComponents:    mockDebugConfig(mockCtrl),
 		debugServer:        &fakeDebugServer,
 	}
-	wg.Add(1)
-	go con.configureDebugServer(cfg)
+	errs := make(chan error)
+	go con.configureDebugServer(cfg, errs)
 
 	updatedConfigMap := v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
@@ -153,8 +151,8 @@ func TestConfigureDebugServerErr(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	wg.Wait()
 
+	getErrorOrTimeout(assert, errs, 1*time.Second)
 	close(stop)
 	assert.Equal(0, fakeDebugServer.startCount)
 	assert.Equal(1, fakeDebugServer.stopCount)
@@ -212,13 +210,10 @@ type FakeDebugServer struct {
 	stopCount  int
 	startCount int
 	stopErr    error
-
-	wg *sync.WaitGroup
 }
 
 func (f *FakeDebugServer) Stop() error {
 	f.stopCount++
-	f.wg.Done()
 	if f.stopErr != nil {
 		return errors.Errorf("Debug server error")
 	}
@@ -227,7 +222,6 @@ func (f *FakeDebugServer) Stop() error {
 
 func (f *FakeDebugServer) Start() {
 	f.startCount++
-	f.wg.Done()
 }
 
 func mockDebugConfig(mockCtrl *gomock.Controller) *debugger.MockDebugServer {
@@ -258,3 +252,11 @@ func setupComponents(namespace, configMapName string, initialDebugServerEnabled
 	cfg := configurator.NewConfigurator(kubeClient, stop, namespace, configMapName)
 	return kubeClient, configMap, cfg, nil
 }
+
+func getErrorOrTimeout(assert *assert.Assertions, errs <-chan error, timeout time.Duration) {
+	select {
+	case <-errs:
+	case <-time.After(timeout):
+		assert.Fail("failed to receive error after " + timeout.String())
+	}
+}