this is a cleanup in `scylla.spec`.
Closesscylladb/scylladb#16097
* github.com:scylladb/scylladb:
dist/redhat: group sub-package preambles together
dist/redhat: drop unused `defines` variable
dist/redhat: remove tags for subpackage which are same as main preamble
this variable was introduced in 6d7d0231. back then, we were still
building the binaries in .spec, but we've switched to the relocatable
package now, so there is no need to use keep these compilation related
flags anymore.
in this change, the `defines` variable is dropped.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
this is a cleanup.
if a subpackage is licensed under a different license from the one
specified in the main preamble, we need to use a distinct License
tag on a per-subpackage basis. but if it is licensed with the
identical license, it is not necessary. since all three
subpackages of "*-{server, conf, kernel-conf}" are licensed under
AGPLv3, there is no need to repeat the "License:" tag in their
own preamble section.
the same applies to the "URL" tag.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Update node_exporter to 1.7.0.
The previous version (1.6.1) was flagged by security scanners (such as
Trivy) with HIGH-severity CVE-2023-39325. 1.7.0 release fixed that
problem.
[Botond: regenerate frozen toolchain]
Fixes#16085Closesscylladb/scylladb#16086Closesscylladb/scylladb#16090
Fixes#15269
If segment being replayed is corrupted/truncated we can attempt skipping
completely bogues byte amounts, which can cause assert (i.e. crash) in
file_data_source_impl. This is not a crash-level error, so ensure we
range check the distance in the reader.
v2: Add to corrupt_size if trying to skip more than available. The amount added is "wrong", but at least will
ensure we log the fact that things are broken
Closesscylladb/scylladb#15270
this change is a cleanup to add `-Wignore-qualifiers` when building the tree.
to mark a return value without value semantics has no effect. these
`const` specifier useless. so let's drop them.
and, if we compile the tree with `-Wignore-qualifiers`, the compiler
would warn like:
```
/home/kefu/dev/scylladb/schema/schema.hh:245:5: error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]
245 | const index_metadata_kind kind() const;
| ^~~~~
```
so this change also silences the above warnings.
Closesscylladb/scylladb#16083
* github.com:scylladb/scylladb:
build: enable -Wignore-qualifiers
treewide: do not mark return value const if this has no effect
`-Wignore-qualifiers` is included by -Wextra. but we are not there yet,
with this change, we can keep the changes introducing -Wignore-qualifiers
warnings out of the repo, before applying `-Wextra`.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
this change is a cleanup.
to mark a return value without value semantics has no effect. these
`const` specifier useless. so let's drop them.
and, if we compile the tree with `-Wignore-qualifiers`, the compiler
would warn like:
```
/home/kefu/dev/scylladb/schema/schema.hh:245:5: error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers]
245 | const index_metadata_kind kind() const;
| ^~~~~
```
so this change also silences the above warnings.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
This is a loose collection of fixes to rare row cache bugs flushed out by running test_concurrent_reads_and_eviction several million times. See individual commits for details.
Fixes#15483Closesscylladb/scylladb#15945
* github.com:scylladb/scylladb:
partition_version: fix violation of "older versions are evicted first" during schema upgrades
cache_flat_mutation_reader: fix a broken iterator validity guarantee in ensure_population_lower_bound()
cache_flat_mutation_reader: fix a continuity loss in maybe_update_continuity()
cache_flat_mutation_reader: fix continuity losses during cache population races with reverse reads
partition_snapshot_row_cursor: fix a continuity loss in ensure_entry_in_latest() with reverse reads
cache_flat_mutation_reader: fix some cache mispopulations with reverse reads
cache_flat_mutation_reader: fix a logic bug in ensure_population_lower_bound() with reverse reads
cache_flat_mutation_reader: never make an unlinked last dummy continuous
A schema upgrade appends a MVCC version B after an existing version A.
The last dummy in B is added to the front of LRU,
so it will be evicted after the entries in A.
This alone doesn't quite violate the "older versions are evicted first" rule,
because the new last dummy carries no information. But apply_monotonically
generally assumes that entries on the same position have the obvious
eviction order, even if they carry no information. Thus, after the merge,
the rule can become broken.
The proposed fix is as follows:
- In the case where A is merged into B, the merged last dummy
inherits the link of A.
- The merging of B into anything is prevented until its merge with A is finished.
This is relatively hacky, because it still involves a state that
goes against some natural expectations granted by the "older versions..."
rule. A less hacky fix would be to ensure that the new dummy is inserted
into a proper place in the eviction order to begin with.
Or, better yet, we could eliminate the rule altogether.
Aside from being very hard to maintain, it also prevents the introduction
of any eviction algorithm other than LRU.
ensure_population_lower_bound() guarantees that _last_row is valid or null.
However, it fails to provide this guarantee in the special rare case when
`_population_range_starts_before_all_rows == true` and _last_row is non-null.
(This can happen in practice if there is a dummy at before_all_clustering_rows
and eviction makes the `(before_all_clustering_rows, ...)` interval
discontinous. When the interval is read in this state, _last_row will point to
the dummy, while _population_range_starts_before_all_rows will still be true.)
In this special case, `ensure_population_lower_bound()` does not refresh
`_last_row`, so it can be non-null but invalid after the call.
If it is accessed in this state, undefined behaviour occurs.
This was observed to happen in a test,
in the `read_from_underlying() -- maybe_drop_last_entry()` codepath.
The proposed fix is to make the meaning of _population_range_starts_before_all_rows
closer to its real intention. Namely: it's supposed to handle the special case of a
left-open interval, not the case of an interval starting at -inf.
To reflect the final range tombstone change in the populated range,
maybe_update_continuity() might insert a dummy at `before_key(_next_row.table_position())`.
But the relevant logic breaks down in the special case when that position is
equal to `_last_row.position()`. The code treats the dummy as a part of
the (_last_row, _next_row) range, but this is wrong in the special case.
This can lead to inconsistent state. For example, `_last_row` can be wrongly made
continuous, or its range tombstone can be wrongly nulled.
The proposed fix is to only modify the dummy if it was actually inserted.
If it had been inserted beforehand (which is true in the special case, because
of the `ensure_population_lower_bound()` call earlier), then it's already in a
valid state and doesn't need changes.
Cache population routines insert new row entries.
In non-reverse reads, the new entries (except for the lower bound of the query
range) are filled with the correct continuity and range tombstones immediately
after insertion, because that information has already arrived from underlying.
when the entries are inserted.
But in reverse reads, it's the interval *after* the newly-inserted entry
that's made continuous. The continuity information in the new entries isn't
filled. When two population routines race, the one which comes later can
punch holes in the continuity left by the first routine, which can break
the "older versions are evicted first" rule and revert the affected
interval to an older version.
To fix this, we must make sure that inserting new row entries doesn't
change the total continuity of the version.
The FIXME comment claims that setting continity isn't very important in this
place, but in fact this is just wrong.
If two calls to read_from_underlying() get into a race, the one which finishes
later can call ensure_entry_in_latest() on a position which lies inside a
continuous interval in the newest version. If we don't take care to preserve
the total continuity of the version, this can punch a hole in the continuity of the
newest version, potentially reverting the affected interval to an older version.
Fix that.
`_last_row` is in table schema, but it is sometimes compared with positions in
query schema. This leads to unexpected behaviour when reverse reads
are used.
The previous patch fixed one such case, which was affecting correctness.
As far as I can tell, the three cases affected by this patch aren't
a correctness problem, but can cause some intervals to fail to be made continuous.
(And they won't be cached even if the same read is repeated many times).
`_last_row` is in table schema, while `cur.position()` is in query schema
(which is either equal to table schema, or its reverse).
Thus, the comparison affected by this patch doesn't work as intended.
In reverse reads, the check will pass even if `_last_row` has the same key,
but opposite bound weight to `cur`, which will lead to the dummy being inserted
at the wrong position, which can e.g. wrongly extend a range tombstone.
Fix that.
It is illegal for an unlinked last dummy to be continuous,
(this is how last dummies respect the "older verions are evicted first" rule),
but it is technically possible for an unlinked last dummy to be
made continuous by read_from_underlying. This commit fixes that.
Found by row_cache_test.
The bug is very unlikely to happen in practice because the relevant
rows_entry is bumped in LRU before read_from_underlying starts.
For the bug to manifest, the entry has to fall down to the end of the
LRU list and be evicted before read_from_underlying() ends.
Usually it takes several minutes for an entry to fall out of LRU,
and read_from_underlying takes maybe a few hundred milliseconds.
And even if the above happened, there still needs to appear a new
version, which needs to have its continuous last dummy evicted
before it's merged.
This commit adds the .. only:: opensource directive
to the Raft page to exclude the link to the 5.2-to-5.4
upgrade guide from the Enterprise documentation.
The Raft page belongs to both OSS and Enterprise
documentation sets, while the upgrade guide
is OSS-only. This causes documentation build
issues in the Enterprise repository, for example,
https://github.com/scylladb/scylla-enterprise/pull/3242.
As a rule, all OSS-only links should be provided
by using the .. only:: opensource directive.
This commit must be backported to branch-5.4
to prevent errors in the documentation for
ScyllaDB Enterprise 2024.1
(backport)
Closesscylladb/scylladb#16064
instead of linking against cryptopp, we should link against
crytopp::crytopp. the latter is the target exposed by
Findcryptopp.cmake, while the former is but a library name which
is not even exposed by any find_package() call.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16060
The jumbo sink is there to upload files that can be potentially larger
than 50Gb (10000*5Mb). For that the sink uploads a set of so called
"pieces" -- files up to 50Gb each -- then uses the copy-upload APi call
to squash the pieces together. After copying the piece is removed. In
case of a crash while uploading pieces remain in the bucket forever
which is not great.
This patch tags pieces with 'kind=piece' tag in order to tell pieces
from regular objects. This can be used, for example, by setting up the
lifecycle tag-based policy and collect dangling pieces eventually.
fixes: #13670
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#16023
developer might notice that when he/she builds 'check-headers',
the whole tree is built. so let's explain this behavior.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16062
As a general rule, tests in test/cql-pytest shouldn't just pass on Scylla - they also should not fail on Cassandra; A test that fails on Cassandra may indicate that the test is wrong, or that Scylla's behavior is wrong and the test just enshrines that wrong behavior. Each time we see a test fail on Cassandra we need to check if this is not the case. We also have special markers scylla_only and cassandra_bug to put on tests that we know _should_ fail on Cassandra because it is missing some Scylla-only feature or there is a bug in Cassandra, respectively. Such tests will be xfailed/skipped when running on Cassandra, and not report failures.
Unfortunately, over time more several tests got into our suite in that did not pass on Cassandra. In this series I went over all of them, and fixed each to pass - or be skipped - on Cassandra, in a way that each patch explains.
Fixes#16027Closesscylladb/scylladb#16033
* github.com:scylladb/scylladb:
test/cql-pytest: fix test_describe.py to not fail on Cassandra
test/cql-pytest: fix select_single_column_relation_test.py to not fail on Cassandra
test/cql-pytest: fix compact_storage_test.py to not fail on Cassandra
test/cql-pytest: fix test_secondary_index.py to not fail on Cassandra
test/cql-pytest: fix test_materialized_view.py to not fail on Cassandra
test/cql-pytest: fix test_keyspace.py to not fail on Cassandra
test/cql-pytest: test_guardrail_replication_strategy.py is Scylla-only
test/cql-pytest: partial fix for test_compaction_strategy_validation.py on Cassandra
test/cql-pytest: fix test_filtering.py to not fail on Cassandra
Yet another test file in cql-pytest which failed when run on Cassandra
(via test/cql-pytest/run-cassandra).
Some of the tests checked on Cassandra things that don't exist there
(namely local secondary indexes) and could skip that part. Other tests
need to be skipped completely ("scylla_only") because they rely on a
Scylla-only feature. We have a bit too many of those in this file, but
I don't want to fix this now.
Yet another test found a real bug in Cassandra 4.1.1 (CASSANDRA-17918)
but passes in Cassandra 4.1.2 and up, so there's nothing to fix except
a comment about the situation.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
In commit 52bbc1065c, we started to allow "IN NULL" - it started to
match nothing instead of being an error as it is in Cassandra. The
commit *incorrectly* "fixed" the existing translated Cassandra unit test
to match the new behavior - but after this "fix" the test started to
fail on Cassandra.
The appropriate fix is just to comment out this part of the test and
not do it. It's a small point where we deliberately decided to deviate
from Cassandra's behavior, so the test it had for this behavior is
irrelevant.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Some error-message checks in this test file (which was translated in
the past from Cassandra) try operations which actually has two errors,
and expected to see one error message - but recent Cassandra prints
the other one. This caused several tests to fail when running on
Cassandra 4.1. Both messages are fine, so let's accept both.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Fixed two tests thich failed when running on Cassandra:
One test waited for a secondary index to appear, but in Cassandra, the
index can be broken (cause a read failure) for a short while and we
need to wait through this failure as well and not fail the entire test.
Another test was for local secondary index, which is a Scylla-only
feature, but we forgot the "scylla_only" tag.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
The test function test_mv_synchronous_updates checks the
synchronous_updates feature, which is a ScyllaDB extension and
doesn't exist in Cassandra. So it should be marked with "scylla_only"
so that it doesn't fail when running the tests on Cassandra.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Yet another test file in cql-pytest which failed when run on Cassandra
(via test/cql-pytest/run-cassandra).
When testing some invalid cases of ALTER TABLE, the test required
that you cannot choose SimpleStrategy without specifying a
replication_factor. As explained in Refs #16028, this isn't true
in Cassandra 4.1 and up - it now has a default value for
replication_factor and it's no longer required.
So in this patch we split that part of the test to a separate test
function and mark it scylla_only.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
The tests in test/cql-pytest/test_guardrail_replication_strategy.py
are for a Scylla-only feature that doesn't exist in Cassandra, so
obviously they all fail on Cassandra. Let's mark them all as
scylla_only.
We use an autouse fixture to automatically mark all tests in this file
as scylla-only, instead of marking each one separately.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Yet another test file in cql-pytest which failed when run on Cassandra
(via test/cql-pytest/run-cassandra).
This patch is only a partial fix - it fixes trivial differences in error
messages, but some potentially-real differences remain so three of the
tests still fail:
1. Trying to set tombstone_threshold to 5.5 is an error in ScyllaDB
("must be between 0.0 and 1.0") but allowed in Cassandra.
2. Trying to set bucket_low to 0.0 is an error in ScyllaDB, giving the
wrong-looking error message "must be between 0.0 and 1.0" (so 0.0 should
have been fine?!) but allowed in Cassandra.
3. Trying to set timestamp_resolution to SECONDS is an error in ScyllaDB
("invalid timestamp resolution SECONDS") but allowed in Cassandra.
I don't think anybody wants to actually use "SECONDS", but it seems
legal in Cassandra, so do we need to support it?
The patch also simplifies the test to use cql-pytest's util.py, instead
of cassandra_tests/porting.py. The latter was meant to make porting
existing Cassandra tests easier - not for writing new ones - and made
using a regular expression for testing error messages harder so I
switched to using pytest.raises() whose "match=" accepts a regular
expression.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Yet another test file in cql-pytest which failed when run on Cassandra
(via test/cql-pytest/run-cassandra).
It turns out that when the token() function is used with incorrect
parameters (it needs to be passed all partition-key columns), the
error message is different in ScyllaDB and Cassandra. Both are
reasonable error messages, so if we insist on checking the error
message - we should allow both.
Also the same test called its second partition-key column "ck". This
is confusing, because we usually use the name "ck" to refer to a clustering
key. So just for clarity, we change this name to "pk2". This is not a
functional change in the test.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
TWCS tables require partition estimation adjustment as incoming streaming data can be segregated into the time windows.
Turns out we had two problems in this area that leads to suboptimal bloom filters.
1) With off-strategy enabled, data segregation is postponed, but partition estimation was adjusted as if segregation wasn't postponed. Solved by not adjusting estimation if segregation is postponed.
2) With off-strategy disabled, data segregation is not postponed, but streaming didn't feed any metadata into partition estimation procedure, meaning it had to assume the max windows input data can be segregated into (100). Solved by using schema's default TTL for a precise estimation of window count.
For the future, we want to dynamically size filters (see https://github.com/scylladb/scylladb/issues/2024), especially for TWCS that might have SSTables that are left uncompacted until they're fully expired, meaning that the system won't heal itself in a timely manner through compaction on a SSTable that had partition estimation really wrong.
Fixes https://github.com/scylladb/scylladb/issues/15704.
Closesscylladb/scylladb#15938
* github.com:scylladb/scylladb:
streaming: Improve partition estimation with TWCS
streaming: Don't adjust partition estimate if segregation is postponed
before this change, `load_sstables()` fills the output sstables vector
by indexing it with the sstable's path. but if there are duplicated
items in the given sstable_names, the returned vector would have uninitialized
shared_sstable instance(s) in it. if we feed such a sstables to the
operation funcs, they would segfault when derferencing the empty
lw_shared_ptr.
in this change, we error out if duplicated sstable names are specified
in the command line.
an alternative is to tolerate this usage by initializing the sstables
vector with a back_inserter, as we always return a dictionary with the
sstable's name as the key, but it might be desirable from user's
perspective to preserve the order, like OrderedDict in Python. so
let's preserve the ordering of the sstables in the command line.
this should address the problem of the segfault if we pass duplicated
sstable paths to this tool.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16048
Compaction tasks which do not have a parent are abortable
through task manager. Their children are aborted recursively.
Compaction tasks of the lowest level are aborted using existing
compaction task executors stopping mechanism.
Closesscylladb/scylladb#16050
* github.com:scylladb/scylladb:
test: test abort of compaction task that isn't started yet
test: test running compaction task abort
tasks: fail if a task was aborted
compaction: abort task manager compaction tasks
without adding `WantedBy=scylla-server.service` in
var-lib-systemd-coredump, if we starts `scylla-server.service`,
it does not necessarily starts `var-lib-systemd-coredump`
even if the latter is installed.
with `WantedBy=scylla-server.service` in var-lib-systemd-coredump,
if we starts `scylla-server.service`, var-lib-systemd-coredump
will be started also. and `Before=scylla-server.service` ensures
that, before `scylla-server.service` is started,
var-lib-systemd-coredump is already ready.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#15984
Make `generic_server::gentle_iterator` a mutable iterator to allow
`for_each_gently` to make changes to the connections.
Fixes: #16035Closesscylladb/scylladb#16036
All two and the upcoming third test cases in the test create the very
same ks.cf pair with the very same sequence of steps. Generalize them.
For the basic test case also tune up the way "expected" rows are
calculated -- now they are SELECT-ed right after insertion and the size
is checked to be non zero. Not _exactly_ the same check, but it's good
enough for basic testing purposes.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#15986
$ID_LIKE = "rhel" works only on RHEL compatible OSes, not for RHEL
itself.
To detect RHEL correctly, we also need to check $ID = "rhel".
Fixes#16040Closesscylladb/scylladb#16041
Boost.Test prints the LHS and RHS when the predicate statement passed
to BOOST_REQUIRE_EQUAL() macro evaluates to false. so the error message
printed by Boost would be more developer friendly when the test fails.
in this test, we replace some BOOST_REQUIRE() with BOOST_REQUIRE_EQUAL()
when appropriate.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16047
This short series fixes test/cql-pytest/test_permissions.py to stop failing on Cassandra.
The second patch fixes these failures (and explains why). The first patch is a new test for UDFs, which helped me prove that one of the test_permissions.py failures in Cassandra is a Cassandra bug - some esoteric error path that prints the right message when no permissions are involved, becomes wrong when permissions are added.
Fixes#15969Closesscylladb/scylladb#15979
* github.com:scylladb/scylladb:
test/cql-pytest: fix test_permissions.py to not fail on Cassandra
test/cql-pytest: add test for DROP FUNCTION
This is continuation of a34c8dc4 (Drop compaction_manager_for_testing).
There's one more wrapper over compaction_manager to access its private fields. All such access was recently moved to sstables::test_env's compaction manager, now it's time to drop the remaining legacy wrapper class.
Closesscylladb/scylladb#16017
* github.com:scylladb/scylladb:
test/utils: Drop compaction_manager_test
test/utils: Get compaction manager from test_env
test/sstables: Introduce test_env_compaction_manager::perform_compaction()
test/env: Add sstables::test_env& to compaction_manager_test::run()
test/utils: Add sstables::test_env& to compact_sstables()
test/utils: Simplify and unify compaction_manager_test::run()
test/utils: Squash two compact_sstables() helpers
test/compaction: Use shorter compact_sstables() helper
test/utils: Keep test task compaction gate on task itself
test/utils: Move compaction_manager_test::propagate_replacement()
We're observing nodes getting stuck during bootstrap inside
`storage_service::wait_for_ring_to_settle()`, which periodically checks
`migration_manager::have_schema_agreement()` until it becomes `true`:
scylladb/scylladb#15393.
There is no obvious reason why that happens -- according to the nodes'
logs, their latest in-memory schema version is the same.
So either the gossiped schema version is for some reason different
(perhaps there is a race in publishing `application_state::SCHEMA`) or
missing entirely.
Alternatively, `wait_for_ring_to_settle` is leaving the
`have_schema_agreement` loop and getting stuck in
`update_topology_change_info` trying to acquire a lock.
Modify logging inside `have_schema_agreement` so details about missing
schema or version mismatch are logged on INFO level, and an INFO level
message is printed before we return `true`. To prevent logs from getting
spammed, rate-limit the periodic messages to once every 5 seconds. This
will still show the reason in our tests which allow the node to hang for
many minutes before timing out. Also these schema agreement checks are
done on relatively rare occasions such as bootstrap, so the additional
logs should not be harmful.
Furthermore, when publishing schema version to gossip, log it on INFO
level. This is happening at most once per schema change so it's a rare
message. If there's a race in publishing schema versions, this should
allow us to observe it.
Ref: scylladb/scylladb#15393Closesscylladb/scylladb#16021
Having values of the duration type is not allowed for clustering
columns, because duration can't be ordered. This is correctly validated
when creating a table but do not validated when we alter the type.
Fixes#12913Closesscylladb/scylladb#16022
Propagate `exceptions::unavailable_exception` error message to the client such as cqlsh.
Fixes#2339Closesscylladb/scylladb#15922
* github.com:scylladb/scylladb:
test: add the auth_cluster test suite
auth: fix error message when consistency level is not met
before this change, we define the CMAKE_CXX_FLAGS_${CONFIG} directly.
and some of the configurations are supposed to generate debugging info with
"-g -gz" options, but they failed to include these options in the cxx
flags.
in this change:
* a macro named `update_cxx_flags` is introduced to set this option.
* this macro also sets -O option
instead of using function, this facility is implemented as a macro so
that we can update the CMAKE_CXX_FLAGS_${CONFIG} without setting
this variable with awkward syntax like set
```cmake
set(${flags} "${${flags}}" PARENT_SCOPE)
```
this mirrors the behavior in configure.py in sense that the latter
sets the option on a per-mode basis, and interprets the option to
compiling option.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#16043
