Merge #59146 #59327 #59350

59146: bazel: generate `//go:generate stringer` files sandbox r=rickystewart a=alan-mas First part of #57787 work. As `gazelle` is not taking care by itself in any changes related to `//go:generate stringer` we are creating a `genrule` to handle it. From all the `//go:generate stringer` files, there are some that are having troubles during bazel build: ``` -pkg/util/encoding/encoding.go -pkg/util/encoding/BUILD.bazel "stringer: can't handle non-integer constant type Type" -pkg/workload/schemachange/schemachange.go -pkg/workload/schemachange/BUILD.bazel "stringer: can't happen: constant is not an integer TxStatusInFailure" ``` Release note: None 59327: build,bazel: inject nodejs support into our bazel setup r=rickystewart a=rickystewart Use the standard Google-supported Bazel tools to do so. This isn't plugged into anything yet, but you can verify that it works with `bazel query @npm//...`. Release note: None 59350: server: wait for SQL readiness in the `/health?ready=1` probe r=joshimhoff a=knz (smaller version than #59191) Fixes #58864. Planning to backport to v20.2. Release note (api change): the health API now checks that the SQL server is ready to accept clients when a readiness check is requested. Co-authored-by: Alanmas <[email protected]> Co-authored-by: Ricky Stewart <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Jan 25, 2021 · bf439cd · bf439cd
4 parents fad50c1 + ef93819 + 6882bb5 + 3f0c542
commit bf439cd
Show file tree

Hide file tree

Showing 29 changed files with 390 additions and 5 deletions.
diff --git a/WORKSPACE b/WORKSPACE
@@ -1,6 +1,9 @@
 # Define the top level namespace. This lets everything be addressable using
 # `@cockroach//...`.
-workspace(name = "cockroach")
+workspace(
+    name = "cockroach",
+    managed_directories = {"@npm": ["node_modules"]},
+)
 
 # Load the things that let us load other things.
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
@@ -16,6 +19,13 @@ http_archive(
     ],
 )
 
+# Like the above, but for nodeJS.
+http_archive(
+    name = "build_bazel_rules_nodejs",
+    sha256 = "6142e9586162b179fdd570a55e50d1332e7d9c030efd853453438d607569721d",
+    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/3.0.0/rules_nodejs-3.0.0.tar.gz"],
+)
+
 # Load gazelle. This lets us auto-generate BUILD.bazel files throughout the
 # repo.
 #
@@ -82,6 +92,15 @@ go_rules_dependencies()
 
 go_register_toolchains(go_version = "1.15.6")
 
+# Configure nodeJS.
+load("@build_bazel_rules_nodejs//:index.bzl", "yarn_install")
+
+yarn_install(
+    name = "npm",
+    package_json = "//pkg/ui:package.json",
+    yarn_lock = "//pkg/ui:yarn.lock",
+)
+
 # NB: @bazel_skylib comes from go_rules_dependencies().
 load("@bazel_skylib//lib:versions.bzl", "versions")
 

diff --git a/pkg/STRINGER.bzl b/pkg/STRINGER.bzl
@@ -0,0 +1,18 @@
+# Common function to create //go:generate stringer files within bazel sandbox
+
+def stringer(file, typ, name):
+   native.genrule(
+      name = name, 
+      srcs = [
+          file,
+      ],
+      outs = [typ.lower() + "_string.go"],
+      cmd = """
+         env PATH=`dirname $(location @go_sdk//:bin/go)` HOME=$(GENDIR) \
+         $(location @org_golang_x_tools//cmd/stringer:stringer) -output=$@ -type={} $<
+      """.format(typ),
+      tools = [
+         "@go_sdk//:bin/go",
+         "@org_golang_x_tools//cmd/stringer",
+       ],
+   )
diff --git a/pkg/base/BUILD.bazel b/pkg/base/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "base",
@@ -14,6 +15,7 @@ go_library(
         "testclusterreplicationmode_string.go",
         "testing_knobs.go",
         "zone.go",
+        ":gen-clusterreplication-stringer",  # keep
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/base",
     visibility = ["//visibility:public"],
@@ -64,3 +66,9 @@ go_test(
         "@com_github_stretchr_testify//require",
     ],
 )
+
+stringer(
+    name = "gen-clusterreplication-stringer",
+    file = "test_server_args.go",
+    typ = "TestClusterReplicationMode",
+)
diff --git a/pkg/ccl/sqlproxyccl/BUILD.bazel b/pkg/ccl/sqlproxyccl/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "sqlproxyccl",
@@ -12,6 +13,7 @@ go_library(
         "metrics.go",
         "proxy.go",
         "server.go",
+        ":gen-errorcode-stringer",  # keep
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl",
     visibility = ["//visibility:public"],
@@ -58,3 +60,9 @@ go_test(
         "@com_github_stretchr_testify//require",
     ],
 )
+
+stringer(
+    name = "gen-errorcode-stringer",
+    file = "error.go",
+    typ = "ErrorCode",
+)
diff --git a/pkg/cli/BUILD.bazel b/pkg/cli/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "cli",
@@ -51,6 +52,7 @@ go_library(
         "tsdump.go",
         "userfile.go",
         "zip.go",
+        ":gen-keytype-stringer",  # keep
     ],
     # keep
     cdeps = [
@@ -336,3 +338,9 @@ go_test(
         "@com_github_stretchr_testify//require",
     ],
 )
+
+stringer(
+    name = "gen-keytype-stringer",
+    file = "flags_util.go",
+    typ = "keyType",
+)
diff --git a/pkg/clusterversion/BUILD.bazel b/pkg/clusterversion/BUILD.bazel
@@ -1,6 +1,7 @@
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "clusterversion",
@@ -11,6 +12,7 @@ go_library(
         "keyed_versions.go",
         "setting.go",
         "testutils.go",
+        ":gen-key-stringer",  # keep
     ],
     embed = [":clusterversion_go_proto"],
     importpath = "github.com/cockroachdb/cockroach/pkg/clusterversion",
@@ -61,3 +63,9 @@ go_proto_library(
         "@com_github_gogo_protobuf//gogoproto",
     ],
 )
+
+stringer(
+    name = "gen-key-stringer",
+    file = "cockroach_versions.go",
+    typ = "Key",
+)
diff --git a/pkg/kv/kvclient/kvcoord/BUILD.bazel b/pkg/kv/kvclient/kvcoord/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "kvcoord",
@@ -28,6 +29,7 @@ go_library(
         "txn_lock_gatekeeper.go",
         "txn_metrics.go",
         "txnstate_string.go",
+        ":gen-txnstate-stringer",  # keep
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord",
     visibility = ["//visibility:public"],
@@ -149,3 +151,9 @@ go_test(
         "@org_golang_x_sync//errgroup",
     ],
 )
+
+stringer(
+    name = "gen-txnstate-stringer",
+    file = "txn_coord_sender.go",
+    typ = "txnState",
+)
diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel
@@ -1,6 +1,7 @@
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "kvserver",
@@ -91,6 +92,7 @@ go_library(
         "testing_knobs.go",
         "track_raft_protos.go",
         "ts_maintenance_queue.go",
+        ":gen-refreshraftreason-stringer",  # keep
     ],
     embed = [":kvserver_go_proto"],
     importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver",
@@ -427,3 +429,9 @@ go_proto_library(
         "@io_etcd_go_etcd_raft_v3//raftpb",
     ],
 )
+
+stringer(
+    name = "gen-refreshraftreason-stringer",
+    file = "replica_raft.go",
+    typ = "refreshRaftReason",
+)
diff --git a/pkg/roachpb/BUILD.bazel b/pkg/roachpb/BUILD.bazel
@@ -1,6 +1,7 @@
 load("@rules_proto//proto:defs.bzl", "proto_library")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "roachpb",
@@ -21,6 +22,8 @@ go_library(
         "span_group.go",
         "tenant.go",
         "version.go",
+        ":gen-errordetailtype-stringer",  # keep
+        ":gen-method-stringer",  # keep
     ],
     embed = [":roachpb_go_proto"],
     importpath = "github.com/cockroachdb/cockroach/pkg/roachpb",
@@ -146,3 +149,17 @@ go_proto_library(
         "@com_github_gogo_protobuf//gogoproto",
     ],
 )
+
+# Using common function for stringer to create method_string.go
+stringer(
+    name = "gen-method-stringer",
+    file = "method.go",
+    typ = "Method",
+)
+
+# Using common function for stringer to create errordetailtype_string.go
+stringer(
+    name = "gen-errordetailtype-stringer",
+    file = "errors.go",
+    typ = "ErrorDetailType",
+)
diff --git a/pkg/server/admin.go b/pkg/server/admin.go
@@ -1358,13 +1358,13 @@ func (s *adminServer) Health(
 		return resp, nil
 	}
 
-	if err := s.checkReadinessForHealthCheck(); err != nil {
+	if err := s.checkReadinessForHealthCheck(ctx); err != nil {
 		return nil, err
 	}
 	return resp, nil
 }
 
-func (s *adminServer) checkReadinessForHealthCheck() error {
+func (s *adminServer) checkReadinessForHealthCheck(ctx context.Context) error {
 	serveMode := s.server.grpc.mode.get()
 	switch serveMode {
 	case modeInitializing:
@@ -1397,6 +1397,10 @@ func (s *adminServer) checkReadinessForHealthCheck() error {
 		return status.Errorf(codes.Unavailable, "node is shutting down")
 	}
 
+	if !s.server.sqlServer.acceptingClients.Get() {
+		return status.Errorf(codes.Unavailable, "node is not accepting SQL clients")
+	}
+
 	return nil
 }
 

diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go
@@ -1355,6 +1355,7 @@ func TestHealthAPI(t *testing.T) {
 
 	s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(ctx)
+	ts := s.(*TestServer)
 
 	// We need to retry because the node ID isn't set until after
 	// bootstrapping.
@@ -1363,15 +1364,26 @@ func TestHealthAPI(t *testing.T) {
 		return getAdminJSONProto(s, "health", &resp)
 	})
 
+	// Make the SQL listener appear unavailable. Verify that health fails after that.
+	ts.sqlServer.acceptingClients.Set(false)
+	var resp serverpb.HealthResponse
+	err := getAdminJSONProto(s, "health?ready=1", &resp)
+	if err == nil {
+		t.Error("server appears ready even though SQL listener is not")
+	}
+	ts.sqlServer.acceptingClients.Set(true)
+	err = getAdminJSONProto(s, "health?ready=1", &resp)
+	if err != nil {
+		t.Errorf("server not ready after SQL listener is ready again: %v", err)
+	}
+
 	// Expire this node's liveness record by pausing heartbeats and advancing the
 	// server's clock.
-	ts := s.(*TestServer)
 	defer ts.nodeLiveness.PauseAllHeartbeatsForTest()()
 	self, ok := ts.nodeLiveness.Self()
 	assert.True(t, ok)
 	s.Clock().Update(self.Expiration.ToTimestamp().Add(1, 0).UnsafeToClockTimestamp())
 
-	var resp serverpb.HealthResponse
 	testutils.SucceedsSoon(t, func() error {
 		err := getAdminJSONProto(s, "health?ready=1", &resp)
 		if err == nil {

diff --git a/pkg/server/drain.go b/pkg/server/drain.go
@@ -188,6 +188,7 @@ func (s *Server) drainClients(ctx context.Context, reporter func(int, redact.Saf
 	// Mark the server as draining in a way that probes to
 	// /health?ready=1 will notice.
 	s.grpc.setMode(modeDraining)
+	s.sqlServer.acceptingClients.Set(false)
 	// Wait for drainUnreadyWait. This will fail load balancer checks and
 	// delay draining so that client traffic can move off this node.
 	time.Sleep(drainWait.Get(&s.st.SV))

diff --git a/pkg/server/server.go b/pkg/server/server.go
@@ -2073,6 +2073,9 @@ func (s *SQLServer) startServeSQL(
 			return err
 		}
 	}
+
+	s.acceptingClients.Set(true)
+
 	return nil
 }
 

diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go
@@ -70,6 +70,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	"github.com/marusama/semaphore"
@@ -111,6 +112,10 @@ type SQLServer struct {
 	// connManager is the connection manager to use to set up additional
 	// SQL listeners in AcceptClients().
 	connManager netutil.Server
+
+	// set to true when the server has started accepting client conns.
+	// Used by health checks.
+	acceptingClients syncutil.AtomicBool
 }
 
 // sqlServerOptionalKVArgs are the arguments supplied to newSQLServer which are

diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+load("//pkg:STRINGER.bzl", "stringer")
 
 go_library(
     name = "sql",
@@ -218,6 +219,10 @@ go_library(
         "zero.go",
         "zigzag_join.go",
         "zone_config.go",
+        ":gen-advancecode-stringer",  # keep
+        ":gen-nodestatus-stringer",  # keep
+        ":gen-txnevent-stringer",  # keep
+        ":gen-txntype-stringer",  # keep
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql",
     visibility = ["//visibility:public"],
@@ -588,3 +593,27 @@ go_test(
         "@org_golang_x_sync//errgroup",
     ],
 )
+
+stringer(
+    name = "gen-txnevent-stringer",
+    file = "txn_state.go",
+    typ = "txnEvent",
+)
+
+stringer(
+    name = "gen-txntype-stringer",
+    file = "txn_state.go",
+    typ = "txnType",
+)
+
+stringer(
+    name = "gen-advancecode-stringer",
+    file = "txn_state.go",
+    typ = "advanceCode",
+)
+
+stringer(
+    name = "gen-nodestatus-stringer",
+    file = "distsql_physical_planner.go",
+    typ = "NodeStatus",
+)
-Original file line number
+Diff line change
@@ Expand Up / @@ -2073,6 +2073,9 @@ func (s *SQLServer) startServeSQL( @@
     			return err
     		}
     	}
+    	s.acceptingClients.Set(true)
     	return nil
     }
@@ Expand Down @@