openstack
/
charm-nova-compute
Code Issues Proposed changes

Do not manage subordinate service restarts 

The subordinate charms should manage the services that
they deploys and configure, not the principle they are related to.
This change switches the approach for restarting services
from having the nova-compute charm doing it directly to having
nova-compute triggering the restart by request a restart down
the existing relations.

Closes-Bug: #1947585

Change-Id: I7419e39d68c70d21a11d03deeff9699421b0571e
This commit is contained in:
Liam Young 2023-02-28 13:31:28 +00:00
parent 7e3ead3389
commit cb04103e08
4 changed files with 100 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
hooks/nova_compute_hooks.py
View File

@ -15,6 +15,7 @@
# limitations under the License.
import base64
import functools
import json
import platform
import sys
@ -103,7 +104,6 @@ from nova_compute_utils import (
public_ssh_key,
restart_map,
services,
restart_failed_subordinate_services,
register_configs,
NOVA_CONF,
ceph_config_file, CEPH_SECRET,
@ -744,7 +744,7 @@ def nova_ceilometer_joined(relid=None, remote_restart=False):
@restart_on_change(restart_map())
def nova_ceilometer_relation_changed():
update_all_configs()
restart_failed_subordinate_services()
trigger_ceilometer_service_restart()
@hooks.hook('nova-vgpu-relation-joined')
@ -759,7 +759,7 @@ def nova_vgpu_joined(relid=None, remote_restart=False):
@restart_on_change(restart_map())
def nova_vgpu_relation_changed():
update_all_configs()
restart_failed_subordinate_services()
trigger_nova_vgpu_service_restart()
@hooks.hook('nrpe-external-master-relation-joined',
@ -804,7 +804,8 @@ def neutron_plugin_changed():
apt_purge('nova-api-metadata', fatal=True)
service_restart_handler(default_service='nova-compute')
CONFIGS.write(NOVA_CONF)
restart_failed_subordinate_services()
trigger_ceilometer_service_restart()
trigger_nova_vgpu_service_restart()
# TODO(jamespage): Move this into charmhelpers for general reuse.
@ -954,6 +955,36 @@ def shared_db_relation_joined():
"WARNING")
def trigger_subordinate_service_restart(relation_name, hook_method):
"""Send restart trigger to subordinate relaion
:param relation_name: Name of relation
:type relation_name: str
:param hook_method: Method to call to send trigger
:type hook_method: callable
"""
if not is_unit_paused_set():
log(
"Sending restart trigger down {} relation".format(relation_name),
"INFO")
for rid in relation_ids(relation_name):
hook_method(
rid,
remote_restart=True)
trigger_ceilometer_service_restart = functools.partial(
trigger_subordinate_service_restart,
'nova-ceilometer',
nova_ceilometer_joined)
trigger_nova_vgpu_service_restart = functools.partial(
trigger_subordinate_service_restart,
'nova-vgpu',
nova_vgpu_joined)
def main():
try:
hooks.execute(sys.argv)

22
hooks/nova_compute_utils.py
View File

@ -43,7 +43,6 @@ from charmhelpers.core.fstab import Fstab
from charmhelpers.core.host import (
mkdir,
service_restart,
service_running,
lsb_release,
rsync,
CompareHostReleases,
@ -465,27 +464,6 @@ def services():
list(get_subordinate_services()))
def restart_failed_subordinate_services():
'''
Restarts all subordinate services if they aren't running.
Doesn't do anything if the unit is paused.
Subordinate charms can advertise a list of services to this principal charm
so that this principal charm can properly manage them when pausing,
resuming and upgrading.
It can be useful to call this function once a relation with a subordinate
charm has been established, in order to mitigate a race condition described
in lp:1947585 in which the subordinate services may have failed to come up
if they were started before the principal services.
'''
if not is_unit_paused_set():
for s in get_subordinate_services():
if not service_running(s):
service_restart(s)
def register_configs():
'''
Returns an OSTemplateRenderer object with all required configs registered.

77
unit_tests/test_nova_compute_hooks.py
View File

@ -105,7 +105,6 @@ TO_PATCH = [
'render',
'remove_old_packages',
'services',
'restart_failed_subordinate_services',
'send_application_name',
]
@ -582,15 +581,20 @@ class NovaComputeRelationsTests(CharmTestCase):
prefix='nova',
)
def test_nova_ceilometer_relation_changed(self):
@patch.object(hooks, 'trigger_ceilometer_service_restart')
def test_nova_ceilometer_relation_changed(
self,
trigger_ceilometer_service_restart):
hooks.nova_ceilometer_relation_changed()
self.update_all_configs.assert_called()
self.restart_failed_subordinate_services.assert_called()
trigger_ceilometer_service_restart.assert_called()
def test_nova_vgpu_relation_changed(self):
hooks.nova_ceilometer_relation_changed()
@patch.object(hooks, 'trigger_nova_vgpu_service_restart')
def test_nova_vgpu_relation_changed(self,
trigger_nova_vgpu_service_restart):
hooks.nova_vgpu_relation_changed()
self.update_all_configs.assert_called()
self.restart_failed_subordinate_services.assert_called()
trigger_nova_vgpu_service_restart.assert_called()
def test_ceph_joined(self):
self.libvirt_daemon.return_value = 'libvirt-bin'
@ -969,10 +973,14 @@ class NovaComputeRelationsTests(CharmTestCase):
app_name='rbd',
compression_mode='fake')
@patch.object(hooks, 'trigger_nova_vgpu_service_restart')
@patch.object(hooks, 'trigger_ceilometer_service_restart')
@patch.object(hooks, 'service_restart_handler')
@patch.object(hooks, 'CONFIGS')
def test_neutron_plugin_changed(self, configs,
service_restart_handler):
service_restart_handler,
trigger_ceilometer_service_restart,
trigger_nova_vgpu_service_restart):
self.nova_metadata_requirement.return_value = (True,
'sharedsecret')
hooks.neutron_plugin_changed()
@ -982,12 +990,17 @@ class NovaComputeRelationsTests(CharmTestCase):
configs.write.assert_called_with('/etc/nova/nova.conf')
service_restart_handler.assert_called_with(
default_service='nova-compute')
self.restart_failed_subordinate_services.assert_called()
trigger_ceilometer_service_restart.assert_called()
trigger_nova_vgpu_service_restart.assert_called()
@patch.object(hooks, 'trigger_nova_vgpu_service_restart')
@patch.object(hooks, 'trigger_ceilometer_service_restart')
@patch.object(hooks, 'service_restart_handler')
@patch.object(hooks, 'CONFIGS')
def test_neutron_plugin_changed_nometa(self, configs,
service_restart_handler):
service_restart_handler,
trigger_ceilometer_service_restart,
trigger_nova_vgpu_service_restart):
self.nova_metadata_requirement.return_value = (False, None)
hooks.neutron_plugin_changed()
self.apt_purge.assert_called_with('nova-api-metadata',
@ -995,12 +1008,17 @@ class NovaComputeRelationsTests(CharmTestCase):
configs.write.assert_called_with('/etc/nova/nova.conf')
service_restart_handler.assert_called_with(
default_service='nova-compute')
self.restart_failed_subordinate_services.assert_called()
trigger_ceilometer_service_restart.assert_called()
trigger_nova_vgpu_service_restart.assert_called()
@patch.object(hooks, 'trigger_nova_vgpu_service_restart')
@patch.object(hooks, 'trigger_ceilometer_service_restart')
@patch.object(hooks, 'service_restart_handler')
@patch.object(hooks, 'CONFIGS')
def test_neutron_plugin_changed_meta(self, configs,
service_restart_handler):
service_restart_handler,
trigger_ceilometer_service_restart,
trigger_nova_vgpu_service_restart):
self.nova_metadata_requirement.return_value = (True, None)
hooks.neutron_plugin_changed()
self.apt_install.assert_called_with(['nova-api-metadata'],
@ -1008,7 +1026,8 @@ class NovaComputeRelationsTests(CharmTestCase):
configs.write.assert_called_with('/etc/nova/nova.conf')
service_restart_handler.assert_called_with(
default_service='nova-compute')
self.restart_failed_subordinate_services.assert_called()
trigger_ceilometer_service_restart.assert_called()
trigger_nova_vgpu_service_restart.assert_called()
@patch('nova_compute_context.config')
@patch.object(hooks, 'get_hugepage_number')
@ -1294,3 +1313,37 @@ class NovaComputeRelationsTests(CharmTestCase):
hooks.upgrade_charm()
self.remove_old_packages.assert_called_once_with()
self.service_restart.assert_called_once_with('nova-compute')
@patch.object(hooks, 'is_unit_paused_set')
@patch.object(hooks, 'nova_ceilometer_joined')
def test_trigger_ceilometer_service_restart(self,
nova_ceilometer_joined,
is_unit_paused_set):
self.uuid.uuid4.return_value = 'uuid1234'
self.relation_ids.return_value = ['nova-ceilometer:43']
is_unit_paused_set.return_value = True
hooks.trigger_ceilometer_service_restart()
self.assertFalse(nova_ceilometer_joined.called)
is_unit_paused_set.return_value = False
hooks.trigger_ceilometer_service_restart()
self.relation_set.assert_called_with(
relation_id='nova-ceilometer:43',
relation_settings={
'restart-trigger': 'uuid1234'})
@patch.object(hooks, 'is_unit_paused_set')
@patch.object(hooks, 'nova_vgpu_joined')
def test_trigger_nova_vgpu_service_restart(self,
nova_vgpu_joined,
is_unit_paused_set):
self.uuid.uuid4.return_value = 'uuid1234'
self.relation_ids.return_value = ['nova-vgpu:12']
is_unit_paused_set.return_value = True
hooks.trigger_nova_vgpu_service_restart()
self.assertFalse(nova_vgpu_joined.called)
is_unit_paused_set.return_value = False
hooks.trigger_nova_vgpu_service_restart()
self.relation_set.assert_called_with(
relation_id='nova-vgpu:12',
relation_settings={
'restart-trigger': 'uuid1234'})

37
unit_tests/test_nova_compute_utils.py
View File

@ -1440,43 +1440,6 @@ class NovaComputeUtilsTests(CharmTestCase):
self.assertEqual(expected_service_set, set(actual_service_list))
self.assertEqual(expected_last_service, actual_service_list[-1])
@patch.object(utils, 'service_restart')
@patch.object(utils, 'service_running')
@patch.object(utils, 'get_subordinate_services')
@patch.object(utils, 'is_unit_paused_set')
def test_restart_failed_subordinate_services(self, _is_unit_paused_set,
_get_subordinate_services,
_service_running,
_service_restart):
_is_unit_paused_set.return_value = True
utils.restart_failed_subordinate_services()
_service_restart.assert_not_called()
# Validate that when a single service is not running, it is restarted
_service_restart.reset_mock()
_is_unit_paused_set.return_value = False
_get_subordinate_services.return_value = ['ceilometer-agent']
_service_running.return_value = False
utils.restart_failed_subordinate_services()
_service_restart.assert_called_once_with('ceilometer-agent')
# Validate that when multiple subordinate services exist, only ones
# that are stopped are restarted.
_service_restart.reset_mock()
def is_running(service):
if service == 'ceilometer-agent':
return True
else:
return False
_service_running.side_effect = is_running
_get_subordinate_services.return_value = ['ceilometer-agent', 'foo']
utils.restart_failed_subordinate_services()
_service_restart.assert_has_calls([
call('foo'),
])
@patch.object(utils, 'render')
def test_install_mount_override(self, render):
utils.install_mount_override('/srv/test')