Skip to content

Commit

Permalink
roachtest: de-flake multitenant-fairness/read-heavy/skewed
Browse files Browse the repository at this point in the history 
Fixes #97448 (possibly).
Fixes #78691.

These tests run under severe CPU overload, and we see the workload
getting observing the following errors:

  ERROR: liveness session expired 571.043163ms before transaction

The SQL liveness lease extension work ends up getting severely starved,
despite extending leases by 40s every 5s. It turns out for tenant
SQL liveness work, we were using admissionpb.NormalPri, so such
starvation was possible. This wasn't true for the system tenant where we
bypassed AC altogether.

Release note: None
  • Loading branch information
@irfansharif
irfansharif committed Mar 21, 2023
1 parent 898a32a commit 15dc772
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 14 deletions.
15 changes: 9 additions & 6 deletions pkg/cmd/roachtest/tests/admission_control_multitenant_fairness.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,15 +255,18 @@ func runMultiTenantFairness(
i := i
pgurl := tenants[i].secureURL()
m1.Go(func(ctx context.Context) error {
// TODO(irfansharif): Occasionally we see SQL Liveness errors of the
// form:
// TODO(irfansharif): Occasionally we see SQL liveness errors of the
// following form. See #78691, #97448.
//
// ERROR: liveness session expired 571.043163ms before transaction
//
// Why do these errors occur? Do we want to give sql liveness
// session goroutines higher priority? Is this test using too high a
// concurrency? Why do we even need this data load step -- why not
// just run the workload generator right away?
// Why do these errors occur? We started using high-pri for tenant
// sql liveness work as of #98785, so this TODO might be stale. If
// it persists, consider extending the default lease duration from
// 40s to something higher, or retrying internally if the sql
// session gets renewed shortly (within some jitter). We don't want
// to --tolerate-errors here and below because we'd see total
// throughput collapse.
cmd := fmt.Sprintf(
"./cockroach workload run kv '%s' --secure --min-block-bytes %d --max-block-bytes %d "+
"--batch %d --max-ops %d --concurrency=25",
Expand Down
8 changes: 4 additions & 4 deletions pkg/kv/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -894,10 +894,10 @@ func (db *DB) NewTxn(ctx context.Context, debugName string) *Txn {
// callback may return either nil or the retryable error. Txn is responsible for
// resetting the transaction and retrying the callback.
//
// TODO(irfansharif): Audit uses of this since API since it bypasses AC. Make
// the other variant (TxnWithAdmissionControl) the default, or maybe rename this
// to be more explicit (TxnWithoutAdmissionControl) so new callers have to be
// conscious about what they want.
// TODO(irfansharif): Audit uses of this since API since it bypasses AC for the
// system tenant. Make the other variant (TxnWithAdmissionControl) the default,
// or maybe rename this to be more explicit (TxnWithoutAdmissionControl) so new
// callers have to be conscious about what they want.
func (db *DB) Txn(ctx context.Context, retryable func(context.Context, *Txn) error) error {
return db.TxnWithAdmissionControl(
ctx, kvpb.AdmissionHeader_OTHER, admissionpb.NormalPri,
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/sqlliveness/slstorage/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ go_library(
"//pkg/clusterversion",
"//pkg/keys",
"//pkg/kv",
"//pkg/kv/kvpb",
"//pkg/multitenant",
"//pkg/roachpb",
"//pkg/server/settingswatcher",
Expand All @@ -26,6 +27,7 @@ go_library(
"//pkg/sql/enum",
"//pkg/sql/sem/eval",
"//pkg/sql/sqlliveness",
"//pkg/util/admission/admissionpb",
"//pkg/util/cache",
"//pkg/util/encoding",
"//pkg/util/hlc",
Expand Down
27 changes: 23 additions & 4 deletions pkg/sql/sqlliveness/slstorage/slstorage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
"github.com/cockroachdb/cockroach/pkg/multitenant"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/server/settingswatcher"
Expand All @@ -27,6 +28,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness"
"github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb"
"github.com/cockroachdb/cockroach/pkg/util/cache"
"github.com/cockroachdb/cockroach/pkg/util/encoding"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
Expand Down Expand Up @@ -304,6 +306,23 @@ func (s *Storage) deleteOrFetchSessionSingleFlightLocked(
return resChan
}

// txn wraps around s.db.TxnWithAdmissionControl, annotating it with the
// appropriate AC metadata for SQL liveness work.
func (s *Storage) txn(ctx context.Context, retryable func(context.Context, *kv.Txn) error) error {
// We use AdmissionHeader_OTHER to bypass AC if we're the system tenant
// (only system tenants are allowed to bypass it altogether,
// AdmissionHeader_OTHER is ignored for non-system tenants further down the
// stack), and admissionpb.HighPri for secondary tenants to avoid the kind
// of starvation we see in #97448.
return s.db.TxnWithAdmissionControl(
ctx,
kvpb.AdmissionHeader_OTHER,
admissionpb.HighPri,
kv.SteppingDisabled,
retryable,
)
}

// deleteOrFetchSession returns whether the query session currently exists by
// reading from the database. If the record exists but is expired, this method
// will delete the record transactionally, moving it to from alive to dead. The
Expand All @@ -314,7 +333,7 @@ func (s *Storage) deleteOrFetchSession(
var deleted bool
var prevExpiration hlc.Timestamp
ctx = multitenant.WithTenantCostControlExemption(ctx)
if err := s.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
if err := s.txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
// Reset captured variable in case of retry.
deleted, expiration, prevExpiration = false, hlc.Timestamp{}, hlc.Timestamp{}

Expand Down Expand Up @@ -464,7 +483,7 @@ func (s *Storage) fetchExpiredSessionIDs(ctx context.Context) ([]sqlliveness.Ses
}

var result []sqlliveness.SessionID
if err := s.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
if err := s.txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
version, err := s.versionGuard(ctx, txn)
if err != nil {
return err
Expand All @@ -486,7 +505,7 @@ func (s *Storage) Insert(
ctx context.Context, sid sqlliveness.SessionID, expiration hlc.Timestamp,
) (err error) {
ctx = multitenant.WithTenantCostControlExemption(ctx)
if err := s.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
if err := s.txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
batch := txn.NewBatch()

version, err := s.versionGuard(ctx, txn)
Expand Down Expand Up @@ -526,7 +545,7 @@ func (s *Storage) Update(
ctx context.Context, sid sqlliveness.SessionID, expiration hlc.Timestamp,
) (sessionExists bool, err error) {
ctx = multitenant.WithTenantCostControlExemption(ctx)
err = s.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
err = s.txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
version, err := s.versionGuard(ctx, txn)
if err != nil {
return err
Expand Down
10 changes: 10 additions & 0 deletions pkg/ts/catalog/chart_catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3717,8 +3717,10 @@ var charts = []sectionDescription{
"admission.admitted.kv-stores.locking-pri",
"admission.admitted.kv-stores.ttl-low-pri",
"admission.admitted.kv-stores.normal-pri",
"admission.admitted.kv-stores.high-pri",
"admission.admitted.kv.locking-pri",
"admission.admitted.kv.normal-pri",
"admission.admitted.kv.high-pri",
"admission.admitted.sql-kv-response.locking-pri",
"admission.admitted.sql-kv-response.normal-pri",
"admission.admitted.sql-leaf-start.locking-pri",
Expand All @@ -3733,8 +3735,10 @@ var charts = []sectionDescription{
"admission.errored.kv-stores.locking-pri",
"admission.errored.kv-stores.ttl-low-pri",
"admission.errored.kv-stores.normal-pri",
"admission.errored.kv-stores.high-pri",
"admission.errored.kv.locking-pri",
"admission.errored.kv.normal-pri",
"admission.errored.kv.high-pri",
"admission.errored.sql-kv-response.locking-pri",
"admission.errored.sql-kv-response.normal-pri",
"admission.errored.sql-leaf-start.locking-pri",
Expand All @@ -3749,8 +3753,10 @@ var charts = []sectionDescription{
"admission.requested.kv-stores.locking-pri",
"admission.requested.kv-stores.ttl-low-pri",
"admission.requested.kv-stores.normal-pri",
"admission.requested.kv-stores.high-pri",
"admission.requested.kv.locking-pri",
"admission.requested.kv.normal-pri",
"admission.requested.kv.high-pri",
"admission.requested.sql-kv-response.locking-pri",
"admission.requested.sql-kv-response.normal-pri",
"admission.requested.sql-leaf-start.locking-pri",
Expand All @@ -3777,8 +3783,10 @@ var charts = []sectionDescription{
"admission.wait_queue_length.kv-stores.locking-pri",
"admission.wait_queue_length.kv-stores.ttl-low-pri",
"admission.wait_queue_length.kv-stores.normal-pri",
"admission.wait_queue_length.kv-stores.high-pri",
"admission.wait_queue_length.kv.locking-pri",
"admission.wait_queue_length.kv.normal-pri",
"admission.wait_queue_length.kv.high-pri",
"admission.wait_queue_length.sql-kv-response.locking-pri",
"admission.wait_queue_length.sql-kv-response.normal-pri",
"admission.wait_queue_length.sql-leaf-start.locking-pri",
Expand All @@ -3805,8 +3813,10 @@ var charts = []sectionDescription{
"admission.wait_durations.kv-stores.locking-pri",
"admission.wait_durations.kv-stores.ttl-low-pri",
"admission.wait_durations.kv-stores.normal-pri",
"admission.wait_durations.kv-stores.high-pri",
"admission.wait_durations.kv.locking-pri",
"admission.wait_durations.kv.normal-pri",
"admission.wait_durations.kv.high-pri",
"admission.wait_durations.sql-kv-response.locking-pri",
"admission.wait_durations.sql-kv-response.normal-pri",
"admission.wait_durations.sql-leaf-start.locking-pri",
Expand Down

0 comments on commit 15dc772

Please sign in to comment.