Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workload/schemachanger: use show cluster settings for version #99162

Closed
wants to merge 1 commit into from
Closed

workload/schemachanger: use show cluster settings for version #99162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions pkg/cmd/roachtest/tests/versionupgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,6 @@ func runVersionUpgrade(ctx context.Context, t test.Test, c cluster.Cluster) {
mvt.InMixedVersion(
"test schema change step",
func(ctx context.Context, l *logger.Logger, rng *rand.Rand, h *mixedversion.Helper) error {
if c.IsLocal() {
l.Printf("skipping step in bors builds while failures are handled -- #99115")
return nil
}
l.Printf("running schema workload step")
runCmd := roachtestutil.NewCommand("./workload run schemachange").Flag("verbose", 1).Flag("max-ops", 10).Flag("concurrency", 2).Arg("{pgurl:1-%d}", len(c.All()))
randomNode := h.RandomNode(rng, c.All())
Expand Down
6 changes: 5 additions & 1 deletion pkg/workload/schemachange/operation_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3689,7 +3689,11 @@ func isClusterVersionLessThan(
ctx context.Context, tx pgx.Tx, targetVersion roachpb.Version,
) (bool, error) {
var clusterVersionStr string
row := tx.QueryRow(ctx, `SHOW CLUSTER SETTING version`)
// Directly querying the cluster version with SHOW CLUSTER
// SETTING version isn't safe, since it will wait for any
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point me at that code? Is that new behavior? I'm also seeing a flake in #99459 that seems like the same thing, but we had that test for years so I'm wondering which PR changed semantics and whether that was intentional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbg This is not a new change, but linked to the behaviour we have inside: getCurrentEncodedVersionSettingValue. We have some long-running upgrade that exceeds 2 minutes. Or that's what my theory here was, it's possible we have a hang too during the upgrade, which would be horrible. But we were stuck on the fence version about 2 minutes which is bad :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not PR author, just looking at the code, but i see that show cluster setting will fail if local value != queried value from settings table (by searching the test error), while show settings seem to be delegating to querying crdb_internal.cluster_settings which doesn't check local values? That's my best guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent was for SHOW CLUSTER VERSION to guarantee that the version on disk and in memory is synced up, indicating that the upgrade is complete. Fence versions only get updated in memory, so this is probably by design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit basically

cockroach/pkg/sql/show_cluster_setting.go

Lines 59 to 103 in a961061

return retry.WithMaxAttempts(ctx, retry.Options{}, math.MaxInt32, func() error {
return p.execCfg.InternalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
datums, err := txn.QueryRowEx(
ctx, "read-setting",
txn.KV(),
sessiondata.RootUserSessionDataOverride,
"SELECT value FROM system.settings WHERE name = $1", name,
)
if err != nil {
return err
}
var kvRawVal []byte
if len(datums) != 0 {
dStr, ok := datums[0].(*tree.DString)
if !ok {
return errors.New("the existing value is not a string")
}
kvRawVal = []byte(string(*dStr))
} else if !p.execCfg.Codec.ForSystemTenant() {
// The tenant clusters in 20.2 did not ever initialize this value
// and utilized this hard-coded value instead. In 21.1, the builtin
// which creates tenants sets up the cluster version state. It also
// is set when the version is upgraded.
tenantDefaultVersion := clusterversion.ClusterVersion{
Version: roachpb.Version{Major: 20, Minor: 2},
}
encoded, err := protoutil.Marshal(&tenantDefaultVersion)
if err != nil {
return errors.WithAssertionFailure(err)
}
kvRawVal = encoded
} else {
// There should always be a version saved; there's a migration
// populating it.
return errors.AssertionFailedf("no value found for version setting")
}
localRawVal := []byte(s.Get(&st.SV))
if !bytes.Equal(localRawVal, kvRawVal) {
// NB: errors.Wrapf(nil, ...) returns nil.
// nolint:errwrap
return errors.Errorf(
"value differs between local setting (%v) and KV (%v); try again later (%v after %s)",
localRawVal, kvRawVal, ctx.Err(), timeutil.Since(tBegin))
}

which is not applicable for show all settings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't understand is that in both #99115 and #99459, I don't see a migration taking more than a few seconds (let alone 2mins) to finish around the time of the test failure. Am I missing something?

Copy link
Contributor

@aliher1911 aliher1911 Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO once version querying code is fixed, we'll see the real issue because version that test expects will diverge and give us more sensible failure. Same in other test.
edit: this one isn't a test, so maybe we shouldn't change it until we understand why upgrade is stuck.

// upgrade to finish. So, instead show all cluster settings,
// which will bypass this strict requirement.
row := tx.QueryRow(ctx, `SELECT value FROM [SHOW CLUSTER SETTINGS] WHERE variable='version'`)
if err := row.Scan(&clusterVersionStr); err != nil {
return false, err
}
Expand Down