openstack
/
charm-hacluster
Code Issues Proposed changes

Fix pacemaker-remote-relation-changed hook error 

This was happening because
trigger_corosync_update_from_leader() was being called
not only in `ha` relation hooks but also in
`pacemaker-remote` relation hooks after the implementation
for the related bug landed.

Closes-Bug: #1920124
Related-Bug: #1400481
Change-Id: I4952ef694589de6b72f04b387e30ca2333bc4cbc
This commit is contained in:
Aurelien Lourot 2021-03-19 10:43:12 +01:00
parent 64e696ae74
commit 06796e6518
2 changed files with 57 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
hooks/hooks.py
View File

@ -45,8 +45,8 @@ from charmhelpers.core.hookenv import (
related_units,
relation_ids,
relation_set,
relation_type,
remote_unit,
principal_unit,
config,
Hooks,
UnregisteredHookError,
@ -326,20 +326,22 @@ def ha_relation_changed():
level=INFO)
return
relid_hanode = relation_ids('hanode')
if relid_hanode:
relids_hanode = relation_ids('hanode')
if relids_hanode:
log('Ready to form cluster - informing peers', level=DEBUG)
relation_set(relation_id=relid_hanode[0], ready=True)
relation_set(relation_id=relids_hanode[0], ready=True)
# If a trigger-corosync-update attribute exists in the relation,
# the Juju leader may have requested all its peers to update
# the corosync.conf list of nodes. If it's the case, no other
# action will be run (a future hook re: ready=True may trigger
# other logic)
if (remote_unit() != principal_unit() and
trigger_corosync_update_from_leader(
remote_unit(), relid_hanode[0]
)):
# NOTE(lourot): it's not necessary to test for
# `remote_unit() != principal_unit()` here as this is only False (both
# units are the same) when the relation type is `ha`.
if (relation_type() == 'hanode' and
trigger_corosync_update_from_leader(remote_unit(),
relids_hanode[0])):
return
else:
@ -590,7 +592,7 @@ def ha_relation_changed():
# Inform peers that local configuration is complete and this member
# is ready
for rel_id in relation_ids('hanode'):
for rel_id in relids_hanode:
relation_set(relation_id=rel_id, member_ready=True)

55
unit_tests/test_hacluster_hooks.py
View File

@ -43,7 +43,7 @@ class TestCorosyncConf(unittest.TestCase):
@mock.patch.object(pcmk.unitdata, 'kv')
@mock.patch.object(hooks, 'remote_unit')
@mock.patch.object(hooks, 'principal_unit')
@mock.patch.object(hooks, 'relation_type')
@mock.patch.object(hooks, 'trigger_corosync_update_from_leader')
@mock.patch.object(hooks, 'is_stonith_configured')
@mock.patch.object(hooks, 'configure_peer_stonith_resource')
@ -83,7 +83,7 @@ class TestCorosyncConf(unittest.TestCase):
configure_peer_stonith_resource,
is_stonith_configured,
trigger_corosync_update_from_leader,
principal_unit, remote_unit, mock_kv):
relation_type, remote_unit, mock_kv):
def fake_crm_opt_exists(res_name):
# res_ubuntu will take the "update resource" route
@ -110,7 +110,8 @@ class TestCorosyncConf(unittest.TestCase):
'failure_timeout': 180,
'cluster_recheck_interval': 60}
trigger_corosync_update_from_leader.return_value = False
principal_unit.return_value = remote_unit.return_value = ""
relation_type.return_value = "hanode"
remote_unit.return_value = "hacluster/0"
config.side_effect = lambda key: cfg.get(key)
@ -173,7 +174,7 @@ class TestCorosyncConf(unittest.TestCase):
'crm -w -F configure %s %s %s' % (kw, name, params))
@mock.patch.object(hooks, 'remote_unit')
@mock.patch.object(hooks, 'principal_unit')
@mock.patch.object(hooks, 'relation_type')
@mock.patch.object(hooks, 'trigger_corosync_update_from_leader')
@mock.patch.object(hooks, 'is_stonith_configured')
@mock.patch.object(hooks, 'configure_peer_stonith_resource')
@ -212,7 +213,7 @@ class TestCorosyncConf(unittest.TestCase):
configure_peer_stonith_resource,
is_stonith_configured,
trigger_corosync_update_from_leader,
principal_unit, remote_unit):
relation_type, remote_unit):
is_stonith_configured.return_value = False
validate_dns_ha.return_value = True
crm_opt_exists.return_value = False
@ -231,7 +232,8 @@ class TestCorosyncConf(unittest.TestCase):
'maas_url': 'http://maas/MAAAS/',
'maas_credentials': 'secret'}
trigger_corosync_update_from_leader.return_value = False
principal_unit.return_value = remote_unit.return_value = ""
relation_type.return_value = "hanode"
remote_unit.return_value = "hacluster/0"
config.side_effect = lambda key: cfg.get(key)
@ -263,7 +265,7 @@ class TestCorosyncConf(unittest.TestCase):
'maas_credentials="secret"')
@mock.patch.object(hooks, 'remote_unit')
@mock.patch.object(hooks, 'principal_unit')
@mock.patch.object(hooks, 'relation_type')
@mock.patch.object(hooks, 'trigger_corosync_update_from_leader')
@mock.patch.object(hooks, 'setup_maas_api')
@mock.patch.object(hooks, 'validate_dns_ha')
@ -289,7 +291,7 @@ class TestCorosyncConf(unittest.TestCase):
configure_corosync, is_leader, crm_opt_exists,
wait_for_pcmk, validate_dns_ha, setup_maas_api,
trigger_corosync_update_from_leader,
principal_unit, remote_unit):
relation_type, remote_unit):
def fake_validate():
raise utils.MAASConfigIncomplete('DNS HA invalid config')
@ -309,7 +311,8 @@ class TestCorosyncConf(unittest.TestCase):
'maas_url': 'http://maas/MAAAS/',
'maas_credentials': None}
trigger_corosync_update_from_leader.return_value = False
principal_unit.return_value = remote_unit.return_value = ""
relation_type.return_value = "hanode"
remote_unit.return_value = "hacluster/0"
config.side_effect = lambda key: cfg.get(key)
@ -331,6 +334,40 @@ class TestCorosyncConf(unittest.TestCase):
mock_status_set.assert_called_with('blocked',
'DNS HA invalid config')
@mock.patch.object(hooks, 'remote_unit')
@mock.patch.object(hooks, 'relation_type')
@mock.patch.object(hooks, 'trigger_corosync_update_from_leader')
@mock.patch.object(hooks, 'get_cluster_nodes')
@mock.patch.object(hooks, 'relation_set')
@mock.patch.object(hooks, 'relation_ids')
@mock.patch.object(hooks, 'get_corosync_conf')
@mock.patch.object(hooks, 'config')
def test_ha_relation_changed_pacemaker_remote(
self, config, get_corosync_conf, relation_ids, relation_set,
get_cluster_nodes, trigger_corosync_update_from_leader,
relation_type, remote_unit):
"""Test pacemaker-remote-relation-changed.
Validates that ha_relation_changed() doesn't call
trigger_corosync_update_from_leader() when called from the
pacemaker-remote-relation-changed hook. See lp:1920124
"""
cfg = {'cluster_count': 3}
config.side_effect = lambda key: cfg.get(key)
# 2 out of 3 so that the function under test finishes early:
get_cluster_nodes.return_value = ['10.0.3.2', '10.0.3.3']
relation_ids.return_value = ['hanode:1']
get_corosync_conf.return_value = True
relation_type.return_value = 'pacemaker-remote'
remote_unit.return_value = 'pacemaker-remote/0'
hooks.ha_relation_changed()
relation_set.assert_any_call(relation_id='hanode:1', ready=True)
trigger_corosync_update_from_leader.assert_not_called()
class TestHooks(test_utils.CharmTestCase):
TO_PATCH = [