Rabbitmq-server: NaN (non existing) Exclusive Queues caused by queue-master-locator argument

[luos]

Hi,

Exclusive queues are affected by the global (config file), policy, and queue argument setting of queue-master-locator. When a client declares an exclusive queue it may happen that in this case the queue is not client-local, for example because the locator is set as random or min-masters.

When a node hosting a non-client-local exclusive queue is restarted the queue dies, but the cleanup method is missing check if the queue pid is alive, something like:

is_dead_exclusive(#amqqueue{exclusive_owner = OwnerPid, pid = QPid}) when is_pid(OwnerPid), is_pid(QPid) ->
    not (rabbit_mnesia:is_process_alive(OwnerPid)).

For this reason the still alive nodes think that the queue exists and do not allow the client to redeclare it.

Two solutions came to our mind:

• If a queue is exclusive, it should be always client local. This may not be that good solution for users who rely on the fact that exclusive queues are also affected by these settings
• Extend the is_dead_exclusive call with the following:
  not (rabbit_mnesia:is_process_alive(OwnerPid) andalso rabbit_mnesia:is_process_alive(QPid)).
  in this case there is some race condition between a client trying to re-declare the queue and it being actually deleted (i am not sure about this)

I'll open a PR to the 3.7.x branch shortly with my proposed changes, please review it and merge it if you think it's OK.

I noticed that the is_dead_exclusive changed on the master branch, but seems like it's now the opposite, it misses the Owner check? Maybe it was refactored to be different. I have not tested it with the new branch.

https://github.com/rabbitmq/rabbitmq-server/blob/master/src/rabbit_amqqueue.erl#L1618-L1622

Thanks,
Lajos

[michaelklishin]

There is a race condition between client actions and exclusive queue cleanup.

The option where exclusive queues are declared as client-local regardless of the locator policy makes sense to me. Adding a clause could be OK, too. @kjnilsson @hairyhum @gerhard @acogoluegnes thoughts?

[michaelklishin]

@luos please submit the same PR against master, we will either accept it or work on an alternative that limits exclusive queues to the client local node.

[velimir]

How much sense does it make to create HA exclusive queues? Feels like I'm missing something here, though if client-local allocation is an option, wouldn't it be better to even create a queue as non-HA if it's an exclusive one? non-HA queues, from what that I can see, are simpler and easier on resources.

Is there any other benefit, that I might missing, of the queue being an HA if it doesn't have any replicas?

[michaelklishin]

This scenario has nothing to do with mirroring. Exclusive queues are never mirrored but they have a different lifecycle from non-exclusive queues, mirrored or not.

[michaelklishin]

It's surprisingly difficult to reach a consensus about this. I'm leaning towards the option proposed by @luos: an exclusive queue should be considered alive only if both its master replica and its owner connection are. There is nothing else about exclusive queues that should make them client-local and I'm sure there are environments that depend on the current master locator behavior.

[michaelklishin]

@luos we agreed to go with the option you've suggested. Feel free to resubmit the PR against master, we will cherry-pick to v3.7.x on our end. Thank you!

[kjnilsson]

I agree with the suggestion that exclusive queues should be local only. It makes no sense to me for them to reside on a separate node causing code complexity.

[luos]

Hi,

So I did some testing and seems to me that my tests pass on master without my changes, this is because on master this is the code:

https://github.com/rabbitmq/rabbitmq-server/blob/9a371273abf8877b8cde4b6df5a2746fa035349c/src/rabbit_amqqueue.erl#L1618-L1622

and on 3.7.x it's this:

https://github.com/rabbitmq/rabbitmq-server/blob/5316f765907a7db8724a632f02872c6a92e6ede7/src/rabbit_amqqueue.erl#L1114-L1117

Notice, this one test only the Owner, while the one on master only tests the QueuePid.

I will have to do more testing to figure out if the change on master is sufficient or Owner still needs to be tested.

I can just add the same tests on master for Owner if you think that's OK.

[michaelklishin]

@luos adding integration tests based on real world observations is never a bad idea ;) thank you!

[luos]

Okay, thanks, I added the test. As I said it does not pass on branch 3.7.x, is there any chance on back-porting the changes so it can go into that line?

[michaelklishin]

Given the difference in behavior we can make an exception and accept a PR against v3.7.x. Reopened, will QA later today.