From 332e7a279909aea57722f0484cd9144daa8deba0 Mon Sep 17 00:00:00 2001 From: Guillaume Boutry Date: Thu, 16 Jul 2026 17:58:08 +0200 Subject: [PATCH 1/2] perf(hooks): skip converged service operations Avoid snapctl service tasks when the requested enabled and active state is already satisfied. Keep corrective operations for every partially converged state. Treat file-transfer as unavailable until its TLS inputs exist so faster reconciliation does not expose a start-limit timing dependency. Assisted-By: Codex (gpt-5-6) Signed-off-by: Guillaume Boutry --- openstack_hypervisor/hooks.py | 43 ++++++++--- tests/unit/test_hooks.py | 141 ++++++++++++++++++++++++++++++++-- 2 files changed, 166 insertions(+), 18 deletions(-) diff --git a/openstack_hypervisor/hooks.py b/openstack_hypervisor/hooks.py index 3fbc70d..77b02de 100644 --- a/openstack_hypervisor/hooks.py +++ b/openstack_hypervisor/hooks.py @@ -414,6 +414,11 @@ def _get_local_ip_by_default_route() -> str: # Required config can be a section like "identity" in which case all keys must # be set or a single key like "identity.password". REQUIRED_CONFIG = { + "file-transfer": [ + "compute.cacert", + "compute.cert", + "compute.key", + ], "nova-compute": [ "identity.password", "identity.username", @@ -649,6 +654,9 @@ def __exit__(self, exc_type, exc_value, exc_traceback): restart_services.extend(self.files[file].get("services", [])) restart_services = set([s for s in restart_services if s not in self.exclude_services]) + if not restart_services: + return + services = self.snap.services.list() for service in restart_services: logging.info(f"Restarting {service}") @@ -656,6 +664,20 @@ def __exit__(self, exc_type, exc_value, exc_traceback): services[service].start(enable=True) +def _ensure_service_started(service: Any) -> None: + """Start and enable a service unless it has already converged.""" + if service.enabled and service.active: + return + service.start(enable=True) + + +def _ensure_service_stopped(service: Any) -> None: + """Stop and disable a service unless it has already converged.""" + if not service.enabled and not service.active: + return + service.stop(disable=True) + + def _update_default_config(snap: Snap) -> None: """Add any missing default configuration keys. @@ -2608,11 +2630,11 @@ def _configure_monitoring_services(snap: Snap) -> None: for service in MONITORING_SERVICES: if ovs_external and service in EXTERNAL_OVS_SERVICES: continue - services[service].start(enable=True) + _ensure_service_started(services[service]) else: logging.info("Disabling all exporter services.") for service in MONITORING_SERVICES: - services[service].stop(disable=True) + _ensure_service_stopped(services[service]) def _configure_masakari_services(snap: Snap) -> None: @@ -2627,16 +2649,17 @@ def _configure_masakari_services(snap: Snap) -> None: if enable_masakari: logging.info("Enabling all masakari services.") for service in MASAKARI_SERVICES: - services[service].start(enable=True) + _ensure_service_started(services[service]) else: logging.info("Disabling all masakari services.") for service in MASAKARI_SERVICES: - services[service].stop(disable=True) + _ensure_service_stopped(services[service]) def services() -> List[str]: """List of services managed by hooks.""" - return sorted(list(set([w for v in TEMPLATES.values() for w in v.get("services", [])]))) + templates = {**TEMPLATES, **TLS_TEMPLATES} + return sorted(set(w for v in templates.values() for w in v.get("services", []))) def _section_complete(section: str, context: dict) -> bool: @@ -2835,10 +2858,10 @@ def _configure_sriov_agent_service(snap: Snap, enabled: bool) -> None: sriov_service = snap.services.list()["neutron-sriov-nic-agent"] if enabled: logging.info("SR-IOV mappings detected, enabling SR-IOV agent.") - sriov_service.start(enable=True) + _ensure_service_started(sriov_service) else: logging.info("No SR-IOV mappings detected, disabling SR-IOV agent.") - sriov_service.stop(disable=True) + _ensure_service_stopped(sriov_service) def _set_config_context(context, group, key, val): @@ -3039,7 +3062,7 @@ def configure(snap: Snap) -> None: external_ovs_deferred = ovs_external and not _external_ovs_ready(snap) ovs_deferred = internal_ovs_deferred or external_ovs_deferred for service in exclude_services: - services[service].stop(disable=True) + _ensure_service_stopped(services[service]) with RestartOnChange(snap, {**TEMPLATES, **TLS_TEMPLATES}, exclude_services): _render_templates(snap, context) @@ -3249,7 +3272,7 @@ def _ensure_internal_ovs_services(snap: Snap, exclude_services: list[str]) -> No if service in MONITORING_SERVICES and not enable_monitoring: continue logging.info("Ensuring internal OVS service is enabled: %s", service) - services[service].start(enable=True) + _ensure_service_started(services[service]) def _ensure_internal_ovs_dependent_services(snap: Snap, exclude_services: list[str]) -> None: @@ -3260,7 +3283,7 @@ def _ensure_internal_ovs_dependent_services(snap: Snap, exclude_services: list[s if service in exclude_services: continue logging.info("Ensuring internal OVS-dependent service is enabled: %s", service) - services[service].start(enable=True) + _ensure_service_started(services[service]) def _get_exclude_services(context: dict) -> list[str]: diff --git a/tests/unit/test_hooks.py b/tests/unit/test_hooks.py index d021143..cc8ac31 100644 --- a/tests/unit/test_hooks.py +++ b/tests/unit/test_hooks.py @@ -19,6 +19,14 @@ from openstack_hypervisor.hooks import OwnedPath +def service_mock(*, enabled: bool, active: bool): + """Return a service mock with an explicit snapd state.""" + service = mock.Mock() + service.enabled = enabled + service.active = active + return service + + class TestOwnedPath: """Tests for the OwnedPath class.""" @@ -243,6 +251,8 @@ def test_configure_hook( def test_configure_hook_exception(self, mocker, snap, os_makedirs, check_call, shutil_chown): """Tests the configure hook raising an exception while writing file.""" mock_template = mocker.Mock() + mocker.patch.object(hooks, "_is_multipathd_available", return_value=False) + mocker.patch.object(hooks, "_ensure_services_stopped") mocker.patch.object(hooks, "_get_template", return_value=mock_template) mocker.patch.object(hooks.Path, "write_text") mocker.patch.object(hooks.Path, "chmod") @@ -257,6 +267,7 @@ def test_services(self): """Test getting a list of managed services.""" assert hooks.services() == [ "ceilometer-compute-agent", + "file-transfer", "libvirtd", "masakari-instancemonitor", "neutron-ovn-metadata-agent", @@ -288,6 +299,7 @@ def test_services_not_ready(self, snap): config = {} assert hooks._services_not_ready(config) == [ "ceilometer-compute-agent", + "file-transfer", "masakari-instancemonitor", "neutron-ovn-metadata-agent", "nova-api-metadata", @@ -296,6 +308,7 @@ def test_services_not_ready(self, snap): config["identity"] = {"username": "user", "password": "pass"} assert hooks._services_not_ready(config) == [ "ceilometer-compute-agent", + "file-transfer", "masakari-instancemonitor", "neutron-ovn-metadata-agent", "nova-api-metadata", @@ -304,6 +317,7 @@ def test_services_not_ready(self, snap): config["rabbitmq"] = {"url": "rabbit://localhost:5672"} config["node"] = {"fqdn": "myhost.maas"} assert hooks._services_not_ready(config) == [ + "file-transfer", "neutron-ovn-metadata-agent", "nova-api-metadata", ] @@ -314,15 +328,23 @@ def test_services_not_ready(self, snap): "ovn_cacert": "cacert", } assert hooks._services_not_ready(config) == [ + "file-transfer", "neutron-ovn-metadata-agent", "nova-api-metadata", ] config["network"]["nova_metadata_proxy_url"] = "http://internal/nova-metadata" assert hooks._services_not_ready(config) == [ + "file-transfer", "neutron-ovn-metadata-agent", "nova-api-metadata", ] config["credentials"] = {"ovn_metadata_proxy_shared_secret": "secret"} + assert hooks._services_not_ready(config) == ["file-transfer"] + config["compute"] = { + "cacert": "cacert", + "cert": "cert", + "key": "key", + } assert hooks._services_not_ready(config) == [] def test_services_not_enabled_by_config(self, snap): @@ -1457,7 +1479,9 @@ class TestEnsureInternalOVSServices: """Tests for _ensure_internal_ovs_services function.""" def test_starts_non_excluded_services(self, snap): - services = {name: mock.Mock() for name in hooks.EXTERNAL_OVS_SERVICES} + services = { + name: service_mock(enabled=False, active=False) for name in hooks.EXTERNAL_OVS_SERVICES + } snap.services.list.return_value = services snap.config.get.return_value = True # monitoring.enable = True @@ -1469,7 +1493,9 @@ def test_starts_non_excluded_services(self, snap): services["ovs-exporter"].start.assert_called_once_with(enable=True) def test_does_not_enable_ovs_exporter_when_monitoring_disabled(self, snap): - services = {name: mock.Mock() for name in hooks.EXTERNAL_OVS_SERVICES} + services = { + name: service_mock(enabled=False, active=False) for name in hooks.EXTERNAL_OVS_SERVICES + } snap.services.list.return_value = services snap.config.get.return_value = False # monitoring.enable = False @@ -1485,7 +1511,7 @@ class TestEnsureInternalOVSDependentServices: """Tests for _ensure_internal_ovs_dependent_services function.""" def test_starts_non_excluded_services(self, snap): - services = {"neutron-ovn-metadata-agent": mock.Mock()} + services = {"neutron-ovn-metadata-agent": service_mock(enabled=False, active=False)} snap.services.list.return_value = services helper = getattr(hooks, "_ensure_internal_ovs_dependent_services", None) @@ -1495,7 +1521,7 @@ def test_starts_non_excluded_services(self, snap): services["neutron-ovn-metadata-agent"].start.assert_called_once_with(enable=True) def test_skips_excluded_services(self, snap): - services = {"neutron-ovn-metadata-agent": mock.Mock()} + services = {"neutron-ovn-metadata-agent": service_mock(enabled=False, active=False)} snap.services.list.return_value = services helper = getattr(hooks, "_ensure_internal_ovs_dependent_services", None) @@ -1537,7 +1563,9 @@ class TestConfigureMonitoringServices: def test_external_ovs_monitoring_enabled_skips_ovs_exporter(self, mocker, snap): """ovs-exporter is not started when OVS is external.""" mocker.patch.object(hooks, "is_ovs_external", return_value=True) - services = {name: mock.Mock() for name in hooks.MONITORING_SERVICES} + services = { + name: service_mock(enabled=False, active=False) for name in hooks.MONITORING_SERVICES + } snap.services.list.return_value = services snap.config.get.return_value = True # monitoring.enable = True @@ -1549,7 +1577,9 @@ def test_external_ovs_monitoring_enabled_skips_ovs_exporter(self, mocker, snap): def test_internal_ovs_monitoring_enabled_starts_all(self, mocker, snap): """Test that all exporters are started when OVS is internal and monitoring enabled.""" mocker.patch.object(hooks, "is_ovs_external", return_value=False) - services = {name: mock.Mock() for name in hooks.MONITORING_SERVICES} + services = { + name: service_mock(enabled=False, active=False) for name in hooks.MONITORING_SERVICES + } snap.services.list.return_value = services snap.config.get.return_value = True # monitoring.enable = True @@ -1561,7 +1591,9 @@ def test_internal_ovs_monitoring_enabled_starts_all(self, mocker, snap): def test_monitoring_disabled_stops_all(self, mocker, snap): """Test that all exporters are stopped when monitoring is disabled.""" mocker.patch.object(hooks, "is_ovs_external", return_value=False) - services = {name: mock.Mock() for name in hooks.MONITORING_SERVICES} + services = { + name: service_mock(enabled=True, active=True) for name in hooks.MONITORING_SERVICES + } snap.services.list.return_value = services snap.config.get.return_value = False # monitoring.enable = False @@ -1571,6 +1603,99 @@ def test_monitoring_disabled_stops_all(self, mocker, snap): services["ovs-exporter"].stop.assert_called_once_with(disable=True) +class TestServiceReconciliation: + """Tests for state-aware individual service reconciliation.""" + + @pytest.mark.parametrize( + "enabled,active,should_start", + [ + (False, False, True), + (False, True, True), + (True, False, True), + (True, True, False), + ], + ) + def test_ensure_service_started(self, enabled, active, should_start): + service = service_mock(enabled=enabled, active=active) + + hooks._ensure_service_started(service) + + if should_start: + service.start.assert_called_once_with(enable=True) + else: + service.start.assert_not_called() + + @pytest.mark.parametrize( + "enabled,active,should_stop", + [ + (False, False, False), + (False, True, True), + (True, False, True), + (True, True, True), + ], + ) + def test_ensure_service_stopped(self, enabled, active, should_stop): + service = service_mock(enabled=enabled, active=active) + + hooks._ensure_service_stopped(service) + + if should_stop: + service.stop.assert_called_once_with(disable=True) + else: + service.stop.assert_not_called() + + def test_masakari_reconciles_enabled_services(self, snap): + services = { + name: service_mock(enabled=False, active=False) for name in hooks.MASAKARI_SERVICES + } + snap.services.list.return_value = services + snap.config.get.return_value = True + + hooks._configure_masakari_services(snap) + + for service in services.values(): + service.start.assert_called_once_with(enable=True) + + @pytest.mark.parametrize("enabled", [False, True]) + def test_sriov_reconciles_service(self, snap, enabled): + service = service_mock(enabled=not enabled, active=not enabled) + snap.services.list.return_value = {"neutron-sriov-nic-agent": service} + + hooks._configure_sriov_agent_service(snap, enabled) + + if enabled: + service.start.assert_called_once_with(enable=True) + else: + service.stop.assert_called_once_with(disable=True) + + +class TestRestartOnChange: + """Tests for change-triggered service restarts.""" + + def test_unchanged_files_do_not_list_services(self, snap): + config = snap.paths.common / "service.conf" + config.parent.mkdir(parents=True) + config.write_text("unchanged") + + with hooks.RestartOnChange(snap, {Path("service.conf"): {"services": ["service"]}}): + pass + + snap.services.list.assert_not_called() + + def test_changed_file_restarts_service(self, snap): + config = snap.paths.common / "service.conf" + config.parent.mkdir(parents=True) + config.write_text("before") + service = mock.Mock() + snap.services.list.return_value = {"service": service} + + with hooks.RestartOnChange(snap, {Path("service.conf"): {"services": ["service"]}}): + config.write_text("after") + + service.stop.assert_called_once_with() + service.start.assert_called_once_with(enable=True) + + class TestConfigureTLS: """Tests for TLS configuration orchestration.""" @@ -1645,7 +1770,7 @@ class TestConfigureOVSDeferred: def test_internal_ovs_not_ready_defers_ovs_configuration(self, mocker, snap): """Internal OVS work is deferred until a later configure hook.""" order = [] - services = {"svc1": mock.Mock()} + services = {"svc1": service_mock(enabled=True, active=True)} snap.services.list.return_value = services mocker.patch.object(hooks, "_mkdirs") mocker.patch.object(hooks, "_update_default_config") From eb762ec6418bb824461af37c36681dc7a7e2f9d7 Mon Sep 17 00:00:00 2001 From: Guillaume Boutry Date: Thu, 16 Jul 2026 19:02:49 +0200 Subject: [PATCH 2/2] perf(hooks): batch service reconciliation Combine service operations within each existing configure-hook phase to reduce snapctl round trips. Preserve pre-command state filtering; snapd applies the queued service operations after the hook exits. Assisted-By: Codex (gpt-5-6) Signed-off-by: Guillaume Boutry --- openstack_hypervisor/hooks.py | 117 ++++++++++++--------- tests/unit/test_hooks.py | 193 +++++++++++++++++++++------------- 2 files changed, 189 insertions(+), 121 deletions(-) diff --git a/openstack_hypervisor/hooks.py b/openstack_hypervisor/hooks.py index 77b02de..911b1b9 100644 --- a/openstack_hypervisor/hooks.py +++ b/openstack_hypervisor/hooks.py @@ -38,7 +38,7 @@ from netifaces import AF_INET, gateways, ifaddresses from pyroute2 import IPRoute from pyroute2.netlink.exceptions import NetlinkError -from snaphelpers import Snap +from snaphelpers import Snap, SnapCtl from snaphelpers._conf import UnknownConfigKey from openstack_hypervisor import netplan, pci @@ -657,25 +657,53 @@ def __exit__(self, exc_type, exc_value, exc_traceback): if not restart_services: return - services = self.snap.services.list() - for service in restart_services: - logging.info(f"Restarting {service}") - services[service].stop() - services[service].start(enable=True) + _restart_services(self.snap, restart_services) -def _ensure_service_started(service: Any) -> None: - """Start and enable a service unless it has already converged.""" - if service.enabled and service.active: +def _service_subset(snap: Snap, names: typing.Iterable[str]) -> tuple[list[str], Dict[str, Any]]: + """Return sorted unique service names and their current state.""" + requested = sorted(set(names)) + if not requested: + return requested, {} + services = snap.services.list() + missing = [name for name in requested if name not in services] + if missing: + raise RuntimeError(f"Services are not defined by the snap: {', '.join(missing)}") + return requested, services + + +# snapd applies service tasks only after the hook exits, so these helpers must +# not re-read status to verify their queued state transitions. +def _ensure_services_started(snap: Snap, names: typing.Iterable[str]) -> None: + """Queue one start-and-enable operation for non-converged services.""" + requested, services = _service_subset(snap, names) + pending = [ + name for name in requested if not (services[name].enabled and services[name].active) + ] + if not pending: + return + SnapCtl(env=snap.environ).start(*pending, enable=True) + + +def _ensure_services_stopped(snap: Snap, names: typing.Iterable[str]) -> None: + """Queue one stop-and-disable operation for non-converged services.""" + requested, services = _service_subset(snap, names) + pending = [name for name in requested if services[name].enabled or services[name].active] + if not pending: return - service.start(enable=True) + SnapCtl(env=snap.environ).stop(*pending, disable=True) -def _ensure_service_stopped(service: Any) -> None: - """Stop and disable a service unless it has already converged.""" - if not service.enabled and not service.active: +def _restart_services(snap: Snap, names: typing.Iterable[str]) -> None: + """Queue batched stop and start operations for the services.""" + requested, _ = _service_subset(snap, names) + if not requested: return - service.stop(disable=True) + for service in requested: + logging.info("Restarting %s", service) + snapctl = SnapCtl(env=snap.environ) + snapctl.stop(*requested) + snapctl.start(*requested, enable=True) def _update_default_config(snap: Snap) -> None: @@ -2619,7 +2647,6 @@ def _configure_monitoring_services(snap: Snap) -> None: :type snap: Snap :return: None """ - services = snap.services.list() enable_monitoring = snap.config.get("monitoring.enable") if enable_monitoring: ovs_external = is_ovs_external() @@ -2627,14 +2654,15 @@ def _configure_monitoring_services(snap: Snap) -> None: logging.info("Enabling exporter services (skipping external OVS exporters).") else: logging.info("Enabling all exporter services.") - for service in MONITORING_SERVICES: - if ovs_external and service in EXTERNAL_OVS_SERVICES: - continue - _ensure_service_started(services[service]) + desired = [ + service + for service in MONITORING_SERVICES + if not (ovs_external and service in EXTERNAL_OVS_SERVICES) + ] + _ensure_services_started(snap, desired) else: logging.info("Disabling all exporter services.") - for service in MONITORING_SERVICES: - _ensure_service_stopped(services[service]) + _ensure_services_stopped(snap, MONITORING_SERVICES) def _configure_masakari_services(snap: Snap) -> None: @@ -2644,16 +2672,13 @@ def _configure_masakari_services(snap: Snap) -> None: :type snap: Snap :return: None """ - services = snap.services.list() enable_masakari = snap.config.get("masakari.enable") if enable_masakari: logging.info("Enabling all masakari services.") - for service in MASAKARI_SERVICES: - _ensure_service_started(services[service]) + _ensure_services_started(snap, MASAKARI_SERVICES) else: logging.info("Disabling all masakari services.") - for service in MASAKARI_SERVICES: - _ensure_service_stopped(services[service]) + _ensure_services_stopped(snap, MASAKARI_SERVICES) def services() -> List[str]: @@ -2855,13 +2880,13 @@ def process_whitelisted_sriov_pfs( def _configure_sriov_agent_service(snap: Snap, enabled: bool) -> None: - sriov_service = snap.services.list()["neutron-sriov-nic-agent"] + service = ["neutron-sriov-nic-agent"] if enabled: logging.info("SR-IOV mappings detected, enabling SR-IOV agent.") - _ensure_service_started(sriov_service) + _ensure_services_started(snap, service) else: logging.info("No SR-IOV mappings detected, disabling SR-IOV agent.") - _ensure_service_stopped(sriov_service) + _ensure_services_stopped(snap, service) def _set_config_context(context, group, key, val): @@ -3053,7 +3078,6 @@ def configure(snap: Snap) -> None: context = _get_configure_context(snap) exclude_services = _get_exclude_services(context) - services = snap.services.list() ovs_external = is_ovs_external() logging.info( "OVS management: %s", "external (microovn)" if ovs_external else "internal (hypervisor)" @@ -3061,8 +3085,7 @@ def configure(snap: Snap) -> None: internal_ovs_deferred = not ovs_external and not _internal_ovs_ready(snap) external_ovs_deferred = ovs_external and not _external_ovs_ready(snap) ovs_deferred = internal_ovs_deferred or external_ovs_deferred - for service in exclude_services: - _ensure_service_stopped(services[service]) + _ensure_services_stopped(snap, exclude_services) with RestartOnChange(snap, {**TEMPLATES, **TLS_TEMPLATES}, exclude_services): _render_templates(snap, context) @@ -3262,28 +3285,24 @@ def _ensure_internal_ovs_services(snap: Snap, exclude_services: list[str]) -> No snap: The snap reference. exclude_services: Services that should remain stopped. """ - services = snap.services.list() enable_monitoring = snap.config.get("monitoring.enable") - - for service in EXTERNAL_OVS_SERVICES: - if service in exclude_services: - continue - # ovs-exporter should only be enabled if monitoring is enabled - if service in MONITORING_SERVICES and not enable_monitoring: - continue - logging.info("Ensuring internal OVS service is enabled: %s", service) - _ensure_service_started(services[service]) + desired = [ + service + for service in EXTERNAL_OVS_SERVICES + if service not in exclude_services + and not (service in MONITORING_SERVICES and not enable_monitoring) + ] + logging.info("Ensuring internal OVS services are enabled: %s", desired) + _ensure_services_started(snap, desired) def _ensure_internal_ovs_dependent_services(snap: Snap, exclude_services: list[str]) -> None: """Ensure services that need internal OVS are enabled when not excluded.""" - services = snap.services.list() - - for service in INTERNAL_OVS_DEPENDENT_SERVICES: - if service in exclude_services: - continue - logging.info("Ensuring internal OVS-dependent service is enabled: %s", service) - _ensure_service_started(services[service]) + desired = [ + service for service in INTERNAL_OVS_DEPENDENT_SERVICES if service not in exclude_services + ] + logging.info("Ensuring internal OVS-dependent services are enabled: %s", desired) + _ensure_services_started(snap, desired) def _get_exclude_services(context: dict) -> list[str]: diff --git a/tests/unit/test_hooks.py b/tests/unit/test_hooks.py index cc8ac31..8a02a78 100644 --- a/tests/unit/test_hooks.py +++ b/tests/unit/test_hooks.py @@ -235,6 +235,9 @@ def test_configure_hook( mocker.patch.object(hooks, "_configure_webdav_apache") mocker.patch.object(hooks, "_process_dpdk_ports") mocker.patch.object(hooks, "_is_multipathd_available", return_value=False) + mocker.patch.object(hooks, "_ensure_services_started") + mocker.patch.object(hooks, "_ensure_services_stopped") + mocker.patch.object(hooks, "_restart_services") mocker.patch.object(hooks, "_get_template", return_value=mock_template) mocker.patch.object(hooks, "OVSCli", spec=hooks.OVSCli) mock_write_text = mocker.patch.object(hooks.Path, "write_text") @@ -801,6 +804,8 @@ def test_nova_conf_cpu_pinning_injection( "_configure_ceph", "_configure_masakari_services", "_configure_sriov_agent_service", + "_ensure_services_stopped", + "_restart_services", "_process_dpdk_ports", "_set_sriov_context", "_set_pci_context", @@ -1478,57 +1483,47 @@ def test_exclude_services_internal_ovs(self, mocker, snap): class TestEnsureInternalOVSServices: """Tests for _ensure_internal_ovs_services function.""" - def test_starts_non_excluded_services(self, snap): - services = { - name: service_mock(enabled=False, active=False) for name in hooks.EXTERNAL_OVS_SERVICES - } - snap.services.list.return_value = services + def test_starts_non_excluded_services(self, mocker, snap): + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") snap.config.get.return_value = True # monitoring.enable = True hooks._ensure_internal_ovs_services(snap, exclude_services=["ovsdb-server"]) - services["ovsdb-server"].start.assert_not_called() - services["ovs-vswitchd"].start.assert_called_once_with(enable=True) - services["ovn-controller"].start.assert_called_once_with(enable=True) - services["ovs-exporter"].start.assert_called_once_with(enable=True) + ensure_started.assert_called_once_with( + snap, ["ovs-vswitchd", "ovn-controller", "ovs-exporter"] + ) - def test_does_not_enable_ovs_exporter_when_monitoring_disabled(self, snap): - services = { - name: service_mock(enabled=False, active=False) for name in hooks.EXTERNAL_OVS_SERVICES - } - snap.services.list.return_value = services + def test_does_not_enable_ovs_exporter_when_monitoring_disabled(self, mocker, snap): + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") snap.config.get.return_value = False # monitoring.enable = False hooks._ensure_internal_ovs_services(snap, exclude_services=[]) - services["ovsdb-server"].start.assert_called_once_with(enable=True) - services["ovs-vswitchd"].start.assert_called_once_with(enable=True) - services["ovn-controller"].start.assert_called_once_with(enable=True) - services["ovs-exporter"].start.assert_not_called() + ensure_started.assert_called_once_with( + snap, ["ovsdb-server", "ovs-vswitchd", "ovn-controller"] + ) class TestEnsureInternalOVSDependentServices: """Tests for _ensure_internal_ovs_dependent_services function.""" - def test_starts_non_excluded_services(self, snap): - services = {"neutron-ovn-metadata-agent": service_mock(enabled=False, active=False)} - snap.services.list.return_value = services + def test_starts_non_excluded_services(self, mocker, snap): + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") helper = getattr(hooks, "_ensure_internal_ovs_dependent_services", None) assert helper is not None helper(snap, exclude_services=[]) - services["neutron-ovn-metadata-agent"].start.assert_called_once_with(enable=True) + ensure_started.assert_called_once_with(snap, ["neutron-ovn-metadata-agent"]) - def test_skips_excluded_services(self, snap): - services = {"neutron-ovn-metadata-agent": service_mock(enabled=False, active=False)} - snap.services.list.return_value = services + def test_skips_excluded_services(self, mocker, snap): + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") helper = getattr(hooks, "_ensure_internal_ovs_dependent_services", None) assert helper is not None helper(snap, exclude_services=["neutron-ovn-metadata-agent"]) - services["neutron-ovn-metadata-agent"].start.assert_not_called() + ensure_started.assert_called_once_with(snap, []) class TestInternalOVSReady: @@ -1563,48 +1558,36 @@ class TestConfigureMonitoringServices: def test_external_ovs_monitoring_enabled_skips_ovs_exporter(self, mocker, snap): """ovs-exporter is not started when OVS is external.""" mocker.patch.object(hooks, "is_ovs_external", return_value=True) - services = { - name: service_mock(enabled=False, active=False) for name in hooks.MONITORING_SERVICES - } - snap.services.list.return_value = services + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") snap.config.get.return_value = True # monitoring.enable = True hooks._configure_monitoring_services(snap) - services["libvirt-exporter"].start.assert_called_once_with(enable=True) - services["ovs-exporter"].start.assert_not_called() + ensure_started.assert_called_once_with(snap, ["libvirt-exporter"]) def test_internal_ovs_monitoring_enabled_starts_all(self, mocker, snap): """Test that all exporters are started when OVS is internal and monitoring enabled.""" mocker.patch.object(hooks, "is_ovs_external", return_value=False) - services = { - name: service_mock(enabled=False, active=False) for name in hooks.MONITORING_SERVICES - } - snap.services.list.return_value = services + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") snap.config.get.return_value = True # monitoring.enable = True hooks._configure_monitoring_services(snap) - services["libvirt-exporter"].start.assert_called_once_with(enable=True) - services["ovs-exporter"].start.assert_called_once_with(enable=True) + ensure_started.assert_called_once_with(snap, hooks.MONITORING_SERVICES) def test_monitoring_disabled_stops_all(self, mocker, snap): """Test that all exporters are stopped when monitoring is disabled.""" mocker.patch.object(hooks, "is_ovs_external", return_value=False) - services = { - name: service_mock(enabled=True, active=True) for name in hooks.MONITORING_SERVICES - } - snap.services.list.return_value = services + ensure_stopped = mocker.patch.object(hooks, "_ensure_services_stopped") snap.config.get.return_value = False # monitoring.enable = False hooks._configure_monitoring_services(snap) - services["libvirt-exporter"].stop.assert_called_once_with(disable=True) - services["ovs-exporter"].stop.assert_called_once_with(disable=True) + ensure_stopped.assert_called_once_with(snap, hooks.MONITORING_SERVICES) class TestServiceReconciliation: - """Tests for state-aware individual service reconciliation.""" + """Tests for state-aware batched service reconciliation.""" @pytest.mark.parametrize( "enabled,active,should_start", @@ -1615,15 +1598,18 @@ class TestServiceReconciliation: (True, True, False), ], ) - def test_ensure_service_started(self, enabled, active, should_start): - service = service_mock(enabled=enabled, active=active) + def test_ensure_services_started(self, mocker, snap, enabled, active, should_start): + before = {"service": service_mock(enabled=enabled, active=active)} + snap.services.list.return_value = before + snapctl = mocker.patch.object(hooks, "SnapCtl").return_value - hooks._ensure_service_started(service) + hooks._ensure_services_started(snap, ["service"]) + snap.services.list.assert_called_once_with() if should_start: - service.start.assert_called_once_with(enable=True) + snapctl.start.assert_called_once_with("service", enable=True) else: - service.start.assert_not_called() + snapctl.start.assert_not_called() @pytest.mark.parametrize( "enabled,active,should_stop", @@ -1634,39 +1620,103 @@ def test_ensure_service_started(self, enabled, active, should_start): (True, True, True), ], ) - def test_ensure_service_stopped(self, enabled, active, should_stop): - service = service_mock(enabled=enabled, active=active) + def test_ensure_services_stopped(self, mocker, snap, enabled, active, should_stop): + before = {"service": service_mock(enabled=enabled, active=active)} + snap.services.list.return_value = before + snapctl = mocker.patch.object(hooks, "SnapCtl").return_value - hooks._ensure_service_stopped(service) + hooks._ensure_services_stopped(snap, ["service"]) + snap.services.list.assert_called_once_with() if should_stop: - service.stop.assert_called_once_with(disable=True) + snapctl.stop.assert_called_once_with("service", disable=True) else: - service.stop.assert_not_called() + snapctl.stop.assert_not_called() - def test_masakari_reconciles_enabled_services(self, snap): - services = { - name: service_mock(enabled=False, active=False) for name in hooks.MASAKARI_SERVICES + def test_batches_sorted_unique_non_converged_services(self, mocker, snap): + before = { + "already-ready": service_mock(enabled=True, active=True), + "service-a": service_mock(enabled=False, active=False), + "service-b": service_mock(enabled=True, active=False), } - snap.services.list.return_value = services + snap.services.list.return_value = before + snapctl_class = mocker.patch.object(hooks, "SnapCtl") + + hooks._ensure_services_started( + snap, ["service-b", "already-ready", "service-a", "service-b"] + ) + + snapctl_class.assert_called_once_with(env=snap.environ) + snapctl_class.return_value.start.assert_called_once_with( + "service-a", "service-b", enable=True + ) + + def test_missing_service_fails_before_command(self, mocker, snap): + snap.services.list.return_value = {} + snapctl_class = mocker.patch.object(hooks, "SnapCtl") + + with pytest.raises(RuntimeError, match="missing"): + hooks._ensure_services_started(snap, ["missing"]) + + snapctl_class.assert_not_called() + + def test_command_failure_propagates(self, mocker, snap): + stopped = {"service": service_mock(enabled=False, active=False)} + snap.services.list.return_value = stopped + snapctl = mocker.patch.object(hooks, "SnapCtl").return_value + snapctl.start.side_effect = RuntimeError("command failed") + + with pytest.raises(RuntimeError, match="command failed"): + hooks._ensure_services_started(snap, ["service"]) + + def test_restart_services_batches_and_verifies(self, caplog, mocker, snap): + before = { + "service-a": service_mock(enabled=True, active=True), + "service-b": service_mock(enabled=True, active=True), + } + snap.services.list.return_value = before + snapctl = mocker.patch.object(hooks, "SnapCtl").return_value + + hooks._restart_services(snap, ["service-b", "service-a", "service-b"]) + + snap.services.list.assert_called_once_with() + snapctl.stop.assert_called_once_with("service-a", "service-b") + snapctl.start.assert_called_once_with("service-a", "service-b", enable=True) + assert [ + record.getMessage() + for record in caplog.records + if record.getMessage().startswith("Restarting ") + ] == ["Restarting service-a", "Restarting service-b"] + + def test_restart_services_ignores_empty_input(self, mocker, snap): + snapctl_class = mocker.patch.object(hooks, "SnapCtl") + + hooks._restart_services(snap, []) + + snap.services.list.assert_not_called() + snapctl_class.assert_not_called() + + def test_masakari_reconciles_enabled_services(self, mocker, snap): + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") snap.config.get.return_value = True hooks._configure_masakari_services(snap) - for service in services.values(): - service.start.assert_called_once_with(enable=True) + ensure_started.assert_called_once_with(snap, hooks.MASAKARI_SERVICES) @pytest.mark.parametrize("enabled", [False, True]) - def test_sriov_reconciles_service(self, snap, enabled): - service = service_mock(enabled=not enabled, active=not enabled) - snap.services.list.return_value = {"neutron-sriov-nic-agent": service} + def test_sriov_reconciles_service(self, mocker, snap, enabled): + ensure_started = mocker.patch.object(hooks, "_ensure_services_started") + ensure_stopped = mocker.patch.object(hooks, "_ensure_services_stopped") hooks._configure_sriov_agent_service(snap, enabled) if enabled: - service.start.assert_called_once_with(enable=True) + ensure_started.assert_called_once_with(snap, ["neutron-sriov-nic-agent"]) + ensure_stopped.assert_not_called() else: - service.stop.assert_called_once_with(disable=True) + ensure_stopped.assert_called_once_with(snap, ["neutron-sriov-nic-agent"]) + ensure_started.assert_not_called() class TestRestartOnChange: @@ -1682,18 +1732,16 @@ def test_unchanged_files_do_not_list_services(self, snap): snap.services.list.assert_not_called() - def test_changed_file_restarts_service(self, snap): + def test_changed_file_restarts_service(self, mocker, snap): config = snap.paths.common / "service.conf" config.parent.mkdir(parents=True) config.write_text("before") - service = mock.Mock() - snap.services.list.return_value = {"service": service} + restart_services = mocker.patch.object(hooks, "_restart_services") with hooks.RestartOnChange(snap, {Path("service.conf"): {"services": ["service"]}}): config.write_text("after") - service.stop.assert_called_once_with() - service.start.assert_called_once_with(enable=True) + restart_services.assert_called_once_with(snap, {"service"}) class TestConfigureTLS: @@ -1778,6 +1826,7 @@ def test_internal_ovs_not_ready_defers_ovs_configuration(self, mocker, snap): mocker.patch.object(hooks, "_detect_compute_flavors") mocker.patch.object(hooks, "_get_configure_context", return_value={"network": {}}) mocker.patch.object(hooks, "_get_exclude_services", return_value=["svc1"]) + ensure_stopped = mocker.patch.object(hooks, "_ensure_services_stopped") mocker.patch.object(hooks, "OVSCli", return_value=mock.Mock()) mocker.patch.object(hooks, "is_ovs_external", return_value=False) mocker.patch.object(hooks, "_internal_ovs_ready", return_value=False) @@ -1812,7 +1861,7 @@ def test_internal_ovs_not_ready_defers_ovs_configuration(self, mocker, snap): hooks.configure(snap) - services["svc1"].stop.assert_called_once_with(disable=True) + ensure_stopped.assert_called_once_with(snap, ["svc1"]) assert order == ["tls", "ensure", "metadata"] def test_internal_ovs_ready_runs_configuration(self, mocker, snap):