Fix some race conditions

wallrj commented 4 years ago

Wait for controller manager to exit before exiting the test (prevents calls to t.Log after the test has finished)
Add Lock around the setting and reading from the fake Etcd client (prevents the controller using a MembershipAPI which has been replaced in the test)
Return a copy of the members list (prevents controller using a members list which might be mutated during the test)

The controller-runtime manager.Start doesn't wait for all its go-routines to finish, which leads to another race when the controller runtime attempts to log (via t.Log) after a test has finished. See:

Part of: #117

wallrj commented 4 years ago

You can see the leaked goroutine in the test results;

Goroutine 270 (running) created at:
  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start()
      /go/pkg/mod/sigs.k8s.io/controller-runtime@v0.3.0/pkg/internal/controller/controller.go:157 +0x4ee
  sigs.k8s.io/controller-runtime/pkg/manager.(*controllerManager).startLeaderElectionRunnables.func1()
      /go/pkg/mod/sigs.k8s.io/controller-runtime@v0.3.0/pkg/manager/internal.go:326 +0x74

--- https://app.circleci.com/jobs/github/improbable-eng/etcd-cluster-operator/668/parallel-runs/0/steps/0-102

wallrj commented 4 years ago

I've removed the -race parameter now. We will have to enable the race checks once the bug has been fixed in controller-runtime. Meanwhile this fixes three of the race conditions.

improbable-eng / etcd-cluster-operator

Fix some race conditions #123