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

[UNTESTED] Fix remote node maintenance (bsc#983617) #187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aspiers
Copy link
Member

@aspiers aspiers commented Mar 20, 2017

We need crm and crm_attribute installed on the remote nodes, and we also need to specify the pacemaker node name when toggling the mode, as explained in https://bugzilla.suse.com/show_bug.cgi?id=983617#c14

Adam Spiers added 2 commits March 20, 2017 16:03
We want to be able to reuse this when constructing the command for
toggling maintenance mode, since when doing it for remotes, you need
to specify the node name.
We need crm and crm_attribute installed on the remote nodes, and we
also need to specify the pacemaker node name when toggling the mode,
as explained in

  https://bugzilla.suse.com/show_bug.cgi?id=983617#c14
@@ -18,7 +18,7 @@
default[:pacemaker][:platform][:packages] =
%w(pacemaker crmsh fence-agents)
default[:pacemaker][:platform][:remote_packages] =
%w(pacemaker-remote fence-agents)
%w(pacemaker-remote pacemaker-cli crmsh fence-agents)

Choose a reason for hiding this comment

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

Style/WordArray: Use [] for an array of words. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylewordarray)

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree with the dog here.

# See https://bugzilla.suse.com/show_bug.cgi?id=870696
!! (`crm_attribute -G -N #{pacemaker_node} -n maintenance -d off -q` =~ /^on$/)
!! (`crm_attribute -G -N #{pacemaker_node_name} -n maintenance -d off -q` =~ /^on$/)

Choose a reason for hiding this comment

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

Style/DoubleNegation: Avoid the use of double negation (!!). (https://github.com/bbatsov/ruby-style-guide#no-bang-bang)
Style/SpaceAfterNot: Do not leave space between ! and its argument. (https://github.com/bbatsov/ruby-style-guide#no-space-bang)

Copy link
Member Author

Choose a reason for hiding this comment

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

and here.

@Itxaka
Copy link
Member

Itxaka commented Mar 21, 2017

Reading the bug comments and the code, this makes sense to me. Unfortunately Im working on the upgrades so Im not sure I can set up an env to test this shortly.

@aspiers How do you get an env running to test this? Just drop the compute nodes into the pacemaker-remote role?

@Itxaka
Copy link
Member

Itxaka commented Mar 21, 2017

I got an env with this so Ill test it

@Itxaka
Copy link
Member

Itxaka commented Apr 11, 2017

@Itxaka
Copy link
Member

Itxaka commented Apr 12, 2017

Tested and it seems to work ok, retriggered the build

def maintenance_mode?
pacemaker_node = if !node[:pacemaker].nil? && node[:pacemaker][:is_remote]
def pacemaker_node_name
if !node[:pacemaker].nil? && node[:pacemaker][:is_remote]
Copy link
Member

Choose a reason for hiding this comment

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

What about just if node[:pacemaker] && node[:pacemaker][:is_remote] ?

Copy link
Member

@jsuchome jsuchome left a comment

Choose a reason for hiding this comment

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

Not tested, but makes sense to me

Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

I tested this and it seems to at least not break anything. I'm having a hard time testing that it does what it's intended to do because I think we don't currently put remote notes in maintenance mode and instead use no_crm_maintenance_mode to avoid it. But it looks good!

@aspiers
Copy link
Member Author

aspiers commented Apr 29, 2017

Thanks @cmurphy :-)

@jsuchome
Copy link
Member

@aspiers why is this labeled UNTESTED? I think several people already tested it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants