Skip to content

Commit c78cb00

Browse files
craig[bot]wenyihu6rafissabhishek-kediafqazi
committed
144618: kvserver: deflake TestRefreshPolicyWithVariousLatencies r=arulajmani a=wenyihu6 **kvserver: deflake TestRefreshPolicyWithVariousLatencies** TestRefreshPolicyWithVariousLatencies verifies RefreshPolicy logic using a test latencies map. It was previously flaky due to: 1. not waiting for SetSpanConfig to take effect before invoking RefreshPolicy, which could trigger the background policy refresher during the test, and 2. background RefreshPolicy calls on leaseholder replicas interfering with the test. This commit resolves both issues by: 1. using SucceedsSoon to wait for SetSpanConfig to take effect, and 2. invoking RefreshPolicy on a non-leaseholder replica instead. Epic: none Release note: none --- **kvserver: fix data race in RefreshPolicy** Previously, replica.RefreshPolicy accessed r.shMu.state.Desc directly without holding r.raftMu or r.mu. This commit fixes that by using the descriptor obtained earlier from r.DescAndSpanConfig(). Epic: none Release note: none 144698: schemachangerccl: increase polling duration for TestSubzonesRemovedByGCAfterIndexSwap r=rafiss a=rafiss fixes #144697 Release note: None 144751: ui: enhance metric display r=abhishek-kedia a=abhishek-kedia Modified the Axis component for goroutines to dynamically render Metric components based on nodeIDs Epic: None Resolves: [CRDB-48025](https://cockroachlabs.atlassian.net/browse/CRDB-48025) Resolves: [#141845](#141845) Release note: None 144770: sql/schemachanger: fix RLS policy ordering when swapping elements r=fqazi a=fqazi Previously, the order of operations when swapping row-level security (RLS) policies was not explicitly enforced, depending instead on their addition order in the builder. New rules for automatically managing `schema_locked` highlighted a risk: without strict ordering, the swap sequence could be inadvertently reversed. This patch addresses this by enforcing that RLS policy drops always occur before the corresponding adds during a swap operation. Fixes: #144767 Fixes: #144762 Release note: None Co-authored-by: wenyihu6 <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: abhishek-kedia <[email protected]> Co-authored-by: Faizan Qazi <[email protected]>
5 parents 4d33777 + e796a04 + 5af3b81 + fbaf2bd + 3396db0 commit c78cb00

File tree

7 files changed

+125
-8
lines changed

7 files changed

+125
-8
lines changed

‎pkg/ccl/schemachangerccl/schemachanger_ccl_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ CREATE TABLE person (
248248
})
249249

250250
// Keep retrying until the old index and temporary index are removed by the GC job.
251-
runner.SucceedsSoonDuration = 12 * time.Second
251+
runner.SucceedsSoonDuration = 30 * time.Second
252252
runner.CheckQueryResultsRetry(t, subzonesQuery, [][]string{
253253
{"3", "north_america", "4", `/3/"CA"`, "NULL"},
254254
{"3", "north_america", "4", `/3/"US"`, "NULL"},

‎pkg/kv/kvserver/replica.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1366,7 +1366,7 @@ func (r *Replica) RefreshPolicy(latencies map[roachpb.NodeID]time.Duration) {
13661366
// policy bucket. This then controls how far in the future timestamps will
13671367
// be closed for the range.
13681368
maxLatency := time.Duration(-1)
1369-
for _, peer := range r.shMu.state.Desc.InternalReplicas {
1369+
for _, peer := range desc.InternalReplicas {
13701370
peerLatency := closedts.DefaultMaxNetworkRTT
13711371
if latency, ok := latencies[peer.NodeID]; ok {
13721372
peerLatency = latency

‎pkg/kv/kvserver/replica_closedts_test.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1119,16 +1119,30 @@ func TestRefreshPolicyWithVariousLatencies(t *testing.T) {
11191119
defer log.Scope(t).Close(t)
11201120

11211121
ctx := context.Background()
1122-
tc := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{})
1122+
tc := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{
1123+
ReplicationMode: base.ReplicationManual,
1124+
})
11231125
defer tc.Stopper().Stop(ctx)
11241126

11251127
// Create a scratch range.
11261128
scratchKey := tc.ScratchRange(t)
1127-
store := tc.GetFirstStoreFromServer(t, 0)
1129+
tc.AddVotersOrFatal(t, scratchKey, tc.Targets(1, 2)...)
1130+
1131+
// Get a non-leaseholder replica to avoid background RefreshPolicy calls that
1132+
// would interfere with testing the policy refresh logic in isolation.
1133+
store := tc.GetFirstStoreFromServer(t, 1)
11281134
repl := store.LookupReplica(roachpb.RKey(scratchKey))
11291135
require.NotNil(t, repl)
11301136
repl.SetSpanConfig(roachpb.SpanConfig{GlobalReads: true}, roachpb.Span{Key: scratchKey})
11311137

1138+
// Verify that the range is properly configured to use global reads.
1139+
testutils.SucceedsSoon(t, func() error {
1140+
if repl.GetRangeInfo(ctx).ClosedTimestampPolicy != roachpb.LEAD_FOR_GLOBAL_READS {
1141+
return errors.New("expected LEAD_FOR_GLOBAL_READS")
1142+
}
1143+
return nil
1144+
})
1145+
11321146
// Define test cases with different latency scenarios.
11331147
testCases := []struct {
11341148
name string

‎pkg/sql/logictest/testdata/logic_test/row_level_security‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3141,3 +3141,42 @@ statement ok
31413141
DROP USER bypassrls_user;
31423142

31433143
subtest end
3144+
3145+
3146+
subtest policy_with_schema_locked
3147+
3148+
statement ok
3149+
CREATE TABLE alter_policy_table_locked (c1 INT8 NOT NULL, c2 STRING NULL, CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC), FAMILY fam_0_c1_c2 (c1, c2)) WITH (schema_locked = true);
3150+
3151+
statement ok
3152+
CREATE POLICY p ON alter_policy_table_locked WITH CHECK (TRUE);
3153+
3154+
statement ok
3155+
ALTER POLICY p ON alter_policy_table_locked RENAME TO p_sel;
3156+
3157+
query TT
3158+
SHOW CREATE TABLE alter_policy_table_locked;
3159+
----
3160+
alter_policy_table_locked CREATE TABLE public.alter_policy_table_locked (
3161+
c1 INT8 NOT NULL,
3162+
c2 STRING NULL,
3163+
CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC),
3164+
FAMILY fam_0_c1_c2 (c1, c2)
3165+
) WITH (schema_locked = true);
3166+
CREATE POLICY p_sel ON public.alter_policy_table_locked AS PERMISSIVE FOR ALL TO public WITH CHECK (true)
3167+
3168+
statement ok
3169+
ALTER POLICY p_sel ON alter_policy_table_locked WITH CHECK (FALSE);
3170+
3171+
query TT
3172+
SHOW CREATE TABLE alter_policy_table_locked;
3173+
----
3174+
alter_policy_table_locked CREATE TABLE public.alter_policy_table_locked (
3175+
c1 INT8 NOT NULL,
3176+
c2 STRING NULL,
3177+
CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC),
3178+
FAMILY fam_0_c1_c2 (c1, c2)
3179+
) WITH (schema_locked = true);
3180+
CREATE POLICY p_sel ON public.alter_policy_table_locked AS PERMISSIVE FOR ALL TO public WITH CHECK (false)
3181+
3182+
subtest end

‎pkg/sql/schemachanger/scplan/internal/rules/current/dep_create_policy.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,28 @@ func init() {
2727
},
2828
)
2929
}
30+
31+
func init() {
32+
// This rule ensures that when a policy dependent is being swapped are
33+
// done in the correct order. The dropped element should disappear before
34+
// the replacement element is added.
35+
registerDepRule(
36+
"policy dependents are swapped in order",
37+
scgraph.Precedence,
38+
"drop-policy-dependent", "add-policy-dependent",
39+
func(from, to NodeVars) rel.Clauses {
40+
return rel.Clauses{
41+
from.TypeFilter(rulesVersionKey, isPolicyDependent),
42+
to.TypeFilter(rulesVersionKey, isPolicyDependent),
43+
// Confirm we are joining on the same types.
44+
from.El.AttrEqVar(rel.Type, "sameType"),
45+
to.El.AttrEqVar(rel.Type, "sameType"),
46+
JoinOnPolicyID(from, to, "table-id", "policy-id"),
47+
from.TargetStatus(scpb.ToAbsent),
48+
from.CurrentStatus(scpb.Status_ABSENT),
49+
to.TargetStatus(scpb.ToPublic, scpb.TransientAbsent),
50+
to.CurrentStatus(scpb.Status_PUBLIC),
51+
}
52+
},
53+
)
54+
}

‎pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4170,6 +4170,22 @@ deprules
41704170
- $index-Node[CurrentStatus] = TRANSIENT_ABSENT
41714171
- joinTargetNode($partial-predicate, $partial-predicate-Target, $partial-predicate-Node)
41724172
- joinTargetNode($index, $index-Target, $index-Node)
4173+
- name: policy dependents are swapped in order
4174+
from: drop-policy-dependent-Node
4175+
kind: Precedence
4176+
to: add-policy-dependent-Node
4177+
query:
4178+
- $drop-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
4179+
- $add-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
4180+
- $drop-policy-dependent[Type] = $sameType
4181+
- $add-policy-dependent[Type] = $sameType
4182+
- joinOnPolicyID($drop-policy-dependent, $add-policy-dependent, $table-id, $policy-id)
4183+
- $drop-policy-dependent-Target[TargetStatus] = ABSENT
4184+
- $drop-policy-dependent-Node[CurrentStatus] = ABSENT
4185+
- $add-policy-dependent-Target[TargetStatus] IN [PUBLIC, TRANSIENT_ABSENT]
4186+
- $add-policy-dependent-Node[CurrentStatus] = PUBLIC
4187+
- joinTargetNode($drop-policy-dependent, $drop-policy-dependent-Target, $drop-policy-dependent-Node)
4188+
- joinTargetNode($add-policy-dependent, $add-policy-dependent-Target, $add-policy-dependent-Node)
41734189
- name: primary index named right before index becomes public
41744190
from: index-name-Node
41754191
kind: SameStagePrecedence
@@ -9091,6 +9107,22 @@ deprules
90919107
- $index-Node[CurrentStatus] = TRANSIENT_ABSENT
90929108
- joinTargetNode($partial-predicate, $partial-predicate-Target, $partial-predicate-Node)
90939109
- joinTargetNode($index, $index-Target, $index-Node)
9110+
- name: policy dependents are swapped in order
9111+
from: drop-policy-dependent-Node
9112+
kind: Precedence
9113+
to: add-policy-dependent-Node
9114+
query:
9115+
- $drop-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
9116+
- $add-policy-dependent[Type] IN ['*scpb.PolicyDeps', '*scpb.PolicyName', '*scpb.PolicyRole', '*scpb.PolicyUsingExpr', '*scpb.PolicyWithCheckExpr']
9117+
- $drop-policy-dependent[Type] = $sameType
9118+
- $add-policy-dependent[Type] = $sameType
9119+
- joinOnPolicyID($drop-policy-dependent, $add-policy-dependent, $table-id, $policy-id)
9120+
- $drop-policy-dependent-Target[TargetStatus] = ABSENT
9121+
- $drop-policy-dependent-Node[CurrentStatus] = ABSENT
9122+
- $add-policy-dependent-Target[TargetStatus] IN [PUBLIC, TRANSIENT_ABSENT]
9123+
- $add-policy-dependent-Node[CurrentStatus] = PUBLIC
9124+
- joinTargetNode($drop-policy-dependent, $drop-policy-dependent-Target, $drop-policy-dependent-Node)
9125+
- joinTargetNode($add-policy-dependent, $add-policy-dependent-Target, $add-policy-dependent-Node)
90949126
- name: primary index named right before index becomes public
90959127
from: index-name-Node
90969128
kind: SameStagePrecedence

‎pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/runtime.tsx‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,15 @@ export default function (props: GraphDashboardProps) {
7777
and fall based on load.`}
7878
showMetricsInTooltip={true}
7979
>
80-
<Axis label="goroutines">
81-
<Metric name="cr.node.sys.goroutines" title="Goroutine Count" />
80+
<Axis units={AxisUnits.Count} label="goroutines">
81+
{nodeIDs.map(nid => (
82+
<Metric
83+
key={nid}
84+
name="cr.node.sys.goroutines"
85+
title={nodeDisplayName(nodeDisplayNameByID, nid)}
86+
sources={[nid]}
87+
/>
88+
))}
8289
</Axis>
8390
</LineGraph>,
8491

@@ -129,7 +136,7 @@ export default function (props: GraphDashboardProps) {
129136
title="GC Runs"
130137
sources={nodeSources}
131138
tenantSource={tenantSource}
132-
tooltip={`The number of times that Go’s garbage collector was invoked per second ${tooltipSelection}.`}
139+
tooltip={`The number of times that Go's garbage collector was invoked per second ${tooltipSelection}.`}
133140
showMetricsInTooltip={true}
134141
>
135142
<Axis label="runs">
@@ -141,7 +148,7 @@ export default function (props: GraphDashboardProps) {
141148
title="GC Pause Time"
142149
sources={nodeSources}
143150
tenantSource={tenantSource}
144-
tooltip={`The amount of processor time used by Go’s garbage collector per second
151+
tooltip={`The amount of processor time used by Go's garbage collector per second
145152
${tooltipSelection}. During garbage collection, application code
146153
execution is paused.`}
147154
showMetricsInTooltip={true}

0 commit comments

Comments
 (0)