From b3b7bf416724f424a280c63f94121fb47e7397e6 Mon Sep 17 00:00:00 2001 From: Tomas Jelinek Date: Mon, 11 Dec 2017 08:54:26 +0100 Subject: [PATCH] warn when a stonith device has method=cycle set --- pcs/status.py | 21 ++++++++++++++-- pcs/test/test_status.py | 47 ++++++++++++++++++++++++----------- pcsd/cluster_entity.rb | 22 +++++++++++++++++ pcsd/test/cib1.xml | 2 ++ pcsd/test/test_cluster_entity.rb | 53 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 129 insertions(+), 16 deletions(-) diff --git a/pcs/status.py b/pcs/status.py index ec10d61..8a517be 100644 --- a/pcs/status.py +++ b/pcs/status.py @@ -125,6 +125,7 @@ def status_stonith_check(): stonith_enabled = True stonith_devices = [] stonith_devices_id_action = [] + stonith_devices_id_method_cycle = [] sbd_running = False cib = utils.get_cib_dom() @@ -155,6 +156,14 @@ def status_stonith_check(): stonith_devices_id_action.append( resource.getAttribute("id") ) + if ( + nvpair.getAttribute("name") == "method" + and + nvpair.getAttribute("value") == "cycle" + ): + stonith_devices_id_method_cycle.append( + resource.getAttribute("id") + ) if not utils.usefile: # check if SBD daemon is running @@ -171,14 +180,22 @@ def status_stonith_check(): if stonith_devices_id_action: print( - "WARNING: following stonith devices have the 'action' attribute" - " set, it is recommended to set {0} instead: {1}".format( + "WARNING: following stonith devices have the 'action' option set, " + "it is recommended to set {0} instead: {1}".format( ", ".join( ["'{0}'".format(x) for x in _STONITH_ACTION_REPLACED_BY] ), ", ".join(sorted(stonith_devices_id_action)) ) ) + if stonith_devices_id_method_cycle: + print( + "WARNING: following stonith devices have the 'method' option set " + "to 'cycle' which is potentially dangerous, please consider using " + "'onoff': {0}".format( + ", ".join(sorted(stonith_devices_id_method_cycle)) + ) + ) # Parse crm_mon for status def nodes_status(argv): diff --git a/pcs/test/test_status.py b/pcs/test/test_status.py index b412b91..1a4fb70 100644 --- a/pcs/test/test_status.py +++ b/pcs/test/test_status.py @@ -21,45 +21,64 @@ class StonithWarningTest(TestCase, AssertPcsMixin): shutil.copy(self.empty_cib, self.temp_cib) self.pcs_runner = PcsRunner(self.temp_cib) - def fixture_stonith(self, action=False): + def fixture_stonith_action(self): self.assert_pcs_success( - "stonith create S fence_apc ipaddr=i login=l {0} --force".format( - "action=reboot" if action else "" - ), + "stonith create Sa fence_apc ipaddr=i login=l action=reboot --force", "Warning: stonith option 'action' is deprecated and should not be" " used, use pcmk_off_action, pcmk_reboot_action instead\n" - if action - else "" + ) + + def fixture_stonith_cycle(self): + self.assert_pcs_success( + "stonith create Sc fence_ipmilan method=cycle" ) def fixture_resource(self): self.assert_pcs_success( - "resource create dummy ocf:pacemaker:Dummy action=reboot --force", - "Warning: invalid resource option 'action', allowed options are: " - "envfile, fail_start_on, fake, op_sleep, passwd, state," - " trace_file, trace_ra\n" + "resource create dummy ocf:pacemaker:Dummy action=reboot " + "method=cycle --force" + , + "Warning: invalid resource options: 'action', 'method', allowed " + "options are: envfile, fail_start_on, fake, op_sleep, passwd, " + "state, trace_file, trace_ra\n" ) def test_warning_stonith_action(self): - self.fixture_stonith(action=True) + self.fixture_stonith_action() + self.fixture_resource() self.assert_pcs_success( "status", stdout_start=dedent("""\ Cluster name: test99 - WARNING: following stonith devices have the 'action' attribute set, it is recommended to set 'pcmk_off_action', 'pcmk_reboot_action' instead: S + WARNING: following stonith devices have the 'action' option set, it is recommended to set 'pcmk_off_action', 'pcmk_reboot_action' instead: Sa Stack: unknown Current DC: NONE """) ) - def test_action_ignored_for_non_stonith_resources(self): - self.fixture_stonith(action=False) + def test_warning_stonith_method_cycle(self): + self.fixture_stonith_cycle() self.fixture_resource() + self.assert_pcs_success( + "status", + stdout_start=dedent("""\ + Cluster name: test99 + WARNING: following stonith devices have the 'method' option set to 'cycle' which is potentially dangerous, please consider using 'onoff': Sc + Stack: unknown + Current DC: NONE + """) + ) + def test_stonith_warnings(self): + self.fixture_stonith_action() + self.fixture_stonith_cycle() + self.fixture_resource() self.assert_pcs_success( "status", stdout_start=dedent("""\ Cluster name: test99 + WARNING: following stonith devices have the 'action' option set, it is recommended to set 'pcmk_off_action', 'pcmk_reboot_action' instead: Sa + WARNING: following stonith devices have the 'method' option set to 'cycle' which is potentially dangerous, please consider using 'onoff': Sc Stack: unknown Current DC: NONE """) diff --git a/pcsd/cluster_entity.rb b/pcsd/cluster_entity.rb index 21092c5..3675719 100644 --- a/pcsd/cluster_entity.rb +++ b/pcsd/cluster_entity.rb @@ -516,6 +516,28 @@ module ClusterEntity @utilization << ClusterEntity::NvPair.from_dom(e) } @stonith = @_class == 'stonith' + if @stonith + @instance_attr.each{ |attr| + if attr.name == 'action' + @warning_list << { + :message => ( + 'This fence-device has the "action" option set, it is ' + + 'recommended to set "pcmk_off_action", "pcmk_reboot_action" ' + + 'instead' + ) + } + end + if attr.name == 'method' and attr.value == 'cycle' + @warning_list << { + :message => ( + 'This fence-device has the "method" option set to "cycle" ' + + 'which is potentially dangerous, please consider using ' + + '"onoff"' + ) + } + end + } + end if @id and rsc_status @crm_status = rsc_status[@id] || [] end diff --git a/pcsd/test/cib1.xml b/pcsd/test/cib1.xml index f603f24..03749ab 100644 --- a/pcsd/test/cib1.xml +++ b/pcsd/test/cib1.xml @@ -28,6 +28,8 @@ + + diff --git a/pcsd/test/test_cluster_entity.rb b/pcsd/test/test_cluster_entity.rb index 2b67e19..60719ef 100644 --- a/pcsd/test/test_cluster_entity.rb +++ b/pcsd/test/test_cluster_entity.rb @@ -719,6 +719,59 @@ class TestPrimitive < Test::Unit::TestCase assert(obj.operations.empty?) end + def test_init_stonith_with_warnings + obj = ClusterEntity::Primitive.new( + @cib.elements["//primitive[@id='node2-stonith']"] + ) + assert_nil(obj.parent) + assert_nil(obj.get_master) + assert_nil(obj.get_clone) + assert_nil(obj.get_group) + assert(obj.meta_attr.empty?) + assert_equal('node2-stonith', obj.id) + assert(obj.error_list.empty?) + assert_equal( + obj.warning_list, + [ + { + :message => ( + 'This fence-device has the "action" option set, it is ' + + 'recommended to set "pcmk_off_action", "pcmk_reboot_action" instead' + ) + }, + { + :message => ( + 'This fence-device has the "method" option set to "cycle" which ' + + 'is potentially dangerous, please consider using "onoff"' + ) + } + ] + ) + assert_equal('stonith:fence_xvm', obj.agentname) + assert_equal('stonith', obj._class) + assert_nil(obj.provider) + assert_equal('fence_xvm', obj.type) + assert(obj.stonith) + instance_attr = ClusterEntity::NvSet.new << ClusterEntity::NvPair.new( + 'node2-stonith-instance_attributes-domain', + 'domain', + 'node2' + ) + instance_attr << ClusterEntity::NvPair.new( + 'node2-stonith-instance_attributes-action', + 'action', + 'monitor' + ) + instance_attr << ClusterEntity::NvPair.new( + 'node2-stonith-instance_attributes-method', + 'method', + 'cycle' + ) + assert_equal_NvSet(instance_attr, obj.instance_attr) + assert(obj.crm_status.empty?) + assert(obj.operations.empty?) + end + def test_init_stonith_with_crm obj = ClusterEntity::Primitive.new( @cib.elements["//primitive[@id='node1-stonith']"], -- 1.8.3.1