* seastar 0b2c25d133...f1b6b95b69 (10):
> Merge "Cap streams buffer sizes with IO limits" from Pavel
> io_queue: fix mismatched class/struct tag for priority_class_data
> perftune.py: instrument bonding tuning flow with 'nic' parameter
> perftune.py: strip a newline off the 'scheduler' file content
> perftune.py: add support for virtual non-raid disk devices
> doc: fix typos in doc/tutorial.md
> Merge "Add IO in-disk stats" from Pavel E
> iotune: Perform fs-check on all directories
> file: Keep reference on io-queue
> Merge "Assorted set of improvements over io-queue" from Pavel E
An out of block log print resulted in repeated prints about removal of
the default service level. The period of this print is every time the
configuration is scanned for changes. It happens when the default
service level is one of the last on the map (sorted as in the map).
Fixes#8567Closes#8576
Introduce a tagged id struct for `group_id`.
Raft code would want to generate quite a lot of unique
raft groups in the future (e.g. tablets). UUID is designed
exactly for that (e.g. larger capacity than `uint64_t`, obviously,
and also has built-in procedures to generate random ids).
Also, this is a preparation to make "raft group 0" use a random
ID instead of a literal fixed `0` as a group id.
The purpose is that every scylla cluster must have a unique ID
for "raft group 0" since we don't want the nodes from some other
cluster to disrupt the current cluster. This can happen if,
for some reason, a foreign node happens to contact a node in
our cluster.
Tests: unit(dev)
Signed-off-by: Pavel Solodovnikov <pa.solodovnikov@scylladb.com>
Message-Id: <20210429170630.533596-3-pa.solodovnikov@scylladb.com>
Many workloads have fairly constant and small request sizes, so we
don't need large reserves for them. These workloads suffer needlessly
from the current large reserve of 10 segments (1.2MB) when they really
need a few hundred bytes. Reduce the reserve to a minimum of 1 segment.
Note that due to #8542 this can make a large difference. Consider a
workload that has a 1000-byte footprint in cache. If we've just
consumed some free memory and reduced the reserve to zero, then
we'll evict about 50,000 objects before proceeding to compact. With
the reserved reduced to 1, we'll evict 128 objects. All this
for 1000 bytes of memory.
Of course, #8542 should be fixed, but reducing the reserve provides
some quick relief and makes sense even with the larger fix. The
reserve will quickly grow for workloads that handle bigger requests,
so they won't see an impact from the reduction.
Closes#8572
Scylla apply iterates over all shards in undetermined order and leaves
the last shard as the current one. This is counter-intuitive and can
lead to surprises as the user might not expect the current shard to be
changed by a command that just executes a command on each shard.
This patch ensures that both in case of the happy and error paths the
current shard is unchanged.
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20210429104937.61315-1-bdenes@scylladb.com>
The error message currently complains about "invalid version" and later
says the reason is that the path is not recognized. This is confusing so
change the error message to start with "invalid path" instead. It is the
path that is invalid not the version after all.
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20210429092749.52659-1-bdenes@scylladb.com>
So it is closed when loading the sstable throws an exception too.
Failing to close the manager will mask the real error as the user will
only see the assert failure due to failing to close the manager.
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20210429092248.50968-1-bdenes@scylladb.com>
We encountered a phenomena where shutting down the messaging service
don't complete, leaving the shutdown process stuck. Since we couldn't
pinpoint where exactly the shutdown went wrong, here we add some
verbosity to the shutdown stages so we can more accurately pinpoint the
culprit.
Closes#8560
The fuzzy test consumes a large chunk of resource from the semaphore
up-front to simulate a contested semaphore. This isn't an accurate
simulation, because no permit will have more than 1 units in reality.
Furthermore this can even cause a deadlock since 8aaa3a7 as now we rely
on all count units being available to make forward progress when memory
is scarce.
This patch just cuts out this part of the test, we now have a dedicated
unit test for checking a heavily contested semaphore, that does it
properly, so no need to try to fix this clumsy attempt that is just
making trouble at this point.
Refs: #8493
Tests: release(multishard_mutation_query_test:fuzzy_test)
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20210429084458.40406-1-bdenes@scylladb.com>
Previously, we checked if all partition-key restrictions were EQ at runtime. This is, however, known already at prep time; no need to redo it on every query execution. Move the check to prep time.
Tests: unit (dev, debug), perf_simple_query
Closes#8565
* github.com:scylladb/scylla:
cql3: Replace runtime check with a prepared flag
cql3: Track IN partition-key restrictions
cql3: Inline add_single_column_restriction
cql3: Inline statement_restrictions::add_restriction
Checking that every PK restriction is an EQ was happening at runtime.
This is wasteful, as the result is always the same. Replace that
check with a flag computed once at preparation time.
Separate the simple-case processing into its own function rather than
pass the flag as an extra parameter.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
Add a bool member to statement_restrictions indicating whether any of
the partition columns are restricted by IN, which requires more
complex processing.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
This fixes a potential cause for reactor stalls during memory
reclamation. Applies only to schemas without clustering columns.
Every partition in cache has a dummy row at the end of the clustering
range (last dummy). That dummy must be evicted last, because MVCC
logic needs it to be there at all times. If LRU picks it for eviction
and it's not the last row, eviction does nothing and moves
on. Eventually, all other rows in this partition will be evicted too
and then the partition will go away with it.
Mutation reader updates the position of rows in the LRU (aka touching)
as it walks over them. However, it was not always touching the last
dummy row. If the partition was fully cached, and schema had no
clustering key, it would exit early before reaching the last dummy
row, here:
inline
void cache_flat_mutation_reader::move_to_next_entry() {
clogger.trace("csm {}: move_to_next_entry(), curr={}", fmt::ptr(this), _next_row.position());
if (no_clustering_row_between(*_schema, _next_row.position(), _upper_bound)) {
move_to_next_range();
That's because no_clustering_row_between() is always true for any key
in tables with no clustering columns, and the reader advances to
end-of-stream without advancing _next_row to the last dummy. This is
expected and desired, it means that the query range ends at the
current row and there is no need to move further. We would not take
this exit for tables with a non-singular clustering key domain and
open-ended query range, since there would be a key gap before the last
dummy row.
Refs #2972.
The effect of leaving the last dummy row not touched will be that such
scans will segregate rows in the LRU, bring all regular rows to the
front, and dummy rows at the tail. When eviction reaches the band of
dummy rows, it will have to walk over it, because evicting them
releases no memory. This can cause a reactor stall.
An easy fix for the scenario would be to always touch the dummy entry
when entering the partition. It's unlikely that the read will not
proceed to the regular rows. It would be best to avoid linking such
dummies in the LRU, but that's a much more complex change.
Discovered in perf_row_cache_update, test_small_partitions(). I saw
200ms stalls with -m8G.
Refs #8541.
Tests:
- row_cache_test (release)
- perf_simple_query [no change]
Message-Id: <20210427111619.296609-1-tgrabiec@scylladb.com>
Invoking statement_restrictions::add_single_column_restriction()
outside the constructor would leave some data members out-of-date.
Prevent it by deleting the method and inlining its body into the only
call site.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
Invoking this method outside the constructor would leave some data
members out-of-date. Prevent it by deleting the method and inlining
its body into the only call site.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
Currently `scylla shard` with no args results in
the following error:
```
(gdb) scylla shard
Traceback (most recent call last):
File "master-scylla-gdb.py", line 2384, in invoke
id = int(arg)
ValueError: invalid literal for int() with base 10: ''
Error occurred in Python: invalid literal for int() with base 10: ''
```
Instead, let it just print the current shard, similar to
`(gdb) thread`.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20210428093913.1070051-1-bhalevy@scylladb.com>
I used {:.0} to truncate to integer, but apparently that resulted
in only one significant digit in the report, so 93.1 was reported as
90. Use the {:5.1f} to avoid truncation, and even get an extra
digit (we can have fractional tasks/op due to batching).
Current result is 93.1 allocs/op, 20.1 tasks/op (which suggests
batch size of around 10).
Closes#8550
"
From now on, offstrategy compaction is triggered on completion of repair-based
removenode. So compaction will no longer act aggressively while removenode
is going on, which helps reducing both latency and operation time.
Refs #5226.
"
* 'offstrategy_removenode' of github.com:raphaelsc/scylla:
repair: Wire offstrategy compaction to repair-based removenode
table: introduce trigger_offstrategy_compaction()
repair/row_level: make operations_supported static const
In the alternator and cql-pytest test frameworks, we have some convenient
contextmanager-based functions that allows us to create a temporary
resource (e.g., a table) that will be automatically deleted, for
example:
with create_stream_test_table(...) as table:
test_something(table)
However, our implementation of these functions wasn't safe. We had
code looking like:
table = ...
yield table
table.delete()
The thinking was that the cleanup part (the table.delete()) will be
called after the user's code. However, if the user's code threw
(i.e., a failed assertion), the cleanup wasn't called... When the user's
code throws, it looks as if the "yield" throws. So the correct code
should look like:
table = ...
try:
yield table
finally:
table.delete()
Python's contextmanager documentation indeed gives this idiom in its
example.
This patch fixes all contextmanager implementations in our tests to do
the cleanup even if the user's "with" block throws.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20210428083748.552203-1-nyh@scylladb.com>
On Debian variants, mdmonitor.service cannnot enable because it missing
[Install] section, so 'systemctl enable mdmonitor.service' will fail,
not able to run mdmonitor after the system restarted.
To force running the service, add Wants=mdmonitor.service on
var-lib-scylla.mount.
Fixes#8494Closes#8530
The range passed to create_sharding_metadata is supposed to be owned or
at least partially owned by the shard. Log keys, range and split
ranges for debugging if the range does not belong to the shard.
This is helpful for debugging "Failed to generate sharding
metadata for foo.db" issues reported.
Refs #7056Closes#8557
In an ongoing effort to drop the `restrictions` class hierarchy, rewrite the partition-range calculation code to use the new `expression` objects.
Refs: #7217#3815
Tests: unit (dev, debug)
Closes#8525
* github.com:scylladb/scylla:
cql3: Specialize partition-range computation for EQ
cql3: Replace some bounds_ranges calls
cql3: Get partition range from expr::expression
cql3: Track partition-range expressions
Save a couple of allocations per request by treating all-EQ cases
specially during the computation of partition ranges.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
removenode_with_repair() runs on all the nodes that need to sync data
from other nodes, so offstrategy compaction can be easily wired by
notifying tables when removenode completes.
From now on, when user runs removenode, new sstables produced in receiving
nodes will be added to table's maintenance set, and when the operation
completes, offstrategy compacted will be started to reshape those new
ssts before integrating them into the main set.
Refs #5226.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
The r5b instances also have ena support. For a confirmation
that all r5b instances have ena, go to the following page:
https://instances.vantage.sh/
Select the r5b and add the 'enhanced networking' column. Then
it will show that for every r5b type there is ena support
Closes#8546
The original series which forward-ported the service levels into open-source omitted important fixes to their infrastructure. The fixes are hereby ported.
Tests: unit(release)
Closes#8540
* github.com:scylladb/scylla:
workload prioritization: Fix configuration change detection
workload prioritization: add exception protection in configuration polling
The configuration detection is based on a loop that
advances two iterators and compares the two collection
for deducing the configuration change. In order to
correctly deduce the changes the iteration have to be
according to the key (service level name) order for both
of the collections. If it doesn't happen the results are
undefined and in some cases can lead to a crash of the
system. The bug is that the _service_level_db field was
implemented using an unordered_map which obviously don't
guarantie the configuration change detection assumption.
The fix was simply to change the field type to a map
instead of unordered_map.
Another problem is that when a static service level (i.e
default) is at the end of the keys list, it is repeatedly
being deleted - which doesn't really do anything since deleting
a static service level is just retaining it's defult values
but it is stil wrong.
Exceptions around the loop polling were not handled properly.
This is an issue due to the fact that if an unhandled exception
slips out to the configuration polling loop itself it will break
it. When the configuration polling loop is broken, any further
change to the configuration will not be acted uppon in the nodes
where the loop is broken until the node is restarted. The chances
for exceptions are now greater than before since in one of the
previous commits we started quering the workload prioritization
configuration table with a sensible, shorter timeout.
This change also adds a logger for the workload prioritization
module and some logging mainly arround the configuration polling loop.
Most logs are added in the info level since they are not expected to
happen frequently but when they do we would like to have some
information by default regarding what broke the loop.
Issues #4476 and #8489, and also Cassandra's CASSANDRA-10715, all request
that filtering with "WHERE v=NULL" should return the rows where the column
v is unset. However, we made a deliberate decision to do something else:
That "WHERE v=NULL" should match no row. Exactly like it does in SQL.
This is what this test verifies - that "WHERE v=NULL" never matches any
row - not even rows where "v" is unset.
This test is expected to fail on Cassandra (so marked cassandra_bug),
because in Cassandra the "WHERE v=NULL" restriction is forbidden,
instead of succeeding and returning nothing.
Although we differ here from Cassandra, after a lot of deliberation we
decided that Scylla's behavior is the correct one, so this test verifies
it.
Refs #4776.
Refs #8489.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20210426183145.323301-1-nyh@scylladb.com>
Seastar seems to have added another layer of indirection to
alloc_site_list_head/backtrace, so scylla_heapprof() can't find the
members it's looking for, resulting in errors. Fix it by
dereferencing the added layer.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
Closes#8551
When probes are sent over a slow network, the leader would send
multiple probes to a lagging follower before it would get a
reject response to the first probe back. After getting a reject, the
leader will be able to correctly position `next_idx` for that
follower and switch to pipeline mode. Then, an out of order reject
to a now irrelevant probe could crash the leader, since it would
effectively request it to "rewind" its `match_idx` for that
follower, and the code asserts this never happens.
We fix the problem by strengthening `is_stray_reject`. The check that
was previously only made in `PIPELINE` case
(`rejected.non_matching_idx <= match_idx`) is now always performed and
we add a new check: `rejected.last_idx < match_idx`. We also strengthen
the assert.
The commit improves the documentation by explaining that
`is_stray_reject` may return false negatives. We also precisely state
the preconditions and postconditions of `is_stray_reject`, give a more
precise definition of `progress.match_idx`, argue how the
postconditions of `is_stray_reject` follow from its preconditions
and Raft invariants, and argue why the (strengthened) assert
must always pass.
Message-Id: <20210423173117.32939-1-kbraun@scylladb.com>
RPC configuration was updated only when an instance was
started with an initial snapshot.
In case we don't have an initial snapshot, but do have
a non-empty log with a configuration entry, the RPC
instance isn't set up correctly.
Fix that by moving RPC setup code outside the check for
snapshot id and look at `_log.get_configuration()` instead.
Also, set up RPC mappings both for `current` and `previous`
components, since in case the last configuration index
points to an entry from the log, it can happen to be
a joint configuration entry.
For example, this can happen if a leader made an attempt
to change configuration, but failed shortly afterwards
without being able to commit the new configuration.
Tests: unit(dev)
Signed-off-by: Pavel Solodovnikov <pa.solodonikov@scylladb.com>
Message-Id: <20210423220718.642470-1-pa.solodovnikov@scylladb.com>
This series is a conceptual revert of 4c8ab10, which turned out to be a
misguided defense mechanism that proved to be a hotbed for bugs. This
protection was superseded by 0fe75571d9 which guarantees forward
progress at all times without all the gotchas and bad interactions
introduced by 4c8ab10.
The latest instance of bad interaction that triggered this series is a
case of resource units being leaked when a previously evicted reader is
re-admitted, leaking already owned resources on each re-admission.
To prove that neither the resource leak, nor the deadlock 4c8ab10 was
supposed to guard against exists after this series, it includes two unit
tests stressing the respective areas: readmission and admission on a
highly contested semaphore.
Fixes: #8493
Also on: https://github.com/denesb/scylla.git
reader-permit-resource-leak-v2
Changelog
v2:
* Rebase over the recently merged reader close series. Fix merge
conflicts and an exposed bug.
* 'reader-permit-resource-leak-v2' of https://github.com/denesb/scylla:
test: mutation_reader_test: add test_reader_concurrency_semaphore_forward_progress
test: mutation_reader_test: add test_reader_concurrency_semaphore_readmission_preserves_units
reader_concurrency_semaphore: add dump_diagnostics()
reader_permit: always forward resources
reader_concurrency_semaphore: inactive_read_handle: abandon(): close reader
This series adds filling the `username` column in alternator tracing info, if the username is available. When alternator is enforcing authorization, each request contains a username in its headers.
The difference is as follows. A tracing entry excerpt before the series:
```
{
(...)
'source_ip': '::',
'table_names': 'alternator_Pets.Pets',
'username': '<unauthenticated request>'
}
```
and after the series:
```
{
(...)
'source_ip': '::',
'table_names': 'alternator_Pets.Pets',
'username': 'alternator'
}
```
This series also modifies one of the tests to check the username column.
Fixes#8547Closes#8548
* github.com:scylladb/scylla:
test: add username verification to alternator tracing tests
alternator: add user context to tracing
alternator: return username when verifying signature
This unit test checks that the semaphore doesn't get into a deadlock
when contended, in the presence of many memory-only reads (that don't
wait for admission). This is tested by simulating the 3 kind of reads we
currently have in the system:
* memory-only: reads that don't pass admission and only own memory.
* admitted: reads that pass admission.
* evictable: admitted reads that are furthermore evictable.
The test creates and runs a large number of these reads in parallel,
read kinds being selected randomly, then creates a watchdog which
kills the test if no progress is being made.
This unit test passes a read through admission again-and-again, just
like an evictable reader would be during its lifetime. When readmitted
the read sometimes has to wait and sometimes not. This is to check that
the readmitting a previously admitted reader doesn't leak any units.
This commit conceptually reverts 4c8ab10. Said commit was meant to
prevent the scenario where memory-only permits -- those that don't pass
admission but still consume memory -- completely prevent the admission
of reads, possibly even causing a deadlock because a permit might even
blocks its own admission. The protection introduced by said commit
however proved to be very problematic. It made the status of resources
on the permit very hard to reason about and created loopholes via which
permits could accumulate without tracking or they could even leak
resources. Instead of continuing to patch this broken system, this
commit does away with this "protection" based on the observation that
deadlocks are now prevented anyway by the admission criteria introduced
by 0fe75571d9, which admits a read anyway when all the initial count
resources are available (meaning no admitted reader is alive),
regardless of availability of memory.
The benefits of this revert is that the semaphore now knows about all
the resources and is able to do its job better as it is not "lied to"
about resource by the permits. Furthermore the status of a permit's
resources is much simpler to reason about, there are no more loopholes
in unexpected state transitions to swallow/leak resources.
To prove that this revert is indeed safe, in the next commit we add
robust tests that stress test admission on a highly contested semaphore.
This patch also does away with the registered/admitted differentiation
of permits, as this doesn't make much sense anymore, instead these two
are unified into a single "active" state. One can always tell whether a
permit was admitted or not from whether it owns count resources anyway.
fa43d7680 recently introduced mandatory closing of readers before they
are destroyed. One reader destroy path that was left not closing the
reader before destruction is `inactive_reader_handle::abandon()`. This
path is executed when the handle is destroyed while still referring to a
non-evicted inactive read. This patch fixes it up to close the reader
and adds a small unit test which checks that this happens.
Before this patch, each entry in alternator tracing included
an "<unauthenticated request>" field. It's not really true,
because most of alternator requests are actually performed
by authenticated users (unless auth is disabled).
