roachtest: update mixedversion to always use secure clusters
#105454
Conversation
renatolabs
requested review from
srosenberg and
smg260
and removed request for
a team
|
This change is |
renatolabs
force-pushed
the
rc/mixedversion-secure-clusters
branch
2 times, most recently
from
899ca1a to
80030d6
Compare
renatolabs
force-pushed
the
rc/mixedversion-secure-clusters
branch
from
80030d6 to
9fd2a46
Compare
|
Rebased and fixed conflicts. |
renatolabs
force-pushed
the
rc/mixedversion-secure-clusters
branch
from
9fd2a46 to
cf171a6
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 23 of 23 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @renatolabs and @smg260)
pkg/cmd/roachtest/cluster.go line 1506 at r3 (raw file):
Flag("exclude-files", fmt.Sprintf("'%s'", excludeFiles)). Flag("url", fmt.Sprintf("{pgurl:%d}", i)). MaybeFlag(c.IsSecure(), "certs-dir", "certs").
Nice!
pkg/cmd/roachtest/cluster.go line 2100 at r3 (raw file):
} // If the test failed (indicated by a canceled ctx), short-circuit. if ctx.Err() != nil {
How could we miss this dead code in PR review(s)?! It's also interesting that it's not flagged by any of the linters, including staticcheck's unused. (None of them perform a control-flow sensitive analysis.) 🤷♂️
pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go line 123 at r3 (raw file):
// nodes in a cluster. defaultClusterSettings = []install.ClusterSettingOption{ install.SecureOption(true),
Nice!
pkg/roachprod/install/cluster_synced.go line 1537 at r3 (raw file):
l.Printf("%s: checking %s", c.Name, path) result, err := c.runCmdOnSingleNode(ctx, l, 1, `$(test -e `+path+`); echo $?`, false, l.Stdout, l.Stderr) return strings.TrimSpace(result.Stdout) == "0", err
Might be worth leaving a comment as to why TrimSpace is required for correctness. Alternatively, echo -n $? would suffice, methinks.
This commit fixes a number of small bugs related to using secure clusters in roachtests, which probably stayed dormant up to this point because we don't use secure clusters enough, especially in tests where nodes are restarted. In this commit, we avoid refetching certs when restarting, fix the checks for certificate existence on nodes, and fix the command to wipe a cluster when certs are to be preserved. Epic: none Release note: None
This parameter already existed in the underlying `cluster_synced` implemenation, but the `cluster` interface exposed to roachtsts forced that argument to `false`. This commit changes the interface to expose the parameter, allowing tests to wipe clusters while keeping the certs intact. Epic: none Release note: None
Secure clusters are closer to production deployments and also allow us to tests features that we couldn't before, like creating new users with passwords during a test, and then performing SQL operations with those users. Epic: CRDB-19321 Release note: None
renatolabs
force-pushed
the
rc/mixedversion-secure-clusters
branch
from
cf171a6 to
c2eaf1f
Compare
There was a problem hiding this comment.
TFTR!
bors r=srosenberg
Reviewable status:
pkg/roachprod/install/cluster_synced.go line 1537 at r3 (raw file):
Previously, srosenberg (Stan Rosenberg) wrote…
Might be worth leaving a comment as to why
TrimSpaceis required for correctness. Alternatively,echo -n $?would suffice, methinks.
Opted to use echo -n, including a comment as to why it's needed.
|
Build succeeded: |
Secure clusters are closer to production deployments and also allow
us to tests features that we couldn't before, like creating new users
with passwords during a test, and then performing SQL operations with
those users.
In the process of getting this to work, there were a few bugs that needed
to be fixed (first commit), and the
clusterinterface needed a small updateas well (second commit).
Epic: CRDB-19321
Release note: None