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.

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

Reorganize data in the Khepri store #11225

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Reorganize data in the Khepri store #11225

merged 2 commits into from
Sep 5, 2024

Conversation

dumbbell
Copy link
Member

@dumbbell dumbbell commented May 14, 2024

Why

The previous layout followed the flat structure we have in Mnesia:

  • In Mnesia, we have tables named after each purpose (exchanges, queues, runtime parameters and so on).
  • In Khepri, we had about the same but the table names were replaced by a tree node in the tree. We ended up with one tree node per purpose at the root of the tree.

Khepri implements a tree. We could benefit from this and organize data to reflect their relationship in RabbitMQ.

How

Here is the new hierarchy implemented by this commit:

rabbitmq
|-- users
|   `-- $username
|-- vhosts
|   `-- $vhost
|       |-- user_permissions
|       |   `-- $username
|       |-- exchanges
|       |   `-- $exchange
|       |       |-- bindings
|       |       |   |-- queue
|       |       |   |   `-- $queue
|       |       |   `-- exchange
|       |       |       `-- $exchange
|       |       |-- consistent_hash_ring_state
|       |       |-- jms_topic
|       |       |-- recent_history
|       |       |-- serial
|       |       `-- user_permissions
|       |           `-- $username
|       |-- queues
|       |   `-- $queue
|       `-- runtime_params
|           `-- $param_name
|-- runtime_params
|   `-- $param_name
|-- mirrored_supervisors
|   `-- $group
|       `-- $id
`-- node_maintenance
    `-- $node

We first define a root path in rabbit/include/khepri.hrl as [rabbitmq]. This could be anything, including an empty path.

All paths are constructed either from this root path definition (users and vhosts paths do that), or from a parent resource's path (exchanges and queues paths are based on a vhost path).

@dumbbell dumbbell requested a review from dcorbacho May 14, 2024 09:58
@dumbbell dumbbell self-assigned this May 14, 2024
@mergify mergify bot added the bazel label May 14, 2024
@mergify mergify bot added the make label Jul 31, 2024
@dumbbell dumbbell force-pushed the reorganize-khepri-tree branch 4 times, most recently from d5f09d6 to de7df84 Compare August 14, 2024 16:12
@michaelklishin michaelklishin changed the title Reorganize the Khepri tree 4.0: Reorganize the Khepri tree Aug 14, 2024
@dumbbell dumbbell force-pushed the reorganize-khepri-tree branch 2 times, most recently from a4a97cb to daf182a Compare August 16, 2024 12:12
@dumbbell dumbbell changed the title 4.0: Reorganize the Khepri tree Reorganize data in the Khepri store Aug 16, 2024
@dumbbell
Copy link
Member Author

@michaelklishin: I removed the "4.0" prefix you added to the title because we will probably backport that to 3.13.x once it is stable.

@dumbbell
Copy link
Member Author

dumbbell commented Aug 16, 2024

The layout is now in place.

The next challenge is the handling of deletions, especially the bindings. Currently the code does something like this:

  1. In a transaction:
    1. A queue is deleted from the database
    2. Bindings that have that queue as their destination are deleted too
    3. If there are source exchanges with the auto-delete flag set and have no more bindings, they are deleted too
  2. Outside of the transaction, deletions are "processed" to emit notifications for the queue, bindings and auto-deleted exchanges.

About the same occurs when a source exchange is deleted.

With this new organization, bindings are implicitly deleted with their source exchange.

Can we cover these situations with keep_while conditions in Khepri?

  • One on bindings so that they are deleted when their destination (queue or exchange) is deleted?
  • One on exchanges so that they are deleted when they no longer have bindings underneath?

The processing of the deletion should remain about the same as it already runs after records were deleted from the database. This would also allow us to drop the transaction and use a single delete.

A related idea: what about using #if_exists{} conditions when creating resources? For example, we could add " if vhost exists" in the path of an exchange. This way, the insert would be rejected if the vhost was deleted concurrently. This won't replace the keep_while conditions.

The tricky part is doing this while maintaining behavior with Mnesia.

@dumbbell
Copy link
Member Author

dumbbell commented Aug 19, 2024

After studying the code more deeply, I believe the current situation is fine:

  • Exchanges and bindings that are supposed to be deleted during the deletion of another queue or exchange are already taker care of in a Khepri transaction, and the rest of the logic is handled in process_deletions() functions that do nod read the database (they get the old records as arguments).
  • Exchange type callback modules are called from these process_deletions() functions, so their specific records were already deleted. Fortunately, the callbacks were also simply deleting them.
  • The vhost code already calls explicitly the deletion function of everything that live underneath before the vhost record is deleted from the database.

Therefore, I think we can defer the improvements to the deletion code to the the time we drop Mnesia.

What do you think @dcorbacho? Am I missing something?

@dcorbacho
Copy link
Contributor

@dumbbell Agree, we're fine with that.

@dumbbell
Copy link
Member Author

Here is an example of the content of Khepri in an unclustered RabbitMQ with a single PerfTest client running with default options:

●
├── '__khepri_mnesia_migration'
│   ╰── m2k_table_copy
│       ╰── <<"rabbitmq_metadata">>
│             Data: {migration,finished,
│                              [rabbit_vhost,rabbit_user,rabbit_user_permission,
│                               rabbit_topic_permission,rabbit_runtime_parameters,
│                               rabbit_queue,rabbit_exchange,rabbit_exchange_serial,
│                               rabbit_route,rabbit_node_maintenance_states,
│                               mirrored_sup_childspec,rabbit_durable_queue,
│                               rabbit_durable_exchange,rabbit_durable_route,
│                               rabbit_semi_durable_route,rabbit_reverse_route,
│                               rabbit_index_route]}
│
╰── rabbitmq
    ├── node_maintenance
    │   ╰── rabbit@giottoData: {node_maintenance_state,rabbit@giotto,regular,#{}}
    │
    ├── runtime_params
    │   ╰── internal_cluster_idData: {runtime_parameters,internal_cluster_id,
    │                                   <<"rabbitmq-cluster-id-9tjXGNZE97EyiRBoH6sHTA">>}
    │
    ├── users
    │   ╰── <<"guest">>
    │         Data: {internal_user,<<"guest">>,
    │                              <<195,20,77,64,29,28,55,234,23,125,234,249,78,76,209,126,
    │                                72,20,135,236,235,100,222,244,185,61,113,44,7,25,67,198,
    │                                179,92,83,95>>,
    │                              [administrator],
    │                              rabbit_password_hashing_sha256,#{}}
    │
    ╰── vhosts
        ╰── <<"/">>
            │ Data: {vhost,<<"/">>,[],
            │              #{description => <<"Default virtual host">>,tags => []}}
            │
            ├── exchanges
            │   ├── <<>>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<>>},
            │   │                     direct,true,false,false,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ├── <<"amq.direct">>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<"amq.direct">>},
            │   │                     direct,true,false,false,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ├── <<"amq.fanout">>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<"amq.fanout">>},
            │   │                     fanout,true,false,false,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ├── <<"amq.headers">>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<"amq.headers">>},
            │   │                     headers,true,false,false,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ├── <<"amq.match">>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<"amq.match">>},
            │   │                     headers,true,false,false,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ├── <<"amq.rabbitmq.trace">>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<"amq.rabbitmq.trace">>},
            │   │                     topic,true,false,true,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ├── <<"amq.topic">>
            │   │     Data: {exchange,{resource,<<"/">>,exchange,<<"amq.topic">>},
            │   │                     topic,true,false,false,[],undefined,undefined,undefined,
            │   │                     {[],[]},
            │   │                     #{user => <<"rmq-internal">>}}
            │   │
            │   ╰── <<"direct">>
            │       │ Data: {exchange,{resource,<<"/">>,exchange,<<"direct">>},
            │       │                 direct,true,false,false,[],undefined,undefined,undefined,
            │       │                 {[],[]},
            │       │                 #{user => <<"guest">>}}
            │       │
            │       ╰── bindings
            │           ╰── queue
            │               ╰── <<"amq.gen-9chRARA1KM5g_0NqZ8OCQQ">>
            │                   ╰── <<"3c299664-94e7-443a-b642-f7dde58b759e">>
            │                         Data: #{{binding,{resource,<<"/">>,exchange,<<"direct">>},
            │                                          <<"3c299664-94e7-443a-b642-f7dde58b759e">>,
            │                                          {resource,<<"/">>,queue,<<"amq.gen-9chRARA1KM5g_0NqZ8OCQQ">>},
            │                                          []} =>
            │                                     []}
            │
            ├── queues
            │   ╰── <<"amq.gen-9chRARA1KM5g_0NqZ8OCQQ">>
            │         Data: {amqqueue,{resource,<<"/">>,queue,<<"amq.gen-9chRARA1KM5g_0NqZ8OCQQ">>},
            │                         false,true,none,[],<0.736.0>,[],[],[],undefined,undefined,[],
            │                         [],live,0,[],<<"/">>,
            │                         #{user => <<"guest">>},
            │                         rabbit_classic_queue,#{}}
            │
            ╰── user_permissions
                ╰── <<"guest">>
                      Data: {user_permission,{user_vhost,<<"guest">>,<<"/">>},
                                             {permission,<<".*">>,<<".*">>,<<".*">>}}

@dumbbell dumbbell force-pushed the reorganize-khepri-tree branch 4 times, most recently from 1a7aba1 to f793716 Compare August 26, 2024 09:46
@dumbbell dumbbell force-pushed the reorganize-khepri-tree branch 2 times, most recently from 5bd6711 to 395dfe7 Compare September 4, 2024 14:58
[Why]

The previous layout followed the flat structure we have in Mnesia:
* In Mnesia, we have tables named after each purpose (exchanges, queues,
  runtime parameters and so on).
* In Khepri, we had about the same but the table names were replaced by
  a tree node in the tree. We ended up with one tree node per purpose
  at the root of the tree.

Khepri implements a tree. We could benefit from this and organize data
to reflect their relationship in RabbitMQ.

[How]

Here is the new hierarchy implemented by this commit:

    rabbitmq
    |-- users
    |   `-- $username
    |-- vhosts
    |   `-- $vhost
    |       |-- user_permissions
    |       |   `-- $username
    |       |-- exchanges
    |       |   `-- $exchange
    |       |       |-- bindings
    |       |       |   |-- queue
    |       |       |   |   `-- $queue
    |       |       |   `-- exchange
    |       |       |       `-- $exchange
    |       |       |-- consistent_hash_ring_state
    |       |       |-- jms_topic
    |       |       |-- recent_history
    |       |       |-- serial
    |       |       `-- user_permissions
    |       |           `-- $username
    |       |-- queues
    |       |   `-- $queue
    |       `-- runtime_params
    |           `-- $param_name
    |-- runtime_params
    |   `-- $param_name
    |-- mirrored_supervisors
    |   `-- $group
    |       `-- $id
    `-- node_maintenance
        `-- $node

We first define a root path in `rabbit/include/khepri.hrl` as
`[rabbitmq]`. This could be anything, including an empty path.

All paths are constructed either from this root path definition (users
and vhosts paths do that), or from a parent resource's path (exchanges
and queues paths are based on a vhost path).
@dumbbell dumbbell marked this pull request as ready for review September 5, 2024 15:13
@dumbbell dumbbell merged commit 30c82d1 into main Sep 5, 2024
240 checks passed
@dumbbell dumbbell deleted the reorganize-khepri-tree branch September 5, 2024 15:14
@michaelklishin michaelklishin added this to the 4.0.0 milestone Sep 5, 2024
@dumbbell dumbbell removed this from the 4.0.0 milestone Sep 10, 2024
dumbbell added a commit that referenced this pull request Sep 10, 2024
Reorganize data in the Khepri store (backport #11225)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants