closes #432 #433
closes #432 #433
Conversation
9ae91c7 to
14faa34
Compare
libraries/helpers.rb
Outdated
| module Helpers | ||
| require 'mixlib/shellout' | ||
|
|
||
| def remove_self_from_cluster |
There was a problem hiding this comment.
This name is not particularly correct. You are resetting the node here, which does not notify the rest of the cluster that the node is gone (and can be used in more scenarios).
There was a problem hiding this comment.
Interesting, according to the documentation I see:
I don't see anything that suggests that it does not communicate these changes back to to cluster, is there something I am missing or do we need to enhance the documentation?
There was a problem hiding this comment.
@michaelklishin this seems to support the documentation: https://gist.github.com/majormoses/da89381260a814046e4003d677c110e1
There was a problem hiding this comment.
@majormoses yes, peers won't list a node that was reset as an online peer. Are you sure that they also won't list it as an offline one? I see the same assumption made over and over, that if a node is not online it must not be a cluster member any more. That's not how things work, as explained in #432 (comment).
There was a problem hiding this comment.
A quick experiment suggests that resetting a node makes it leave the cluster permanently. I'd still rename the function.
There was a problem hiding this comment.
I prefer none of those. Sorry but isn't it obvious that the cluster here means an "rmq" cluster? This cookbook doesn't provision any other clusters AFAIK.
There was a problem hiding this comment.
Any wrapper cookbooks probably won't manage other services either.
There was a problem hiding this comment.
A wrapper cookbook would never manage rabbitmq and other services? I find this hard to believe as there are cookbooks like sensu that manage RMQ, redis, etc.
Would it help if we made it more like remove_self_from_rabbitmq_current_cluster ?
There was a problem hiding this comment.
Sensu and Logstash are pretty much the only ones that I can think of. Anyhow, this is not C and namespacing of functions is a solved problem. You can always invoke a function with a module name specified in case there are potential conflicts or ambiguity.
There was a problem hiding this comment.
I'd accept reset_current_node or reset_this_node. rmq or rabbitmq are absolutely unnecessary and self is less specific than current_node.
|
@jjasghar looks like those failures are nothing to do with this PR |
14faa34 to
e98aa4b
Compare
|
@michaelklishin renamed function. |
|
@jjasghar OK with you to merge? |
|
@majormoses can you rebase off master? We have a ton more testing now thanks to @rmoriz and would love to see how it shakes out. |
e98aa4b to
b9ecff2
Compare
This adds some helper functions to allow a wrapper cookbook to implement removing a node from a cluster in 2 scenarios: - removing self from cluster (helpful for normal decommission) - removing any node from cluster (helpful for abruptly terminated machines) These both should not be actually consumed in this cookbook and simply be provided for wrappers to use as they see fit. This should be implemented only after careful thought, design, testing, etc.
b9ecff2 to
4556fa9
Compare
|
@jjasghar rebased |
|
Awesome! |
This adds some helper functions to allow a wrapper cookbook to implement removing a node from a cluster in 2 scenarios:
These both should not be actually consumed in this cookbook and simply be provided for wrappers to use as they see fit. This should be implemented only after careful thought, design, testing, etc.