Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#125] - Enhancing clustering functionality #238

Merged
merged 4 commits into from Apr 24, 2015

Conversation

Projects
None yet
4 participants
@sunggun-yu
Copy link
Contributor

commented Mar 8, 2015

Transfering rabbitmq-cluster cookbook into rabbitmq cookbook

  • Changing attribute prefix : [:rabbitmq-cluster] to [:rabbitmq][:clustering]
  • Enhancement on regex : user ruby matcher
  • Removing unnecessary resource : node_type on change_cluster_node_type action
  • Some other enhancement on provider

Enhancing from rabbitmq-cluster as the manual clustering

  • Removing master/slave concept
  • Removing cluster node type
  • Introducing new attribute that defines all the cluster nodes and its cluster node type : [:rabbitmq][:clustering][:cluster_nodes]
  • Adding set_cluster_name action

Applying new features and attributes into auto clustring

  • Using default[:rabbitmq][:clustering][:cluster_nodes] attribute instead of ['rabbitmq']['cluster_disk_nodes']
  • Applying set_cluster_name and change_cluster_node_type for auto clustering
  • Long story short, All the nodes will be set as disc node in the rabbitmq.config even though you have some ram nodes. however, change_cluster_node_type action will fix the node type as you expected.

Additional changes on the others

  • Fixing typo in default receipe for stopping service to set erlang cookie

Attributes example

default[:rabbitmq][:clustering][:use_auto_clustering] = false
default[:rabbitmq][:clustering][:cluster_name] = 'seoul_tokyo_newyork'
default[:rabbitmq][:clustering][:cluster_nodes] = [
    {
        :name => 'rabbit@rabbit1',
        :type => 'disc'
    },
    {
        :name => 'rabbit@rabbit2',
        :type => 'ram'
    },
    {
        :name => 'rabbit@rabbit3',
        :type => 'disc'
    }
]
Enhancing clustering functionality
Transfering rabbitmq-cluster cookbook into rabbitmq cookbook
* Changing attribute prefix : [:rabbitmq-cluster] to [:rabbitmq][:clustering]
* Enhancement on regex : user ruby matcher
* Removing unnecessary resource : node_type on change_cluster_node_type action
* Some other enhancement on provider

Enhancing from rabbitmq-cluster as the manual clustering
* Removing master/slave concept
* Removing cluster node type
* Introducing new attribute that defines all the cluster nodes and its cluster node type : `[:rabbitmq][:clustering][:cluster_nodes]`
* Adding set_cluster_name action

Applying new features and attributes into auto clustring
* Using `default[:rabbitmq][:clustering][:cluster_nodes]` attribute instead of `['rabbitmq']['cluster_disk_nodes']`
* Applying set_cluster_name and change_cluster_node_type for auto clustering
* Long story short, All the nodes will be set as `disc` node in the rabbitmq.config even though you have some ram nodes. however, change_cluster_node_type action will fix the node type as you expected.

Additional changes on the others
* Fixing typo in default receipe for stopping service to set erlang cookie

Attributes example
```
default[:rabbitmq][:clustering][:use_auto_clustering] = false
default[:rabbitmq][:clustering][:cluster_name] = 'seoul_tokyo_newyork'
default[:rabbitmq][:clustering][:cluster_nodes] = [
    {
        :name => 'rabbit@rabbit1',
        :type => 'disc'
    },
    {
        :name => 'rabbit@rabbit2',
        :type => 'ram'
    },
    {
        :name => 'rabbit@rabbit3',
        :type => 'disc'
    }
]
```

Redesign of manual clustering
- remove master/slave
- remove cluster node type for each node
- introducing new attribute that defines all the cluster nodes and its cluster node type
- adding set_cluster_name feature
- preparing of auto clustering in cluster recipe

Adding set cluster name action for manual clustering.

Apply [:rabbitmq][:clustering][:cluster_nodes] attribute on rabbitmq.config

fix typo

Apply set_cluster_name and change_cluster_node_type for auto clustering
@cmluciano

This comment has been minimized.

Copy link
Collaborator

commented Mar 8, 2015

Thank you for taking the time to work on this. I am taking a look through it now.

Can you fix the style issues that travis is complaining about?

Also I think we might want to a section to the README describing how to use these changes.

@sunggun-yu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2015

@cmluciano
Sure, I will.

sunggun-yu added some commits Mar 9, 2015

Fixing the style issues for Travis. Adding documentation on README
- Fix some code to handle style issue.
@sunggun-yu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2015

@cmluciano I fixed style issue and rubocop issue. and Travis has passed. please review it.
Thank you.

# execute > rabbitmqctl cluster_status | sed "1d" | tr "\n" " " | tr -d " "
# rabbitmqctl cluster_status returns "Cluster status of node rabbit@rabbit1 ..." at the first line.
# To parse the result string, it is removed by sed "1d"
cmd = 'rabbitmqctl cluster_status | sed "1d" | tr "\n" " " | tr -d " "'

This comment has been minimized.

Copy link
@cmluciano

cmluciano Mar 10, 2015

Collaborator

I'm a little confused by the need to pipe to sed, tr. etc. Is this to try to normalize the output?

This comment has been minimized.

Copy link
@jjasghar

jjasghar Mar 10, 2015

Collaborator

Yeah i have to parrot @cmluciano here, this cmd scares me. Is there a way we could leverage some ruby or something other then sed tr tr?

This comment has been minimized.

Copy link
@sunggun-yu

sunggun-yu Mar 10, 2015

Author Contributor

Yeah, that is for normalize the output. because, rabbitmqctl output, mostly indent and line feed, was different from servers and versions.

This comment has been minimized.

Copy link
@sunggun-yu

sunggun-yu Mar 10, 2015

Author Contributor

I'l try with ruby.

This comment has been minimized.

Copy link
@sunggun-yu

sunggun-yu Mar 10, 2015

Author Contributor

@jjasghar @cmluciano

It would be changed like this.

def cluster_status
  # execute > rabbitmqctl cluster_status"
  # To parse the result string, this function normalize the output string
  # - Removing first line
  # - Removing "... Done" : old version returns this
  cmd = 'rabbitmqctl cluster_status'
  Chef::Log.debug("[rabbitmq_cluster] Executing #{cmd}")
  cmd = get_shellout(cmd)
  cmd.run_command
  cmd.error!
  result = cmd.stdout.split(/\n/, 2).last.squeeze(' ').gsub(/\n/, '').gsub('...done.', '')
  Chef::Log.debug("[rabbitmq_cluster] rabbitmqctl cluster_status : #{result}")
  result
end

This comment has been minimized.

Copy link
@jjasghar

jjasghar Mar 10, 2015

Collaborator

👍

err = cmd.stderr
Chef::Log.warn("[rabbitmq_cluster] #{err}")
if err.include?('{ok,already_member}')
Chef::Log.info('[rabbitmq_cluster] Node is already member of cluster, error will be ignored.')

This comment has been minimized.

Copy link
@cmluciano

cmluciano Mar 10, 2015

Collaborator

Can you change this to

[rabbitmq_cluster] Node is already a member of the cluster, error will be ignored.

This comment has been minimized.

Copy link
@sunggun-yu

sunggun-yu Mar 10, 2015

Author Contributor

sure, i'll change. Thank you for the correction ;-)

[#238] Apply the code review result.
- Using ruby code instead of linux command : `sed`, `tr` has removed.
- Fixing english grammar
- Removing `json` tag from README : its not from the code review result
@sunggun-yu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2015

@jjasghar @cmluciano
seems like,,,, Travis has been stopped by connection timed out...
https://travis-ci.org/jjasghar/rabbitmq/builds/53843704

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2015

I restarted it, lets see if it passes.

@sunggun-yu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2015

@jjasghar Looks good!

@jesseadams

This comment has been minimized.

Copy link

commented Apr 2, 2015

👍

# Join in cluster
rabbitmq_cluster cluster_nodes do
action :join
end

This comment has been minimized.

Copy link
@jesseadams

jesseadams Apr 2, 2015

Should this unless block be a not_if {} in the resource call instead?

This comment has been minimized.

Copy link
@sunggun-yu

sunggun-yu Apr 7, 2015

Author Contributor

@jesseadams
node['rabbitmq']['clustering']['use_auto_clustering'] is the flag for using auto clustering or manual clustering. should I use not_if? can you please explain to me how not_if different from unless at this code?

This comment has been minimized.

Copy link
@jesseadams

jesseadams Apr 23, 2015

It is fundamentally the same. I thought that not using not_if was a FoodCritic violation in this case. Maybe not?

@jesseadams

This comment has been minimized.

Copy link

commented Apr 23, 2015

Can we please get this reviewed once more and merged? Thanks!

@jjasghar jjasghar merged commit 4e13403 into rabbitmq:master Apr 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jjasghar jjasghar removed the in progress label Apr 24, 2015

@jjasghar jjasghar referenced this pull request Oct 21, 2015

Closed

RabbitMQ Auto-Clustering #156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.