corosync: Implement double ring #5
Conversation
For increased stability, people want to have multiple corosync rings, so if one network fails, node still won't be fenced. This change adds the `ring_mode` and `second_ring_network` attributes to the proposal. `ring_mode` can be either `single_ring` or `dual_ring`: - `single_ring`: only one ring used by corosync, and it is using the admin network - `dual_ring`: two rings used, one is on the admin network, the other network can be specified by the user by providing a value for `second_ring_network` Notes: - double ring configuration only implemented for UDPU, if you use a different mode, the propsal will fail to validate - after moving to double ring configuration, user has to manually re-start the corosync cluster. He can do it by running the file: /opt/dell/chef/cookbooks/pacemaker/files/default/reload_corosync_configuration.sh
houndci-bot
left a comment
houndci-bot
left a comment
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
.rubocop.yml: Style/AlignHash has the wrong namespace - should be Layout
.rubocop.yml: Style/AlignHash has the wrong namespace - should be Layout .rubocop.yml: Style/AlignParameters has the wrong namespace - should be Layout .rubocop.yml: Style/EmptyLinesAroundBlockBody has the wrong namespace - should be Layout .rubocop.yml: Style/MultilineOperationIndentation has the wrong namespace - should be Layout Error: obsolete parameter RunRailsCops (for AllCops) found in .rubocop.yml Use the following configuration instead: Rails: Enabled: true obsolete parameter AlignWith (for Lint/EndAlignment) found in .rubocop.yml `AlignWith` has been renamed to `EnforcedStyleAlignWith`
|
|
||
| = select_field %w(corosync ring_mode), :collection => :ring_modes_for_corosync, "data-showit" => "dual_ring", "data-showit-target" => "#dual_ring_container", "data-showit-direct" => "true" | ||
|
|
||
| %div{ :id => "dual_ring_container" } |
There was a problem hiding this comment.
The hound is probably right here too: dual-ring-container.
| .panel-body | ||
| = select_field %w(corosync transport), :collection => :transport_for_pacemaker | ||
|
|
||
| = select_field %w(corosync ring_mode), :collection => :ring_modes_for_corosync, "data-showit" => "dual_ring", "data-showit-target" => "#dual_ring_container", "data-showit-direct" => "true" |
aspiers
left a comment
aspiers
left a comment
There was a problem hiding this comment.
Mostly looks great! Just a few nits and some hardening to the upgrade script.
| default[:corosync][:transport] = "udp" | ||
| default[:corosync][:ring_mode] = "single_ring" | ||
|
|
||
| default[:corosync][:second_ring_used] = false |
There was a problem hiding this comment.
Why do we need both :ring_mode and :second_ring_used? Is there a circumstance in which :ring_mode would be "double_ring" and :second_ring_used would be false?
| # dual corosync ring. | ||
| # After you applied the modified proposal successfully, run this script on | ||
| # crowbar | ||
| # Please stop all chef-client runs while running this script. |
There was a problem hiding this comment.
Can't this be automated, or at least checked before proceeding?
| # Please stop all chef-client runs while running this script. | ||
| set -eux | ||
|
|
||
| CLUSTER_PROPOSAL_NAME="$1" |
There was a problem hiding this comment.
Please do some input validation on this.
| # if this command fails, the script exits | ||
| crowbarctl proposal show pacemaker "$CLUSTER_PROPOSAL_NAME" | ||
|
|
||
| NODES=$(crowbarctl \ |
There was a problem hiding this comment.
Tiny nit (take it or leave it): stylistically, with multi-line $( ... ) I prefer a line-break after the $( so that the arguments to crowbarctl are indented more than crowbarctl, not less, which also means that the commands in the pipeline are vertically aligned, and the $( ) clearly delimit the whole pipeline:
NODES=$(
crowbarctl proposal show pacemaker "$CLUSTER_PROPOSAL_NAME" \
--format=plain \
--filter "deployment.pacemaker.elements.pacemaker-cluster-member" |
cut -d " " -f 2
)
| done | ||
|
|
||
| # Print out initial configuration | ||
| ssh "$FIRST_NODE" corosync-cfgtool -s |
| role.default_attributes["corosync"]["members"] = member_nodes.map{ |n| n.get_network_by_type("admin")["address"] } | ||
| role.default_attributes["corosync"]["transport"] = role.default_attributes["pacemaker"]["corosync"]["transport"] | ||
|
|
||
| # Set up the second ring if dual_ring mode configured |
There was a problem hiding this comment.
Please can you add this large chunk of code as a separate method? Long methods really harm readability and maintainability.
| role.default_attributes["corosync"]["second_ring_bind_addr"] = second_ring_net["network"]["subnet"] | ||
| role.default_attributes["corosync"]["second_ring_mcast_addr"] = role.default_attributes["pacemaker"]["corosync"]["mcast_addr"] | ||
| role.default_attributes["corosync"]["second_ring_mcast_port"] = role.default_attributes["pacemaker"]["corosync"]["mcast_port"] | ||
| role.default_attributes["corosync"]["second_ring_members"] = second_ring_members |
There was a problem hiding this comment.
Is it just me or do you now have both [:corosync][:second_ring_used] and [:pacemaker][:corosync][:second_ring_used]? And I don't see why either are needed - they seem to be duplicating the purpose of ring_mode.
| .panel-body | ||
| = select_field %w(corosync transport), :collection => :transport_for_pacemaker | ||
|
|
||
| = select_field %w(corosync ring_mode), :collection => :ring_modes_for_corosync, "data-showit" => "dual_ring", "data-showit-target" => "#dual_ring_container", "data-showit-direct" => "true" |
|
|
||
| = select_field %w(corosync ring_mode), :collection => :ring_modes_for_corosync, "data-showit" => "dual_ring", "data-showit-target" => "#dual_ring_container", "data-showit-direct" => "true" | ||
|
|
||
| %div{ :id => "dual_ring_container" } |
There was a problem hiding this comment.
The hound is probably right here too: dual-ring-container.
| hae_repo: 'The HAE repositories have not been setup.' | ||
| transport_value: 'Invalid transport value: %{transport}.' | ||
| ring_mode_value: 'Invalid ring mode: %{ring_mode}.' | ||
| second_ring_network_value: 'Network not found for setting up second corosync ring: %{second_ring_network}.' |
There was a problem hiding this comment.
Nit: I prefer Network "%{second_ring_network}" not found for setting up second corosync ring.' - I think it's very slightly clearer.
3 participants
Implement having multiple corosync rings. See commit message for more details