Newly joining nodes may not have a host id yet. Handle this and print a
"?" for these nodes, instead of the host-id.
Extend the existing test for joining node case (also rename it and add
comment).
Closesscylladb/scylladb#17853
test/raft/replication.cc defines a symbol named `tlogger`, while
test/raft/randomized_nemesis_test.cc also defines a symbol with
the same name. when linking the test with mold, it identified the ODR
violation.
in this change, we extract test-raft-helper out, so that
randomized_nemesis_test can selectively only link against this library.
this also matches with the behavior of the rules generated by `configure.py`.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17836
in gossiping_property_file_snitch_test, we use
`BOOST_REQUIRE_EQUAL(dc_racks[i], dc_racks[0])` to check the equality
of two instances of `pair<sstring, sstring`, like:
```c++
BOOST_REQUIRE_EQUAL(dc_racks[i], dc_racks[0])
```
since the standard library does not provide the formatter for printing
`std::pair<>`, we rely on the homebrew generic formatter to
print `std::pair<>, which in turn uses operator<< to format the
elements in the `pair`, but we intend to remove this formatter
in future, as the last step of #13245 .
so in order to enable Boost.test to print out lhs and rhs when
`BOOST_REQUIRE_EQUAL` check fails, we are adding
`boost_test_print_type()` for `pair<sstring,sstring>`. the helper
function uses {fmt} to print the `pair<>`.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17831
this change prepares for the fmt::formatter based formatter used by
tests, which will use {fmt} to print the elements in a container,
so we need to define the formatter using fmt::formatter for these
element. the operator<< for service_level_options::workload_type is
preserved, as the tests are still using it.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17837
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
row_level_diff_detect_algorithm. please note, we already have
`format_as()` overload for this type, but we cannot use it as a
fallback of the proper `fmt::formatter<>` specialization before
{fmt} v10. so before we update our CI to a distro with {fmt} v10,
`fmt::formatter<row_level_diff_detect_algorithm>` is still
needed.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17824
The assert_that_failed(future) pair of helpers are templates with
variadic futures, but since they are gone in seastar, so should they in
test/lib
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#17830
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `db::functions::function`.
please note, because we use `std::ostream` as the parameter of
the polymorphism implementation of `function::print()`.
without an intrusive change, we have to use `fmt::ostream_formatter`
or at least use similar technique to format the `function` instance
into an instance of `ostream` first. so instead of implementing
a "native" `fmt::formatter`, in this change, we just use
`fmt::ostream_formatter`.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17832
Empty histograms are missing some of the members that non-empty
histograms have. The code handling these histograms assumed all required
members are always present and thus error out when receiving an empty
histogram.
Add tests for empty histograms and fix the code handling them to check
for the potentially missing members, instead of making assumptions.
Closesscylladb/scylladb#17816
Fix aiohttp usage issue in python 3.12:
"Timeout context manager should be used inside a task"
This occurs due to UnixRESTClient created in one event loop (created
inside pytest) but used in another (created in rewriten event_loop
fixture), now it is fixed by updating UnixRESTClient object for every new
loop.
Closesscylladb/scylladb#17760
Remove an unused function from test/cql-pytest/test_using_timeout.py.
Some linters can complain that this function used re.compile(), but
the "re" package was never imported. Since this function isn't used,
the right fix is to remove it - and not add the missing import.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#17801
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `parition_entry::printer`,
and drop its operator<< .
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17812
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `schema_mutations`,
and drop its operator<< .
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17815
before this change, we rely on the homebrew generic formatter to
print unordered_set<>, which in turn uses operator<< to format the
elements in the `unordered_set`, but we intend to remove this formatter
in future, as the last step of #13245 .
so enable Boost.test to print out lhs and rhs when `BOOST_REQUIRE_EQUAL`
check fails, we are adding `boost_test_print_type()` for
`unordered_set<fruit>`. the helper function uses {fmt} to print the
`unordered_set<>`, so we are adding a fmt::formatter for `fruit`, the
operator<< for this type is dropped, as it is not used anymore.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17813
This series adds notification before dropping views and indices so that the
tablet_allocator can generate mutations to respectively drop all tablets associated with them from system.tablets.
Additional unit tests were added for these cases.
Note that one case is not yet tested: where a table is allowed to be dropped while having views that depend on it, when it is dropped from the alternator path.
This is tested indirectly by testing dropping a table with live secondary index as it follows the same notification path as views in this series.
Fixes#17627Closesscylladb/scylladb#17773
* github.com:scylladb/scylladb:
migration_manager: notify before_drop_column_family when dropping indices
schema_tables: make_update_indices_mutations: use find_schema to lookup the view of dropped indices
migration_manager: notify before_drop_column_family before dropping views
cql-pytest: test_tablets: add test_tablets_are_dropped_when_dropping_table
tablet_allocator: on_before_drop_column_family: remove unused result variable
Loader was changed to quickly determine ownership after consuming
sharding metadata only. If it's not available, it falls back to
reading first and last keys from summary. The fallback is only there
for backward compatibility and it costs a lot more as we don't
skip to the end where keys are located in summary.
With tablets, sharding metadata is only first and last keys so
we can do it without sharder. So loader will be able to use it
instead of looking up keys in summary.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closesscylladb/scylladb#17805
When jumping from streaming stage into cleanup_target, session must also
be cleared as pending replica may still process some incoming mutations
blocked in the pipeline. Deleting session prior to executing barrier
makes sure those mutations will not be applied.
fixes: #17682
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#17800
When dropping indices, we don't need to go through
`create_view_for_index` in order to drop the index.
That actually creates a new schema for this view
which is used just for its metadata for generating mutations
dropping it.
Instead, use `find_schema` to lookup the current schema
for the dropped index.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Call the before_drop_column_family notifications
before dropping the views to allow the tablet_allocator
to delete the view's tablets.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Tablet transition would get stuck anyway for such nodes, so it's not worth trying
refs: #16372 (not fixes, because there's also repair transitions with same problem)
Closesscylladb/scylladb#17796
* github.com:scylladb/scylladb:
topology_coordinator: Skip dead nodes when balancing tablets
test: Add test for load_balancer skiplist
tablet_allocator: Add skiplist to load_balancer
The status command has an extensive amount of requests to the server. To be able to handle this more easily, the rest api mock server is refactored extensively to be more flexible, accepting expected requests out-of-order. While at it, the rest api mock server also moves away from a deprecated `aiohttp` feature: providing custom router argument to the `aiohttp` app. This forces us to pre-register all API endpoints that any test currently uses, although due to some templateing support, this is not as bad as it sounds. Still, this is an annoyance, but this point we have implemented almost all commands, so this won't be much a of a problem going forward.
Refs: https://github.com/scylladb/scylladb/issues/15588Closesscylladb/scylladb#17547
* github.com:scylladb/scylladb:
tools/scylla-nodetool: implement the status command
test/nodetool: rest_api_mock.py: match requests out-of-order
test/nodetool: rest_api_mock.py: remove trailing / from request paths
test/nodetool: rest_api_mock.py: use static routes
test/nodetool: check only non-exhausted requests
tools/scylla-nodetool: repair: set the jobThreads request parameter
It's too late to call `remove_rpc_client_with_ignored_topology` on messaging service when a node becomes normal. Data plane requests can be routed to the node much earlier, at least when topology switches to `write_both_read_new`. The `remove_rpc_client_with_ignored_topology` function shutdowns sockets and causes such requests to timeout.
In this PR we move the `remove_rpc_client_with_ignored_topology` call to the earliest point possible when a node first appears in `token_metadata.topology`.
From the topology coordinator perspective this happens when a joining node moves to `node_state::bootstrapping` and the topology moves to `transition_state::join_group0`. In `sync_raft_topology_nodes` the node should be contained in transition_nodes. The successful `wait_for_ip` before entering `transition_state::join_group0` ensures that update_topology should find a node's IP and put it into the topology. The barrier in `commit_cdc_generation` will ensure that all nodes in the cluster are using the proper connection parameters.
Only outgoing connections are tracked by `remove_rpc_client_with_ignored_topology`, those created by the current node. This means we need to call `remove_rpc_client_with_ignored_topology` on each node of the cluster.
fixesscylladb/scylladb#17445Closesscylladb/scylladb#17757
* github.com:scylladb/scylladb:
test_remove_rpc_client_with_pending_requests: add a regression test
remove_rpc_client_with_ignored_topology: call it earlier
storage_service: decouple remove_rpc_client_with_ignored_topology from notify_joined
Since 4c767c379c we can reach a situation
where we know that we have admitted too many expensive view update
operations and the mechanism of dropping the following view updates
can be triggerred in a wider range of scenarios. Ideally, we would
want to fail whole requests on the coordinator level, but for now, we
change the behavior to failing just the base writes. This allows us
to avoid creating inconsistencies between base replicas and views
at the cost of introducing inconsistencies between different base
replicas. This, however, can be fixed by repair, in contrast to
base-view inconsistencies which we don't have a good method of fixing.
Fixes#17795Closesscylladb/scylladb#17777
Since 6b87778 regular compaction tasks are removed from task manager
immediately after they are finished.
test_regular_compaction_task lists compaction tasks and then requests
their statuses. Only one regular compaction task is guaranteed to still
be running at that time, the rest of them may finish before their status
is requested and so it will no longer be in task manager, causing the test
to fail.
Fix statuses check to consider the possibility of a regular compaction
task being removed from task manager.
Fixes: #17776.
Closesscylladb/scylladb#17784
Commit 0665d9c346 changed the gossiper
failure detector in the following way: when live endpoints change
and per-node failure detectors finish their loops, the main failure
detector calls gossiper::convict for those nodes which were alive when
the current iteration of the main FD started but now are not. This was
changed in order to make sure that nodes are marked as down, because
some other code in gossiper could concurrently remove nodes from
the live node lists without marking them properly.
This was committed around 3 years ago and the situation changed:
- After 75d1dd3a76
the `endpoint_state::_is_alive` field was removed and liveness
of a node is solely determined by its presence
in the `gossiper::_live_endpoints` field.
- Currently, all gossiper code which modifies `_live_endpoints`
takes care to trigger relevant callback. The only function which
modifies the field but does not trigger notifications
is `gossiper::evict_from_membership`, but it is either called
after `gossiper::remove_endpoint` which triggers callbacks
by itself, or when a node is already dead and there is no need
to trigger callbacks.
So, it looks like the reasons it was introduced for are not relevant
anymore. What's more important though is that it is involved in a bug
described in scylladb/scylladb#17515. In short, the following sequence
of events may happen:
1. Failure detector for some remote node X decides that it was dead
long enough and `convict`s it, causing live endpoints to be updated.
2. The gossiper main loop sends a successful echo to X and *decides*
to mark it as alive.
3. At the same time, failure detector for all nodes other than X finish
and main failure detector continues; it notices that node X is
not alive (because it was convicted in point 1.) and *decides*
to convict it.
4. Actions planned in 2 and 3 run one after another, i.e. node is first
marked as alive and then immediately as dead.
This causes `on_alive` callbacks to run first and then `on_dead`. The
second one is problematic as it closes RPC connections to node X - in
particular, if X is in the process of replacing another node with the
same IP then it may cause the replace operation to fail.
In order to simplify the code and fix the bug - remove the piece
of logic in question.
Fixes: scylladb/scylladb#17515Closesscylladb/scylladb#17754
Nodetool currently assumes that positional arguments are only keyspaces.
ks.tbl pairs are only provided when --kt-list or friends are used. This
is not the case however. So check positional args too, and if they look
like ks.tbl, handle them accordingly.
While at it, also make sure that alternator keyspace and tables names
are handled correctly.
Closesscylladb/scylladb#17480
The method in question can have a shorter name that matches all other injections in this class, and can be non-template
Closesscylladb/scylladb#17734
* github.com:scylladb/scylladb:
error_injection: De-template inject() with handler
error_injection: Overload inject() instead of inject_with_handler()
This test reproduces the problem from scylladb/scylladb#17445.
It fails quite reliably without the fix from the previous
commit.
The test just bootstraps a new node while bombarding the cluster
with read requests.
In this commit we move the remove_rpc_client_with_ignored_topology
call to the earliest point possible - when a node first appears
in token_metadata.topology.
From the topology coordinator perspective this happens when a joining
node moves to node_state::bootstrapping and the topology moves to
transition_state::join_group0. In sync_raft_topology_nodes
the node should be contained in transition_nodes. The successful
wait_for_ip before entering transition_state::join_group0 ensures
that update_topology should find a node's IP and put it into the topology.
The barrier in commit_cdc_generation will ensure that all nodes
in the cluster are using the proper connection parameters.
Only outgoing connections are tracked by remove_rpc_client_with_ignored_topology,
those created by the current node. This means we need to call
remove_rpc_client_with_ignored_topology on each node of the cluster.
fixesscylladb/scylladb#17445
notify_joined
It's too late to call remove_rpc_client_with_ignored_topology on
messaging service when a node becomes normal. Data
plane requests can be routed to the node much earlier,
at least when topology switches to write_both_read_new.
The remove_rpc_client_with_ignored_topology function
shutdowns sockets and causes such requests to timeout.
We intend to call remove_rpc_client_with_ignored_topology
as soon as a node becomes part of token_metadata topology.
In this preparatory commit we refactor
storage_service::notify_joined. We remove the
remove_rpc_client_with_ignored_topology call from it
call it separately from the two call sites of notify_joined.
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, since boost::program_options::options_description is
defined by boost.program_options library, and it only provides the
operator<< overload. we're inclined to not specializing `fmt::formatter`
for it at this moment, because
* this class is not in defined by scylla project. we would have to
find a home for this formatter.
* we are not likely to reuse the formatter in multiple places
so, in this change we just print it using `fmt::streamed`.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17791
The coordinator can find out which nodes are marked as DOWN, thus when
calling tablets balancer it can feed it a skiplist
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The test is inspired by the test_load_balancing_with_empty_node one and
verifies that when a node is skiplisted, balancer doesn't put load on it
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Currently load balancer skips nodes only based on its "administrative"
state, i.e. whether it's drained/decommissioned/removed/etc. There's no
way to exclude any node from balancing decision based on anything else.
This patch add this ability by adding skiplist argument to
balance_tablets() method. When a node is in it, it will not be
considered, as if it was removenode-d.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* db::commitlog::segment::cf_mark
* db::commitlog::segment_manager::named_file
* db::commitlog::segment_manager::dispose_mode
* db::commitlog::segment_manager::byte_flow<T>
please note, the formatter of `db::commitlog::segment` is not
included in this commit, as we are formatting it in the inline
definition of this class. so we cannot define the specialization
of `fmt::formatter` for this class before its callers -- we'd
either use `format_as()` provided by {fmt} v10, or use `fmt::streamed`.
either way, it's different from the theme of this commit, and we
will handle it in a separated commit.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17792
Contrary to Origin, the single-token case is not discriminated in the
native implementation, for two reasons:
* ScyllaDB doesn't ever run with a single token, it is even moving away
from vnodes.
* Origin implemented the logic to detect single-token with a mistake: it
compares the number of tokens to the number of DCs, not the number of
nodes.
Another difference is that the native implementation doesn't request
ownership information when a keyspace argument was not provided -- it is
not printed anyway.
In the previous patch, we made matching requests to different endpoints
be matched out-of-order. In this patch we go one step further and make
matching requests to the same endpoint match out-of-order too.
With this, tests can register the expected requests in any order, not in
the same order as the nodetool-under-test is expected to send them. This
makes testing more flexible. Also, how requests are ordered is not
interesting from the correctness' POV anyway.
The legacy nodetool likes to append an "/" to the requests paths every
now and then, but not consistently. Unfortunately, request path matching
in the mock rest server and in aiohttp is quite sensitive to this
currently. Reduce friction by removing trailing "/" from paths in the
mock api, allowing paths to match each other even if one has a trailing
"/" but the other doesn't.
Unfortunately there is nothing we can do about the aiohttp part, so some
API endpoints have to be registered with a trailing "/".
The mock server currently provides its own router to the aiohttp.web
app. The ability to provide custom routers however is deprecated and
can be removed at any point. So refactor the mock server to use the
built-in router. This requires some changes, because the built-in router
does not allow adding/removing routes once the server starts. However
the mock server only learns of the used routes when the tests run.
This unfortunately means that we have to statically register all
possible routes the tests will use. Fortunately, aiohttp has variable
route support (templated routes) and with this, we can get away with
just 9 statically registered routes, which is not too bad.
A (desired) side-effect of this refactoring is that now requests to
different routes do not have to arrive in order. This constraint of the
previous implementation proved to be not useful, and even made writing
certain tests awkward.
Refactor how the tests check for expected requests which were never
invoked. At the end of every test, the nodetool fixture requests all
unconsumed expected requests from the rest_api_mock.py and checks that
there is none. This mechanism has some interaction with requests which
have a "multiple" set: rest_api_mock.py allows registering requests with
different "multiple" requirements -- how many times a request is
expected to be invoked:
* ANY: [0, +inf)
* ONE: 1
* MULTIPLE: [1, +inf)
Requests are stored in a stack. When a request arrives, we pop off
requests from the top until we find a perfect match. We pop off
requests, iff: multiple == ANY || multiple == MULTIPLE and was hit at
least once.
This works as long as we don't have an multiple=ANY request at the
bottom of the stack which is never invoked. Or a multiple=MULTIPLE one.
This will get worse once we refactor requests to be not stored in a
stack.
So in this patch, we filter requests when collecting unexhausted ones,
dropping those which would be qualified to be popped from the stack.
Although ScyllaDB ignores this request parameter, the Java nodetools
sets it, so it is better to have the native one do the same for
symmetry. It makes testing easier.
Discovered with the more strict request matching introduced in the next
patches.
It is not supported currently.
If a user passes the option, the request will be rejected with:
The hosts option is not supported for tablet repair
The ignore_nodes option is not supported for tablet repair
This option is useful to select nodes to repair.
Fixes: #17742
Tests: repair_additional_test.py::TestRepairAdditional::test_repair_ignore_nodes
repair_additional_test.py::TestRepairAdditional::test_repair_ignore_nodes_errors
repair_additional_test.py::TestRepairAdditional::test_repair_option_pr_dc_host
Closesscylladb/scylladb#17767
