Peer discovery: add an option to opt out of registration #13194
Conversation
michaelklishin
changed the title
|
@frederikbosch thank you for taking the time to contribute! At first glance, this is a reasonable PR. We will discuss this with the team. Specifically @mkuratczyk and @dumbbell have been working on peer discovery-related things. In my opinion, this is a good option to have. Can you please, however, document a justification for this feature to someone who would not be aware of #11233? We will need it both for release notes and the doc guides. |
|
@michaelklishin I have changed the text under Proposed Changes. It now includes justification for this feature and references earlier exchanges I had with @dumbbell on this topic. Also, I added a PR for documentation. |
| %% Register node during cluster formation when backend supports registration. | ||
| %% | ||
|
|
||
| {mapping, "cluster_formation.registration", "rabbit.cluster_formation.register", [ |
There was a problem hiding this comment.
The key used in rabbit_peer_discovery is registration, not register. I will correct this on your behalf.
There was a problem hiding this comment.
This PR as it is does not compile:
rabbit_peer_discovery.erl:1000:16: syntax error before: '.'
rabbit_peer_discovery.erl:1002:14: ?FUNCTION_NAME can only be used within a function
rabbit_peer_discovery.erl:1025:16: syntax error before: '.'
rabbit_peer_discovery.erl:1027:14: ?FUNCTION_NAME can only be used within a function
rabbit_peer_discovery.erl:19:2: function maybe_register/0 undefined
rabbit_peer_discovery.erl:19:2: function maybe_unregister/0 undefined
rabbit_peer_discovery.erl:981:2: spec for undefined function maybe_register/0
rabbit_peer_discovery.erl:1008:2: spec for undefined function maybe_unregister/0
rabbit_peer_discovery.erl:90:1: function registration/0 is unused
rabbit_peer_discovery.erl:1050:1: function register/1 is unused
rabbit_peer_discovery.erl:1071:1: function unregister/1 is unused
I will finish it and add a rabbitmq.conf schema test.
| %% Register node during cluster formation when backend supports registration. | ||
| %% | ||
|
|
||
| {mapping, "cluster_formation.registration", "rabbit.cluster_formation.register", [ |
There was a problem hiding this comment.
I would prefer a name that implicitly tells the user it wants a boolean. For instance register (like application environment name you picked) or perform_registration because "register" is both a verb and a noun.
| @@ -82,6 +85,16 @@ node_type() -> | |||
| ?DEFAULT_NODE_TYPE | |||
| end. | |||
|
|
|||
| -spec registration() -> true | false. | |||
There was a problem hiding this comment.
You can use boolean() instead of true | false. That’s the builtin type spec for this.
| case application:get_env(rabbit, cluster_formation) of | ||
| {ok, Proplist} -> | ||
| proplists:get_value(registration, Proplist, ?DEFAULT_REGISTRATION); |
There was a problem hiding this comment.
If the value is set to something else that a list, this code will crash.
You can do something like:
case application:get_env(rabbit, cluster_formation) of
{ok, Proplist} when is_list(Proplist) ->
proplists:get_value(registration, Proplist, ?DEFAULT_REGISTRATION);
{ok, Other} ->
?LOG_ERROR(
"Peer discovery: ignoring invalid value for `register`: ~0p",
[Other],
#{domain => ?RMQLOG_DOMAIN_PEER_DISC}),
?DEFAULT_REGISTRATION;There was a problem hiding this comment.
Also, you use the name registration but the schema uses rabbit.cluster_formation.register. I think there is an inconsistency here.
| ok; | ||
| false -> | ||
| ?LOG_DEBUG( | ||
| "Peer discovery: registration unsupported, skipping register", |
There was a problem hiding this comment.
You may need to split the string to fit in the 80 columns:
"Peer discovery: registration unsupported, skipping "
"register",| "Peer discovery: registration unsupported, skipping register", | ||
| #{domain => ?RMQLOG_DOMAIN_PEER_DISC}), | ||
| ok | ||
| end. |
There was a problem hiding this comment.
This is syntactically incorrect Erlang.
| "Peer discovery: registration unsupported, skipping unregister", | ||
| #{domain => ?RMQLOG_DOMAIN_PEER_DISC}), | ||
| ok | ||
| end. |
There was a problem hiding this comment.
Same as above, this is syntactically incorrect Erlang.
|
You can also squash everything in a single commit and copy the reasoning you added to this pull request to the commit message. This is the only thing that’s guarantied to stay wherever this repository lives, unlike the pull request. |
|
@dumbbell will do, I am finishing this PR. |
|
@frederikbosch: One more thing, could you please add a testcase? |
|
@frederikbosch I have corrected the compilation issues, added a The result is #13201. I hope you don't mind us finishing the PR for you. You will be given full credit in release notes, I just wanted to speed up the process because we are working towards a round of (preview) releases for |
|
@michaelklishin Not at all, please go ahead. No need for full credit, but a mention is nice. I am very happy with the feature as it will probably make the setup in my cluster easier. Thanks for your efforts. |
|
@frederikbosch we cannot possibly not give you credit since you have come up with the idea and wrote at least some of the code :) Let's continue in #13201. |
Proposed Changes
When forming a cluster, registration of the node joining the cluster might be left to (container) orchestration tools like Nomad or Kubernetes. This PR add the config option
cluster_formation.registrationwhich defaults totrue. When set tofalsenode registration at the backend is disabled.When using container orchestration - we are using Nomad, it is the orchestrator that deploys containers with all requirements like network, storage, and also the registration in the service registry (Consul) with a per-service health check. There is at least one important advantage using this over the application doing the registration. When the application is not stopped gracefully for whatever reason, e.g. its OOM killed, it cannot deregister the service. This leaves behind an unlinked service entry in the registry. This caused the bug in #11233, also see my remarks in PR #11045 and the exchange I had with @dumbbell in this discussion. When leaving registration and deregistration to the orchestrator, you cannot run in such problems.
Types of Changes
Checklist
CONTRIBUTING.mddocumentFurther Comments
When trying to solve #11233 I was looking how I should disable node registration for Consul peer discovery. I found out that the
maybe_registerfunction insiderabbit_peer_discoverywas the best option. This does however makes the option to disable registration a global peer discovery option rather specifically for Consul. Before continuing I'd like to know how this direction is perceived.Please take into account, this is my first code in Erlang. I have additional work to do to make this a complete PR.