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

Periodically reconcile virtual host processes #11363

Conversation

michaelklishin
Copy link
Member

for up to 10 times.

When a cluster is formed from scratch and a virtual host is declared in the process (can be via
definitions, or plugins, or any other way),
that virtual host's process tree will be started
on all reachable cluster nodes at that moment
in time.

However, this can be just a subset of all nodes
expected to join the cluster.

The most effective solution is to run this
reconciliation process on a timer for up to
5 minutes by default. This matches how long
some other parts of RabbitMQ (3.x) expect
cluster formation to take, at most.

Per discussion with @dcorbacho @mkuratczyk.

@michaelklishin michaelklishin requested a review from mkuratczyk June 4, 2024 03:19
@michaelklishin michaelklishin force-pushed the mk-make-sure-virtual-hosts-are-started-on-all-nodes-after-definition-import branch 2 times, most recently from ff0cf27 to 41dcab0 Compare June 4, 2024 04:03
@michaelklishin
Copy link
Member Author

Investigating the //deps/rabbit:dialyze failure.

@michaelklishin michaelklishin force-pushed the mk-make-sure-virtual-hosts-are-started-on-all-nodes-after-definition-import branch from 41dcab0 to a8d4451 Compare June 4, 2024 18:57
@michaelklishin
Copy link
Member Author

I will add a CLI command as discussed with @dcorbacho @mkuratczyk.

@michaelklishin michaelklishin force-pushed the mk-make-sure-virtual-hosts-are-started-on-all-nodes-after-definition-import branch 7 times, most recently from e9d373e to 6831880 Compare June 6, 2024 03:23

%% Same as rabbit_vhost:exists/1.
-spec exists(vhost:name()) -> boolean().
exists(VirtualHost) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing against it, but why this function here? I can't see where it is being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, exists/1, and list_names/1 already exist in rabbit_vhost. Why not use them?

Copy link
Member Author

@michaelklishin michaelklishin Jun 6, 2024

Choose a reason for hiding this comment

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

Some virtual host functions are in rabbit_vhost, others are in the supervision tree-related modules. We need a place for all key functions.

There's also a comment at the top that explains why the module was originally introduced:

%% This module exists to avoid circular module dependencies between
%% several others virtual hosts-related modules.

Copy link
Contributor

@mkuratczyk mkuratczyk left a comment

Choose a reason for hiding this comment

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

Works as advertised.

Michael Klishin and others added 6 commits June 6, 2024 21:21
for up to 10 times.

When a cluster is formed from scratch and a virtual
host is declared in the process (can be via
definitions, or plugins, or any other way),
that virtual host's process tree will be started
on all reachable cluster nodes at that moment
in time.

However, this can be just a subset of all nodes
expected to join the cluster.

The most effective solution is to run this
reconciliation process on a timer for up to
5 minutes by default. This matches how long
some other parts of RabbitMQ (3.x) expect
cluster formation to take, at most.

Per discussion with @dcorbacho @mkuratczyk.
Certain test suites need virtual hosts to be down
for an extended period of time, and this feature
gets in a way ;)
@michaelklishin michaelklishin force-pushed the mk-make-sure-virtual-hosts-are-started-on-all-nodes-after-definition-import branch from 0f1c49f to 959d3fd Compare June 7, 2024 01:21
@michaelklishin
Copy link
Member Author

Something is up with Actions, they are not picking up my latest changes. Will submit a new PR instead.

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