openstack
/
charm-nova-cloud-controller
Code Issues Proposed changes

Refactor compute_changed() hook handler 

The compute_changed() function (which handled the cloud-compute relation
changed hook event) was also used in the config-changed and update-charm
hooks.  This meant that it did a lot of work that wasn't necessary for
those hooks.

This patch splits the function up into separate functions that deal with
one thing, and then introduces a new function to call those.  This means
that the other usages compute_changed() now use the actual features that
they need.

Change-Id: I59e52076480729beec9e125f66714a208303908d
Related-Bug: 1833420
This commit is contained in:
Alex Kavanagh 2019-06-26 16:24:54 +01:00
parent ee97d576f8
commit cb60bab6f7
2 changed files with 100 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
hooks/nova_cc_hooks.py
View File

@ -279,7 +279,10 @@ def config_changed():
if ch_utils.config_value_changed('region'):
for rid in hookenv.relation_ids('cloud-compute'):
for unit in hookenv.related_units(rid):
compute_changed(rid, unit)
# Note(ajkavanagh) This used to also update all of the ssh keys
# and hosts to every unit; but that's almost certainly not
# needed on a config changed hook
notify_region_to_unit(rid, unit)
ncc_utils.update_aws_compat_services()
@ -624,16 +627,71 @@ def compute_joined(rid=None, remote_restart=False):
@hooks.hook('cloud-compute-relation-changed')
def compute_changed(rid=None, unit=None):
rel_settings = hookenv.relation_get(rid=rid, unit=unit)
if not rel_settings.get('region', None) == hookenv.config('region'):
hookenv.relation_set(relation_id=rid, region=hookenv.config('region'))
def cloud_compute_relation_changed():
"""Performs actions associated with when the cloud compute relation changes
for a unit.
* add hosts to the cell when ready
* notifies the region to the unit, if it has changed
* notifies the ssh known hosts and authorized keys to the unit
"""
add_hosts_to_cell_when_ready()
notify_region_to_unit(rid=None, unit=None)
update_ssh_keys_and_notify_compute_units(rid=None, unit=None)
def add_hosts_to_cell_when_ready():
"""Helper function to call add_hosts_to_cell() when the unit is the leader
and the cellv2 and database are ready.
:raises: subprocess.CalledProcessError if cells command fails
"""
if (hookenv.is_leader() and
ncc_utils.is_cellv2_init_ready() and
ncc_utils.is_db_initialised()):
ncc_utils.add_hosts_to_cell()
def notify_region_to_unit(rid=None, unit=None):
"""Helper function that if the relation data set by the unit differs from
the config on nova-cloud-controller, then nova-cc sets the new region for
that relation to trigger a change for any units that see it differently.
:param rid: The relation to check/set, or if None, the current one related
to the hook.
:type rid: Union[str. None]
:param unit: the unit to check, of None for the current one according to
the hook.
:type unit: Union[str, None]
"""
rel_settings = hookenv.relation_get(rid=rid, unit=unit)
if not rel_settings.get('region', None) == hookenv.config('region'):
hookenv.relation_set(relation_id=rid, region=hookenv.config('region'))
def update_ssh_keys_and_notify_compute_units(rid=None, unit=None):
"""Update and notify the collected ssh keys to nova-compute units
Update/add and notify, for the associated nova-compute unit, the ssh key to
all the other nova-compute units.
If rid=None and unit=None, then this function is being called in the
context of a cloud-compute relation changed hook, and will relate to that
unit. If rid and unit are set, then this function is being called to
refresh and update all specific units.
TODO: split this function in to updating/adding SSH keys and notifying
(set_relation) all of the other units.
:param rid: The relation to check/set, or if None, the current one related
to the hook.
:type rid: Union[str, None]
:param unit: the unit to check, of None for the current one according to
the hook.
:type unit: Union[str, None]
"""
rel_settings = hookenv.relation_get(rid=rid, unit=unit)
if 'migration_auth_type' not in rel_settings:
return
if rel_settings['migration_auth_type'] == 'ssh':
@ -899,10 +957,13 @@ def upgrade_charm():
identity_joined(rid=r_id)
for r_id in hookenv.relation_ids('cloud-compute'):
for unit in hookenv.related_units(r_id):
compute_changed(r_id, unit)
notify_region_to_unit(r_id, unit)
update_ssh_keys_and_notify_compute_units(r_id, unit)
for r_id in hookenv.relation_ids('shared-db'):
db_joined(relation_id=r_id)
add_hosts_to_cell_when_ready()
leader_init_db_if_ready_allowed_units()
update_nrpe_config()

60
unit_tests/test_nova_cc_hooks.py
View File

@ -316,8 +316,9 @@ class NovaCCHooksTests(CharmTestCase):
@patch('charmhelpers.fetch.filter_installed_packages')
@patch.object(hooks, 'configure_https')
@patch.object(hooks, 'compute_joined')
@patch.object(hooks, 'compute_changed')
def test_config_changed_region_change(self, mock_compute_changed,
@patch.object(hooks, 'notify_region_to_unit')
def test_config_changed_region_change(self,
mock_notify_region_to_unit,
mock_compute_joined,
mock_config_https,
mock_filter_packages,
@ -338,15 +339,27 @@ class NovaCCHooksTests(CharmTestCase):
self.os_release.return_value = 'diablo'
hooks.resolve_CONFIGS()
hooks.config_changed()
mock_compute_changed.assert_has_calls([call('generic_rid', 'unit/0')])
mock_notify_region_to_unit.assert_has_calls(
[call('generic_rid', 'unit/0')])
mock_compute_joined.assert_has_calls(
[call(rid='generic_rid', remote_restart=False)])
self.assertTrue(mock_update_aws_compat_services.called)
@patch('hooks.nova_cc_utils.is_cellv2_init_ready')
@patch('hooks.nova_cc_utils.is_db_initialised')
def test_compute_changed_ssh_migration(self, mock_is_db_initialised,
mock_is_cellv2_init_ready):
@patch.object(hooks, 'add_hosts_to_cell_when_ready')
@patch.object(hooks, 'notify_region_to_unit')
@patch.object(hooks, 'update_ssh_keys_and_notify_compute_units')
def test_cloud_compute_relation_changed(
self,
mock_update_ssh_keys_and_notify_compute_units,
mock_notify_region_to_unit,
mock_add_hosts_to_cell_when_ready):
hooks.cloud_compute_relation_changed()
mock_add_hosts_to_cell_when_ready.assert_called_once_with()
mock_notify_region_to_unit.assert_called_once_with(rid=None, unit=None)
mock_update_ssh_keys_and_notify_compute_units.assert_called_once_with(
rid=None, unit=None)
def test_update_ssh_keys_and_notify_compute_units_ssh_migration(self):
self.test_relation.set({
'migration_auth_type': 'ssh', 'ssh_public_key': 'fookey',
'private-address': '10.0.0.1', 'region': 'RegionOne'})
@ -354,9 +367,7 @@ class NovaCCHooksTests(CharmTestCase):
'k_h_0', 'k_h_1', 'k_h_2']
self.ssh_authorized_keys_lines.return_value = [
'auth_0', 'auth_1', 'auth_2']
mock_is_db_initialised.return_value = False
mock_is_cellv2_init_ready.return_value = False
hooks.compute_changed()
hooks.update_ssh_keys_and_notify_compute_units()
self.ssh_compute_add.assert_called_with('fookey', rid=None, unit=None)
expected_relations = [
call(relation_settings={'authorized_keys_0': 'auth_0'},
@ -375,10 +386,7 @@ class NovaCCHooksTests(CharmTestCase):
call(known_hosts_max_index=3, relation_id=None)]
self.relation_set.assert_has_calls(expected_relations, any_order=True)
@patch('hooks.nova_cc_utils.is_cellv2_init_ready')
@patch('hooks.nova_cc_utils.is_db_initialised')
def test_compute_changed_nova_public_key(self, mock_is_db_initialised,
mock_is_cellv2_init_ready):
def test_update_ssh_keys_and_notify_compute_units_nova_public_key(self):
self.test_relation.set({
'migration_auth_type': 'sasl', 'nova_ssh_public_key': 'fookey',
'private-address': '10.0.0.1', 'region': 'RegionOne'})
@ -386,9 +394,7 @@ class NovaCCHooksTests(CharmTestCase):
'k_h_0', 'k_h_1', 'k_h_2']
self.ssh_authorized_keys_lines.return_value = [
'auth_0', 'auth_1', 'auth_2']
mock_is_db_initialised.return_value = False
mock_is_cellv2_init_ready.return_value = False
hooks.compute_changed()
hooks.update_ssh_keys_and_notify_compute_units()
self.ssh_compute_add.assert_called_with('fookey', user='nova',
rid=None, unit=None)
expected_relations = [
@ -413,14 +419,14 @@ class NovaCCHooksTests(CharmTestCase):
@patch('hooks.nova_cc_utils.is_cellv2_init_ready')
@patch('hooks.nova_cc_utils.is_db_initialised')
@patch('hooks.nova_cc_utils.add_hosts_to_cell')
def test_compute_changed_add_hosts_leader(self,
mock_add_hosts_to_cell,
mock_is_db_initialised,
mock_is_cellv2_init_ready):
def test_add_hosts_to_cell_when_ready_leader(self,
mock_add_hosts_to_cell,
mock_is_db_initialised,
mock_is_cellv2_init_ready):
self.is_leader.return_value = True
mock_is_db_initialised.return_value = True
mock_is_cellv2_init_ready.return_value = True
hooks.compute_changed()
hooks.add_hosts_to_cell_when_ready()
self.assertTrue(self.is_leader.called)
self.assertTrue(mock_is_db_initialised.called)
self.assertTrue(mock_is_cellv2_init_ready.called)
@ -429,14 +435,14 @@ class NovaCCHooksTests(CharmTestCase):
@patch('hooks.nova_cc_utils.is_cellv2_init_ready')
@patch('hooks.nova_cc_utils.is_db_initialised')
@patch('hooks.nova_cc_utils.add_hosts_to_cell')
def test_compute_changed_add_hosts_nonleader(self,
mock_add_hosts_to_cell,
mock_is_db_initialised,
mock_is_cellv2_init_ready):
def test_add_hosts_to_cell_when_ready_nonleader(self,
mock_add_hosts_to_cell,
mock_is_db_initialised,
mock_is_cellv2_init_ready):
self.is_leader.return_value = False
mock_is_db_initialised.return_value = True
mock_is_cellv2_init_ready.return_value = True
hooks.compute_changed()
hooks.add_hosts_to_cell_when_ready()
self.assertTrue(self.is_leader.called)
self.assertFalse(mock_is_db_initialised.called)
self.assertFalse(mock_is_cellv2_init_ready.called)