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

Use pg_local to track AMQP 1.0 connections #9374

Merged
merged 1 commit into from
Sep 16, 2023
Merged

Conversation

lukebakken
Copy link
Collaborator

@lukebakken lukebakken commented Sep 12, 2023

Fixes #9371

Since each AMQP 1.0 connection opens several direct AMQP connections, we
must assign each direct connection a unique name to prevent multiple
entries in the connection_created_stats table.

Also, use pg_local to track AMQP 1.0 connections instead of walking
the supervisor trees.

@lukebakken lukebakken self-assigned this Sep 12, 2023
@lukebakken lukebakken requested a review from ansd September 12, 2023 12:20
@mergify mergify bot added the bazel label Sep 12, 2023
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

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

Why is pg_local moved to rabbit_common?
rabbit_common is for code common between rabbit and AMQP 0.9.1 client.

@lukebakken
Copy link
Collaborator Author

Why is pg_local moved to rabbit_common? rabbit_common is for code common between rabbit and AMQP 0.9.1 client.

Did we change the "rules" when it comes to sharing code between sub-projects in deps/? As far as I know, plugins can't use code in deps/rabbit. cc @dumbbell

The rabbitmq_stream plugin also uses pg_local which is "against the rules". At least, the last time I dealt with something like this.

@lukebakken lukebakken requested review from dumbbell and ansd September 12, 2023 13:22
@dumbbell
Copy link
Member

Did we change the "rules" when it comes to sharing code between sub-projects in deps/? As far as I know, plugins can't use code in deps/rabbit. cc @dumbbell

That's the opposite: plugins must depend on rabbit. We should not add code to rabbit_common unless the amqp_client needs it. Otherwise, the client inherits everything the server depends on.

There are still many circular dependency problems around rabbit_common, mostly because of the "direct connection" feature of amqp_client which requires a (too) deep knowledge of the server.

As amqp_client almost never evolves, we should basically never add code in rabbit_common.

@lukebakken lukebakken marked this pull request as ready for review September 12, 2023 13:32
@lukebakken lukebakken marked this pull request as draft September 12, 2023 13:32
@lukebakken lukebakken force-pushed the rabbitmq-server-9371 branch 2 times, most recently from 141a184 to 85b95d2 Compare September 12, 2023 14:20
@lukebakken lukebakken marked this pull request as ready for review September 12, 2023 14:21
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

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

1.)
I like the idea of using a separate process group for AMQP 1.0 connections. We likely need such a process group for Native AMQP anyway.
However, I would prefer to use local only pg as done for Native MQTT instead of pg_local. pg_local becomes a bottleneck as shown in https://github.com/rabbitmq/mqtt-testing/tree/79329012092f9b77f3663086a590b4631cb812a9/one-to-one#mass-client-disconnection
However, I could also to this changes as part of #9022. I don't think scalability matters much for non-native AMQP as shown by the supervisor:which_children/1 calls prior to this PR.

2.)
Closing the connection via the management UI shows inconsistencies in the AMQP 1.0 connections.
To repro:

  1. Create an AMQP 1.0 connection.
  2. You will see that connection the Management UI
  3. Force Close that connection from the Management UI.
  4. The Management UI will not show the connection.
  5. However, rabbitmqctl list_amqp10_connections will still show the connection.

From my point of view, this bug is acceptable for now, I'll fix it for 4.0 as part of #9022

deps/rabbitmq_management_agent/src/rabbit_mgmt_format.erl Outdated Show resolved Hide resolved
@lukebakken lukebakken requested a review from ansd September 12, 2023 16:26
@lukebakken lukebakken removed the request for review from dumbbell September 12, 2023 16:28
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

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

Tests are failing.

@michaelklishin
Copy link
Member

Looks like the

bazel test //deps/rabbitmq_amqp1_0:proxy_protocol_SUITE

failures are consistent.

@lukebakken
Copy link
Collaborator Author

@michaelklishin I'm working on it!

@lukebakken lukebakken requested a review from ansd September 15, 2023 13:01
@lukebakken
Copy link
Collaborator Author

@ansd @michaelklishin this should be good to go after the next CI round.

Fixes #9371

Since each AMQP 1.0 connection opens several direct AMQP connections, we
must assign each direct connection a unique name to prevent multiple
entries in the `connection_created_stats` table.

Also, use `pg_local` to track AMQP 1.0 connections instead of walking
the supervisor trees.

Nuke authz_backends from connection created event 💥

Fix regex for connection name because UniqueId is part of it now (channel number)
Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

Expanding the list of columns we return for AMQP 1.0 connections would be very nice but this (avoiding crashes and querying supervisors for children) is definitely an improvement as is.

@michaelklishin michaelklishin added this to the 3.12.5 milestone Sep 16, 2023
@michaelklishin michaelklishin merged commit ee7c6d9 into main Sep 16, 2023
@michaelklishin michaelklishin deleted the rabbitmq-server-9371 branch September 16, 2023 02:15
michaelklishin added a commit that referenced this pull request Sep 16, 2023
Use pg_local to track AMQP 1.0 connections (backport #9374)
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.

Listing AMQP 1.0 connections causes crashes that are logged
4 participants