remove invalid cluster_node_type 'disk'

[danoe]

Pull Request (PR) description

Hi,

I would like to propose this little change, as to what I was able to find out is not a valid option, and i'm not sure if it ever was.

The disk option was added in module version 5.0.0 (2014-12-22)
See commit: 6c67f33
and Puppetlabs ticket: https://tickets.puppetlabs.com/browse/MODULES-1186

The part on the clustering page referenced there is probably this:
https://www.rabbitmq.com/clustering.html#cluster-node-types

A node can be a disk node or a RAM node. (Note: disk and disc are used interchangeably).
RAM nodes store internal database tables in RAM only. This does not include messages, 
message store indices, queue indices and other node state.

Though it doesn't mention that the parameters can be used interchangeably. And from what i've seen in the rest of the rabbitmq documentation and source code, i don't think this was ever a valid option.

Results of my research

• rabbitmqctl man page says valid options are disc and ram
  • https://www.rabbitmq.com/rabbitmqctl.8.html#change_cluster_node_type
• No change in the last 4 years there.
  • https://github.com/rabbitmq/rabbitmq-server/blame/master/docs/rabbitmqctl.8#L289
• erlang spec saying only valid options are disc and ram
  • https://github.com/rabbitmq/rabbitmq-server/blob/master/src/rabbit_mnesia.erl#L1077
  • No change in the last 6 years there.

Reason I found this out was that i just spent 2 days debugging why the cluster wouldn't form until i realized that i had set the cluster_node_type to disk. Though the first node starts correctly, probably because the node type is ignored as in any cluster must exist at least one disc node. So the first node was running fine, but none of the other nodes were able to join.

This Pull Request (PR) fixes the following issues

cluster_node_type 'disk' is not a valid option.

[wyardley]

Thanks for the contribution, @danoe!

This seems sensible to me, and thanks for including all the research / details. I agree with your interpretation.

I want to doublecheck with @bastelfreak / @alexjfisher whether we need to flag this as backwards-incompatible (I'd argue maybe not, since this was always broken, and probably generates an invalid config).

[ekohl]

Thanks @danoe for the excellent PR. This should be an example because it makes the life of a reviewer very easy.

@wyardley I think this does technically qualifies as a bugfix since it was never valid. While it's an API change, I get the impression that a server would never start up with a value disk so this just prevents misconfigurations. That's why I'd be fine with including it in a .z.

[igalic]

This piece of code,

https://github.com/rabbitmq/rabbitmq-server/blob/21231bb4da7d7ca8ac0e5950aca251273d1476ac/src/rabbit_mnesia.erl#L147

              when is_list(Nodes) andalso (Type == disc orelse Type == disk orelse Type == ram) ->

is the only reference to disk, everything else references disc.
however, if the spec says it only accepts disc, is that actually enforced at run-time? or only at compile-time?

[alexjfisher]

This piece of code,

https://github.com/rabbitmq/rabbitmq-server/blob/21231bb4da7d7ca8ac0e5950aca251273d1476ac/src/rabbit_mnesia.erl#L147

              when is_list(Nodes) andalso (Type == disc orelse Type == disk orelse Type == ram) ->

is the only reference to disk, everything else references disc.
however, if the spec says it only accepts disc, is that actually enforced at run-time? or only at compile-time?

I'm also not sure that rabbitmq/rabbitmq-server@23d8485 ever worked.
https://github.com/rabbitmq/rabbitmq-server/blob/bdb6f9b508dd1aaad5e618d9a620adb7972e618f/src/rabbit_mnesia.erl#L152 (which predates the above commit) suggests not??

[alexjfisher]

I'm happy that this probably never worked, so doesn't need to be marked as backwards incompatible.

[danoe]

Thanks guys, appreciate the feedback and quick merge. 👍