Skip to content
This repository has been archived by the owner on Jan 20, 2025. It is now read-only.

Consul: allow clustering with nodes in warning status #10

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

ValFadeev
Copy link

moved from aweber#140

Consul agent may be configured with additional health checks, besides the TTL one created by the plugin. Some of these checks may expect a converged cluster. If any of those additional checks is not passing the corresponding nodes will be excluded from the list for clustering which is not always the desired situation.

This change aims to provide flexibility to define additional health checks while avoiding interference with auto clustering. This is consistent with Consul's only-passing option which determines whether nodes with warning status are excluded from the DNS response.

@michaelklishin
Copy link
Member

@ValFadeev thank you!

@@ -28,6 +28,7 @@
{config, cluster_name, "CLUSTER_NAME", "undefined", string, false}, %% Consul && etcd

{config, consul_acl_token, "CONSUL_ACL_TOKEN", "undefined", string, false}, %% Consul
{config, consul_allow_warn, "CONSUL_ALLOW_WARN", false, atom, false},
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to find a more descriptive name. CONSUL_TOLERATE_WARN or CONSUL_ALLOW_WARN seem too short to me. It's not clear what are we allowing here.

I'd go with something like CONSUL_INCLUDE_NODES_WITH_WARNINGS. While longer, it is more or less to the point. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

@michaelklishin agree, in hindsight the current name may also cause confusion with CLEANUP_WARN_ONLY option. Happy to go with the suggested name, as can't think of anything shorter to convey the meaning

@michaelklishin michaelklishin requested a review from dcorbacho April 3, 2017 12:03
@dcorbacho dcorbacho changed the base branch from master to stable April 5, 2017 11:26
ValFadeev and others added 3 commits April 5, 2017 12:27
* added filtering by health check status
* added tests
Ensures consul calls include the filter, otherwise it might return
unfiltered warnings
@dcorbacho dcorbacho force-pushed the consul-allow-warning branch from edc0ee4 to 43045ea Compare April 5, 2017 11:29
Copy link

@dcorbacho dcorbacho left a comment

Choose a reason for hiding this comment

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

Thanks @ValFadeev! It looks good, only added an additional test case.

@dcorbacho dcorbacho merged commit a51783a into rabbitmq:stable Apr 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants