Skip to content

Commit

Permalink
Merge #108029 #108192
Browse files Browse the repository at this point in the history 
108029: upgrademanager: unskip TestMigrationFailure r=msbutler a=adityamaru

The test no longer fails with our change in #107570.

Fixes: #106648
Fixes: #106762
Release note: None

108192: ccl/sqlproxyccl: deflake TestConnectionMigration r=darinpp a=jaylim-crl

Fixes #106885.

This test flake seems extremely rare, and it's unclear why it occurred in the
first place. The past 1000 runs (all of what TC has) have been successful.
Regardless, this commit attempts at deflaking TestConnectionMigration. Given
that some subtests transfer connections through `transferConnWithRetries`, it
is possible that the transfer was retried, causing the metric to be
incremented.

Release note: None

Epic: none

Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Jay <[email protected]>
  • Loading branch information
@adityamaru @jaylim-crl
3 people committed Aug 4, 2023
3 parents 374835f + 9cd15a2 + ee9ea9a commit a1c86ea
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 14 deletions.
9 changes: 6 additions & 3 deletions pkg/ccl/sqlproxyccl/proxy_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2056,6 +2056,7 @@ func TestConnectionMigration(t *testing.T) {

// Now attempt a transfer concurrently with requests.
initSuccessCount := f.metrics.ConnMigrationSuccessCount.Count()
initErrorRecoverableCount := f.metrics.ConnMigrationErrorRecoverableCount.Count()
subCtx, cancel := context.WithCancel(tCtx)
defer cancel()

Expand Down Expand Up @@ -2095,7 +2096,7 @@ func TestConnectionMigration(t *testing.T) {

// Check metrics.
require.True(t, f.metrics.ConnMigrationSuccessCount.Count() > initSuccessCount+4)
require.Equal(t, int64(0), f.metrics.ConnMigrationErrorRecoverableCount.Count())
require.Equal(t, initErrorRecoverableCount, f.metrics.ConnMigrationErrorRecoverableCount.Count())
require.Equal(t, int64(0), f.metrics.ConnMigrationErrorFatalCount.Count())

validateMiscMetrics(t)
Expand All @@ -2105,6 +2106,7 @@ func TestConnectionMigration(t *testing.T) {
// transfers should not close the connection.
t.Run("failed_transfers_with_tx", func(t *testing.T) {
initSuccessCount := f.metrics.ConnMigrationSuccessCount.Count()
initErrorRecoverableCount := f.metrics.ConnMigrationErrorRecoverableCount.Count()
initAddr := queryAddr(tCtx, t, db)

err = crdb.ExecuteTx(tCtx, db, nil /* txopts */, func(tx *gosql.Tx) error {
Expand Down Expand Up @@ -2139,15 +2141,16 @@ func TestConnectionMigration(t *testing.T) {
// still be active.
require.Nil(t, f.ctx.Err())
require.Equal(t, initSuccessCount, f.metrics.ConnMigrationSuccessCount.Count())
require.Equal(t, int64(5), f.metrics.ConnMigrationErrorRecoverableCount.Count())
require.Equal(t, initErrorRecoverableCount+5,
f.metrics.ConnMigrationErrorRecoverableCount.Count())
require.Equal(t, int64(0), f.metrics.ConnMigrationErrorFatalCount.Count())

// Once the transaction is closed, transfers should work.
require.NoError(t, transferConnWithRetries(t, f))
require.NotEqual(t, initAddr, queryAddr(tCtx, t, db))
require.Nil(t, f.ctx.Err())
require.Equal(t, initSuccessCount+1, f.metrics.ConnMigrationSuccessCount.Count())
require.Equal(t, int64(5), f.metrics.ConnMigrationErrorRecoverableCount.Count())
require.True(t, f.metrics.ConnMigrationErrorRecoverableCount.Count() >= initErrorRecoverableCount+5)
require.Equal(t, int64(0), f.metrics.ConnMigrationErrorFatalCount.Count())

validateMiscMetrics(t)
Expand Down
26 changes: 19 additions & 7 deletions pkg/jobs/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,32 +156,44 @@ func JobExists(
return row != nil, nil
}

// IsJobTypeColumnDoesNotExistError returns true if the error is of the form
// isJobTypeColumnDoesNotExistError returns true if the error is of the form
// `column "job_type" does not exist`.
func isJobTypeColumnDoesNotExistError(err error) bool {
return pgerror.GetPGCode(err) == pgcode.UndefinedColumn &&
strings.Contains(err.Error(), "column \"job_type\" does not exist")
}

// isJobInfoTableDoesNotExistError returns true if the error is of the form
// `related "job_info" does not exist`.
func isJobInfoTableDoesNotExistError(err error) bool {
return pgerror.GetPGCode(err) == pgcode.UndefinedTable &&
strings.Contains(err.Error(), "relation \"system.job_info\" does not exist")
}

// MaybeGenerateForcedRetryableError returns a
// TransactionRetryWithProtoRefreshError that will cause the txn to be retried
// if the error is because of an undefined job_type column.
// if the error is because of an undefined job_type column or missing job_info
// table.
//
// In https://github.com/cockroachdb/cockroach/issues/106762 we noticed that if
// a query is executed with an AS OF SYSTEM TIME clause that picks a transaction
// timestamp before the job_type migration, then parts of the jobs
// infrastructure will attempt to query the job_type column even though it
// doesn't exist at the transaction's timestamp.
//
// As a short term fix, when we encounter an `UndefinedColumn` error we
// generate a synthetic retryable error so that the txn is pushed to a
// higher timestamp at which the upgrade will have completed and the
// `job_type` column will be visible. The longer term fix is being tracked
// in https://github.com/cockroachdb/cockroach/issues/106764.
// As a short term fix, when we encounter an `UndefinedTable` or
// `UndefinedColumn` error we generate a synthetic retryable error so that the
// txn is pushed to a higher timestamp at which the upgrade will have completed
// and the table/column will be visible. The longer term fix is being tracked in
// https://github.com/cockroachdb/cockroach/issues/106764.
func MaybeGenerateForcedRetryableError(ctx context.Context, txn *kv.Txn, err error) error {
if err != nil && isJobTypeColumnDoesNotExistError(err) {
return txn.GenerateForcedRetryableError(ctx, "synthetic error "+
"to push timestamp to after the `job_type` upgrade has run")
}
if err != nil && isJobInfoTableDoesNotExistError(err) {
return txn.GenerateForcedRetryableError(ctx, "synthetic error "+
"to push timestamp to after the `job_info` upgrade has run")
}
return err
}
1 change: 0 additions & 1 deletion pkg/upgrade/upgrademanager/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ go_test(
"//pkg/sql/sem/eval",
"//pkg/testutils",
"//pkg/testutils/serverutils",
"//pkg/testutils/skip",
"//pkg/testutils/sqlutils",
"//pkg/testutils/testcluster",
"//pkg/upgrade",
Expand Down
3 changes: 0 additions & 3 deletions pkg/upgrade/upgrademanager/manager_external_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/testutils/skip"
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
"github.com/cockroachdb/cockroach/pkg/upgrade"
Expand Down Expand Up @@ -683,8 +682,6 @@ func TestMigrationFailure(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

skip.WithIssue(t, 106648)

ctx := context.Background()

// Configure the range of versions used by the test
Expand Down

0 comments on commit a1c86ea

Please sign in to comment.