roachtest: move decl_schema_change/job-compat to new framework#114043
roachtest: move decl_schema_change/job-compat to new framework#114043craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
f6282ae to
d498aba
Compare
|
CI lint issues - this is mostly good, I just need to make sure I didnt miss any DDLs and that the DDLs I am using (order, syntax) are good. going to use mixed-version backup's enable/disable job functions & leave the ones in backup.go for Renato (Renato, if you would like for me to delete these, feel free to lmk). Opening this for any nits that I can fix/any general questions, comments, concerns for what I have so far |
rafiss
left a comment
There was a problem hiding this comment.
looks like it's on the right track!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @annrpom, @DarrylWong, and @renatolabs)
pkg/cmd/roachtest/tests/backup.go line 291 at r1 (raw file):
// registry from adopting a job. // // TODO(renato): remove this duplicated function once
based on this TODO, it looks like there might be another function we should use
pkg/cmd/roachtest/tests/backup.go line 355 at r1 (raw file):
// registry from adopting a job. // // TODO(renato): remove this duplicated function once
based on this TODO, it looks like there might be another function we should use
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 34 at r1 (raw file):
// supported in the "previous" major release. r.Add(registry.TestSpec{ Name: "declarative_schema_changer/job-compatibility-mixed-version-V222-V231",
nit: we should rename this, since now we want to test V231-V232
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 91 at r1 (raw file):
} // executeSupportedDDLs tests all DDLs supported in V22_2 and V23_1.
i think as a separate commit in this same PR, we should change this function so it tests v23_1 <-> v23_2 compatibility.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 168 at r1 (raw file):
) { mvt := mixedversion.NewTest( ctx, t, t.L(), c, c.All(),
nit: this test specifically is testing compat between the last version and this one, so we should add a mixedversion.NumUpgrades(1) option
906d7b8 to
d6b6dca
Compare
Xiang-Gu
left a comment
There was a problem hiding this comment.
Good work! I've left a few comments and thanks for doing this!
| node option.NodeListOption, | ||
| schemaChangeStmt string, | ||
| ) { | ||
| ) error { |
There was a problem hiding this comment.
curious: Why do you add a error as the return? It seems it's not used anyway.
There was a problem hiding this comment.
this gets bubbled up to our executeSupportedDDLs function; that error is then handled by the new mixed-version framework (since we call executeSupportedDDLs in InMixedVersion):
cockroach/pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go
Lines 161 to 169 in fbd1bb8
| // This test requires us to come back and change the to-be-tests stmts to be those | ||
| // supported in the "previous" major release. | ||
| r.Add(registry.TestSpec{ | ||
| Name: "declarative_schema_changer/job-compatibility-mixed-version-V231-V232", |
There was a problem hiding this comment.
I think in commit 1, this should be V222-V231?
| // Disable job adoption for all nodes of the set we are testing [upgradedNodes, oldNodes] so that the | ||
| // other respective set can adopt the job (ex. for upgradedNodes, we want the oldNodes to be able to adopt | ||
| // these jobs). | ||
| if err := testUtils.disableJobAdoption(ctx, t.L(), nodes); err != nil { |
There was a problem hiding this comment.
great that we have this now!
| } | ||
|
|
||
| // DDLs supported since V23_1. | ||
| v231DDls := []string{ |
There was a problem hiding this comment.
I think we also supported the following in V23_1 in DSC. Can you also include them here?
ADD FOREIGN KEYADD FOREIGN KEY NOT VALIDADD UNIQUE WITHOUT INDEXADD UNIQUE WITHOUT INDEX NOT VALIDVALIDATE CONSTRAINTDROP CONSTRAINT
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 128 at r4 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
I think we also supported the following in V23_1 in DSC. Can you also include them here?
ADD FOREIGN KEYADD FOREIGN KEY NOT VALIDADD UNIQUE WITHOUT INDEXADD UNIQUE WITHOUT INDEX NOT VALIDVALIDATE CONSTRAINTDROP CONSTRAINT
do we need to use the sql.schema.force_declarative_statements cluster setting to make these use DSC when we want it?
renatolabs
left a comment
There was a problem hiding this comment.
Thanks for updating another test! 🎉
A question about the test itself: I'm confused about why we have a specific upgrade in the test name ("V231-V232") but the test itself is running DDLs that are "supported since 22.2". Does it matter, then, what upgrade is being run, as long as it's at least V22_2?
IMO, having the version in the test name is confusing also because it might not reflect reality. For example, master will soon become 24.1, at which point we are not testing V231-V232 anymore. Should we skip the test at that point? From the test name, I'd say yes, but now that I took a closer look at the code, the answer is probably no (?).
I'm aware this PR is just updating an existing test, so these questions are optional and just for general consideration.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status:
pkg/cmd/roachtest/tests/backup.go line 370 at r3 (raw file):
require.NoError(t, err) } }
Thank you! 🔥
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 31 at r3 (raw file):
to-be-tests stmts
Is this a typo? Not sure what this means.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 54 at r3 (raw file):
_, err := db.ExecContext(ctx, query, args...) return err }
Can we change runQuery to be:
runQuery := func(query string, args ...interface{}) error {
return h.Exec(r, query, args...)
}pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 71 at r3 (raw file):
return err } return nil
This should be h.Exec(r, "ALTER RANGE ...").
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 99 at r3 (raw file):
var nodes option.NodeListOption if testingUpgradedNodes { nodes = helper.Context().ToVersionNodes
This branch has drifted a bit from master. The functions you'll want to use for the purposes of this test are: helper.Context.NodesInPreviousVersion() and helper.Context.NodesInNextVersion().
Also, something to be aware of is that we are not guaranteed to be in mixed-binary state when this function is called. For example, it might be called when all nodes are running the master binary, but the cluster version is still fixed at V23_1. In this case, NodesInPreviousVersion will return an empty list. You can detect whether this is the case by using helper.Context.MixedBinary().
I'll let your team decide what to do in this situation. A common approach is to just pick a random node in this case (example below). I think this is what should happen here as well since testing this scenario is important too.
cockroach/pkg/cmd/roachtest/tests/secondary_indexes.go
Lines 67 to 72 in b847659
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 102 at r3 (raw file):
} nodes = helper.Context().FromVersionNodes testUtils, err := newCommonTestUtils(ctx, t, c, nodes)
I believe it is currently technically harmless for the use of testUtils in this test, but that last argument (nodes) is supposed to encode the set of cockroach nodes in the cluster. You can use helper.Context.CockroachNodes.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 113 at r3 (raw file):
} node := option.NodeListOption{helper.RandomNode(r, nodes)} planAndRunSchemaChange(ctx, t, c, node, `COMMENT ON DATABASE testdb IS 'this is a database comment'`)
I don't think this planAndRunSchemaChange function is adding much -- IIUC, it just logs the statement and executes it.
I suggest we drop that function and just have a list of statements, iterate over item and run them with h.ExecWithGateway(rng, nodes, stmt).
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 143 at r3 (raw file):
// Set up the testing state (e.g. create a few databases and tables) and always use declarative schema // changer on all nodes. Queries run in this function should be idempotent.
Doesn't this function need to be called more than just OnStartup? Some of the tables/sequences are dropped during the test.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 158 at r3 (raw file):
CREATE SEQUENCE IF NOT EXISTS testdb.testsc.s; CREATE VIEW IF NOT EXISTS testdb.testsc.v AS (SELECT i*2 FROM testdb.testsc.t); CREATE OR REPLACE FUNCTION fn(a INT) RETURNS INT AS 'SELECT a*a' LANGUAGE SQL;
Nit: Indentation on this last statement is off.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 160 at r3 (raw file):
CREATE OR REPLACE FUNCTION fn(a INT) RETURNS INT AS 'SELECT a*a' LANGUAGE SQL; ` if _, err := db.ExecContext(ctx, setUpQuery); err != nil {
No need to create a new connection pool (db above) for this. We can use h.Exec(r, setupQuery).
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 169 at r3 (raw file):
// buried by the fallback. for _, node := range c.All() { db := c.Conn(ctx, t.L(), node)
This is creating a new connection pool for every node, and not closing it (you'll see the related "leaked goroutines" list when you run this test on a gceworker).
I suggest using h.Exec(r, "SET ...").
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 181 at r3 (raw file):
mvt.OnStartup("set short job interval", setShortJobIntervalsStep) mvt.OnStartup("set short gcttl", setShortGCTTLInSystemZoneConfig) mvt.OnStartup("", testSetupResetStep)
Can we give this step a name?
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 194 at r3 (raw file):
// Testing that declarative schema change jobs created by nodes running older binary version can be adopted and // finished by nodes running newer binary versions. mvt.InMixedVersion("run test step on non upgraded node", executeSupportedDDLs(c, t, false /* testingUpgradedNodes */))
I believe we won't be able to have these steps as separate InMixedVersion functions. This means that they might possibly run in parallel, leading to a state where we disable job adoption on all nodes.
I think the upgrade path here will be solely dependent on which branch we are on (as in - I don't believe we want to have a test plan where the upgrade starts at V22_2 on this branch since a group of the DDL statements that will be executed are only enabled on V23_1); thus, we will have to update this test whenever master changes to a new version I guess it won't necessarily hurt if we execute these statements on V22_2 since I believe it will just test job compatibility in the legacy schema changer, but that sounds like it would miss the point of this test (?) Maybe to make this point more evident, I can delete the batch of V22_2 statements on this branch (then the only place where this group will occur is on the backport) |
38f5ca1 to
77b60f8
Compare
annrpom
left a comment
There was a problem hiding this comment.
master will soon become 24.1, at which point we are not testing V231-V232 anymore
ah also yes - i'll make sure to update this to be V232-V241 accordingly once that changes
also, i just realized i have not published these comments :/
Reviewable status:
pkg/cmd/roachtest/tests/backup.go line 291 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
based on this TODO, it looks like there might be another function we should use
yes - it looks like these aren't used anymore by anything @renatolabs let me know if you would like for me to keep these in
pkg/cmd/roachtest/tests/backup.go line 355 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
based on this TODO, it looks like there might be another function we should use
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 34 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: we should rename this, since now we want to test
V231-V232
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 91 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i think as a separate commit in this same PR, we should change this function so it tests v23_1 <-> v23_2 compatibility.
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 168 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: this test specifically is testing compat between the last version and this one, so we should add a
mixedversion.NumUpgrades(1)option
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 31 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
to-be-tests stmts
Is this a typo? Not sure what this means.
I reworded it - to me, this emphasized the fact that we will have to perform some maintenance every time master changes
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 54 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Can we change
runQueryto be:runQuery := func(query string, args ...interface{}) error { return h.Exec(r, query, args...) }
indeed we can - done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 71 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
This should be
h.Exec(r, "ALTER RANGE ...").
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 99 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
This branch has drifted a bit from master. The functions you'll want to use for the purposes of this test are:
helper.Context.NodesInPreviousVersion()andhelper.Context.NodesInNextVersion().Also, something to be aware of is that we are not guaranteed to be in mixed-binary state when this function is called. For example, it might be called when all nodes are running the master binary, but the cluster version is still fixed at
V23_1. In this case,NodesInPreviousVersionwill return an empty list. You can detect whether this is the case by usinghelper.Context.MixedBinary().I'll let your team decide what to do in this situation. A common approach is to just pick a random node in this case (example below). I think this is what should happen here as well since testing this scenario is important too.
cockroach/pkg/cmd/roachtest/tests/secondary_indexes.go
Lines 67 to 72 in b847659
ah good point - done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 102 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
I believe it is currently technically harmless for the use of
testUtilsin this test, but that last argument (nodes) is supposed to encode the set of cockroach nodes in the cluster. You can usehelper.Context.CockroachNodes.
done - thank you for catching this
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 113 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
I don't think this
planAndRunSchemaChangefunction is adding much -- IIUC, it just logs the statement and executes it.I suggest we drop that function and just have a list of statements, iterate over item and run them with
h.ExecWithGateway(rng, nodes, stmt).
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 169 at r3 (raw file):
Previously, renatolabs (Renato Costa) wrote…
This is creating a new connection pool for every node, and not closing it (you'll see the related "leaked goroutines" list when you run this test on a gceworker).
I suggest using
h.Exec(r, "SET ...").
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 128 at r4 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
do we need to use the
sql.schema.force_declarative_statementscluster setting to make these use DSC when we want it?
https://cockroachlabs.slack.com/archives/C04N0AS14CT/p1700596759258929 there was a discussion here about this question, but I clarified with Faizan and this is only needed for statements are not "on" (CREATE [SCHEMA, SEQUENCE])
the group of statements we are testing are all on by default - i don't think we need to do this for this PR
77b60f8 to
3a1e9c9
Compare
renatolabs
left a comment
There was a problem hiding this comment.
Minor comments, this looks great! 👏
Reviewable status:
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 75 at r8 (raw file):
testingUpgradedNodes bool, ) error { nodes := c.All().RandNode()
I suggest we use helper.RandomNode instead to make sure we always pick the same node when running with the test with a specific seed (e.g., trying to reproduce a failure). c.All().RandNode() uses the global RNG, which is outside of our control.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 186 at r8 (raw file):
mvt.OnStartup("set short gcttl", setShortGCTTLInSystemZoneConfig) mvt.InMixedVersion("run test step", func(ctx context.Context, l *logger.Logger, r *rand.Rand, helper *mixedversion.Helper) error {
Should we run this function after the upgrade finishes too?
3a1e9c9 to
2df2cc0
Compare
annrpom
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 75 at r8 (raw file):
Previously, renatolabs (Renato Costa) wrote…
I suggest we use
helper.RandomNodeinstead to make sure we always pick the same node when running with the test with a specific seed (e.g., trying to reproduce a failure).c.All().RandNode()uses the global RNG, which is outside of our control.
done
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 186 at r8 (raw file):
Previously, renatolabs (Renato Costa) wrote…
Should we run this function after the upgrade finishes too?
it won't hurt if we do - done
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go line 128 at r4 (raw file):
Previously, annrpom (annie pompa) wrote…
https://cockroachlabs.slack.com/archives/C04N0AS14CT/p1700596759258929 there was a discussion here about this question, but I clarified with Faizan and this is only needed for statements are not "on" (CREATE [SCHEMA, SEQUENCE])
the group of statements we are testing are all on by default - i don't think we need to do this for this PR
i see - but i think that maybe we should test CREATE SCHEMA/SEQUENCE in this test if possible. since those were added during 23.2.
I could be wrong, but I understood that the tests will test that the jobs created by statements implemented in the earlier testing version (so 23.1 in this case) could be picked up by newer versions (23.2) and vice versa for forwards and backwards compatibility adding statements in 23.2 would mean that we would expect a job created by the dsc to be picked up by an earlier node version with the legacy schema changer (and vice versa) - is that true/is that what we want to test here? cc: @Xiang-Gu |
rafiss
left a comment
There was a problem hiding this comment.
ah i see, yes i think that's right. this looks good from my end! just check if @renatolabs has any other comments
Reviewed 1 of 2 files at r5, 1 of 1 files at r11, all commit messages.
Reviewable status:
renatolabs
left a comment
There was a problem hiding this comment.
Reviewable status:
2df2cc0 to
cf3a7b4
Compare
|
TFTRs! ('-')7 bors r=rafiss, renatolabs |
|
Build failed (retrying...): |
|
bors r- |
|
Canceled. |
This patch updates the `declarative_schema_changer/job-compatibility-mixed-version-V222-V231` roachtest to use the new mixed version testing framework - which in turn provides testing enhancements. Informs: cockroachdb#110536 Epic: None Release note: None
This patch updates the list of supported DDLs/the job compat roachtest to declarative_schema_changer/job-compatibility-mixed-version-V231-V232. Fixes: cockroachdb#110536 Epic: None Release note: None
cf3a7b4 to
7d89e18
Compare
|
needed to rebase TFTRs! ('-')7 bors r=rafiss, renatolabs |
|
Build succeeded: |
|
blathers backport 23.2 |
roachtest: move decl_schema_change/job-compat to new framework
This patch updates the
declarative_schema_changer/job-compatibility-mixed-version-V222-V231roachtest to use the new mixed version testing framework - which in turn provides testing enhancements.Informs: #110536
Epic: None
Release note: None
roachtest: update version of job compat mixed-version roachtest
This patch updates the list of supported DDLs/the job compat roachtest
to declarative_schema_changer/job-compatibility-mixed-version-V231-V232.
Fixes: #110536
Epic: None
Release note: None