Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Manage STM creation outside of cluster::partition #15424

Merged
merged 20 commits into from
Jan 3, 2024
Merged

Manage STM creation outside of cluster::partition #15424

merged 20 commits into from
Jan 3, 2024

Conversation

mmaslankaprv
Copy link
Member

@mmaslankaprv mmaslankaprv commented Dec 13, 2023 •
edited

Changed the way how state machines are managed in Redpanda. Previously all the STMs were a part of cluster::partition. This was convenient as the partition STM lifecycle is in general bound with the partition lifecycle. There were two main problems with that approach:

  1. all the state machines had to be defined in cluster module or other modules that the cluster module dependent on
  2. cluster::partition interface and behaviour was customised in runtime based solely on the partition name.

The new approach introduced with this PR makes it possible to create STMs outside of the cluster module as the STM interface is generalised at the cluster module level. Newly introduced state_machine_registry allows registering an STM factory. Factory decide whether the stm is required for given Raft group and if that is the case the STM is then created.

With the new approach STM implementer can simply create an STM by implementing either a raft::state_machine_base interface or extending raft::persisted_stm. Implementer provides and register a factory in partition manager instance.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

  • none

ztlpn
ztlpn reviewed Dec 13, 2023
View reviewed changes
src/v/raft/fwd.h Outdated Show resolved Hide resolved
@mmaslankaprv mmaslankaprv marked this pull request as ready for review December 14, 2023 17:28
@mmaslankaprv mmaslankaprv marked this pull request as draft December 14, 2023 17:28
@mmaslankaprv
Copy link
Member Author

/dt

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Dec 14, 2023 •
edited

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42801#018c699f-4552-4562-a4a1-534d21153dac

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42801#018c69aa-27e6-4bfa-bae9-60f9ce39dba0

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42854#018c6d1a-53e6-4c7d-99b7-1f3cb6039ab8

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42854#018c6d1a-53e3-419b-b451-2602b66d0542

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/43119#018c86a1-5e2c-4be8-a4a6-f1d8c5636618

@mmaslankaprv mmaslankaprv force-pushed the stm-registry branch 2 times, most recently from 4e07d39 to d3e24cb Compare December 15, 2023 09:13
@mmaslankaprv
Copy link
Member Author

/ci-repeat 1

@redpanda-data redpanda-data deleted a comment from vbotbuildovich Dec 15, 2023
@mmaslankaprv mmaslankaprv force-pushed the stm-registry branch 2 times, most recently from 1d66e8a to 8b95a3e Compare December 15, 2023 13:40
@mmaslankaprv
Copy link
Member Author

/ci-repeat 1

@redpanda-data redpanda-data deleted a comment from vbotbuildovich Dec 17, 2023
@mmaslankaprv
Copy link
Member Author

/ci-repeat 1

@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/42950#018c77c9-8961-4121-8f44-eb3541208d67:

"rptest.tests.delete_records_test.DeleteRecordsTest.test_delete_records_concurrent_truncations.cloud_storage_enabled=True"

@mmaslankaprv
Copy link
Member Author

/ci-repeat 1

@mmaslankaprv mmaslankaprv marked this pull request as ready for review December 18, 2023 17:31
@mmaslankaprv mmaslankaprv force-pushed the stm-registry branch 2 times, most recently from 240256f to e41ef2e Compare December 20, 2023 08:59
ztlpn
ztlpn reviewed Dec 20, 2023
View reviewed changes
src/v/cluster/state_machine_registry.h Outdated
* Must return true if STM should be created for a partition underlaid by
* passed raft group
*/
virtual bool is_applicable_for(const raft::consensus&) const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this in context of deletion of unmanaged partitions - it would be nice to be able to determine if the stm is applicable for a partition without creating a consensus instance. AFAICS currently all STMs require just an ntp, maybe we should start with just ntp as a parameter (and maybe add something more if the need arises, but never a full consensus instance).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was considering passing ntp_config which is what is currently required, will change that

src/v/cluster/id_allocator_stm.cc

void id_allocator_stm_factory::create(
raft::state_machine_manager_builder& builder, raft::consensus* raft) {
builder.create_stm<id_allocator_stm>(clusterlog, raft);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the line

raft->log()->stm_manager()->add_stm(stm);

required here?

Maybe it could be automated for all persisted stms?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually id_allocator_stm manages eviction on its own hence it is not registered in STM manager

src/v/cluster/partition.h Outdated
@@ -198,8 +191,8 @@ class partition {
/// that any truncation-delaying activitiy (like uploading to tiered
/// storage) could be expedited to enable local disk space to be reclaimed.
std::optional<model::offset> eviction_requested_offset() {
if (_log_eviction_stm) {
return _log_eviction_stm->eviction_requested_offset();
if (log_eviction_stm()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not a big deal, but doing a lookup each time seems wasteful. Maybe it is better to keep this and tm_stm as fields (initialized with stm_manager()->get() once)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see that it is there, but unused for some reason?

src/v/transform/rpc/deps.cc Outdated
auto stm = partition->transform_offsets_stm();
auto stm
= partition->raft()->stm_manager()->get<transform_offsets_stm_t>(
"distributed_kv_stm");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mismatch between the name and the type looks weird and implies that we can't have e.g. more than 1 distributed_kv_stm per partition. Maybe it is better to pass the stm name when registering the factory instead of tying it to the stm type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, i am not a fan of those names either, will add ability to pass a name as a part of factory registration interface

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to do partition->raft()->stm_manager()->get<stm_type>() right? And key everything off the class type?

So the underlying container is absl::flat_hash_map<std::type_index, state_machine_manager::named_stm> and the get method can create the index using: std::type_index(typeid(T)).

This assumes that each stm is it's own type, which I think is fine. For the case of the distributed_kv, I'd expect them to all have different keys and values, otherwise we can add a tag type to distinguish

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've ended up with adding a static name field to stm this way we do not need to relay ion typeid in human readable parts f.e. snapshot map or logs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah much better - love it!

src/v/transform/transform_offsets_stm.cc
"When creating transform stm the topic configuration must exists");

builder.create_stm<transform_offsets_stm_t>(
cfg->partition_count, tlog, raft);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stm_manager registration needed?

rockwotj
rockwotj approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for this 🎉 Looking forward to be able to untangle more deps and clean up the build graph because of this.

src/v/raft/state_machine_manager.h Outdated Show resolved Hide resolved
@mmaslankaprv
stm_manager: make state machine name static field of state machine
9a09680 
Removed `get_name` method from `raft::state_machine_base` and made it a
static variable. This way called does not have to provide a name and we
can easily query state machine by type rather than by name.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
r/state_machine_manager: added method to query certain type of stm
dd5798c 
Added an API method that will allow caller to query for an stm with a
specific name. The `dynamic_cast` is used to provide a basic level of
runtime correctness check.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/stm_registry: introduced state_machine_registry class
54cd31a 
State machine registry is a class holding all registered state machines
factories. Registry is used by partition_manger whenever a new partition
instance is created. Registry builds all state machines that are
required for a give partition instance.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition_manager: wire in stm registry into state machine manager
f619d71 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use state machine registry in partition
da0fa4b 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
r/stm_base: added support for handling local persistent state
d20b5e5 
Some of the state machines in Redpanda may persists its own local state.
State machine manager must be able to handle the local state i.e. query
for its size and remove it if required.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
r/stm_manager: added method to remove local state from all stms
6eeeb6c 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use stm manager to handle local stm state
e05e627 
Use stm manager APIs to query the size and delete stm local persistent
state

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use stm registry to create tm_stm
5530b07 
Added transactional manager state machine factory and using a state
machine registry to create manager stm only if partition requires it.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use stm registry to create id allocator stm
8f4cc25 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
utils: introduce fixed_string helper
4da976f 
Introduced `fixed_string` helper allowing to use string literals as
template parameters.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/distributed_kv: made stm name configurable via template argument
3164400 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
transforms: move transform offsets stm to transform module
2535aba 
Now that the stm creation is decoupled from its implementation we may
move transform stm implementation outside the cluster module.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use stm registry to create rm_stm
b339159 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use stm registry to create log eviction stm
840662f 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/archival_stm: create mem_tracked inside of archival_stm
9d4dc47 
Creating `mem_tracker` inside of archival stm to make it independent
from partition instance.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: use stm registry to create archival_metadata_stm
5aaf8c3 
Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition: cleanup partition dependency
d376196 
Now as the state machines are created outside of the partition we may
cleanup the dependencies used by partition abstraction.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@mmaslankaprv
c/partition_manager: do not pass topic configuration into manage method
44590d0 
We no longer need `topic_configuration` to create partition instance as
the stm creation process is managed outside of the partition.

Signed-off-by: Michal Maslanka <michal@redpanda.com>
rockwotj
rockwotj approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mmaslankaprv
Copy link
Member Author

ci failure:
#15042

@mmaslankaprv mmaslankaprv merged commit 0806712 into redpanda-data:dev Jan 3, 2024
17 of 19 checks passed
@mmaslankaprv mmaslankaprv deleted the stm-registry branch January 3, 2024 20:28
@andrwng andrwng mentioned this pull request Mar 25, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rockwotj rockwotj rockwotj approved these changes

@bharathv bharathv Awaiting requested review from bharathv

@Lazin Lazin Awaiting requested review from Lazin

@ztlpn ztlpn Awaiting requested review from ztlpn

@dotnwat dotnwat Awaiting requested review from dotnwat

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mmaslankaprv @vbotbuildovich @dotnwat @ztlpn @rockwotj