From 2ff99c591218a2165aca209a83ad5a20b9feee25 Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Sat, 1 Sep 2018 09:19:23 -0400 Subject: [PATCH 1/7] roachtest: add comment about acceptance tests Release note: None --- pkg/cmd/roachtest/acceptance.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/roachtest/acceptance.go b/pkg/cmd/roachtest/acceptance.go index 16c9e0c89eca..703c390ab109 100644 --- a/pkg/cmd/roachtest/acceptance.go +++ b/pkg/cmd/roachtest/acceptance.go @@ -18,6 +18,9 @@ package main import "context" func registerAcceptance(r *registry) { + // The acceptance tests all share a 4-node cluster and run sequentially. In + // local mode the acceptance tests should be configured to run within a + // minute or so as these tests are run on every merge to master. spec := testSpec{ Name: "acceptance", Nodes: nodes(4), From 80face91cf6befa559c48ab1ae2c7e5bc1ee4837 Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Sat, 1 Sep 2018 09:51:07 -0400 Subject: [PATCH 2/7] roachtest: add acceptance/gossip/{peerings,restart} Move the gossip-peerings and gossip-restart acceptance tests to new acceptance/gossip/{peerings,restart} roachtests. See #29151 Release note: None --- pkg/cmd/roachtest/acceptance.go | 4 + pkg/cmd/roachtest/gossip.go | 178 ++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) diff --git a/pkg/cmd/roachtest/acceptance.go b/pkg/cmd/roachtest/acceptance.go index 703c390ab109..aca98a5a05d8 100644 --- a/pkg/cmd/roachtest/acceptance.go +++ b/pkg/cmd/roachtest/acceptance.go @@ -30,11 +30,15 @@ func registerAcceptance(r *registry) { name string fn func(ctx context.Context, t *test, c *cluster) }{ + // Sorted. Please keep it that way. {"bank/cluster-recovery", runBankClusterRecovery}, {"bank/node-restart", runBankNodeRestart}, {"build-info", runBuildInfo}, {"cli/node-status", runCLINodeStatus}, {"event-log", runEventLog}, + {"gossip/peerings", runGossipPeerings}, + {"gossip/restart", runGossipRestart}, + {"gossip/restart-first-node-needs-incoming", runGossipRestartFirstNodeNeedsIncoming}, {"rapid-restart", runRapidRestart}, {"status-server", runStatusServer}, } diff --git a/pkg/cmd/roachtest/gossip.go b/pkg/cmd/roachtest/gossip.go index ea90ac7e8e34..a09027604101 100644 --- a/pkg/cmd/roachtest/gossip.go +++ b/pkg/cmd/roachtest/gossip.go @@ -19,11 +19,16 @@ import ( "context" gosql "database/sql" "fmt" + "net/http" "strings" "time" "unicode" + "github.com/cockroachdb/cockroach/pkg/gossip" + "github.com/cockroachdb/cockroach/pkg/util/httputil" + "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/pkg/errors" ) func registerGossip(r *registry) { @@ -117,3 +122,176 @@ SELECT string_agg(source_id::TEXT || ':' || target_id::TEXT, ',') }, }) } + +type gossipUtil struct { + waitTime time.Duration + urlMap map[int]string +} + +func newGossipUtil(ctx context.Context, c *cluster, waitTime time.Duration) *gossipUtil { + urlMap := make(map[int]string) + for i, addr := range c.ExternalAdminUIAddr(ctx, c.All()) { + urlMap[i+1] = `http://` + addr + } + return &gossipUtil{ + waitTime: waitTime, + urlMap: urlMap, + } +} + +type checkGossipFunc func(map[string]gossip.Info) error + +// checkGossip fetches the gossip infoStore from each node and invokes the +// given function. The test passes if the function returns 0 for every node, +// retrying for up to the given duration. +func (g *gossipUtil) check(ctx context.Context, c *cluster, f checkGossipFunc) error { + return retry.ForDuration(g.waitTime, func() error { + var infoStatus gossip.InfoStatus + for i := 1; i <= c.nodes; i++ { + url := g.urlMap[i] + `/_status/gossip/local` + if err := httputil.GetJSON(http.Client{}, url, &infoStatus); err != nil { + return errors.Wrapf(err, "failed to get gossip status from node %d", i) + } + if err := f(infoStatus.Infos); err != nil { + return errors.Wrapf(err, "node %d", i) + } + } + + return nil + }) +} + +// hasPeers returns a checkGossipFunc that passes when the given number of +// peers are connected via gossip. +func (gossipUtil) hasPeers(expected int) checkGossipFunc { + return func(infos map[string]gossip.Info) error { + count := 0 + for k := range infos { + if strings.HasPrefix(k, gossip.KeyNodeIDPrefix) { + count++ + } + } + if count != expected { + return errors.Errorf("expected %d peers, found %d", expected, count) + } + return nil + } +} + +// hasSentinel is a checkGossipFunc that passes when the sentinel gossip is present. +func (gossipUtil) hasSentinel(infos map[string]gossip.Info) error { + if _, ok := infos[gossip.KeySentinel]; !ok { + return errors.Errorf("sentinel not found") + } + return nil +} + +// hasClusterID is a checkGossipFunc that passes when the cluster ID gossip is present. +func (gossipUtil) hasClusterID(infos map[string]gossip.Info) error { + if _, ok := infos[gossip.KeyClusterID]; !ok { + return errors.Errorf("cluster ID not found") + } + return nil +} + +func (g *gossipUtil) checkConnectedAndFunctional(ctx context.Context, t *test, c *cluster) { + c.l.Printf("waiting for gossip to be connected\n") + if err := g.check(ctx, c, g.hasPeers(c.nodes)); err != nil { + t.Fatal(err) + } + if err := g.check(ctx, c, g.hasClusterID); err != nil { + t.Fatal(err) + } + if err := g.check(ctx, c, g.hasSentinel); err != nil { + t.Fatal(err) + } + + for i := 1; i <= c.nodes; i++ { + db := c.Conn(ctx, i) + defer db.Close() + if i == 1 { + if _, err := db.Exec("CREATE DATABASE IF NOT EXISTS test"); err != nil { + t.Fatal(err) + } + if _, err := db.Exec("CREATE TABLE IF NOT EXISTS test.kv (k INT PRIMARY KEY, v INT)"); err != nil { + t.Fatal(err) + } + if _, err := db.Exec(`UPSERT INTO test.kv (k, v) VALUES (1, 0)`); err != nil { + t.Fatal(err) + } + } + rows, err := db.Query(`UPDATE test.kv SET v=v+1 WHERE k=1 RETURNING v`) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + var count int + if rows.Next() { + if err := rows.Scan(&count); err != nil { + t.Fatal(err) + } + if count != i { + t.Fatalf("unexpected value %d for write #%d (expected %d)", count, i, i) + } + } else { + t.Fatalf("no results found from update") + } + } +} + +func runGossipPeerings(ctx context.Context, t *test, c *cluster) { + c.Put(ctx, cockroach, "./cockroach") + c.Start(ctx) + + // Repeatedly restart a random node and verify that all of the nodes are + // seeing the gossiped values. + + g := newGossipUtil(ctx, c, 30*time.Second) + deadline := timeutil.Now().Add(time.Minute) + + for i := 1; timeutil.Now().Before(deadline); i++ { + if err := g.check(ctx, c, g.hasPeers(c.nodes)); err != nil { + t.Fatal(err) + } + if err := g.check(ctx, c, g.hasClusterID); err != nil { + t.Fatal(err) + } + if err := g.check(ctx, c, g.hasSentinel); err != nil { + t.Fatal(err) + } + c.l.Printf("%d: OK\n", i) + + // Restart a random node. + node := c.All().randNode() + c.l.Printf("%d: restarting node %d\n", i, node[0]) + c.Stop(ctx, node) + c.Start(ctx, node) + } +} + +func runGossipRestart(ctx context.Context, t *test, c *cluster) { + c.Put(ctx, cockroach, "./cockroach") + c.Start(ctx) + + // Repeatedly stop and restart a cluster and verify that we can perform basic + // operations. This is stressing the gossiping of the first range descriptor + // which is required for any node to be able do even the most basic + // operations on a cluster. + + g := newGossipUtil(ctx, c, 30*time.Second) + deadline := timeutil.Now().Add(time.Minute) + + for i := 1; timeutil.Now().Before(deadline); i++ { + g.checkConnectedAndFunctional(ctx, t, c) + c.l.Printf("%d: OK\n", i) + + c.l.Printf("%d: killing all nodes\n", i) + c.Stop(ctx) + + c.l.Printf("%d: restarting all nodes\n", i) + c.Start(ctx) + } +} + +func runGossipRestartFirstNodeNeedsIncoming(ctx context.Context, t *test, c *cluster) { +} From 7f1724726030b8bd34e4313156b13b7bffdf7c06 Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Sat, 1 Sep 2018 10:32:27 -0400 Subject: [PATCH 3/7] roachtest: add acceptance/gossip/restart-node-one Move the gossip-restart-first-node-needs-incoming acceptance test to a new acceptance/gossip/restart-node-one roachtest. See #29151 Fixes #29115 Release note: None --- pkg/acceptance/gossip_peerings_test.go | 307 ------------------------- pkg/acceptance/util_gossip.go | 82 ------- pkg/cmd/roachtest/acceptance.go | 2 +- pkg/cmd/roachtest/gossip.go | 110 ++++++++- 4 files changed, 105 insertions(+), 396 deletions(-) delete mode 100644 pkg/acceptance/gossip_peerings_test.go delete mode 100644 pkg/acceptance/util_gossip.go diff --git a/pkg/acceptance/gossip_peerings_test.go b/pkg/acceptance/gossip_peerings_test.go deleted file mode 100644 index 211702bdaac6..000000000000 --- a/pkg/acceptance/gossip_peerings_test.go +++ /dev/null @@ -1,307 +0,0 @@ -// Copyright 2015 The Cockroach Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or -// implied. See the License for the specific language governing -// permissions and limitations under the License. - -package acceptance - -import ( - "context" - "io/ioutil" - "math/rand" - "path/filepath" - "testing" - "time" - - "github.com/cockroachdb/cockroach/pkg/acceptance/cluster" - "github.com/cockroachdb/cockroach/pkg/acceptance/localcluster" - "github.com/cockroachdb/cockroach/pkg/testutils" - "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/retry" - "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/pkg/errors" -) - -const waitTime = 2 * time.Minute - -func TestGossipPeerings(t *testing.T) { - s := log.Scope(t) - defer s.Close(t) - - RunLocal(t, func(t *testing.T) { - runTestWithCluster(t, testGossipPeeringsInner) - }) -} - -func testGossipPeeringsInner( - ctx context.Context, t *testing.T, c cluster.Cluster, cfg cluster.TestConfig, -) { - num := c.NumNodes() - - deadline := timeutil.Now().Add(cfg.Duration) - - for timeutil.Now().Before(deadline) { - if err := CheckGossip(ctx, c, waitTime, HasPeers(num)); err != nil { - t.Fatal(err) - } - - // Restart the first node. - log.Infof(ctx, "restarting node 0") - if err := c.Restart(ctx, 0); err != nil { - t.Fatal(err) - } - if err := CheckGossip(ctx, c, waitTime, HasPeers(num)); err != nil { - t.Fatal(err) - } - - // Restart another node (if there is one). - var pickedNode int - if num > 1 { - pickedNode = rand.Intn(num-1) + 1 - } - log.Infof(ctx, "restarting node %d", pickedNode) - if err := c.Restart(ctx, pickedNode); err != nil { - t.Fatal(err) - } - if err := CheckGossip(ctx, c, waitTime, HasPeers(num)); err != nil { - t.Fatal(err) - } - } -} - -// TestGossipRestart verifies that the gossip network can be -// re-bootstrapped after a time when all nodes were down -// simultaneously. -func TestGossipRestart(t *testing.T) { - s := log.Scope(t) - defer s.Close(t) - - ctx := context.Background() - cfg := ReadConfigFromFlags() - RunLocal(t, func(t *testing.T) { - c := StartCluster(ctx, t, cfg) - defer c.AssertAndStop(ctx, t) - - testGossipRestartInner(ctx, t, c, cfg) - }) -} - -func testGossipRestartInner( - ctx context.Context, t *testing.T, c cluster.Cluster, cfg cluster.TestConfig, -) { - // This already replicates the first range (in the local setup). - // The replication of the first range is important: as long as the - // first range only exists on one node, that node can trivially - // acquire the range lease. Once the range is replicated, however, - // nodes must be able to discover each other over gossip before the - // lease can be acquired. - num := c.NumNodes() - - deadline := timeutil.Now().Add(cfg.Duration) - - for timeutil.Now().Before(deadline) { - log.Infof(ctx, "waiting for initial gossip connections") - if err := CheckGossip(ctx, c, waitTime, HasPeers(num)); err != nil { - t.Fatal(err) - } - if err := CheckGossip(ctx, c, waitTime, hasClusterID); err != nil { - t.Fatal(err) - } - if err := CheckGossip(ctx, c, waitTime, hasSentinel); err != nil { - t.Fatal(err) - } - - log.Infof(ctx, "killing all nodes") - for i := 0; i < num; i++ { - if err := c.Kill(ctx, i); err != nil { - t.Fatal(err) - } - } - - log.Infof(ctx, "restarting all nodes") - var chs []<-chan error - for i := 0; i < num; i++ { - // We need to restart asynchronously because the local cluster nodes - // like to wait until they are ready to serve. - chs = append(chs, c.(*localcluster.LocalCluster).RestartAsync(ctx, i)) - } - for i, ch := range chs { - if err := <-ch; err != nil { - t.Errorf("error restarting node %d: %s", i, err) - } - } - if t.Failed() { - t.FailNow() - } - - testClusterConnectedAndFunctional(ctx, t, c) - } -} - -func testClusterConnectedAndFunctional(ctx context.Context, t *testing.T, c cluster.Cluster) { - num := c.NumNodes() - - log.Infof(ctx, "waiting for gossip to be connected") - if err := CheckGossip(ctx, c, waitTime, HasPeers(num)); err != nil { - t.Fatal(err) - } - if err := CheckGossip(ctx, c, waitTime, hasClusterID); err != nil { - t.Fatal(err) - } - if err := CheckGossip(ctx, c, waitTime, hasSentinel); err != nil { - t.Fatal(err) - } - - for i := 0; i < num; i++ { - db, err := c.NewDB(ctx, i) - if err != nil { - t.Fatal(err) - } - if i == 0 { - if _, err := db.Exec("CREATE DATABASE IF NOT EXISTS test"); err != nil { - t.Fatal(err) - } - if _, err := db.Exec("CREATE TABLE IF NOT EXISTS test.kv (k INT PRIMARY KEY, v INT)"); err != nil { - t.Fatal(err) - } - if _, err := db.Exec(`UPSERT INTO test.kv (k, v) VALUES (1, 0)`); err != nil { - t.Fatal(err) - } - } - rows, err := db.Query(`UPDATE test.kv SET v=v+1 WHERE k=1 RETURNING v`) - if err != nil { - t.Fatal(err) - } - defer rows.Close() - var count int - if rows.Next() { - if err := rows.Scan(&count); err != nil { - t.Fatal(err) - } - if count != (i + 1) { - t.Fatalf("unexpected value %d for write #%d (expected %d)", count, i, i+1) - } - } else { - t.Fatalf("no results found from update") - } - } -} - -// This test verifies that when the first node is restarted and has no valid -// join flags or persisted Gossip bootstrap records (achieved through restarting -// the other nodes and thus randomizing their ports), it will still be able to -// accept incoming Gossip connections and manages to bootstrap that way. -// -// See https://github.com/cockroachdb/cockroach/issues/18027. -func TestGossipRestartFirstNodeNeedsIncoming(t *testing.T) { - s := log.Scope(t) - defer s.Close(t) - - ctx := context.Background() - cfg := ReadConfigFromFlags() - RunLocal(t, func(t *testing.T) { - c := StartCluster(ctx, t, cfg) - defer c.AssertAndStop(ctx, t) - - testGossipRestartFirstNodeNeedsIncomingInner(ctx, t, c, cfg) - }) -} - -func testGossipRestartFirstNodeNeedsIncomingInner( - ctx context.Context, t *testing.T, c cluster.Cluster, cfg cluster.TestConfig, -) { - num := c.NumNodes() - - if err := c.Kill(ctx, 0); err != nil { - t.Fatal(err) - } - - // Configure the first node to repel all replicas. - - lc := c.(*localcluster.LocalCluster) - lc.Nodes[0].Cfg.ExtraArgs = append(lc.Nodes[0].Cfg.ExtraArgs, "--attrs", "empty") - - if err := c.Restart(ctx, 0); err != nil { - t.Fatal(err) - } - - const zoneCfg = "constraints: [-empty]" - zoneFile := filepath.Join(lc.Nodes[0].Cfg.DataDir, ".customzone") - - if err := ioutil.WriteFile(zoneFile, []byte(zoneCfg), 0644); err != nil { - t.Fatal(err) - } - - for _, zone := range []string{".default", ".meta", ".liveness"} { - if _, _, err := c.ExecCLI(ctx, 0, []string{"zone", "set", zone, "-f", zoneFile}); err != nil { - t.Fatal(err) - } - } - - db := makePGClient(t, c.PGUrl(ctx, 0)) - defer db.Close() - - // NB: This was flaky with `SucceedsSoon`. - if err := retry.ForDuration(2*time.Minute, func() error { - const query = "SELECT count(replicas) FROM crdb_internal.ranges WHERE array_position(replicas, 1) IS NOT NULL" - var count int - if err := db.QueryRow(query).Scan(&count); err != nil { - t.Fatal(err) - } - if count > 0 { - err := errors.Errorf("first node still has %d replicas", count) - log.Info(ctx, err) - return err - } - return nil - }); err != nil { - t.Fatal(err) - } - - log.Infof(ctx, "killing all nodes") - for i := 0; i < num; i++ { - if err := c.Kill(ctx, i); err != nil { - t.Fatal(err) - } - } - - log.Infof(ctx, "restarting only first node so that it writes its advertised address") - - // Note that at this point, the first node has no join flags and no useful - // persisted Gossip info (since all other nodes are down and they'll get new - // ports when they respawn). - chs := []<-chan error{lc.RestartAsync(ctx, 0)} - - testutils.SucceedsSoon(t, func() error { - if lc.Nodes[0].AdvertiseAddr() != "" { - return nil - } - return errors.New("no advertised addr yet") - }) - - // The other nodes will be joined to the first node, so they can - // reach out to it (and in fact they will, since that's the only - // reachable peer they have -- their persisted info is outdated, too!) - log.Infof(ctx, "restarting the other nodes") - for i := 1; i < num; i++ { - chs = append(chs, lc.RestartAsync(ctx, i)) - } - - log.Infof(ctx, "waiting for healthy cluster") - for i, ch := range chs { - if err := <-ch; err != nil { - t.Errorf("error restarting node %d: %s", i, err) - } - } - - testClusterConnectedAndFunctional(ctx, t, c) -} diff --git a/pkg/acceptance/util_gossip.go b/pkg/acceptance/util_gossip.go deleted file mode 100644 index 53c0de423b37..000000000000 --- a/pkg/acceptance/util_gossip.go +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright 2015 The Cockroach Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or -// implied. See the License for the specific language governing -// permissions and limitations under the License. - -package acceptance - -import ( - "context" - "strings" - "time" - - "github.com/cockroachdb/cockroach/pkg/acceptance/cluster" - "github.com/cockroachdb/cockroach/pkg/gossip" - "github.com/cockroachdb/cockroach/pkg/util/httputil" - "github.com/cockroachdb/cockroach/pkg/util/retry" - "github.com/pkg/errors" -) - -// CheckGossipFunc is the type of callback used in CheckGossip. -type CheckGossipFunc func(map[string]gossip.Info) error - -// CheckGossip fetches the gossip infoStore from each node and invokes the given -// function. The test passes if the function returns 0 for every node, -// retrying for up to the given duration. -func CheckGossip(ctx context.Context, c cluster.Cluster, d time.Duration, f CheckGossipFunc) error { - return errors.Wrapf(retry.ForDuration(d, func() error { - var infoStatus gossip.InfoStatus - for i := 0; i < c.NumNodes(); i++ { - if err := httputil.GetJSON(cluster.HTTPClient, c.URL(ctx, i)+"/_status/gossip/local", &infoStatus); err != nil { - return errors.Wrapf(err, "failed to get gossip status from node %d", i) - } - if err := f(infoStatus.Infos); err != nil { - return errors.Wrapf(err, "node %d", i) - } - } - - return nil - }), "condition failed to evaluate within %s", d) -} - -// HasPeers returns a CheckGossipFunc that passes when the given -// number of peers are connected via gossip. -func HasPeers(expected int) CheckGossipFunc { - return func(infos map[string]gossip.Info) error { - count := 0 - for k := range infos { - if strings.HasPrefix(k, "node:") { - count++ - } - } - if count != expected { - return errors.Errorf("expected %d peers, found %d", expected, count) - } - return nil - } -} - -// hasSentinel is a checkGossipFunc that passes when the sentinel gossip is present. -func hasSentinel(infos map[string]gossip.Info) error { - if _, ok := infos[gossip.KeySentinel]; !ok { - return errors.Errorf("sentinel not found") - } - return nil -} - -// hasClusterID is a checkGossipFunc that passes when the cluster ID gossip is present. -func hasClusterID(infos map[string]gossip.Info) error { - if _, ok := infos[gossip.KeyClusterID]; !ok { - return errors.Errorf("cluster ID not found") - } - return nil -} diff --git a/pkg/cmd/roachtest/acceptance.go b/pkg/cmd/roachtest/acceptance.go index aca98a5a05d8..6e7e9050ffec 100644 --- a/pkg/cmd/roachtest/acceptance.go +++ b/pkg/cmd/roachtest/acceptance.go @@ -38,7 +38,7 @@ func registerAcceptance(r *registry) { {"event-log", runEventLog}, {"gossip/peerings", runGossipPeerings}, {"gossip/restart", runGossipRestart}, - {"gossip/restart-first-node-needs-incoming", runGossipRestartFirstNodeNeedsIncoming}, + {"gossip/restart-node-one", runGossipRestartNodeOne}, {"rapid-restart", runRapidRestart}, {"status-server", runStatusServer}, } diff --git a/pkg/cmd/roachtest/gossip.go b/pkg/cmd/roachtest/gossip.go index a09027604101..e8021becb40a 100644 --- a/pkg/cmd/roachtest/gossip.go +++ b/pkg/cmd/roachtest/gossip.go @@ -19,7 +19,10 @@ import ( "context" gosql "database/sql" "fmt" + "net" "net/http" + "net/url" + "strconv" "strings" "time" "unicode" @@ -126,16 +129,18 @@ SELECT string_agg(source_id::TEXT || ':' || target_id::TEXT, ',') type gossipUtil struct { waitTime time.Duration urlMap map[int]string + conn func(ctx context.Context, i int) *gosql.DB } -func newGossipUtil(ctx context.Context, c *cluster, waitTime time.Duration) *gossipUtil { +func newGossipUtil(ctx context.Context, c *cluster) *gossipUtil { urlMap := make(map[int]string) for i, addr := range c.ExternalAdminUIAddr(ctx, c.All()) { urlMap[i+1] = `http://` + addr } return &gossipUtil{ - waitTime: waitTime, + waitTime: 30 * time.Second, urlMap: urlMap, + conn: c.Conn, } } @@ -207,7 +212,7 @@ func (g *gossipUtil) checkConnectedAndFunctional(ctx context.Context, t *test, c } for i := 1; i <= c.nodes; i++ { - db := c.Conn(ctx, i) + db := g.conn(ctx, i) defer db.Close() if i == 1 { if _, err := db.Exec("CREATE DATABASE IF NOT EXISTS test"); err != nil { @@ -246,7 +251,7 @@ func runGossipPeerings(ctx context.Context, t *test, c *cluster) { // Repeatedly restart a random node and verify that all of the nodes are // seeing the gossiped values. - g := newGossipUtil(ctx, c, 30*time.Second) + g := newGossipUtil(ctx, c) deadline := timeutil.Now().Add(time.Minute) for i := 1; timeutil.Now().Before(deadline); i++ { @@ -278,7 +283,7 @@ func runGossipRestart(ctx context.Context, t *test, c *cluster) { // which is required for any node to be able do even the most basic // operations on a cluster. - g := newGossipUtil(ctx, c, 30*time.Second) + g := newGossipUtil(ctx, c) deadline := timeutil.Now().Add(time.Minute) for i := 1; timeutil.Now().Before(deadline); i++ { @@ -293,5 +298,98 @@ func runGossipRestart(ctx context.Context, t *test, c *cluster) { } } -func runGossipRestartFirstNodeNeedsIncoming(ctx context.Context, t *test, c *cluster) { +func runGossipRestartNodeOne(ctx context.Context, t *test, c *cluster) { + c.Put(ctx, cockroach, "./cockroach") + c.Start(ctx, racks(c.nodes)) + + db := c.Conn(ctx, 1) + defer db.Close() + + run := func(stmt string) { + c.l.Printf("%s\n", stmt) + if _, err := db.ExecContext(ctx, stmt); err != nil { + t.Fatal(err) + } + } + + // Evacuate all of the ranges off node 1 with zone config constraints. See + // the racks setting specified when the cluster was started. + run(`ALTER RANGE default EXPERIMENTAL CONFIGURE ZONE 'constraints: {"-rack=0"}'`) + run(`ALTER RANGE meta EXPERIMENTAL CONFIGURE ZONE 'constraints: {"-rack=0"}'`) + run(`ALTER RANGE liveness EXPERIMENTAL CONFIGURE ZONE 'constraints: {"-rack=0"}'`) + + var lastCount int + if err := retry.ForDuration(2*time.Minute, func() error { + const query = ` +SELECT count(replicas) + FROM crdb_internal.ranges + WHERE array_position(replicas, 1) IS NOT NULL +` + var count int + if err := db.QueryRow(query).Scan(&count); err != nil { + t.Fatal(err) + } + if count > 0 { + err := errors.Errorf("node 1 still has %d replicas", count) + if count != lastCount { + lastCount = count + c.l.Printf("%s\n", err) + } + return err + } + return nil + }); err != nil { + t.Fatal(err) + } + + c.l.Printf("killing all nodes\n") + c.Stop(ctx) + + // Restart node 1, but have it listen on a different port for internal + // connections. This will require node 1 to reach out to the other nodes in + // the cluster for gossip info. + err := c.RunE(ctx, c.Node(1), + `./cockroach start --insecure --background --store={store-dir} `+ + `--log-dir={log-dir} --cache=10% --max-sql-memory=10% `+ + `--listen-addr=:$[{pgport:1}+10000] --http-port=$[{pgport:1}+1] `+ + `> {log-dir}/cockroach.stdout 2> {log-dir}/cockroach.stderr`) + if err != nil { + t.Fatal(err) + } + + // Restart the other nodes. These nodes won't be able to talk to node 1 until + // node 1 talks to it (they have out of date address info). Node 1 needs + // incoming gossip info in order to determine where range 1 is. + c.Start(ctx, c.Range(2, c.nodes)) + + // We need to override DB connection creation to use the correct port for + // node 1. This is more complicated than it should be and a limitation of the + // current infrastructure which doesn't know about cockroach nodes started on + // non-standard ports. + g := newGossipUtil(ctx, c) + g.conn = func(ctx context.Context, i int) *gosql.DB { + if i != 1 { + return c.Conn(ctx, i) + } + url, err := url.Parse(c.ExternalPGUrl(ctx, c.Node(1))[0]) + if err != nil { + t.Fatal(err) + } + host, port, err := net.SplitHostPort(url.Host) + if err != nil { + t.Fatal(err) + } + v, err := strconv.Atoi(port) + if err != nil { + t.Fatal(err) + } + url.Host = fmt.Sprintf("%s:%d", host, v+10000) + db, err := gosql.Open("postgres", url.String()) + if err != nil { + t.Fatal(err) + } + return db + } + + g.checkConnectedAndFunctional(ctx, t, c) } From ffcbb732ea7c965ebe0b0aa631e6c6032406d797 Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Sat, 1 Sep 2018 11:03:34 -0400 Subject: [PATCH 4/7] roachtest: give acceptance tests a 10m timeout Release note: None --- pkg/cmd/roachtest/acceptance.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/roachtest/acceptance.go b/pkg/cmd/roachtest/acceptance.go index 6e7e9050ffec..4d80803ed12f 100644 --- a/pkg/cmd/roachtest/acceptance.go +++ b/pkg/cmd/roachtest/acceptance.go @@ -15,7 +15,10 @@ package main -import "context" +import ( + "context" + "time" +) func registerAcceptance(r *registry) { // The acceptance tests all share a 4-node cluster and run sequentially. In @@ -45,8 +48,9 @@ func registerAcceptance(r *registry) { for _, tc := range testCases { tc := tc spec.SubTests = append(spec.SubTests, testSpec{ - Name: tc.name, - Stable: true, // DO NOT COPY to new tests + Name: tc.name, + Timeout: 10 * time.Minute, + Stable: true, // DO NOT COPY to new tests Run: func(ctx context.Context, t *test, c *cluster) { c.Wipe(ctx) tc.fn(ctx, t, c) From 21b961b3fc445dd8fa6280331ea7bb6e2300417b Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Sat, 1 Sep 2018 12:47:29 -0400 Subject: [PATCH 5/7] roachtest: avoid flakiness in acceptance/rapid-restarts on remote clusters Release note: None --- pkg/cmd/roachtest/rapid_restart.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/roachtest/rapid_restart.go b/pkg/cmd/roachtest/rapid_restart.go index 7c4a6244b66c..b8f02167b7b3 100644 --- a/pkg/cmd/roachtest/rapid_restart.go +++ b/pkg/cmd/roachtest/rapid_restart.go @@ -60,7 +60,19 @@ func runRapidRestart(ctx context.Context, t *test, c *cluster) { break } - time.Sleep(time.Duration(rand.Int63n(int64(time.Second)))) + waitTime := time.Duration(rand.Int63n(int64(time.Second))) + if !c.isLocal() { + // TODO(peter): This is hacky: the signal might be sent before the + // cockroach process starts, which is especially true on remote + // clusters. Perhaps combine this with a monitor so that we can detect + // as soon as the process starts before killing it. Or a custom kill + // script which loops looking for a cockroach process and kills it as + // soon as it appears. Using --pid_file or --background isn't quite + // right as we want to be able to kill the process before it is ready. + waitTime += time.Second + } + time.Sleep(waitTime) + sig := [2]string{"2", "9"}[rand.Intn(2)] c.Stop(ctx, nodes, stopArgs("--sig="+sig)) select { From f65fcf993d9164ecaa5a30b43d41a7124c36de1c Mon Sep 17 00:00:00 2001 From: Peter Mattis Date: Sat, 1 Sep 2018 13:02:05 -0400 Subject: [PATCH 6/7] roachtest: acceptance/cli/node-status requires a 3 node cluster Release note: None --- pkg/cmd/roachtest/cli.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/roachtest/cli.go b/pkg/cmd/roachtest/cli.go index 43494895f6f6..e161394a2894 100644 --- a/pkg/cmd/roachtest/cli.go +++ b/pkg/cmd/roachtest/cli.go @@ -24,7 +24,7 @@ import ( func runCLINodeStatus(ctx context.Context, t *test, c *cluster) { c.Put(ctx, cockroach, "./cockroach") - c.Start(ctx) + c.Start(ctx, c.Range(1, 3)) db := c.Conn(ctx, 1) defer db.Close() From 90df1f1b265ec72f9d23c33f6bd21e0cc9992f76 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Thu, 30 Aug 2018 02:23:20 -0400 Subject: [PATCH 7/7] storage: fix a nasty merge deadlock Fix a nasty edge case which could cause a concurrent merge and split to deadlock. See the comment on TestStoreRangeMergeConcurrentSplit for details. Release note: None --- pkg/storage/client_merge_test.go | 73 ++++++++++++++++++++++++++++++++ pkg/storage/replica.go | 65 ++++++++++++++++------------ 2 files changed, 112 insertions(+), 26 deletions(-) diff --git a/pkg/storage/client_merge_test.go b/pkg/storage/client_merge_test.go index d535cc0701da..2eccedb96f23 100644 --- a/pkg/storage/client_merge_test.go +++ b/pkg/storage/client_merge_test.go @@ -980,6 +980,79 @@ func TestStoreRangeMergeInFlightTxns(t *testing.T) { }) } +// TestStoreRangeMergeConcurrentSplit (occasionally) reproduces a race where a +// concurrent merge and split could deadlock. +// +// The bug works like this. A merge of adjacent ranges P and Q and a split of Q +// execute concurrently, though the merge executes with an earlier timestamp. +// The merge updates Q's meta2 range descriptor. The split updates Q's local +// range descriptor, then tries to update Q's meta2 range descriptor, but runs +// into the merge's intent and attempts to push the merge. Under our current +// concurrency control strategy, this results in the split waiting for the merge +// to complete. The merge then tries to update Q's local range descriptor but +// runs into the split's intent. While pushing the split, the merge realizes +// that waiting for the split to complete would cause deadlock, so it aborts the +// split instead. +// +// But before the split can clean up its transaction record and intents, the +// merge locks Q and launches a goroutine to unlock Q when the merge commits. +// Then the merge completes, which has a weird side effect: the split's push of +// the merge will succeed! How is this possible? The split's push request is not +// guaranteed to notice that the split has been aborted before it notices that +// the merge has completed. So the aborted split winds up resolving the merge's +// intent on Q's meta2 range descriptor and leaving its own intent in its place. +// +// In the past, the merge watcher goroutine would perform a range lookup for Q; +// this would indirectly wait for the merge to complete by waiting for its +// intent in meta2 to be resolved. In this case, however, its the *split*'s +// intent that the watcher goroutine sees. This intent can't be resolved because +// the split's transaction record is located on the locked range Q! And so Q can +// never be unlocked. +// +// This bug was fixed by teaching the watcher goroutine to push the merge +// transaction directly instead of doing so indirectly by querying meta2. +// +// Attempting a foolproof reproduction of the bug proved challenging and would +// have required a mess of store filters. This test takes a simpler approach of +// running the necessary split and a merge concurrently and allowing the race +// scheduler to occasionally strike the right interleaving. At the time of +// writing, the test would reliably reproduce the bug in about 50 runs (about +// ten seconds of stress on an eight core laptop). +func TestStoreRangeMergeConcurrentSplit(t *testing.T) { + defer leaktest.AfterTest(t)() + + ctx := context.Background() + storeCfg := storage.TestStoreConfig(nil) + storeCfg.TestingKnobs.DisableReplicateQueue = true + mtc := &multiTestContext{storeConfig: &storeCfg} + mtc.Start(t, 1) + defer mtc.Stop() + distSender := mtc.distSenders[0] + + lhsDesc, rhsDesc, err := createSplitRanges(ctx, mtc.Store(0)) + if err != nil { + t.Fatal(err) + } + + splitErrCh := make(chan error) + go func() { + time.Sleep(10 * time.Millisecond) + splitArgs := adminSplitArgs(rhsDesc.StartKey.AsRawKey().Next()) + _, pErr := client.SendWrapped(ctx, distSender, splitArgs) + splitErrCh <- pErr.GoError() + }() + + mergeArgs := adminMergeArgs(lhsDesc.StartKey.AsRawKey()) + _, pErr := client.SendWrapped(ctx, distSender, mergeArgs) + if pErr != nil && !testutils.IsPError(pErr, "range changed during merge") { + t.Fatal(pErr) + } + + if err := <-splitErrCh; err != nil { + t.Fatal(err) + } +} + // TestStoreRangeMergeRHSLeaseExpiration verifies that, if the right-hand range // in a merge loses its lease while a merge is in progress, the new leaseholder // does not incorrectly serve traffic before the merge completes. diff --git a/pkg/storage/replica.go b/pkg/storage/replica.go index d998c6557760..bb4d7eaa2109 100644 --- a/pkg/storage/replica.go +++ b/pkg/storage/replica.go @@ -27,7 +27,6 @@ import ( "time" "unsafe" - "github.com/cockroachdb/cockroach/pkg/storage/rditer" "github.com/coreos/etcd/raft" "github.com/coreos/etcd/raft/raftpb" "github.com/google/btree" @@ -51,6 +50,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/storage/engine" "github.com/cockroachdb/cockroach/pkg/storage/engine/enginepb" "github.com/cockroachdb/cockroach/pkg/storage/rangefeed" + "github.com/cockroachdb/cockroach/pkg/storage/rditer" "github.com/cockroachdb/cockroach/pkg/storage/spanset" "github.com/cockroachdb/cockroach/pkg/storage/stateloader" "github.com/cockroachdb/cockroach/pkg/storage/storagebase" @@ -61,6 +61,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/humanizeutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/protoutil" + "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -2824,33 +2825,45 @@ func (r *Replica) maybeWatchForMerge(ctx context.Context) error { taskCtx := r.AnnotateCtx(context.Background()) err = r.store.stopper.RunAsyncTask(taskCtx, "wait-for-merge", func(ctx context.Context) { - rs, _, err := client.RangeLookup(ctx, r.DB().NonTransactionalSender(), desc.StartKey.AsRawKey(), - roachpb.CONSISTENT, 0 /* prefetchNum */, false /* reverse */) - if err != nil { - select { - case <-r.store.stopper.ShouldQuiesce(): - // The server is shutting down. The error while fetching the range - // descriptor was probably caused by the shutdown, so ignore it. - return - default: - // Otherwise, this replica is good and truly hosed because we couldn't - // determine its true range descriptor. - // - // TODO(benesch): a retry loop would be better than fataling, but we - // want to smoke out any unexpected errors at first. - log.Fatal(ctx, err) + for retry := retry.Start(base.DefaultRetryOptions()); retry.Next(); { + // Wait for the merge transaction to complete by attempting to push it. We + // don't want to accidentally abort the merge transaction, so we use the + // minimum transaction priority. Note that a push type of + // roachpb.PUSH_TOUCH, though it might appear more semantically correct, + // returns immediately and causes us to spin hot, whereas + // roachpb.PUSH_ABORT efficiently blocks until the transaction completes. + _, pErr := client.SendWrapped(ctx, r.DB().NonTransactionalSender(), &roachpb.PushTxnRequest{ + RequestHeader: roachpb.RequestHeader{Key: intents[0].Txn.Key}, + PusherTxn: roachpb.Transaction{ + TxnMeta: enginepb.TxnMeta{Priority: roachpb.MinTxnPriority}, + }, + PusheeTxn: intents[0].Txn, + Now: r.Clock().Now(), + PushType: roachpb.PUSH_ABORT, + }) + if pErr != nil { + select { + case <-r.store.stopper.ShouldQuiesce(): + // The server is shutting down. The error while fetching the range + // descriptor was probably caused by the shutdown, so ignore it. + return + default: + log.Warningf(ctx, "error while watching for merge to complete: %s", pErr) + // We can't safely unblock traffic until we can prove that the merge + // transaction is committed or aborted. Nothing to do but try again. + continue + } } + // Unblock pending requests. If the merge committed, the requests will + // notice that the replica has been destroyed and return an appropriate + // error. If the merge aborted, the requests will be handled normally. + r.mu.Lock() + r.mu.mergeComplete = nil + close(mergeCompleteCh) + r.mu.Unlock() + return } - if len(rs) != 1 { - log.Fatalf(ctx, "expected 1 range descriptor, got %d", len(rs)) - } - // Unblock pending requests. If the merge committed, the requests will - // notice that the replica has been destroyed and return an appropriate - // error. If the merge aborted, the requests will be handled normally. - r.mu.Lock() - r.mu.mergeComplete = nil - close(mergeCompleteCh) - r.mu.Unlock() + log.Fatal(ctx, "unreachable") }) if err == stop.ErrUnavailable { // We weren't able to launch a goroutine to watch for the merge's completion