roachprod, roachtest: use same cluster name sanitization #123961

DarrylWong · 2024-05-10T18:51:20Z

Previously, roachtest had it's own function to sanitize cluster names, while roachprod had it's own function to verify (but not sanitize) cluster names. This change removes both and opts instead to use vm.DNSSafeName.

This change also introduces MalformedClusterNameError which gives a hint on what is wrong with the name and
tells roachtest not to retry cluster creation.

Fixes: #122633
Epic: none
Release note: none

cockroach-teamcity · 2024-05-10T18:51:34Z

This change is

srosenberg · 2024-05-13T22:34:56Z

pkg/roachprod/vm/vm.go

 // Unsafe characters are dropped. No length check is performed.
-func DNSSafeAccount(account string) string {
+func DNSSafeName(name string) string {
 	safe := func(r rune) rune {
 		switch {
 		case r >= 'a' && r <= 'z':
 			return r
 		case r >= 'A' && r <= 'Z':
 			return unicode.ToLower(r)


Nice! This is indeed more efficient than strings.ToLower when it's known that the char sequence is ascii. It's essentially inlined to this r += 'a' - 'A'.

pkg/roachprod/roachprod.go

srosenberg · 2024-05-13T22:42:56Z

pkg/cmd/roachtest/cluster.go

@@ -973,6 +966,9 @@ func (f *clusterFactory) newCluster(
 			// or a destroy from the previous iteration failed.
 			return nil, nil, err
 		}
+		if errors.HasType(err, (*roachprod.MalformedClusterNameError)(nil)) {


Let's add a unit test to cover this case.

I gave it a shot, but I wasn't sure how to feasibly do this without mocking a ton of things. There already is a way to mock cluster creation via TestSpec + r.MakeClusterSpec(0), but that skips roachprod.Create and this entire retry loop where we validate the cluster name.

I could mock roachprod.Create to just return a MalformedclusterNameError, but I don't think that test would be very useful.

I could mock roachprod.Create to just return a MalformedclusterNameError, but I don't think that test would be very useful.

That would have been my suggestion as well. I think it would be useful to unit test roachprod.Create especially in the context of [1]; i.e., ClusterAlreadyExistsError, MalformedClusterNameError and potentially future type of errors are non-retryable, so we should be able to cover the fact via a unit test. For now, the mock could randomly return the two non-retryable errors, and we assert that the mock is invoked exactly once, i.e., no retries. Wdyt?

[1] #114523

Done. I was trying to add a case where it didn't error out and that was quite tricky to do without mocking a bunch of things. Mocking the two non-retryable errors cases was simple though.

srosenberg

Could use a couple of new unit tests (for verifyClusterName and newCluster), otherwise looks great!

DarrylWong · 2024-05-14T21:07:14Z

pkg/roachprod/roachprod.go

+
+// _findActiveAccounts is a test only variable, used for mocking a provider finding
+// active accounts in a unit test, where we don't want to actually access a provider.
+var _findActiveAccounts func(l *logger.Logger) (map[string]string, error)


Is it okay to do this for a package variable? I couldn't think of an easier way to mock vm.FindActiveAccounts. AFAICT, the tests in a package are run sequentially, so no concurrency in accessing this.

Yep, this is a totally legit. way of mocking which is also used sometimes for dependency injection. E.g., InitTestConnectorFactory [1] is used to inject (or mock) a test-specific implementation of the KV connector.

However, we could initialize it with the actual implementation, i.e.,

var _findActiveAccounts = vm.FindActiveAccounts

and get rid of the conditional logic below.

[1]

cockroach/pkg/kv/kvclient/kvtenant/test_utils.go

Line 16 in 6ec873a

Factory = testConnectorFactory{}

Done! Thanks for tip

pkg/roachprod/roachprod.go

DarrylWong · 2024-05-14T21:21:14Z

pkg/cmd/roachtest/cluster.go

@@ -973,6 +966,9 @@ func (f *clusterFactory) newCluster(
 			// or a destroy from the previous iteration failed.
 			return nil, nil, err
 		}
+		if errors.HasType(err, (*roachprod.MalformedClusterNameError)(nil)) {


I gave it a shot, but I wasn't sure how to feasibly do this without mocking a ton of things. There already is a way to mock cluster creation via TestSpec + r.MakeClusterSpec(0), but that skips roachprod.Create and this entire retry loop where we validate the cluster name.

I could mock roachprod.Create to just return a MalformedclusterNameError, but I don't think that test would be very useful.

srosenberg · 2024-05-15T00:38:17Z

pkg/roachprod/roachprod.go

-		active, err := vm.FindActiveAccounts(l)
+		var active map[string]string
+		var err error
+		if _findActiveAccounts != nil {


Can elide this check and just use active, err = _findActiveAccounts(l) as per above comment.

srosenberg · 2024-05-15T00:41:31Z

I am guessing check_generated_code failed because you forgot to run,

./dev generate bazel

which creates the missing roachprod_test package,

--- a/pkg/roachprod/BUILD.bazel
+++ b/pkg/roachprod/BUILD.bazel
@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

 go_library(
     name = "roachprod",
@@ -38,3 +38,13 @@ go_library(
         "@org_golang_x_sys//unix",
     ],
 )
+
+go_test(
+    name = "roachprod_test",
+    srcs = ["roachprod_test.go"],
+    embed = [":roachprod"],
+    deps = [
+        "//pkg/roachprod/logger",
+        "@com_github_stretchr_testify//assert",
+    ],
+)

Previously, roachtest had it's own function to sanitize cluster names, while roachprod had it's own function to verify cluster names. This change removes both and opts instead to use vm.DNSSafeName. This change also introduces MalformedClusterNameError which gives a hint on what is wrong with the name and tells roachtest not to retry cluster creation.

srosenberg

LGTM. Thanks for adding the unit tests!

DarrylWong · 2024-05-16T15:21:45Z

TFTR!

bors r=srosenberg

craig · 2024-05-16T16:41:17Z

Build failed:

docker_image_amd64

DarrylWong · 2024-05-16T17:15:49Z

🥺

bors retry

craig · 2024-05-16T20:32:25Z

Build succeeded:

blathers-crl · 2024-05-16T20:32:41Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 278679f to blathers/backport-release-24.1-123961: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

DarrylWong force-pushed the invalid-username-fix branch 4 times, most recently from 0016299 to 69cc304 Compare May 13, 2024 16:11

DarrylWong marked this pull request as ready for review May 13, 2024 18:56

DarrylWong requested a review from a team as a code owner May 13, 2024 18:56

DarrylWong requested review from nameisbhaskar and vidit-bhat and removed request for a team May 13, 2024 18:56

srosenberg self-requested a review May 13, 2024 22:28

srosenberg reviewed May 13, 2024

View reviewed changes

pkg/roachprod/roachprod.go Show resolved Hide resolved

srosenberg reviewed May 13, 2024

View reviewed changes

srosenberg approved these changes May 13, 2024

View reviewed changes

DarrylWong force-pushed the invalid-username-fix branch 2 times, most recently from 2d3446b to 927338d Compare May 14, 2024 21:06

DarrylWong commented May 14, 2024

View reviewed changes

DarrylWong force-pushed the invalid-username-fix branch from 927338d to 4e09a63 Compare May 14, 2024 21:28

srosenberg reviewed May 15, 2024

View reviewed changes

DarrylWong force-pushed the invalid-username-fix branch from 4e09a63 to 27ab2c2 Compare May 15, 2024 19:59

DarrylWong force-pushed the invalid-username-fix branch from 27ab2c2 to 278679f Compare May 15, 2024 20:04

srosenberg approved these changes May 15, 2024

View reviewed changes

DarrylWong added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. labels May 16, 2024

craig bot merged commit 6fa15a6 into cockroachdb:master May 16, 2024
22 checks passed

This was referenced May 16, 2024

release-23.1: roachprod, roachtest: use same cluster name sanitization #124296

Merged

release-23.2: roachprod, roachtest: use same cluster name sanitization #124298

Merged

DarrylWong mentioned this pull request May 16, 2024

release-24.1: roachprod, roachtest: use same cluster name sanitization #124304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachprod, roachtest: use same cluster name sanitization #123961

roachprod, roachtest: use same cluster name sanitization #123961

DarrylWong commented May 10, 2024 •

edited

Loading

cockroach-teamcity commented May 10, 2024

srosenberg May 13, 2024

srosenberg May 13, 2024

DarrylWong May 14, 2024

srosenberg May 15, 2024

DarrylWong May 15, 2024

srosenberg left a comment

DarrylWong May 14, 2024

srosenberg May 15, 2024

DarrylWong May 15, 2024

DarrylWong May 14, 2024

srosenberg May 15, 2024

srosenberg commented May 15, 2024

srosenberg left a comment

DarrylWong commented May 16, 2024

craig bot commented May 16, 2024

DarrylWong commented May 16, 2024

craig bot commented May 16, 2024

blathers-crl bot commented May 16, 2024

roachprod, roachtest: use same cluster name sanitization #123961

roachprod, roachtest: use same cluster name sanitization #123961

Conversation

DarrylWong commented May 10, 2024 • edited Loading

cockroach-teamcity commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srosenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srosenberg commented May 15, 2024

srosenberg left a comment

Choose a reason for hiding this comment

DarrylWong commented May 16, 2024

craig bot commented May 16, 2024

DarrylWong commented May 16, 2024

craig bot commented May 16, 2024

blathers-crl bot commented May 16, 2024

DarrylWong commented May 10, 2024 •

edited

Loading