From 511799a111c547070e7a3365f000100755154013 Mon Sep 17 00:00:00 2001 From: Bilal Akhtar Date: Wed, 1 Feb 2023 17:22:57 -0500 Subject: [PATCH 1/4] logictest: Skip if using cockroach-go/testserver Skip of flaky test. Informs: #96387 Epic: none Release note: None --- pkg/sql/logictest/logic.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index d9c5f6b3e622..281d7f6c620c 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1785,6 +1785,7 @@ func (t *logicTest) setup( t.testCleanupFuncs = append(t.testCleanupFuncs, tempExternalIODirCleanup) if cfg.UseCockroachGoTestserver { + skip.WithIssue(t.t(), 96387, "flakes with panic over connection string") skip.UnderStress(t.t(), "test takes a long time and downloads release artifacts") if runtime.GOARCH == "arm64" && strings.HasPrefix(cfg.CockroachGoBootstrapVersion, "v22.1") { skip.IgnoreLint(t.t(), "Skip under ARM64. There are no ARM release artifacts for v22.1.") From 76cc703cd12e6fede98b3daf51e2bd8de70d64e1 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Thu, 2 Feb 2023 16:24:06 -0500 Subject: [PATCH 2/4] logictest: refactor client connection creation logic This patch abstracts the logic for creating client connections and persisting them into a separate function. As a side benefit, it also fixes the problem of clients created in `execQuery` not being properly tracked in the `clients` field of `logicTest`. Release note: None --- pkg/sql/logictest/logic.go | 76 +++++++++++--------------------------- 1 file changed, 22 insertions(+), 54 deletions(-) diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 20ccce092f77..738347aea6ae 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1161,35 +1161,35 @@ func (t *logicTest) outf(format string, args ...interface{}) { // setUser sets the DB client to the specified user and connects // to the node in the cluster at index nodeIdx. -// It returns a cleanup function to be run when the credentials -// are no longer needed. -func (t *logicTest) setUser(user string, nodeIdx int) func() { - if db, ok := t.clients[user][nodeIdx]; ok { - t.db = db - t.user = user +func (t *logicTest) setUser(user string, nodeIdx int) { + db := t.getOrOpenClient(user, nodeIdx) + t.db = db + t.user = user + t.nodeIdx = nodeIdx +} - // No cleanup necessary, but return a no-op func to avoid nil pointer dereference. - return func() {} +// getOrOpenClient returns the existing client for the given user and nodeIdx, +// if one exists. Otherwise, it opens and returns a new client. +func (t *logicTest) getOrOpenClient(user string, nodeIdx int) *gosql.DB { + if db, ok := t.clients[user][nodeIdx]; ok { + return db } - var addr string var pgURL url.URL - var pgUser string - var cleanupFunc func() - pgUser = strings.TrimPrefix(user, "host-cluster-") + pgUser := strings.TrimPrefix(user, "host-cluster-") if t.cfg.UseCockroachGoTestserver { pgURL = *t.testserverCluster.PGURLForNode(nodeIdx) pgURL.User = url.User(pgUser) - pgURL.Path = "test" - cleanupFunc = func() {} } else { - addr = t.cluster.Server(nodeIdx).ServingSQLAddr() + addr := t.cluster.Server(nodeIdx).ServingSQLAddr() if len(t.tenantAddrs) > 0 && !strings.HasPrefix(user, "host-cluster-") { addr = t.tenantAddrs[nodeIdx] } + var cleanupFunc func() pgURL, cleanupFunc = sqlutils.PGUrl(t.rootT, addr, "TestLogic", url.User(pgUser)) - pgURL.Path = "test" + t.clusterCleanupFuncs = append(t.clusterCleanupFuncs, cleanupFunc) } + pgURL.Path = "test" db := t.openDB(pgURL) @@ -1213,11 +1213,8 @@ func (t *logicTest) setUser(user string, nodeIdx int) func() { t.clients[user] = make(map[int]*gosql.DB) } t.clients[user][nodeIdx] = db - t.db = db - t.user = pgUser - t.nodeIdx = nodeIdx - return cleanupFunc + return db } func (t *logicTest) openDB(pgURL url.URL) *gosql.DB { @@ -1306,8 +1303,9 @@ func (t *logicTest) newTestServerCluster(bootstrapBinaryPath string, upgradeBina } t.testserverCluster = ts - t.clusterCleanupFuncs = append(t.clusterCleanupFuncs, t.setUser(username.RootUser, 0 /* nodeIdx */)) t.clusterCleanupFuncs = append(t.clusterCleanupFuncs, ts.Stop) + + t.setUser(username.RootUser, 0 /* nodeIdx */) } // newCluster creates a new cluster. It should be called after the logic tests's @@ -1721,9 +1719,7 @@ func (t *logicTest) newCluster( ) } - // db may change over the lifetime of this function, with intermediate - // values cached in t.clients and finally closed in t.close(). - t.clusterCleanupFuncs = append(t.clusterCleanupFuncs, t.setUser(username.RootUser, 0 /* nodeIdx */)) + t.setUser(username.RootUser, 0 /* nodeIdx */) } // waitForTenantReadOnlyClusterSettingToTakeEffectOrFatal waits until all tenant @@ -2960,7 +2956,7 @@ func (t *logicTest) processSubtest( nodeIdx = int(idx) } } - cleanupUserFunc := t.setUser(fields[1], nodeIdx) + t.setUser(fields[1], nodeIdx) // In multi-tenant tests, we may need to also create database test when // we switch to a different tenant. if t.cfg.UseTenant && strings.HasPrefix(fields[1], "host-cluster-") { @@ -2968,7 +2964,6 @@ func (t *logicTest) processSubtest( return errors.Wrapf(err, "error creating database on admin tenant") } } - defer cleanupUserFunc() case "skip": reason := "skipped" @@ -3311,30 +3306,8 @@ func (t *logicTest) execQuery(query logicQuery) error { t.noticeBuffer = nil db := t.db - var closeDB func() if query.nodeIdx != t.nodeIdx { - var pgURL url.URL - if t.testserverCluster != nil { - pgURL = *t.testserverCluster.PGURLForNode(query.nodeIdx) - pgURL.User = url.User(t.user) - pgURL.Path = "test" - } else { - addr := t.cluster.Server(query.nodeIdx).ServingSQLAddr() - if len(t.tenantAddrs) > 0 { - addr = t.tenantAddrs[query.nodeIdx] - } - var cleanupFunc func() - pgURL, cleanupFunc = sqlutils.PGUrl(t.rootT, addr, "TestLogic", url.User(t.user)) - defer cleanupFunc() - pgURL.Path = "test" - } - - db = t.openDB(pgURL) - closeDB = func() { - if err := db.Close(); err != nil { - t.Fatal(err) - } - } + db = t.getOrOpenClient(t.user, query.nodeIdx) } if query.expectAsync { @@ -3354,9 +3327,6 @@ func (t *logicTest) execQuery(query logicQuery) error { startedChan := make(chan struct{}) go func() { - if closeDB != nil { - defer closeDB() - } startedChan <- struct{}{} rows, err := db.Query(query.sql) pending.resultChan <- pendingQueryResult{rows, err} @@ -3364,8 +3334,6 @@ func (t *logicTest) execQuery(query logicQuery) error { <-startedChan return nil - } else if closeDB != nil { - defer closeDB() } rows, err := db.Query(query.sql) From 203a795e2edcb9c8ff12ae1634877d0b59ea39e9 Mon Sep 17 00:00:00 2001 From: babusrithar Date: Fri, 3 Feb 2023 10:37:24 -0500 Subject: [PATCH 3/4] authors: add BabuSrithar to authors Release note: None Epic: None --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index cc3c4cd6b7c0..b03a5bad560e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -88,6 +88,7 @@ Artem Ervits Arul Ajmani Asit Mahato Austen McClernon +BabuSrithar Barry He bc Ben Bardin From 067312c4729998bf424eb9022f4c69c0b1292468 Mon Sep 17 00:00:00 2001 From: healthy-pod Date: Thu, 2 Feb 2023 10:31:12 -0800 Subject: [PATCH 4/4] roachtest: add aws weekly script This change adds the scripts to be used by the aws weekly roachtest teamcity job. Release note: None Epic: none --- .../nightlies/roachtest_weekly_aws.sh | 11 ++++++++ .../nightlies/roachtest_weekly_aws_impl.sh | 26 +++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100755 build/teamcity/cockroach/nightlies/roachtest_weekly_aws.sh create mode 100755 build/teamcity/cockroach/nightlies/roachtest_weekly_aws_impl.sh diff --git a/build/teamcity/cockroach/nightlies/roachtest_weekly_aws.sh b/build/teamcity/cockroach/nightlies/roachtest_weekly_aws.sh new file mode 100755 index 000000000000..728bd12d33ce --- /dev/null +++ b/build/teamcity/cockroach/nightlies/roachtest_weekly_aws.sh @@ -0,0 +1,11 @@ +#!/usr/bin/env bash + +set -exuo pipefail + +dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" + +source "$dir/teamcity-support.sh" # For $root +source "$dir/teamcity-bazel-support.sh" # For run_bazel + +BAZEL_SUPPORT_EXTRA_DOCKER_ARGS="-e LITERAL_ARTIFACTS_DIR=$root/artifacts -e AWS_ACCESS_KEY_ID -e AWS_ACCESS_KEY_ID_ASSUME_ROLE -e AWS_KMS_KEY_ARN_A -e AWS_KMS_KEY_ARN_B -e AWS_KMS_REGION_A -e AWS_KMS_REGION_B -e AWS_ROLE_ARN -e AWS_SECRET_ACCESS_KEY -e AWS_SECRET_ACCESS_KEY_ASSUME_ROLE -e BUILD_TAG -e BUILD_VCS_NUMBER -e CLOUD -e COCKROACH_DEV_LICENSE -e TESTS -e COUNT -e GITHUB_API_TOKEN -e GITHUB_ORG -e GITHUB_REPO -e GOOGLE_EPHEMERAL_CREDENTIALS -e SLACK_TOKEN -e TC_BUILDTYPE_ID -e TC_BUILD_BRANCH -e TC_BUILD_ID -e TC_SERVER_URL" \ + run_bazel build/teamcity/cockroach/nightlies/roachtest_weekly_aws_impl.sh diff --git a/build/teamcity/cockroach/nightlies/roachtest_weekly_aws_impl.sh b/build/teamcity/cockroach/nightlies/roachtest_weekly_aws_impl.sh new file mode 100755 index 000000000000..5c2aaed89a3b --- /dev/null +++ b/build/teamcity/cockroach/nightlies/roachtest_weekly_aws_impl.sh @@ -0,0 +1,26 @@ +#!/usr/bin/env bash + +set -exuo pipefail + +dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))" + +source "$dir/teamcity-support.sh" + +if [[ ! -f ~/.ssh/id_rsa.pub ]]; then + ssh-keygen -q -C "roachtest-weekly-bazel $(date)" -N "" -f ~/.ssh/id_rsa +fi + +source $root/build/teamcity/cockroach/nightlies/roachtest_compile_bits.sh + +artifacts=/artifacts +source $root/build/teamcity/util/roachtest_util.sh + +build/teamcity-roachtest-invoke.sh \ + tag:aws-weekly \ + --cloud="${CLOUD}" \ + --build-tag="${BUILD_TAG}" \ + --cluster-id "${TC_BUILD_ID}" \ + --cockroach "$PWD/bin/cockroach" \ + --artifacts=/artifacts \ + --artifacts-literal="${LITERAL_ARTIFACTS_DIR:-}" \ + --slack-token="${SLACK_TOKEN}"