-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle Nomad leadership flapping (attempt 2) #6977
Changes from all commits
ccd9c14
2810bf3
0912400
97f20bd
94a75b4
8ae03c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import ( | |
"net" | ||
"reflect" | ||
"testing" | ||
"time" | ||
|
||
version "github.com/hashicorp/go-version" | ||
"github.com/hashicorp/nomad/helper/uuid" | ||
|
@@ -258,3 +259,176 @@ func TestMaxUint64(t *testing.T) { | |
t.Fatalf("bad") | ||
} | ||
} | ||
|
||
func TestDropButLastChannelDropsValues(t *testing.T) { | ||
sourceCh := make(chan bool) | ||
shutdownCh := make(chan struct{}) | ||
defer close(shutdownCh) | ||
|
||
dstCh := dropButLastChannel(sourceCh, shutdownCh) | ||
|
||
// timeout duration for any channel propagation delay | ||
timeoutDuration := 5 * time.Millisecond | ||
|
||
// test that dstCh doesn't emit anything initially | ||
select { | ||
case <-dstCh: | ||
require.Fail(t, "received a message unexpectedly") | ||
case <-time.After(timeoutDuration): | ||
// yay no message - it could have been a default: but | ||
// checking for goroutine effect | ||
} | ||
|
||
sourceCh <- false | ||
select { | ||
case v := <-dstCh: | ||
require.False(t, v, "unexpected value from dstCh Ch") | ||
case <-time.After(timeoutDuration): | ||
require.Fail(t, "timed out waiting for source->dstCh propagation") | ||
} | ||
|
||
// channel is drained now | ||
select { | ||
case v := <-dstCh: | ||
require.Failf(t, "received a message unexpectedly", "value: %v", v) | ||
case <-time.After(timeoutDuration): | ||
// yay no message - it could have been a default: but | ||
// checking for goroutine effect | ||
} | ||
|
||
// now enqueue many messages and ensure only last one is received | ||
// enqueueing should be fast! | ||
sourceCh <- false | ||
sourceCh <- false | ||
sourceCh <- false | ||
sourceCh <- false | ||
sourceCh <- true | ||
|
||
// I suspect that dstCh may contain a stale (i.e. `false`) value if golang executes | ||
// this select before the implementation goroutine dequeues last value. | ||
// | ||
// However, never got it to fail in test - so leaving it now to see if it ever fails; | ||
// and if/when test fails, we can learn of how much of an issue it is and adjust | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can fail it pretty trivially if we push the values into the channel concurrently to the test thread, but I'm not sure that tells us anything other than we didn't get a chance to consume everything on the channel. If we pull all the values off, we're fine: package main
import (
"fmt"
"testing"
"time"
"github.com/stretchr/testify/assert"
)
func TestDropButLastChannel(t *testing.T) {
testFunc := func(t *testing.T) {
t.Parallel()
shutdownCh := make(chan struct{})
src := make(chan bool)
dst := dropButLastChannel(src, shutdownCh)
timeoutDuration := 1 * time.Millisecond
go func() {
src <- false
src <- false
src <- false
src <- false
src <- false
src <- false
src <- true
src <- false
src <- true
}()
var v bool
BREAK:
for {
select {
case v = <-dst:
fmt.Println("ok")
case <-time.After(timeoutDuration):
break BREAK
}
}
assert.True(t, v)
close(shutdownCh)
}
for i := 0; i < 1000; i++ {
t.Run(fmt.Sprintf("test-%d", i), testFunc)
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct - if it's running on different goroutine, we have no guarantees of delivery. This feels like another test to add. Here, I wanted to test that intermediate messages get sent but get dropped when no receive is happening on the channel - so I made the sends happen in the same goroutine. Though, in current form, we still cannot 100% guarantee that the first message we receive is the last sent message, but this hasn't happened in practice yet, hence my comment. Your test is good to have in that we should check that ultimately, we always send the last message last. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW - seeing your test, I realized I didn't support |
||
select { | ||
case v := <-dstCh: | ||
require.True(t, v, "unexpected value from dstCh Ch") | ||
case <-time.After(timeoutDuration): | ||
require.Fail(t, "timed out waiting for source->dstCh propagation") | ||
} | ||
|
||
sourceCh <- true | ||
sourceCh <- true | ||
sourceCh <- true | ||
sourceCh <- true | ||
sourceCh <- true | ||
sourceCh <- false | ||
select { | ||
case v := <-dstCh: | ||
require.False(t, v, "unexpected value from dstCh Ch") | ||
case <-time.After(timeoutDuration): | ||
require.Fail(t, "timed out waiting for source->dstCh propagation") | ||
} | ||
} | ||
|
||
// TestDropButLastChannel_DeliversMessages asserts that last | ||
// message is always delivered, some messages are dropped but never | ||
// introduce new messages. | ||
// On tight loop, receivers may get some intermediary messages. | ||
func TestDropButLastChannel_DeliversMessages(t *testing.T) { | ||
sourceCh := make(chan bool) | ||
shutdownCh := make(chan struct{}) | ||
defer close(shutdownCh) | ||
|
||
dstCh := dropButLastChannel(sourceCh, shutdownCh) | ||
|
||
// timeout duration for any channel propagation delay | ||
timeoutDuration := 5 * time.Millisecond | ||
|
||
sentMessages := 100 | ||
go func() { | ||
for i := 0; i < sentMessages-1; i++ { | ||
sourceCh <- true | ||
} | ||
sourceCh <- false | ||
}() | ||
|
||
receivedTrue, receivedFalse := 0, 0 | ||
var lastReceived *bool | ||
|
||
RECEIVE_LOOP: | ||
for { | ||
select { | ||
case v := <-dstCh: | ||
lastReceived = &v | ||
if v { | ||
receivedTrue++ | ||
} else { | ||
receivedFalse++ | ||
} | ||
|
||
case <-time.After(timeoutDuration): | ||
break RECEIVE_LOOP | ||
} | ||
} | ||
|
||
t.Logf("receiver got %v out %v true messages, and %v out of %v false messages", | ||
receivedTrue, sentMessages-1, receivedFalse, 1) | ||
|
||
require.NotNil(t, lastReceived) | ||
require.False(t, *lastReceived) | ||
require.Equal(t, 1, receivedFalse) | ||
require.LessOrEqual(t, receivedTrue, sentMessages-1) | ||
} | ||
|
||
// TestDropButLastChannel_DeliversMessages_Close asserts that last | ||
// message is always delivered, some messages are dropped but never | ||
// introduce new messages, even with a closed signal. | ||
func TestDropButLastChannel_DeliversMessages_Close(t *testing.T) { | ||
sourceCh := make(chan bool) | ||
shutdownCh := make(chan struct{}) | ||
defer close(shutdownCh) | ||
|
||
dstCh := dropButLastChannel(sourceCh, shutdownCh) | ||
|
||
// timeout duration for any channel propagation delay | ||
timeoutDuration := 5 * time.Millisecond | ||
|
||
sentMessages := 100 | ||
go func() { | ||
for i := 0; i < sentMessages-1; i++ { | ||
sourceCh <- true | ||
} | ||
sourceCh <- false | ||
close(sourceCh) | ||
}() | ||
|
||
receivedTrue, receivedFalse := 0, 0 | ||
var lastReceived *bool | ||
|
||
RECEIVE_LOOP: | ||
for { | ||
select { | ||
case v, ok := <-dstCh: | ||
if !ok { | ||
break RECEIVE_LOOP | ||
} | ||
lastReceived = &v | ||
if v { | ||
receivedTrue++ | ||
} else { | ||
receivedFalse++ | ||
} | ||
|
||
case <-time.After(timeoutDuration): | ||
require.Fail(t, "timed out while waiting for messages") | ||
} | ||
} | ||
|
||
t.Logf("receiver got %v out %v true messages, and %v out of %v false messages", | ||
receivedTrue, sentMessages-1, receivedFalse, 1) | ||
|
||
require.NotNil(t, lastReceived) | ||
require.False(t, *lastReceived) | ||
require.Equal(t, 1, receivedFalse) | ||
require.LessOrEqual(t, receivedTrue, sentMessages-1) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unnecessary indeed. I'd like to keep though just because I find it easier to see all state machine transitions in goto statements.