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

Dynamic Shovel deletion: use a more effective way (backport #11321) #11324

Merged
merged 2 commits into from
May 31, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented May 25, 2024

Dynamic Shovels keep track of their status in
a node-local ETS table which is updated as
Shovels go through the lifecycle: start,
init (connect, declare the topology), stop.

This makes failing Shovels a bit special:
their status records will not be long lived,
which means it will be considered not to exist
by certain code paths.

In addition, for such Shovels we do not know what
node they are hosted on. But that's fine:
we just need to clear their runtime parameter
and a periodic Shovel cleanup will remove all
children for which no schema database entry
(a runtime parameter one) does not exist.

References #4242.


This is an automatic backport of pull request #11321 done by Mergify.

Copy link
Author

mergify bot commented May 25, 2024

Cherry-pick of cae964d has failed:

On branch mergify/bp/v3.13.x/pr-11321
Your branch is up to date with 'origin/v3.13.x'.

You are currently cherry-picking commit cae964dba1.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   deps/rabbit_common/src/rabbit_misc.erl
	modified:   deps/rabbitmq_ct_helpers/src/rabbit_mgmt_test_util.erl
	modified:   deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.DeleteShovelCommand.erl
	modified:   deps/rabbitmq_shovel/src/Elixir.RabbitMQ.CLI.Ctl.Commands.ShovelStatusCommand.erl
	modified:   deps/rabbitmq_shovel/src/rabbit_shovel_status.erl
	modified:   deps/rabbitmq_shovel/src/rabbit_shovel_util.erl
	modified:   deps/rabbitmq_shovel/src/rabbit_shovel_worker.erl
	modified:   deps/rabbitmq_shovel_management/BUILD.bazel
	modified:   deps/rabbitmq_shovel_management/app.bzl
	modified:   deps/rabbitmq_shovel_management/src/rabbit_shovel_mgmt.erl
	renamed:    deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_util_SUITE.erl -> deps/rabbitmq_shovel_management/test/unit_SUITE.erl

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   deps/rabbitmq_shovel_management/test/http_SUITE.erl
	deleted by them: deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot added the bazel label May 25, 2024
michaelklishin added a commit that referenced this pull request May 25, 2024
@michaelklishin michaelklishin added this to the 3.13.3 milestone May 25, 2024
michaelklishin added a commit that referenced this pull request May 27, 2024
michaelklishin and others added 2 commits May 30, 2024 10:57
Dynamic Shovels keep track of their status in
a node-local ETS table which is updated as
Shovels go through the lifecycle: start,
init (connect, declare the topology), stop.

This makes failing Shovels a bit special:
their status records will not be long lived,
which means it will be considered not to exist
by certain code paths.

In addition, for such Shovels we do not know what
node they are hosted on. But that's fine:
we just need to clear their runtime parameter
and a periodic Shovel cleanup will remove all
children for which no schema database entry
(a runtime parameter one) does not exist.

rabbitmq_shovel_management's key integration
suite has been reworked and expanded to include
a case where the Shovel has no chance of
successfully connecting.

This also deletes a mock-based test suite
which does not serve much of a purpose.

(cherry picked from commit cae964d)

# Conflicts:
#	deps/rabbitmq_shovel_management/test/http_SUITE.erl
#	deps/rabbitmq_shovel_management/test/rabbit_shovel_mgmt_SUITE.erl
@michaelklishin
Copy link
Member

I have tested this PR with a three node cluster where two nodes were RabbitMQ 3.12.0 and 3.12.14, and rabbitmqctl delete_shovel with these changes works fine against all nodes, and successfully deletes both running and starting (in a start-fail-restart loop) shovels.

@michaelklishin michaelklishin merged commit 774ea9b into v3.13.x May 31, 2024
16 checks passed
@michaelklishin michaelklishin deleted the mergify/bp/v3.13.x/pr-11321 branch May 31, 2024 07:35
michaelklishin added a commit to michaelklishin/rabbit-hole that referenced this pull request Jun 15, 2024
1. There is no need to test Shovel deletion in
   this specific test
2. When a Shovel is starting, it cannot be deleted
   with several RabbitMQ versions due to [A]

A. rabbitmq/rabbitmq-server#11324
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.

None yet

1 participant