From d0120ff76aa41d4624c6d61453757ecf58eb2372 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Fri, 17 Mar 2023 01:17:36 +0000 Subject: [PATCH 1/4] roachtest: enable kv/gracefuldraining/nodes=3 This commit re-enables the `kv/gracefuldraining/nodes=3` roachtest. The test is still likely to fail occasionally however has produced interesting findings just in testing to re-enable. Informs: #59094 Release note: None --- pkg/cmd/roachtest/tests/kv.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/roachtest/tests/kv.go b/pkg/cmd/roachtest/tests/kv.go index 6776749ecf1b..1cdc9ad59ad7 100644 --- a/pkg/cmd/roachtest/tests/kv.go +++ b/pkg/cmd/roachtest/tests/kv.go @@ -463,7 +463,6 @@ func registerKVGracefulDraining(r registry.Registry) { r.Add(registry.TestSpec{ Name: "kv/gracefuldraining/nodes=3", Owner: registry.OwnerKV, - Skip: "https://github.com/cockroachdb/cockroach/issues/59094", Cluster: r.MakeClusterSpec(4), Run: func(ctx context.Context, t test.Test, c cluster.Cluster) { nodes := c.Spec().NodeCount - 1 From 071876a92970099abee21d8fe3d37996099a197a Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Fri, 5 May 2023 19:06:24 +0000 Subject: [PATCH 2/4] storepool: limit storelist string to 2 decimals The store list string returned the mean leases, ranges and queries-per-second float values without limiting the number of decimal places. This led to log lines with needlessly long decimals: `avg-ranges=40.66666666666667... avg-leases=10.166666666666666...` This commit updates the store list string formatting to 2 decimal places for float values. Release note: None --- pkg/kv/kvserver/allocator/storepool/store_pool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kv/kvserver/allocator/storepool/store_pool.go b/pkg/kv/kvserver/allocator/storepool/store_pool.go index d6ba4881ea47..de3b33194af1 100644 --- a/pkg/kv/kvserver/allocator/storepool/store_pool.go +++ b/pkg/kv/kvserver/allocator/storepool/store_pool.go @@ -1087,7 +1087,7 @@ func MakeStoreList(descriptors []roachpb.StoreDescriptor) StoreList { func (sl StoreList) String() string { var buf bytes.Buffer fmt.Fprintf(&buf, - " candidate: avg-ranges=%v avg-leases=%v avg-disk-usage=%v avg-queries-per-second=%v avg-store-cpu-per-second=%v", + " candidate: avg-ranges=%.2f avg-leases=%.2f avg-disk-usage=%s avg-queries-per-second=%.2f avg-store-cpu-per-second=%s", sl.CandidateRanges.Mean, sl.CandidateLeases.Mean, humanizeutil.IBytes(int64(sl.candidateLogicalBytes.Mean)), From 8da1ceac8682c2b2b08a8957d93d526076edc100 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Fri, 5 May 2023 18:22:37 +0000 Subject: [PATCH 3/4] kvserver: annotate rebalance ctx with objective Previously, the easiest method of determining the current rebalance objective from logs was to view the cluster setting and check for logging indicating a mixed version cluster - this was cumbersome. This commit annotates the ctx in the store rebalancer loop with an additional tag: `obj`. The `obj` tag indicates the current rebalance objective, either `cpu` or `qps` currently. resolves: #102812 Release note: None --- pkg/kv/kvserver/rebalance_objective.go | 16 ++++++++++++---- pkg/kv/kvserver/store_rebalancer.go | 3 +++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/pkg/kv/kvserver/rebalance_objective.go b/pkg/kv/kvserver/rebalance_objective.go index 3514f3c4fc99..268db3fb8c23 100644 --- a/pkg/kv/kvserver/rebalance_objective.go +++ b/pkg/kv/kvserver/rebalance_objective.go @@ -92,6 +92,17 @@ const ( LBRebalancingCPU ) +// LoadBasedRebalancingObjectiveMap maps the LoadBasedRebalancingObjective enum +// value to a string. +var LoadBasedRebalancingObjectiveMap map[int64]string = map[int64]string{ + int64(LBRebalancingQueries): "qps", + int64(LBRebalancingCPU): "cpu", +} + +func (lbro LBRebalancingObjective) String() string { + return LoadBasedRebalancingObjectiveMap[int64(lbro)] +} + // LoadBasedRebalancingObjective is a cluster setting that defines the load // balancing objective of the cluster. var LoadBasedRebalancingObjective = settings.RegisterEnumSetting( @@ -101,10 +112,7 @@ var LoadBasedRebalancingObjective = settings.RegisterEnumSetting( "the cluster will attempt to balance qps among stores, if set to "+ "`cpu` the cluster will attempt to balance cpu usage among stores", "cpu", - map[int64]string{ - int64(LBRebalancingQueries): "qps", - int64(LBRebalancingCPU): "cpu", - }, + LoadBasedRebalancingObjectiveMap, ).WithPublic() // ToDimension returns the equivalent allocator load dimension of a rebalancing diff --git a/pkg/kv/kvserver/store_rebalancer.go b/pkg/kv/kvserver/store_rebalancer.go index a7b1082c7782..5bd40e08a72c 100644 --- a/pkg/kv/kvserver/store_rebalancer.go +++ b/pkg/kv/kvserver/store_rebalancer.go @@ -282,6 +282,9 @@ func (sr *StoreRebalancer) Start(ctx context.Context, stopper *stop.Stopper) { continue } objective := sr.RebalanceObjective() + sr.AddLogTag("obj", objective) + ctx = sr.AnnotateCtx(ctx) + hottestRanges := sr.replicaRankings.TopLoad(objective.ToDimension()) options := sr.scorerOptions(ctx, objective.ToDimension()) rctx := sr.NewRebalanceContext(ctx, options, hottestRanges, mode) From 7d0f283c03aff9627b4d334ba5a43176eee6b406 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Tue, 18 Apr 2023 14:38:52 +0100 Subject: [PATCH 4/4] streamingccl: don't require TLS certificates Users may want to use password auth to simplify their replication setup. While we may recommend TLS certificate auth, I don't see a strong reason to _require_ it. Epic: none Release note: None --- .../streamingest/stream_ingestion_planning.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/ccl/streamingccl/streamingest/stream_ingestion_planning.go b/pkg/ccl/streamingccl/streamingest/stream_ingestion_planning.go index 43d7cf24b514..88c76a2aa04f 100644 --- a/pkg/ccl/streamingccl/streamingest/stream_ingestion_planning.go +++ b/pkg/ccl/streamingccl/streamingest/stream_ingestion_planning.go @@ -166,16 +166,6 @@ func ingestionPlanHook( if err != nil { return err } - q := streamURL.Query() - - // Operator should specify a postgres scheme address with cert authentication. - if hasPostgresAuthentication := (q.Get("sslmode") == "verify-full") && - q.Has("sslrootcert") && q.Has("sslkey") && q.Has("sslcert"); (streamURL.Scheme == "postgres") && - !hasPostgresAuthentication { - return errors.Errorf( - "stream replication address should have cert authentication if in postgres scheme: %s", streamAddress) - } - streamAddress = streamingccl.StreamAddress(streamURL.String()) // TODO(adityamaru): Add privileges checks. Probably the same as RESTORE.