From 208fc20659c006a70e711a9302dedb6f6f35d81d Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Tue, 22 Aug 2023 12:02:14 -0400 Subject: [PATCH 1/4] Fix more uses of random --- cluster/bootstrap.go | 8 ++--- db/db.go | 2 -- db/db_test.go | 19 ++--------- disco/service.go | 9 ++---- {store/random => random}/random.go | 16 +++++++++- random/random_test.go | 51 ++++++++++++++++++++++++++++++ store/random/random_test.go | 25 --------------- store/store_test.go | 8 ++--- tcp/pool/channel_test.go | 9 +++--- 9 files changed, 82 insertions(+), 65 deletions(-) rename {store/random => random}/random.go (67%) create mode 100644 random/random_test.go delete mode 100644 store/random/random_test.go diff --git a/cluster/bootstrap.go b/cluster/bootstrap.go index 29bc6c63..c98a32b3 100644 --- a/cluster/bootstrap.go +++ b/cluster/bootstrap.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "log" - "math/rand" "net/http" "os" "strings" @@ -15,12 +14,9 @@ import ( "time" rurl "github.com/rqlite/rqlite/http/url" + "github.com/rqlite/rqlite/random" ) -func init() { - rand.Seed(time.Now().UnixNano()) -} - var ( // ErrBootTimeout is returned when a boot operation does not // complete within the timeout. @@ -260,5 +256,5 @@ func NewAddressProviderString(ss []string) AddressProvider { // useful to prevent nodes across the cluster performing certain operations // all at the same time. func jitter(duration time.Duration) time.Duration { - return duration + time.Duration(rand.Float64()*float64(duration)) + return duration + time.Duration(random.Float64()*float64(duration)) } diff --git a/db/db.go b/db/db.go index b1bb02eb..cf541cc7 100644 --- a/db/db.go +++ b/db/db.go @@ -10,7 +10,6 @@ import ( "expvar" "fmt" "io" - "math/rand" "os" "path/filepath" "strconv" @@ -53,7 +52,6 @@ var DBVersion string var stats *expvar.Map func init() { - rand.Seed(time.Now().UnixNano()) DBVersion, _, _ = sqlite3.Version() stats = expvar.NewMap("db") ResetStats() diff --git a/db/db_test.go b/db/db_test.go index 09b03fb5..67c0610e 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -4,8 +4,6 @@ import ( "database/sql" "fmt" "io" - "io/ioutil" - "math/rand" "os" "path/filepath" "sync" @@ -25,7 +23,7 @@ func Test_RemoveFiles(t *testing.T) { t.Fatalf("failed to remove files: %s", err.Error()) } - files, err := ioutil.ReadDir(d) + files, err := os.ReadDir(d) if err != nil { t.Fatalf("failed to read directory: %s", err.Error()) } @@ -906,7 +904,7 @@ func asJSON(v interface{}) string { // mustTempFile returns a path to a temporary file in directory dir. It is up to the // caller to remove the file once it is no longer needed. func mustTempFile() string { - tmpfile, err := ioutil.TempFile("", "rqlite-db-test") + tmpfile, err := os.CreateTemp("", "rqlite-db-test") if err != nil { panic(err.Error()) } @@ -915,7 +913,7 @@ func mustTempFile() string { } func mustTempDir() string { - tmpdir, err := ioutil.TempDir("", "rqlite-db-test") + tmpdir, err := os.MkdirTemp("", "rqlite-db-test") if err != nil { panic(err.Error()) } @@ -947,12 +945,6 @@ func mustCopyFile(dst, src string) { } } -func mustRenameFile(oldpath, newpath string) { - if err := os.Rename(oldpath, newpath); err != nil { - panic(err) - } -} - func mustCreateClosedFile(path string) { f, err := os.Create(path) if err != nil { @@ -975,8 +967,3 @@ func mustFileSize(path string) int64 { fi := mustStat(path) return fi.Size() } - -func mustRandomInt(n int) int { - rand.Seed(time.Now().UnixNano()) - return rand.Intn(n) -} diff --git a/disco/service.go b/disco/service.go index 70cfce97..e58a0b92 100644 --- a/disco/service.go +++ b/disco/service.go @@ -3,15 +3,12 @@ package disco import ( "fmt" "log" - "math/rand" "os" "sync" "time" -) -func init() { - rand.Seed(time.Now().UnixNano()) -} + "github.com/rqlite/rqlite/random" +) const ( leaderChanLen = 5 // Support any fast back-to-back leadership changes. @@ -143,5 +140,5 @@ func (s *Service) updateContact(t time.Time) { } func jitter(duration time.Duration) time.Duration { - return duration + time.Duration(rand.Float64()*float64(duration)) + return duration + time.Duration(random.Float64()*float64(duration)) } diff --git a/store/random/random.go b/random/random.go similarity index 67% rename from store/random/random.go rename to random/random.go index 3f34431f..54d58033 100644 --- a/store/random/random.go +++ b/random/random.go @@ -15,7 +15,7 @@ func init() { } // RandomString returns a random string of 20 characters -func RandomString() string { +func String() string { mu.Lock() defer mu.Unlock() var output strings.Builder @@ -27,3 +27,17 @@ func RandomString() string { } return output.String() } + +// Float64 returns a random float64 +func Float64() float64 { + mu.Lock() + defer mu.Unlock() + return r.Float64() +} + +// Intn returns a random int +func Intn(n int) int { + mu.Lock() + defer mu.Unlock() + return r.Intn(n) +} diff --git a/random/random_test.go b/random/random_test.go new file mode 100644 index 00000000..7df44a01 --- /dev/null +++ b/random/random_test.go @@ -0,0 +1,51 @@ +package random + +import ( + "testing" +) + +func Test_StringLength(t *testing.T) { + str := String() + if len(str) != 20 { + t.Errorf("String() returned a string of length %d; want 20", len(str)) + } +} + +func Test_StringUniqueness(t *testing.T) { + const numStrings = 10 + strs := make(map[string]bool, numStrings) + + for i := 0; i < numStrings; i++ { + str := String() + if strs[str] { + t.Errorf("String() returned a non-unique string: %s", str) + } + strs[str] = true + } +} + +func Test_Float64Uniqueness(t *testing.T) { + const numFloat64s = 10 + floats := make(map[float64]bool, numFloat64s) + + for i := 0; i < numFloat64s; i++ { + f := Float64() + if floats[f] { + t.Errorf("Float64() returned a non-unique float64: %f", f) + } + floats[f] = true + } +} + +func Test_IntnUniqueness(t *testing.T) { + const numIntns = 10 + intns := make(map[int]bool, numIntns) + + for i := 0; i < numIntns; i++ { + n := Intn(1000000000) + if intns[n] { + t.Errorf("Intn() returned a non-unique int: %d", n) + } + intns[n] = true + } +} diff --git a/store/random/random_test.go b/store/random/random_test.go deleted file mode 100644 index 795f5651..00000000 --- a/store/random/random_test.go +++ /dev/null @@ -1,25 +0,0 @@ -package random - -import ( - "testing" -) - -func Test_RandomStringLength(t *testing.T) { - str := RandomString() - if len(str) != 20 { - t.Errorf("RandomString() returned a string of length %d; want 20", len(str)) - } -} - -func Test_RandomStringUniqueness(t *testing.T) { - const numStrings = 100 - strs := make(map[string]bool, numStrings) - - for i := 0; i < numStrings; i++ { - str := RandomString() - if strs[str] { - t.Errorf("RandomString() returned a non-unique string: %s", str) - } - strs[str] = true - } -} diff --git a/store/store_test.go b/store/store_test.go index 52215141..7d651113 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -15,7 +15,7 @@ import ( "github.com/rqlite/rqlite/command" "github.com/rqlite/rqlite/command/encoding" "github.com/rqlite/rqlite/db" - "github.com/rqlite/rqlite/store/random" + "github.com/rqlite/rqlite/random" "github.com/rqlite/rqlite/testdata/chinook" ) @@ -2866,18 +2866,18 @@ func mustNewStoreAtPathsLn(id, dataPath, sqlitePath string, fk bool) (*Store, ne } func mustNewStore(t *testing.T) (*Store, net.Listener) { - return mustNewStoreAtPathsLn(random.RandomString(), t.TempDir(), "", false) + return mustNewStoreAtPathsLn(random.String(), t.TempDir(), "", false) } func mustNewStoreFK(t *testing.T) (*Store, net.Listener) { - return mustNewStoreAtPathsLn(random.RandomString(), t.TempDir(), "", true) + return mustNewStoreAtPathsLn(random.String(), t.TempDir(), "", true) } func mustNewStoreSQLitePath(t *testing.T) (*Store, net.Listener, string) { dataDir := t.TempDir() sqliteDir := t.TempDir() sqlitePath := filepath.Join(sqliteDir, "explicit-path.db") - s, ln := mustNewStoreAtPathsLn(random.RandomString(), dataDir, sqlitePath, true) + s, ln := mustNewStoreAtPathsLn(random.String(), dataDir, sqlitePath, true) return s, ln, sqlitePath } diff --git a/tcp/pool/channel_test.go b/tcp/pool/channel_test.go index 069157b8..d98f8012 100644 --- a/tcp/pool/channel_test.go +++ b/tcp/pool/channel_test.go @@ -2,11 +2,12 @@ package pool import ( "log" - "math/rand" "net" "sync" "testing" "time" + + "github.com/rqlite/rqlite/random" ) var ( @@ -21,8 +22,6 @@ func init() { // used for factory function go simpleTCPServer() time.Sleep(time.Millisecond * 300) // wait until tcp server has been settled - - rand.Seed(time.Now().UTC().UnixNano()) } func TestNew(t *testing.T) { @@ -229,7 +228,7 @@ func TestPoolConcurrent2(t *testing.T) { for i := 0; i < 10; i++ { go func(i int) { conn, _ := p.Get() - time.Sleep(time.Millisecond * time.Duration(rand.Intn(100))) + time.Sleep(time.Millisecond * time.Duration(random.Intn(100))) conn.Close() wg.Done() }(i) @@ -240,7 +239,7 @@ func TestPoolConcurrent2(t *testing.T) { wg.Add(1) go func(i int) { conn, _ := p.Get() - time.Sleep(time.Millisecond * time.Duration(rand.Intn(100))) + time.Sleep(time.Millisecond * time.Duration(random.Intn(100))) conn.Close() wg.Done() }(i) From 26773497afed4f741bf6bb3eca14d23a51a49efd Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Tue, 22 Aug 2023 12:03:02 -0400 Subject: [PATCH 2/4] CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 446db278..24a60da3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ When officially released 8.0 will support (mostly) seamless upgrades from the 7. - [PR #1355](https://github.com/rqlite/rqlite/pull/1355): Database layer can run an integrity check. - [PR #1385](https://github.com/rqlite/rqlite/pull/1358): Remove support for in-memory databases. - [PR #1360](https://github.com/rqlite/rqlite/pull/1360): 'go mod' updates, and move to go 1.21. -- [PR #1369](https://github.com/rqlite/rqlite/pull/1369): Use singleton, sync'ed, random source for Store testing. +- [PR #1369](https://github.com/rqlite/rqlite/pull/1369), [PR #1370](https://github.com/rqlite/rqlite/pull/1370): Use singleton, sync'ed, random source for Store testing. ## 7.21.4 (July 8th 2023) ### Implementation changes and bug fixes From 9c5d9de501c30bf3068dbb26e87ed3235d960af2 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Tue, 22 Aug 2023 12:03:37 -0400 Subject: [PATCH 3/4] CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24a60da3..0dceab3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ When officially released 8.0 will support (mostly) seamless upgrades from the 7. - [PR #1355](https://github.com/rqlite/rqlite/pull/1355): Database layer can run an integrity check. - [PR #1385](https://github.com/rqlite/rqlite/pull/1358): Remove support for in-memory databases. - [PR #1360](https://github.com/rqlite/rqlite/pull/1360): 'go mod' updates, and move to go 1.21. -- [PR #1369](https://github.com/rqlite/rqlite/pull/1369), [PR #1370](https://github.com/rqlite/rqlite/pull/1370): Use singleton, sync'ed, random source for Store testing. +- [PR #1369](https://github.com/rqlite/rqlite/pull/1369), [PR #1370](https://github.com/rqlite/rqlite/pull/1370): Use singleton, sync'ed, random source. ## 7.21.4 (July 8th 2023) ### Implementation changes and bug fixes From 170772587e90c21a9b9f84bf9a9526bdc64b3ba9 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Tue, 22 Aug 2023 12:06:39 -0400 Subject: [PATCH 4/4] Implement Jitter() --- cluster/bootstrap.go | 11 ++--------- disco/service.go | 6 +----- random/random.go | 7 +++++++ 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/cluster/bootstrap.go b/cluster/bootstrap.go index c98a32b3..50259807 100644 --- a/cluster/bootstrap.go +++ b/cluster/bootstrap.go @@ -112,7 +112,7 @@ func (b *Bootstrapper) SetBasicAuth(username, password string) { func (b *Bootstrapper) Boot(id, raftAddr string, done func() bool, timeout time.Duration) error { timeoutT := time.NewTimer(timeout) defer timeoutT.Stop() - tickerT := time.NewTimer(jitter(time.Millisecond)) + tickerT := time.NewTimer(random.Jitter(time.Millisecond)) defer tickerT.Stop() for { @@ -127,7 +127,7 @@ func (b *Bootstrapper) Boot(id, raftAddr string, done func() bool, timeout time. b.setBootStatus(BootDone) return nil } - tickerT.Reset(jitter(b.Interval)) // Move to longer-period polling + tickerT.Reset(random.Jitter(b.Interval)) // Move to longer-period polling targets, err := b.provider.Lookup() if err != nil { @@ -251,10 +251,3 @@ func (s *stringAddressProvider) Lookup() ([]string, error) { func NewAddressProviderString(ss []string) AddressProvider { return &stringAddressProvider{ss} } - -// jitter adds a little bit of randomness to a given duration. This is -// useful to prevent nodes across the cluster performing certain operations -// all at the same time. -func jitter(duration time.Duration) time.Duration { - return duration + time.Duration(random.Float64()*float64(duration)) -} diff --git a/disco/service.go b/disco/service.go index e58a0b92..510b2a7b 100644 --- a/disco/service.go +++ b/disco/service.go @@ -76,7 +76,7 @@ func (s *Service) Register(id, apiAddr, addr string) (bool, string, error) { return true, apiAddr, nil } - time.Sleep(jitter(s.RegisterInterval)) + time.Sleep(random.Jitter(s.RegisterInterval)) } } @@ -138,7 +138,3 @@ func (s *Service) updateContact(t time.Time) { defer s.mu.Unlock() s.lastContact = t } - -func jitter(duration time.Duration) time.Duration { - return duration + time.Duration(random.Float64()*float64(duration)) -} diff --git a/random/random.go b/random/random.go index 54d58033..856db65f 100644 --- a/random/random.go +++ b/random/random.go @@ -41,3 +41,10 @@ func Intn(n int) int { defer mu.Unlock() return r.Intn(n) } + +// Jitter adds a little bit of randomness to a given duration. This is +// useful to prevent nodes across the cluster performing certain operations +// all at the same time. +func Jitter(d time.Duration) time.Duration { + return d + time.Duration(Float64()*float64(d)) +}