Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't bootstrap when bootstrap_expect is zero #9672

Merged
merged 4 commits into from
Jan 4, 2021
Merged

Don't bootstrap when bootstrap_expect is zero #9672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d68bede
add a failing test for unexpected bootstrapping
Dec 17, 2020
44be6e3
Only bootstrap when `bootstrap_expect`
Dec 17, 2020
926cf25
tweak bootstrap testing
Jan 4, 2021
c582bfb
add changelog
Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion nomad/serf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (s *Server) nodeJoin(me serf.MemberEvent) {
s.peerLock.Unlock()

// If we still expecting to bootstrap, may need to handle this
if atomic.LoadInt32(&s.config.Bootstrapped) == 0 {
if s.config.BootstrapExpect != 0 && atomic.LoadInt32(&s.config.Bootstrapped) == 0 {
s.maybeBootstrap()
}
}
Expand Down
40 changes: 40 additions & 0 deletions nomad/serf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import (
"os"
"path"
"strings"
"sync/atomic"
"testing"
"time"

"github.com/hashicorp/nomad/testutil"
"github.com/hashicorp/serf/serf"
"github.com/stretchr/testify/require"
)

func TestNomad_JoinPeer(t *testing.T) {
Expand Down Expand Up @@ -442,3 +444,41 @@ func TestNomad_BadExpect(t *testing.T) {
t.Fatalf("should have 0 peers: %v", err)
})
}

// TestNomad_NonBootstraping_ShouldntBootstap asserts that if BootstrapExpect is zero,
// the server shouldn't bootstrap
func TestNomad_NonBootstraping_ShouldntBootstap(t *testing.T) {
t.Parallel()

dir := tmpDir(t)
defer os.RemoveAll(dir)

s1, cleanupS1 := TestServer(t, func(c *Config) {
c.BootstrapExpect = 0
c.DevMode = false
c.DataDir = path.Join(dir, "node")
})
defer cleanupS1()

testutil.WaitForResult(func() (bool, error) {
s1.peerLock.Lock()
p := len(s1.localPeers)
s1.peerLock.Unlock()
if p != 1 {
return false, fmt.Errorf("%d", p)
}

return true, nil
}, func(err error) {
t.Fatalf("expected 1 local peer: %v", err)
})

time.Sleep(500 * time.Millisecond)

bootstrapped := atomic.LoadInt32(&s1.config.Bootstrapped)
require.Zero(t, bootstrapped, "expecting non-bootstrapped servers")

p, _ := s1.numPeers()
require.Zero(t, p, "number of peers in Raft")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be put in a WaitForResult instead of using a sleep? If not please comment on the sleep so it's clear why sleeping is the preferred approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tricky, as we are testing for a negative in a concurrent environment: the server starts in non-bootstrap mode with 0 peers, and we want to test that the server remains so even it registers itself into serf. Without time delay, the test may pass just because the server hasn't setup raft yet. The sleep doesn't completely rule out that possibility, but makes it less likely.

I'll add a note.


}