Fix race condition in tests
continuous-integration/drone/push Build is passing Details

In normal usage, the io.Writer passed to the watcher is typically
`os.Stdout` and does not require locking because it is not being read
from inside the same process.

In tests however, the io.Writer was a *bytes.Buffer which is read
concurrently from another goroutine, introducing a race condition. This
fixes the issue in the test suite by introducing a wrapper around
*bytes.Buffer which implements the required locking.
This commit is contained in:
Rob Watson 2022-07-08 19:39:10 +02:00
parent 41f53c88df
commit cbdcef097f
4 changed files with 27 additions and 16 deletions

View File

@ -20,8 +20,6 @@ import (
toolswatch "k8s.io/client-go/tools/watch" toolswatch "k8s.io/client-go/tools/watch"
) )
const nl = "\n"
// logStream represents the logStream from an individual pod. // logStream represents the logStream from an individual pod.
type logStream struct { type logStream struct {
podName string podName string

View File

@ -194,8 +194,8 @@ func TestPodWatcher(t *testing.T) {
client := logs.KubernetesClient{Typed: &clientset} client := logs.KubernetesClient{Typed: &clientset}
selector := labels.SelectorFromSet(map[string]string{"foo": "bar"}) selector := labels.SelectorFromSet(map[string]string{"foo": "bar"})
var buf bytes.Buffer var cb concurrentBuffer
pw := logs.NewPodWatcher(client, corev1.NamespaceDefault, "mycontainer", selector, &buf, discardLogger()) pw := logs.NewPodWatcher(client, corev1.NamespaceDefault, "mycontainer", selector, &cb, discardLogger())
go func() { go func() {
for _, pod := range tc.podEvents { for _, pod := range tc.podEvents {
@ -215,7 +215,7 @@ func TestPodWatcher(t *testing.T) {
} }
if tc.wantOut != nil { if tc.wantOut != nil {
lines := bufToLines(&buf) lines := bufToLines(&cb)
require.Len(t, lines, len(tc.wantOut)) require.Len(t, lines, len(tc.wantOut))
assert.ElementsMatch(t, tc.wantOut, lines) assert.ElementsMatch(t, tc.wantOut, lines)
} }

View File

@ -63,6 +63,10 @@ type Watcher struct {
} }
// NewWatcher creates a new Watcher. // NewWatcher creates a new Watcher.
//
// The resource defined by WatcherParams will be monitored and all associated
// logs will be written to `dst`. Note that this writer should provide its own
// locking mechanisms if needed.
func NewWatcher(params WatcherParams, client KubernetesClient, podWatcherFunc PodWatcherFunc, dst io.Writer, logger *log.Logger) *Watcher { func NewWatcher(params WatcherParams, client KubernetesClient, podWatcherFunc PodWatcherFunc, dst io.Writer, logger *log.Logger) *Watcher {
return &Watcher{ return &Watcher{
params: params, params: params,

View File

@ -26,16 +26,26 @@ import (
k8stest "k8s.io/client-go/testing" k8stest "k8s.io/client-go/testing"
) )
type concurrentWriter struct { // concurrentBuffer is a buffer which implements io.Writer and fmt.Stringer. It
w io.Writer // is useful in tests to allow the output to be read back from the destination
// buffer.
type concurrentBuffer struct {
bytes.Buffer
mu sync.Mutex mu sync.Mutex
} }
func (cw *concurrentWriter) Write(p []byte) (int, error) { func (cb *concurrentBuffer) Write(p []byte) (int, error) {
cw.mu.Lock() cb.mu.Lock()
defer cw.mu.Unlock() defer cb.mu.Unlock()
return cw.w.Write(p) return cb.Buffer.Write(p)
}
func (cb *concurrentBuffer) String() string {
cb.mu.Lock()
defer cb.mu.Unlock()
return cb.Buffer.String()
} }
type mockPodWatcher struct{ err error } type mockPodWatcher struct{ err error }
@ -134,10 +144,9 @@ func TestWatcherWithPodWatcher(t *testing.T) {
untypedClient.PrependWatchReactor("deployments", k8stest.DefaultWatchReactor(deploymentsWatcher, nil)) untypedClient.PrependWatchReactor("deployments", k8stest.DefaultWatchReactor(deploymentsWatcher, nil))
client := logs.KubernetesClient{Typed: typedClient, Untyped: untypedClient} client := logs.KubernetesClient{Typed: typedClient, Untyped: untypedClient}
var buf bytes.Buffer var cb concurrentBuffer
cw := concurrentWriter{w: &buf}
params := logs.WatcherParams{Name: "mydeployment", Type: "deployments", Namespace: "default"} params := logs.WatcherParams{Name: "mydeployment", Type: "deployments", Namespace: "default"}
watcher := logs.NewWatcher(params, client, logs.NewPodWatcher, &cw, discardLogger()) watcher := logs.NewWatcher(params, client, logs.NewPodWatcher, &cb, discardLogger())
go func() { go func() {
deployment := buildDeployment(t, "mydeployment") deployment := buildDeployment(t, "mydeployment")
@ -159,12 +168,12 @@ func TestWatcherWithPodWatcher(t *testing.T) {
err := watcher.Watch(context.Background()) err := watcher.Watch(context.Background())
require.NoError(t, err) require.NoError(t, err)
lines := bufToLines(&buf) lines := bufToLines(&cb)
require.Len(t, lines, 2) require.Len(t, lines, 2)
assert.ElementsMatch(t, []string{"[foo] fake logs", "[bar] fake logs"}, lines) assert.ElementsMatch(t, []string{"[foo] fake logs", "[bar] fake logs"}, lines)
} }
// bufToLines splits the output buffer removing newline characters. // bufToLines splits the output buffer removing newline characters.
func bufToLines(buf *bytes.Buffer) []string { func bufToLines(buf *concurrentBuffer) []string {
return strings.Split(strings.TrimSpace(buf.String()), "\n") return strings.Split(strings.TrimSpace(buf.String()), "\n")
} }