The test_restore_primary_replica_same_domain and test_restore_primary_replica_different_domain tests have very much in common. Previously both tests were also split each into two, so we have four tests, and now we have two that can also be squashed, the lines-of-code savings still worth it.
This is the continuation of #28569
Tests improvement, not backporting
Closesscylladb/scylladb#28994
* github.com:scylladb/scylladb:
test: Replace a bunch of ternary operators with an if-else block
test: Squash test_restore_primary_replica_same|different_domain tests
test: Use the same regexp in test_restore_primary_replica_different|same_domain-s
The do_test_backup_abort() fetched the node's workdir and resolved cf_dir
solely to construct a unique-ish backup prefix:
prefix = f'{cf_dir}/backup'
The comment already acknowledged this was only "unique(ish)" — relying
on the UUID-derived cf_dir name as a uniqueness source is roundabout.
unique_name() is already imported and used for exactly this purpose
elsewhere in the file.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#29030
Trie-based sstable indexes are supposed to be (hopefully) a better default than the old BIG indexes.
Make the new format a new default for new clusters by naming ms in the default scylla.yaml.
New functionality. No backport needed.
This PR is basically Michał's one https://github.com/scylladb/scylladb/pull/26377, Jakub's https://github.com/scylladb/scylladb/pull/27332 fixing `sstables_manager::get_highest_supported_format()` and one test fix.
Closesscylladb/scylladb#28960
* github.com:scylladb/scylladb:
db/config: announce ms format as highest supported
db/config: enable `ms` sstable format by default
cluster/dtest/bypass_cache_test: switch from highest_supported_sstable_format to chosen_sstable_format
api/system: add /system/chosen_sstable_version
test/cluster/dtest: reduce num_tokens to 16
A followup of the merge of two test cases that happened in the previous
patch. Both used `foo = N if domain == bar else M` to evaluate the
parameters for topology. Using if-else block makes it immediately obvious
which topology and scope apply for each domain value without having to
evaluate multiple inline conditionals.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The two tests differ only in the way they set up the topology for the
cluster and the post-restore checks against the resulting streams.
The merge happens with the help of a "scope_is_same" boolean parameter
and corresponding updates in the topology setup and post-checks.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The one in "different domain" test is simpler because the test performs
less checks. Next patch will merge both tests and making regexp-s look
identical makes the merge even smother.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Fixes: SCYLLADB-244
Disables snapshot control such that any active ops finish/fail
before proceeding with decommission.
Note: snapshot control provided as argument, not member ref
due to storage_service being used from both main and cql_test_env.
(The latter has no snapshot_ctl to provide).
Could do the snapshot lockout on API level, but want to do
pre-checks before this.
Note: this just disables backup/snapshot fully. Could re-enable
after decommission, but this seems somewhat pointless.
v2:
* Add log message to snapshot shutdown
* Make test use log waiting instead of timeouts
Closesscylladb/scylladb#28980
Some tests, when create a cluster, configure nodes with the rf-rack-valid option, because sometimes they want to have it OFF. For that the option is explicitly carried around, but the cluster creating helper can guess this option itself -- out of the provided topology and replication factor.
Removing this option simplifies the code and (which a nicer outcome) the test "signature" that's used e.g. in command-line to run a specific test.
Improving tests, not backporting
Closesscylladb/scylladb#28860
* github.com:scylladb/scylladb:
test: Relax topology_rf_validity parameter for some tests
test: Auto detect rf-rack-valid option in create_cluster()
changes in this commit:
1)rename class from 'TestContext' to 'Context' so pytest will not consider this class as a test
2)extend pytest filterwarnings list to ignore warnings from external libs
3) use datetime.datetime.now(datetime.UTC) unstead datetime.datetime.utcnow()
4) use ResultSet.one() instead ResultSet[0]
Fixes SCYLLADB-904
Fixes SCYLLADB-908
Related SCYLLADB-902
Closesscylladb/scylladb#28956
Tests that call create_cluster() helper no longer need to carry the
rf-validity parameter. This simplifies the code and test signature.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The helper accepts its as boolean argument, but it can easily estimate
one from the provided topology.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Fixes#25084
Add slirp4netns and use for nested containers. This will allow nested container port aliasing, helping CI stability.
Note: this contains and updated Dockerfile for dbuild image, but since chicken and eggs, right now will force install slirp4netns before anything in dbuild script.
Updates the mock server handling to use ephemeral ports and query from container, ensuring we don't get port collisions. (boost as well as pytest).
Includes a timeout up, and a tweak to our scylla_cluster handling, ensuring we don't deadlock when pipe size is less than requires for our sys notify messages.
Closesscylladb/scylladb#28727
* github.com:scylladb/scylladb:
gcs_fixture: Change to use docker helper
aws_kms_fixture: Modify to use docker helper
test/lib/proc_util: Add docker helper
pytest: use ephemeral port publish for docker mock servers
dbuild: Use container network in dbuild nested containers
scylla_cluster: Read notify sock in background to prevent deadlock
Changes dockerized_service to use ephermal port publish, and
query the published port from podman/docker.
Modifies client code to use slightly changed usage syntax.
The test description of refreshing test is very elaborated and it's
worth having it as the description of the streaming scopes test itself.
Callers of the helper can go with smaller descriptions.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This helper does two things -- sorts sstables per server according to
scope in use and calls sstables_storage.restore(). The code looks better
if the sorting of sstables stays in a helper and the call for .restore()
is moved to the caller.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Now it's possible to replace the whole body of the
test_refresh_with_streaming_scopes() test by calling the corresponding
helper function from backup/restore test module. This helper does
exactly the same, and the SSTablesOnLocalStorage class provides the
necessary save/restore implementations.
One more thing to mention -- the refreshing test for some reason only
wants to run with restored min-tablet-count equal to the original one.
The do_test_streaming_scopes() needs to account for that, as it runs the
tests for more options.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The class in question performs two operations for
do_test_streaming_scopes(): saves sstables and restores them. Current
caller of the helper is the test_restore_with_streaming_scopes() test
that need to backup sstables on object storage and restore them from
there with the restoration API. The SSTablesOnObjectStorage class does
exactly that.
The change in do_load_sstables() that checks for sstables_storage to be
non None is needed to keep test_refresh_with_streaming_scopes() work --
that test doesn't provide sstables_storage (yet) and the function in
question will call the load_fn callback. Next patch will eliminate it.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The body of this test is duplicated by
test_refresh_with_streaming_scopes() test from other module. Keeping it
in a non-test top-level function will help generalizing these two tests.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Trie-based sstable indexes are supposed to be (hopefully)
a better default than the old BIG indexes.
Make them the new default.
If we change our mind, this change can be reverted later.
The one accepts long list of arguments, some of those is not really needed. Also some callers can be relaxed not to provide default values for arguments with such.
Improving tests, not backporting
Closesscylladb/scylladb#28861
* github.com:scylladb/scylladb:
test: Remove passing default "expected_replicas" to check_mutation_replicas()
test: Remove scope and primary-replica-only arguments from check_mutation_replicas() helper
This change is a bit more careful, as the test collects files from
snapshot directory several times. Before patching it to use the helper,
it collected _all_ the files. Now the helper only provides TOC-s, but
that's fine -- the only check that relies on that may also re-collect
TOC-s and compare new set with old set.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Some of those tests need to update the hard-coded 'backup' snapshot name
to use the one provided by take_snapshot() helper. Other than that, the
patching is pretty straightforward.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The take_snapshot() helper returns a dict(server: list[string]). When
there's only one server to work with, it's more handy to just get a
single list of sstables.
Next patches will make use of that helper.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
These two are only used to print into logs on error. However, their
values can be found from previous logs and test execution context.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The test uses create_dataset helper duplicating the existing code that does the same. This PR patches basic tests to use standard facilities.
Also the PR simplifies the 3-level nested loops used to combine several sets of restoration parameters by using itertools.product facility.
Continuation of #28600.
Cleaning tests, not backporting
Closesscylladb/scylladb#28608
* github.com:scylladb/scylladb:
test/object_store: Use itertools.product() for deeply nested loops
test/object_store: Replace dataset creation usage with standard methods
test/object_store: Shift indentation right for test_restore_with_streaming_scopes
The helper is very simple yet generic -- it takes a snapshot of a keyspace on all servers and collects the resulting sstables from workdirs. Re-using it in all test cases saves some lines of code. Also, the method is "sequential", making it "parallel" reduces the waiting time a bit.
Will help generalizing existing backup/restore tests to support clustered snapshot/backup/restore API (see #28525) later.
Cleaning up tests, not backporting.
Closesscylladb/scylladb#28660
* github.com:scylladb/scylladb:
test/backup: Run keyspace flush and snapshot taking API in parallel
test/backup: Re-use take_snapshot() helper in do_abort_restore()
test/backup: Move take_snapshot() helper up
The test_restore_with_streaming_scopes want to run some loop body for
all (almost) combinations of scope, primary-replica-only and min tablet
count. For that three nested loops are used. Using itertools.product()
makes the code shorter, less indented and more explicit.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Two places are fixed
1. The call to create_dataset() is replaced with three "library"
methods. This makes it explicit which options and schema are used
for that. Eventually, the large and bulky create_dataset will be
removed
2. The part that restores data into a fresh new table calls some CQLs by
hand, and partially re-uses variables obtained from previous call to
create_dataset(). Using the same "library" methods to re-create an
empty table makes this part much simpler
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This is preparational patch. Next will need to replace
foo()
bar()
with
with something() as s:
foo()
bar()
Effectively -- only add the `with something()` line. Not to shift the
whole file right together with that future change, do it here.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The take_snapshot() helper runs these API sequentially for every server.
Running them with asyncio.gather() slightly reduces the wait-time thus
improving the total runtime.
Before:
CPU utilization: 2.1%
real 0m33,871s
user 0m22,500s
sys 0m13,207s
After:
CPU utilization: 2.4%
real 0m29,532s
user 0m22,351s
sys 0m12,890s
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The test in question does _exactly_ what this helper does, but in a
longer way. The only difference is that it uses server_id as key to dict
with sstable components, but it's easy to tune.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
So that it's not in the middle of tests themselves, but near other
"helper" functions in the .py file
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
remove hand rolled error handling from object storage client
and replace with common machinery that supports exception
handling and retrying when appropriate
The test uses create_ks_and_cf helper duplicating the existing code that does the same. This PR patches basic tests to use standard facilities. Also it prepares the ground for testing keyspace storage options with rf=3
Cleaning tests, not backporting
Closesscylladb/scylladb#28600
* https://github.com/scylladb/scylladb:
test/object_store: Remove create_ks_and_cf() helper
test/object_store: Replace create_ks_and_cf() usage with standard methods
test/object_store: Shift indentation right for test cases
The test_restore_with_streaming_scopes among other things checks how data streams flow while restoring. Whether or not to check the streams is decided based on the min tablet count value, which is compared with a hardcoded 512. This value of 512 matched the tablet count used by this test until it was "optimized" by #27839, where this number changed to 5 and streaming checks became off.
Good news is that the very same checks are still performed by test_refresh_with_streaming_scopes. But it's better to have a working restoration test anyway.
Minor test fix, not backporting
Closesscylladb/scylladb#28607
* github.com:scylladb/scylladb:
test: Fix the condition for streaming directions validation
test: Split test_backup.py::check_data_is_back() into two
There's a bunch of incremental repair tests that want to call scylla
sstable command. For that they try to find where scylla binary by
scanning /proc directory (see local_process_id and get_scylla_path
helpers).
There's shorter way -- just call manager.get_server_exe().
Same for backup-restore test.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#28676
There are three tests and a function with a pair of boolean parameters
called by those. It's less code if the function becomes a test with
parameters.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#28677
The test_backup_simple creates a ks/cf, takes a snapshot, backs it up,
then checks that the files were uploaded. The test_backup_move does the
same, but also plays with 'move_files' parameter to be true/false.
In fact, the "move" test was the copy of "simple" one that dropepd check
for scheduling group being "streaming" (backup with --move-files can
check the same, it's not bad), and check for destination bucket to
contain needed files (same here -- checking that files arrived to bucket
after --move-files is good).
In the end of the day, after the change backup test is run two times,
instead of three, and performs extra checks for --move-files case.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#28606
Commit ea8a661119 tried to reduce the dataset for restoration tests.
While doing it effectively disabled part of itself -- the checks for
streaming directions were never ran after this change. The thing is that
this check only runs if restored tablet count matches some hardcoded one
of 512. This was the real dataset size of the test before the
aforementioned commit, but after it it had changed to over values, and
the comparison with 512 became always False.
Fix it with a local variable to prevent such mistakes in the future.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This method does two things -- checks that the data is indeed back, and
validates streaming directions. The latter is not quite about "data is
back", so better to have it as explicit dedicated method.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
To create a keyspace theres new_test_keyspace helper
Table is created with a single cql.run_async with explicit schema
Dataset is populated with a single parallel INSERT as well
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This is preparational patch. Next will need to replace
foo()
bar()
with
with something() as s:
foo()
bar()
Effectively -- only add the `with something()` line. Not to shift the
whole file right together with that future change, do it here.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The difference is very tiny:
@@ -1,12 +1,12 @@
@pytest.mark.asyncio
async def test_restore_primary_replica_same_...(manager: ManagerClient, object_storage):
''' comment '''
- topology = topo(rf = 4, nodes = 8, racks = 2, dcs = 1)
- scope = "rack"
+ topology = topo(rf = 4, nodes = 8, racks = 2, dcs = 2)
+ scope = "dc"
ks = 'ks'
cf = 'cf'
@@ -42,7 +42,7 @@ async def test_restore_primary_replica_s
for r in res:
nodes_by_operation[r[1].group(1)].append(r[1].group(2))
- scope_nodes = set([ str(host_ids[s.server_id]) for s in servers if s.rack == servers[i].rack ])
+ scope_nodes = set([ str(host_ids[s.server_id]) for s in servers if s.datacenter == servers[i].datacenter ])
for op, nodes in nodes_by_operation.items():
logger.info(f'Operation {op} streamed to nodes {nodes}')
assert len(nodes) == 1, "Each streaming operation should stream to exactly one primary replica"
The (removed in the above example) test description comments differ only
in their usage of "rack" and "dc" words.
Squashing them into one parametrized test makes perfect sense.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
