feature(views-with-tablets): enable testing materialized views with tablets #9615

wmitros · 2024-12-24T13:54:08Z

We plan to disallow materialized views in tablet keyspaces, until the remaining issues with them
are resolved. For that, we've introduced the "views-with-tablets" experimental feature, so that
they can be enabled regardless, for example for testing. Currently, the feature has no effect,
but we want to prepare the cluster tests to use it, so that they won't start failing when it
starts actually having an effect.

While improving views with tablets, we should make sure that the existing tests keep passing,
so we should run materialized view (and index) tests with tablets using the experimental feature.

The feature doesn't affect tests ran on vnodes. For tablets, the tests would fail without the feature
due to unsupported configuration.

Considering that, we should enable the feature by default for all tests running on 2025.1 or later.

The change includes:

adjusting experimental_features based on scylla version in sct_config, setting the new feature
when running on 2025.1 or later
adding a new parameter "enable_views_with_tablets_on_upgrade". When it's set, we enable the new
feature in upgrade_test.py after upgrading a node. Currently, there are no upgrade tests to 2025.1,
so it isn't used
adding a config file configurations/enable_views_with_tablets_on_upgrade.yaml that enables the
mentioned parameter without setting an env variable

wmitros · 2024-12-24T14:30:51Z

Looks like yaml tests aren't using the default configurations, fixed in rebase

wmitros · 2025-01-07T15:38:05Z

Refs scylladb/scylladb#21832

piodul · 2025-01-13T07:38:29Z

@wmitros why is this PR a draft? What is missing?

wmitros · 2025-01-13T10:40:20Z

@wmitros why is this PR a draft? What is missing?

I think it's ready, forgot to change the type

piodul · 2025-01-14T13:44:20Z

defaults/test_default.yaml

@@ -110,6 +110,8 @@ skip_download: false
 authenticator_user: ''
 authenticator_password: ''

+experimental_features: ['views-with-tablets']


Materialized views are currently not allowed in tablet keyspaces, due to some
pending issues. However, they can be enabled using the "views-with-tablets" experimental feature.

This is not technically true, they can be created in tablet keyspaces right now but we want to forbid this. We introduced an experimental feature first, now we want to adjust the test and, finally, we will forbid creating materialized views without the experimental feature being enabled.

Just clarifying so that other reviewers won't get confused why tests that use MVs are still working. I recommend updating the description.

When testing upgrades to future versions, we'll need to
keep the experimental_feature also for base version, with an exception for sequential upgrades,
(currently we only have 2021.1->2022.1->2023.1 test in test_custom_profile_sequential_rolling_upgrade)
where the base version of the sequence might not have this experimental feature, so we'll need
to adjust it accordingly.

It would be great if the feature were enabled programmatically for the base version as well, but I'm not familiar with SCT enough to tell you how to do that. @fruch perhaps you will know?

If it's not feasible, alternatively you could add a comment near the experimental_features key which explains when the views-with-tablets feature needs to be provided and when not.

Materialized views are currently not allowed in tablet keyspaces, due to some
pending issues. However, they can be enabled using the "views-with-tablets" experimental feature.

This is not technically true, they can be created in tablet keyspaces right now but we want to forbid this.

I updated the description for the current state

It would be great if the feature were enabled programmatically for the base version as well, but I'm not familiar with SCT enough to tell you how to do that. @fruch perhaps you will know?

I'm not sure there is such a way in general for all tests. I saw that when tablets were introduced, we added a enable_tablets_on_upgrade config option that we used on specific test pipelines - we could probably do something like that here as well, but currently no tests would use it, as we don't have tests for upgrading to 2025.1, and the developers that were adding these tests would have to remember about it.

If it's not feasible, alternatively you could add a comment near the experimental_features key which explains when the views-with-tablets feature needs to be provided and when not.

For now I added a comment for the experimental_features configuration option

The only downside of using this in default for everything, that is gonna block people from using older versions

And it's gonna waste lots of people's time,

We are using older releases during development

I think this is different from the regular extra configuration in scylla yaml that wasn't there before, and existence of it does not break anything

I think moving it into the with the same version logic you used in upgrade tests

Older versions of scylla which do not support this experimental feature will not recognize it, and will fail to start. If we are using SCT with older versions of scylla then it sounds like it will be a problem.

@fruch is there a way to adjust the configuration programmatically?

I found an old PR which removed the experimental "tablets" feature from SCT (#7512). It looks like the approach there was to modify Jenkinsfiles (configurations/raft/enable_raft_experimental.yaml). Would it be a viable approach here instead of adding "views-with-tablets" to the list of default features?

@piodul
I agree with @fruch that defaults must not limit us to only one scylla version.
For example, I use various scylla versions with the master SCT branch all the time.

Yes, additional small file in the configurations is good solution.

Also, note that each CI job can be provided with the updated SCT config option in the jenkins pipeline extra_environment_variables parameter.
One of the recent examples:

In this case it is not needed to update SCT configs.

So, choose whatever fits your needs better.

By the way, today I was hit with the same problem in test/cqlpy/run's "--release" feature which is supposed to allow running old versions of Scylla, but the newly added views-with-tablets doesn't work on old versions. My fix is scylladb/scylladb#22350. Basically test/cqlpy/run.py has a rather-ugly list of configuration "overrides", specific options that need to be added or removed from specific past versions. This list is ugly, but isn't long or hard, and I'm rather please with the power it gives me to run tests against old versions (e.g., just today in scylladb/scylladb#22354 I wrote a test and then ran it against 10 different versions of Scylla).

@piodul

there two place we can do it programmatically:

in proposed_scylla_yaml, where we read append_scylla_yaml and use it

in sct_config.py after sct_config.get_version_based_on_conf() is called, and we have the version information

the first is the simplest, but it's buried in not so clear place
the 2nd might be better and clear place for more changes per release we need to do in SCT configuration.

in sct_config.py after sct_config.get_version_based_on_conf() is called, and we have the version information

I added a new method for changing the config based on the release in sct_config.py after sct_config.get_version_based_on_conf(), please take a look

piodul · 2025-01-14T14:01:46Z

upgrade_test.py

+        version = ComparableScyllaVersion(self.scylla_version)
+        if self.is_enterprise and version >= "2025.1.0~dev" or not self.is_enterprise and version >= "6.3.0~dev":
+            scylla_yaml_updates.update({"experimental_features": ["views-with-tablets"]})


Do we need something similar in _rollback_node, too?

I assumed we reused the original scylla.yaml after rollback because we don't revert other updates (like enable_tablets) to it either, but I don't see it explicitly so @scylladb/qa-maintainers please confirm or deny

I don't know, someone need to cross check it in the code

Anyhow I would recommend appending to the flags, now the code is override anything that any might have put in there before

We restore original scylla.yaml file. See following:
https://github.com/scylladb/scylla-cluster-tests/blame/c22fba4d/upgrade_test.py#L434

upgrade_test.py

vponomaryov · 2025-01-16T18:11:33Z

defaults/test_default.yaml

@@ -110,6 +110,8 @@ skip_download: false
 authenticator_user: ''
 authenticator_password: ''

+experimental_features: ['views-with-tablets']


@piodul
I agree with @fruch that defaults must not limit us to only one scylla version.
For example, I use various scylla versions with the master SCT branch all the time.

Yes, additional small file in the configurations is good solution.

Also, note that each CI job can be provided with the updated SCT config option in the jenkins pipeline extra_environment_variables parameter.
One of the recent examples:

In this case it is not needed to update SCT configs.

So, choose whatever fits your needs better.

docs/configuration_options.md

vponomaryov · 2025-01-16T18:37:39Z

upgrade_test.py

+        version = ComparableScyllaVersion(self.scylla_version)
+        if self.is_enterprise and version >= "2025.1.0~dev" or not self.is_enterprise and version >= "6.3.0~dev":
+            scylla_yaml_updates.update({"experimental_features": ["views-with-tablets"]})


We restore original scylla.yaml file. See following:
https://github.com/scylladb/scylla-cluster-tests/blame/c22fba4d/upgrade_test.py#L434

wmitros · 2025-01-19T23:40:01Z

In the new version I change the views-with-tablets feature to be enabled programmatically, based on the version.
I also added a new config parameter enable_view_with_tablets_on_upgrade to be used for 2025.1 upgrade tests

docs/configuration_options.md

piodul · 2025-01-20T07:11:14Z

sdcm/sct_config.py

@@ -2770,6 +2776,10 @@ def update_argus_with_version(self, scylla_version: str, package_name: str):
        except Exception as exc:  # pylint: disable=broad-except
            self.log.exception("Failed to save target Scylla version in Argus", exc_info=exc)

+    def update_config_based_on_version(self):
+        if self.is_enterprise and ComparableScyllaVersion(self.scylla_version) >= "2025.1.0~dev":
+            self['experimental_features'] = ['views-with-tablets']


Shouldn't we rather add the views-with-tablets feature to the list instead of overwriting it? What if there are other experimental features enabled there already?

I don't think any test is currently using other experimental features, but it should be possible for new/manual tests. Added in rebase

piodul · 2025-01-20T07:11:50Z

sdcm/sct_config.py

+        dict(name="enable_views_with_tablets_on_upgrade", env="SCT_ENABLE_VIEWS_WITH_TABLETS_ON_UPGRADE", type=boolean,
+             help="Enables creating materialized views in keyspaces using tablets by adding an experimental feature."
+                  "It should not be used when upgrading to versions before 2025.1 and it should be used for upgrades"
+             "where we create such views."),


Nit: one more indent on this line, please

Unfortunately, pre-commit is requiring this indentation

…ablets We plan to disallow materialized views in tablet keyspaces, until the remaining issues with them are resolved. For that, we've introduced the "views-with-tablets" experimental feature, so that they can be enabled regardless, for example for testing. Currently, the feature has no effect, but we want to prepare the cluster tests to use it, so that they won't start failing when it starts actually having an effect. While improving views with tablets, we should make sure that the existing tests keep passing, so we should run materialized view (and index) tests with tablets using the experimental feature. The feature doesn't affect tests ran on vnodes. For tablets, the tests would fail without the feature due to unsupported configuration. Considering that, we should enable the feature by default for all tests running on 2025.1 or later. The change includes: * adjusting experimental_features based on scylla version in sct_config, setting the new feature when running on 2025.1 or later * adding a new parameter "enable_views_with_tablets_on_upgrade". When it's set, we enable the new feature in upgrade_test.py after upgrading a node. Currently, there are no upgrade tests to 2025.1, so it isn't used * adding a config file configurations/enable_views_with_tablets_on_upgrade.yaml that enables the mentioned parameter without setting an env variable

piodul · 2025-01-21T09:21:48Z

@fruch @vponomaryov please re-review - we need this to unblock scylladb/scylladb#22217, which is required for 2025.1

fruch

LGTM

request were resolved

fruch · 2025-01-21T19:30:23Z

configurations/enable_views_with_tablets_on_upgrade.yaml

nothing is using this one, it's for doing things manual ?

do we want to leave a job that is using this ?

Annamikhlin · 2025-01-21T21:28:12Z

artifacts-centos9-test - https://jenkins.scylladb.com/job/releng-testing/job/artifacts/job/artifacts-centos9-test/87/
artifacts-rocky8-test - https://jenkins.scylladb.com/job/releng-testing/job/artifacts/job/artifacts-rocky8-test/88/

failed with error:

p:ERROR >     if 'views-with-tablets' not in self.get('experimental_features', []):
p:ERROR > TypeError: SCTConfiguration.get() takes 2 positional arguments but 3 were given
21:42:00  ERROR: script returned exit code 1

@wmitros / @fruch - could you please take a look? it's preventing from next to pass

…atures Because the SCTConfiguration is also a dict, in scylladb#9615 we used the dict's `get` method with a default value to check the contents of the list of experimental features (there may be none). However, SCTConfiguration overrides this `get` method without allowing any defaults. As a result, we called it with too many parameters. We work around this in this patch by using an extra if.

wmitros · 2025-01-21T22:22:18Z

artifacts-centos9-test - https://jenkins.scylladb.com/job/releng-testing/job/artifacts/job/artifacts-centos9-test/87/ artifacts-rocky8-test - https://jenkins.scylladb.com/job/releng-testing/job/artifacts/job/artifacts-rocky8-test/88/

failed with error:
p:ERROR >     if 'views-with-tablets' not in self.get('experimental_features', []):
p:ERROR > TypeError: SCTConfiguration.get() takes 2 positional arguments but 3 were given
21:42:00  ERROR: script returned exit code 1
@wmitros / @fruch - could you please take a look? it's preventing from next to pass

I posted #9894, I'm surprised this wasn't caught by CI

fruch · 2025-01-21T22:26:10Z

artifacts-centos9-test - https://jenkins.scylladb.com/job/releng-testing/job/artifacts/job/artifacts-centos9-test/87/ artifacts-rocky8-test - https://jenkins.scylladb.com/job/releng-testing/job/artifacts/job/artifacts-rocky8-test/88/
failed with error:
p:ERROR >     if 'views-with-tablets' not in self.get('experimental_features', []):
p:ERROR > TypeError: SCTConfiguration.get() takes 2 positional arguments but 3 were given
21:42:00  ERROR: script returned exit code 1
@wmitros / @fruch - could you please take a look? it's preventing from next to pass
I posted #9894, I'm surprised this wasn't caught by CI

you beat me in few minutes,

I've posted a fix as well (but with a unittest)
#9895

fruch · 2025-01-21T22:30:07Z

@wmitros

CI unittest test didn't cover it, cause it's a new functionality that was added

other test even if we would have run them (integration, or longevity) might miss that if we were running it with anything but master (CI doesn't run with master by default, it run with older releases)

wmitros · 2025-01-21T22:36:38Z

@wmitros / @fruch - could you please take a look? it's preventing from next to pass

I posted #9894, I'm surprised this wasn't caught by CI

you beat me in few minutes,

I've posted a fix as well (but with a unittest) #9895

Adding a unittest seems useful, so I'll close mine

github-actions bot assigned wmitros Dec 24, 2024

wmitros added the backport/none Backport is not required label Dec 24, 2024

wmitros force-pushed the forbid-tablet-mv branch from 0beb533 to 2d59969 Compare December 24, 2024 14:30

wmitros force-pushed the forbid-tablet-mv branch from 2d59969 to 8380a4c Compare December 28, 2024 13:02

wmitros marked this pull request as ready for review January 13, 2025 10:39

wmitros force-pushed the forbid-tablet-mv branch from 8380a4c to 8e26385 Compare January 13, 2025 21:36

wmitros requested a review from fruch January 13, 2025 22:00

piodul reviewed Jan 14, 2025

View reviewed changes

wmitros force-pushed the forbid-tablet-mv branch 3 times, most recently from 573509a to 5f575d9 Compare January 15, 2025 22:08

vponomaryov previously requested changes Jan 16, 2025

View reviewed changes

wmitros force-pushed the forbid-tablet-mv branch 4 times, most recently from 3553708 to 50162cd Compare January 19, 2025 23:12

piodul reviewed Jan 20, 2025

View reviewed changes

wmitros force-pushed the forbid-tablet-mv branch from 50162cd to 5f9267d Compare January 20, 2025 09:54

piodul approved these changes Jan 20, 2025

View reviewed changes

piodul requested a review from vponomaryov January 20, 2025 17:34

fruch approved these changes Jan 21, 2025

View reviewed changes

fruch reviewed Jan 21, 2025

View reviewed changes

fruch merged commit 0ccab43 into scylladb:master Jan 21, 2025
6 checks passed

scylladbbot added the promoted-to-master label Jan 21, 2025

wmitros mentioned this pull request Jan 21, 2025

fix(sct_config): use the 'get' correctly when reading experimental features #9894

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(views-with-tablets): enable testing materialized views with tablets #9615

feature(views-with-tablets): enable testing materialized views with tablets #9615

wmitros commented Dec 24, 2024 •

edited

Loading

wmitros commented Dec 24, 2024

wmitros commented Jan 7, 2025

piodul commented Jan 13, 2025

wmitros commented Jan 13, 2025

piodul Jan 14, 2025

piodul Jan 14, 2025

wmitros Jan 15, 2025

fruch Jan 16, 2025

piodul Jan 16, 2025

vponomaryov Jan 16, 2025

nyh Jan 16, 2025

fruch Jan 16, 2025

wmitros Jan 19, 2025

piodul Jan 14, 2025

wmitros Jan 15, 2025

fruch Jan 16, 2025

vponomaryov Jan 16, 2025

vponomaryov Jan 16, 2025

vponomaryov Jan 16, 2025

wmitros commented Jan 19, 2025

piodul Jan 20, 2025

wmitros Jan 20, 2025

piodul Jan 20, 2025

wmitros Jan 20, 2025

piodul commented Jan 21, 2025

fruch left a comment

fruch Jan 21, 2025

Annamikhlin commented Jan 21, 2025

wmitros commented Jan 21, 2025

fruch commented Jan 21, 2025

fruch commented Jan 21, 2025

wmitros commented Jan 21, 2025

feature(views-with-tablets): enable testing materialized views with tablets #9615

feature(views-with-tablets): enable testing materialized views with tablets #9615

Conversation

wmitros commented Dec 24, 2024 • edited Loading

wmitros commented Dec 24, 2024

wmitros commented Jan 7, 2025

piodul commented Jan 13, 2025

wmitros commented Jan 13, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmitros commented Jan 19, 2025

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

piodul commented Jan 21, 2025

fruch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Annamikhlin commented Jan 21, 2025

wmitros commented Jan 21, 2025

fruch commented Jan 21, 2025

fruch commented Jan 21, 2025

wmitros commented Jan 21, 2025

wmitros commented Dec 24, 2024 •

edited

Loading