From f7a5a1f7249efd00256f74105b2580e32470bb50 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 31 May 2022 16:18:41 -0400 Subject: [PATCH 01/41] Hacking. --- update-server/otupdate/buildroot/__init__.py | 82 +++-- update-server/otupdate/buildroot/__main__.py | 17 +- update-server/otupdate/buildroot/constants.py | 3 - update-server/otupdate/common/constants.py | 3 - update-server/otupdate/common/control.py | 5 +- .../common/name_management/__init__.py | 29 +- .../otupdate/common/name_management/avahi.py | 286 +++++++++++------- .../common/name_management/name_manager.py | 77 +++++ .../otupdate/openembedded/__init__.py | 3 +- .../tests/buildroot/test_name_endpoints.py | 51 ---- .../name_management/test_http_endpoints.py | 50 +++ 11 files changed, 378 insertions(+), 228 deletions(-) create mode 100644 update-server/otupdate/common/name_management/name_manager.py delete mode 100644 update-server/tests/buildroot/test_name_endpoints.py create mode 100644 update-server/tests/common/name_management/test_http_endpoints.py diff --git a/update-server/otupdate/buildroot/__init__.py b/update-server/otupdate/buildroot/__init__.py index 3e30681f4e7..10ff3de35e6 100644 --- a/update-server/otupdate/buildroot/__init__.py +++ b/update-server/otupdate/buildroot/__init__.py @@ -1,8 +1,9 @@ """ update-server implementation for buildroot systems """ import asyncio import logging +import textwrap import json -from typing import Any, Mapping, Optional +from typing import Any, AsyncGenerator, Mapping, Optional from aiohttp import web from otupdate.common import ( @@ -12,6 +13,7 @@ name_management, constants, update, + systemd, ) from . import update_actions @@ -52,40 +54,60 @@ def get_app( system_version_file = BR_BUILTIN_VERSION_FILE version = get_version(system_version_file) - name = name_override or name_management.get_pretty_hostname() boot_id = boot_id_override or control.get_boot_id() config_obj = config.load(config_file_override) - LOG.info( - "Setup: " - + "\n\t".join( - [ - f"Device name: {name}", - "Buildroot version: " - f'{version.get("buildroot_version", "unknown")}', - "\t(from git sha " f'{version.get("buildroot_sha", "unknown")}', - "API version: " - f'{version.get("opentrons_api_version", "unknown")}', - "\t(from git sha " - f'{version.get("opentrons_api_sha", "unknown")}', - "Update server version: " - f'{version.get("update_server_version", "unknown")}', - "\t(from git sha " - f'{version.get("update_server_sha", "unknown")}', - "Smoothie firmware version: TODO", - ] - ) - ) + app = web.Application(middlewares=[log_error_middleware]) - if not loop: - loop = asyncio.get_event_loop() + async def _setup_and_cleanup_ctx( + app: web.Application + ) -> AsyncGenerator[None, None]: + # Stuff everything inside here so that: + # - Getting the order right is more foolproof + # - We can log it all together + + # FIX BEFORE MERGE: Do name override. + + app[config.CONFIG_VARNAME] = config_obj + app[constants.RESTART_LOCK_NAME] = asyncio.Lock() + app[constants.DEVICE_BOOT_ID_NAME] = boot_id + + update_actions.OT2UpdateActions.build_and_insert(app) + + async with name_management.build_and_insert(app): + name = name_management.NameManager.from_app(app).get_name() + LOG.info( + "Setup: " + + "\n\t".join( + [ + f"Device name: {name}", + "Buildroot version: " + f'{version.get("buildroot_version", "unknown")}', + "\t(from git sha " f'{version.get("buildroot_sha", "unknown")}', + "API version: " + f'{version.get("opentrons_api_version", "unknown")}', + "\t(from git sha " + f'{version.get("opentrons_api_sha", "unknown")}', + "Update server version: " + f'{version.get("update_server_version", "unknown")}', + "\t(from git sha " + f'{version.get("update_server_sha", "unknown")}', + "Smoothie firmware version: TODO", + ] + ) + ) + + LOG.info(f"Notifying {systemd.SOURCE} that service is up.") + systemd.notify_up() + + LOG.info(f"Serving requests.") + yield + LOG.info("Running teardown code.") + + LOG.info("Done running teardown code.") + + app.cleanup_ctx.append(_setup_and_cleanup_ctx) - app = web.Application(middlewares=[log_error_middleware]) - app[config.CONFIG_VARNAME] = config_obj - app[constants.RESTART_LOCK_NAME] = asyncio.Lock() - app[constants.DEVICE_BOOT_ID_NAME] = boot_id - app[constants.DEVICE_NAME_VARNAME] = name - update_actions.OT2UpdateActions.build_and_insert(app) app.router.add_routes( [ web.get( diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index 1b4e2d6784b..19f841d58d6 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -18,24 +18,15 @@ def main() -> None: loop = asyncio.get_event_loop() systemd.configure_logging(getattr(logging, args.log_level.upper())) + # Because this involves restarting Avahi, this must happen early, + # before the NameManager starts up and connects to Avahi. LOG.info("Setting static hostname") - hostname = loop.run_until_complete(name_management.set_up_static_hostname()) - LOG.info(f"Set static hostname to {hostname}") + static_hostname = loop.run_until_complete(name_management.set_up_static_hostname()) + LOG.info(f"Set static hostname to {static_hostname}") LOG.info("Building buildroot update server") app = get_app(args.version_file, args.config_file) - name = app[constants.DEVICE_NAME_VARNAME] # Set by get_app(). - - LOG.info(f"Setting Avahi service name to {name}") - try: - loop.run_until_complete(name_management.set_avahi_service_name(name)) - except Exception: - LOG.exception( - "Error setting the Avahi service name." - " mDNS + DNS-SD advertisement may not work." - ) - LOG.info(f"Notifying {systemd.SOURCE} that service is up") systemd.notify_up() diff --git a/update-server/otupdate/buildroot/constants.py b/update-server/otupdate/buildroot/constants.py index ab6bf6b6c88..33783d32769 100644 --- a/update-server/otupdate/buildroot/constants.py +++ b/update-server/otupdate/buildroot/constants.py @@ -6,9 +6,6 @@ RESTART_LOCK_NAME = APP_VARIABLE_PREFIX + "restartlock" #: Name for the asyncio lock in the application dictlike -DEVICE_NAME_VARNAME = APP_VARIABLE_PREFIX + "name" -#: Name for the device - DEVICE_BOOT_ID_NAME = APP_VARIABLE_PREFIX + "boot_id" #: A random string that changes every time the device boots. #: diff --git a/update-server/otupdate/common/constants.py b/update-server/otupdate/common/constants.py index cb5a27ee7d0..8718ae54f72 100644 --- a/update-server/otupdate/common/constants.py +++ b/update-server/otupdate/common/constants.py @@ -6,9 +6,6 @@ RESTART_LOCK_NAME = APP_VARIABLE_PREFIX + "restartlock" #: Name for the asyncio lock in the application dictlike -DEVICE_NAME_VARNAME = APP_VARIABLE_PREFIX + "name" -#: Name for the device - DEVICE_BOOT_ID_NAME = APP_VARIABLE_PREFIX + "boot_id" #: A random string that changes every time the device boots. #: diff --git a/update-server/otupdate/common/control.py b/update-server/otupdate/common/control.py index 29faf7fd8a1..73cf324aeb7 100644 --- a/update-server/otupdate/common/control.py +++ b/update-server/otupdate/common/control.py @@ -12,7 +12,8 @@ from aiohttp import web -from .constants import RESTART_LOCK_NAME, DEVICE_BOOT_ID_NAME, DEVICE_NAME_VARNAME +from .constants import RESTART_LOCK_NAME, DEVICE_BOOT_ID_NAME +from .name_management import NameManager LOG = logging.getLogger(__name__) @@ -41,7 +42,7 @@ async def health(request: web.Request) -> web.Response: { **health_response, **{ - "name": request.app[DEVICE_NAME_VARNAME], + "name": NameManager.from_request(request).get_name(), "serialNumber": get_serial(), "bootId": request.app[DEVICE_BOOT_ID_NAME], }, diff --git a/update-server/otupdate/common/name_management/__init__.py b/update-server/otupdate/common/name_management/__init__.py index 60e1d7b14bc..7ef56d92035 100644 --- a/update-server/otupdate/common/name_management/__init__.py +++ b/update-server/otupdate/common/name_management/__init__.py @@ -41,30 +41,16 @@ See `set_name_endpoint()`. """ +from __future__ import annotations import json from aiohttp import web -from ..constants import DEVICE_NAME_VARNAME - -from .avahi import set_avahi_service_name -from .pretty_hostname import ( - get_pretty_hostname, - persist_pretty_hostname, -) +from .name_manager import NameManager, build_and_insert from .static_hostname import set_up_static_hostname -async def set_name(app: web.Application, new_name: str) -> str: - """See `set_name_endpoint()`.""" - await set_avahi_service_name(new_name) - # Setting the Avahi service name can fail if Avahi doesn't like the new name. - # Persist only after it succeeds, so we don't persist something invalid. - persisted_pretty_hostname = await persist_pretty_hostname(new_name) - return persisted_pretty_hostname - - async def set_name_endpoint(request: web.Request) -> web.Response: """Set the robot's name. @@ -104,9 +90,9 @@ def build_400(msg: str) -> web.Response: if not isinstance(name_to_set, str): return build_400('"name" key is not a string"') - new_name = await set_name(app=request.app, new_name=name_to_set) + name_manager = NameManager.from_request(request) + new_name = await name_manager.set_name(new_name=name_to_set) - request.app[DEVICE_NAME_VARNAME] = new_name return web.json_response( # type: ignore[no-untyped-call,no-any-return] data={"name": new_name}, status=200 ) @@ -120,15 +106,16 @@ async def get_name_endpoint(request: web.Request) -> web.Response: GET /server/name -> 200 OK, {'name': robot name} """ + name_manager = NameManager.from_request(request) return web.json_response( # type: ignore[no-untyped-call,no-any-return] - data={"name": request.app[DEVICE_NAME_VARNAME]}, status=200 + data={"name": name_manager.get_name()}, status=200 ) __all__ = [ - "get_pretty_hostname", + "NameManager", + "build_and_insert", "set_up_static_hostname", - "set_avahi_service_name", "get_name_endpoint", "set_name_endpoint", ] diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 677c87ebc66..0339d66fb7f 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -1,47 +1,125 @@ """Control the Avahi daemon.""" +from __future__ import annotations + +import abc import asyncio -from logging import getLogger -from typing import Optional +import contextlib +import logging +from typing import AsyncGenerator, Awaitable, Callable, Optional, cast -_log = getLogger(__name__) +_COLLISION_POLL_INTERVAL = 5 -try: - import dbus +_log = logging.getLogger(__name__) - class DBusState: - """Bundle of state for dbus""" - def __init__( - self, - bus: dbus.SystemBus, - server: dbus.Interface, - entrygroup: dbus.Interface, - ) -> None: - """ - Build the state bundle. +async def restart_daemon() -> None: + proc = await asyncio.create_subprocess_exec( + "systemctl", + "restart", + "avahi-daemon", + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await proc.communicate() + ret = proc.returncode + if ret != 0: + _log.error( + f"Error restarting avahi-daemon: {ret} " + f"stdout: {stdout!r} stderr: {stderr!r}" + ) + raise RuntimeError("Error restarting avahi") - :param bus: The system bus instance - :param server: An org.freedesktop.Avahi.Server interface - :param entrygroup: An org.freedesktop.Avahi.EntryGroup interface - """ - self.bus = bus - #: The system bus - self.entrygroup = entrygroup - #: The entry group interface - self.server = server - #: The avahi server interface - _BUS_STATE: Optional[DBusState] = None +class AvahiClient: + def __init__(self, sync_client: _SyncClient) -> None: + """For internal use by this class only. Use `connect()` instead.""" + self._sync_client = sync_client + self._lock = asyncio.Lock() + + @classmethod + async def connect(cls) -> AvahiClient: + # TODO: Handle case when dbus isn't available. + sync_client = await asyncio.get_running_loop().run_in_executor( + executor=None, + func=_SyncClient.connect, + ) + return cls(sync_client=sync_client) - def _set_avahi_service_name_sync(new_service_name: str) -> None: - """The synchronous implementation of setting the Avahi service name. + async def start_advertising(self, service_name: str) -> None: + """ + To document: + * Picks up current static hostname and domain + * Replaces an existing advertisement if there is one + * Advertisement will automatically stop upon collision + """ + async with self._lock: + await asyncio.get_running_loop().run_in_executor( + None, self._sync_client.start_advertising, service_name + ) + + async def alternative_service_name(self, current_service_name: str) -> str: + async with self._lock: + return await asyncio.get_running_loop().run_in_executor( + None, self._sync_client.alternative_service_name, current_service_name + ) + # Also document why we ask Avahi to do this for us. + # Test whether Avahi gracefully handles overlong names. - The dbus module doesn't natively support async/await. + @contextlib.asynccontextmanager + async def collision_callback( + self, callback: CollisionCallback + ) -> AsyncGenerator[None, None]: """ + To document: + * Potentially duplicate collision events if resolving a collision takes too long + * May cancel callback when this exits + """ + + async def _poll_and_call_back() -> None: + _log.info("Beginning polling.") + while True: + if await self._is_collided(): + await callback() # TODO: Log exception? + await asyncio.sleep(_COLLISION_POLL_INTERVAL) + + background_task = asyncio.create_task(_poll_and_call_back()) + + try: + yield + + finally: + background_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await background_task + + async def _is_collided(self) -> bool: + async with self._lock: + return await asyncio.get_running_loop().run_in_executor( + None, self._sync_client.is_collided + ) + + +CollisionCallback = Callable[[], Awaitable[None]] + + +try: + import dbus +except ImportError: + _avahi_available = False +else: + _avahi_available = True + + class _SyncClient: + """ + To document: + Does I/O and is not thread-safe. + dbus module doesn't natively support async/await. + """ + # For semantics of the methods we're calling, see Avahi's API docs. # For example: https://www.avahi.org/doxygen/html/index.html#good_publish # It's mostly in terms of the C API, but the semantics should be the same. @@ -49,91 +127,91 @@ def _set_avahi_service_name_sync(new_service_name: str) -> None: # For exact method names and argument types, see Avahi's D-Bus bindings, # which they specify across several machine-readable files. For example: # https://github.com/lathiat/avahi/blob/v0.7/avahi-daemon/org.freedesktop.Avahi.EntryGroup.xml - global _BUS_STATE - if not _BUS_STATE: + def __init__( + self, + bus: dbus.SystemBus, + server: dbus.Interface, + entry_group: dbus.Interface, + ) -> None: + """For internal use by this class only. Use `connect()` instead. + + Args: + bus: The system bus instance. + server: An org.freedesktop.Avahi.Server interface. + entry_group: An org.freedesktop.Avahi.EntryGroup interface. + """ + self._bus = bus + self._server = server + self._entry_group = entry_group + + @classmethod + def connect(cls) -> _SyncClient: + _log.info("Connecting to Avahi daemon.") + bus = dbus.SystemBus() server_obj = bus.get_object("org.freedesktop.Avahi", "/") server_if = dbus.Interface(server_obj, "org.freedesktop.Avahi.Server") - entrygroup_path = server_if.EntryGroupNew() - entrygroup_obj = bus.get_object("org.freedesktop.Avahi", entrygroup_path) - entrygroup_if = dbus.Interface( - entrygroup_obj, "org.freedesktop.Avahi.EntryGroup" + entry_group_path = server_if.EntryGroupNew() + entry_group_obj = bus.get_object("org.freedesktop.Avahi", entry_group_path) + entry_group_if = dbus.Interface( + entry_group_obj, "org.freedesktop.Avahi.EntryGroup" + ) + return cls( + bus=bus, + server=server_if, + entry_group=entry_group_if, ) - _BUS_STATE = DBusState(bus, server_if, entrygroup_if) - - _BUS_STATE.entrygroup.Reset() - - hostname = _BUS_STATE.server.GetHostName() - domainname = _BUS_STATE.server.GetDomainName() - - # TODO(mm, 2022-05-06): This isn't exception-safe. - # Since we've already reset the entrygroup, if this fails - # (for example because Avahi doesn't like the new name), - # we'll be left with no entrygroup and Avahi will stop advertising the machine. - _BUS_STATE.entrygroup.AddService( - dbus.Int32(-1), # avahi.IF_UNSPEC - dbus.Int32(-1), # avahi.PROTO_UNSPEC - dbus.UInt32(0), # flags - new_service_name, # sname - "_http._tcp", # stype - domainname, # sdomain (.local) - f"{hostname}.{domainname}", # shost (hostname.local) - dbus.UInt16(31950), # port - dbus.Array([], signature="ay"), - ) - _BUS_STATE.entrygroup.Commit() - - # TODO(mm, 2022-05-04): Recover from AVAHI_ENTRY_GROUP_COLLISION in case - # this name collides with another device on the network. - # https://github.com/Opentrons/opentrons/issues/10126 - -except ImportError: - _log.exception("Couldn't import dbus, name setting will be nonfunctional") - - def _set_avahi_service_name_sync(new_service_name: str) -> None: - _log.warning("Not setting name, dbus could not be imported") - -_BUS_LOCK = asyncio.Lock() + def start_advertising(self, service_name: str) -> None: + _log.info(f"Starting advertising with name {service_name}.") + # TODO(mm, 2022-05-26): Can we leave these as the empty string? + # The avahi_entry_group_add_service() C API recommends passing NULL + # to let the daemon decide these values. + hostname = self._server.GetHostName() + domainname = self._server.GetDomainName() + + self._entry_group.Reset() + _log.info(f"Reset entry group.") + + # TODO(mm, 2022-05-06): This isn't exception-safe. + # We've already reset the entry group, so if this fails + # (for example because Avahi doesn't like the new name), + # we'll be left with no entry group, + # and Avahi will stop advertising the machine. + self._entry_group.AddService( + dbus.Int32(-1), # avahi.IF_UNSPEC + dbus.Int32(-1), # avahi.PROTO_UNSPEC + dbus.UInt32(0), # flags + service_name, # sname + "_http._tcp", # stype + domainname, # sdomain (.local) + f"{hostname}.{domainname}", # shost (hostname.local) + dbus.UInt16(31950), # port + dbus.Array([], signature="ay"), + ) + _log.info(f"Added service with {hostname} {domainname}.") -async def set_avahi_service_name(new_service_name: str) -> None: - """Set the Avahi service name. + self._entry_group.Commit() - See the `name_management` package docstring for background on the service name - and how it's distinct from other names on the machine. + _log.info(f"Committed.") - The new service name will only apply to the current boot. + def alternative_service_name(self, current_service_name: str) -> str: + result = self._server.GetAlternativeServiceName(current_service_name) + assert isinstance(result, str) + return result - Avahi requires a service name. - It will not advertise the system over mDNS + DNS-SD until one is set. + def is_collided(self) -> bool: + """ + To document: + Will be the case if another device on the network has + the same service name or host. + """ - Since the Avahi service name corresponds to the DNS-SD instance name, - it's a human-readable string of mostly arbitrary Unicode, - at most 63 octets (not 63 code points or 63 characters!) long. - (See: https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1) - Avahi will raise an exception through this function if it thinks - the new service name is invalid. - """ - async with _BUS_LOCK: - await asyncio.get_event_loop().run_in_executor( - None, _set_avahi_service_name_sync, new_service_name - ) + state = self._entry_group.GetState() + # The value 3 comes from: + # https://github.com/lathiat/avahi/blob/v0.8/avahi-common/defs.h#L234 + avahi_entry_group_collision = dbus.Int32(3) -async def restart_daemon() -> None: - proc = await asyncio.create_subprocess_exec( - "systemctl", - "restart", - "avahi-daemon", - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - ) - stdout, stderr = await proc.communicate() - ret = proc.returncode - if ret != 0: - _log.error( - f"Error restarting avahi-daemon: {ret} " - f"stdout: {stdout!r} stderr: {stderr!r}" - ) - raise RuntimeError("Error restarting avahi") + return cast(bool, state == avahi_entry_group_collision) diff --git a/update-server/otupdate/common/name_management/name_manager.py b/update-server/otupdate/common/name_management/name_manager.py new file mode 100644 index 00000000000..4a0e071d3ea --- /dev/null +++ b/update-server/otupdate/common/name_management/name_manager.py @@ -0,0 +1,77 @@ +from __future__ import annotations + +from contextlib import asynccontextmanager +from logging import getLogger +from typing import AsyncGenerator + +from aiohttp import web + +from otupdate.common.constants import APP_VARIABLE_PREFIX +from .avahi import AvahiClient +from .pretty_hostname import get_pretty_hostname, persist_pretty_hostname + + +_NAME_MANAGER_VARNAME = APP_VARIABLE_PREFIX + "name_manager" +_log = getLogger(__name__) + + +class NameManager: + def __init__(self, avahi_client: AvahiClient) -> None: + """For internal use by this class only.""" + self._avahi_client = avahi_client + + @classmethod + @asynccontextmanager + async def build( + cls, avahi_client: AvahiClient + ) -> AsyncGenerator[NameManager, None]: + name_manager = NameManager(avahi_client) + async with avahi_client.collision_callback(name_manager._on_avahi_collision): + await avahi_client.start_advertising(service_name=name_manager.get_name()) + yield name_manager + + @classmethod + def from_request(cls, request: web.Request) -> NameManager: + return cls.from_app(request.app) + + @staticmethod + def from_app(app: web.Application) -> NameManager: + name_manager = app.get(_NAME_MANAGER_VARNAME, None) + assert isinstance( + name_manager, NameManager + ), f"Unexpected type {type(name_manager)}. Incorrect Application setup?" + return name_manager + + async def set_name(self, new_name: str) -> str: + """See `set_name_endpoint()`.""" + await self._avahi_client.start_advertising(service_name=new_name) + # Setting the Avahi service name can fail if Avahi doesn't like the new name. + # Persist only after it succeeds, so we don't persist something invalid. + persisted_pretty_hostname = await persist_pretty_hostname(new_name) + return persisted_pretty_hostname + + def get_name(self) -> str: + return get_pretty_hostname() + + async def _on_avahi_collision(self) -> None: + current_name = self.get_name() + # Assume that the service name was the thing that collided. + # Theoretically it also could have been the static hostname, + # but our static hostnames are unique in practice, so that's unlikely. + + # TODO: Is this some kind of race condition? + alternative_name = await self._avahi_client.alternative_service_name( + current_name + ) + _log.info( + f"Name collision detected by Avahi. Changing name to {alternative_name}." + ) + await self.set_name(new_name=alternative_name) + + +@asynccontextmanager +async def build_and_insert(app: web.Application) -> AsyncGenerator[None, None]: + avahi_client = await AvahiClient.connect() + async with NameManager.build(avahi_client=avahi_client) as name_manager: + app[_NAME_MANAGER_VARNAME] = name_manager + yield diff --git a/update-server/otupdate/openembedded/__init__.py b/update-server/otupdate/openembedded/__init__.py index 7700783fa1b..82b4fa3a7ed 100644 --- a/update-server/otupdate/openembedded/__init__.py +++ b/update-server/otupdate/openembedded/__init__.py @@ -67,7 +67,8 @@ def get_app( app[config.CONFIG_VARNAME] = config_obj app[constants.RESTART_LOCK_NAME] = asyncio.Lock() app[constants.DEVICE_BOOT_ID_NAME] = boot_id - app[constants.DEVICE_NAME_VARNAME] = name + # FIX BEFORE MERGE + # app[constants.DEVICE_NAME_VARNAME] = name LOG.info( "Setup: " diff --git a/update-server/tests/buildroot/test_name_endpoints.py b/update-server/tests/buildroot/test_name_endpoints.py deleted file mode 100644 index f845ad3cd9a..00000000000 --- a/update-server/tests/buildroot/test_name_endpoints.py +++ /dev/null @@ -1,51 +0,0 @@ -from otupdate.common import name_management - - -async def test_name_endpoint(test_cli, monkeypatch): - async def fake_set_name(app, new_name): - return new_name + new_name - - monkeypatch.setattr(name_management, "set_name", fake_set_name) - - # Valid: - to_set = "check out this cool name" - resp = await test_cli.post("/server/name", json={"name": to_set}) - assert resp.status == 200 - body = await resp.json() - assert body["name"] == to_set + to_set - health = await test_cli.get("/server/update/health") - health_body = await health.json() - assert health_body["name"] == to_set + to_set - get_name = await test_cli.get("/server/name") - name_body = await get_name.json() - assert name_body["name"] == to_set + to_set - - # Invalid, field is not a str: - resp = await test_cli.post("/server/name", json={"name": 2}) - assert resp.status == 400 - body = await resp.json() - assert "message" in body - health = await test_cli.get("/server/update/health") - health_body = await health.json() - assert health_body["name"] == to_set + to_set - get_name = await test_cli.get("/server/name") - name_body = await get_name.json() - assert name_body["name"] == to_set + to_set - - # Invalid, field is missing: - resp = await test_cli.post("/server/name", json={}) - assert resp.status == 400 - body = await resp.json() - assert "message" in body - health = await test_cli.get("/server/update/health") - health_body = await health.json() - assert health_body["name"] == to_set + to_set - - # Invalid, body is not JSON: - resp = await test_cli.post("/server/name", data="bada bing bada boom") - assert resp.status == 400 - body = await resp.json() - assert "message" in body - health = await test_cli.get("/server/update/health") - health_body = await health.json() - assert health_body["name"] == to_set + to_set diff --git a/update-server/tests/common/name_management/test_http_endpoints.py b/update-server/tests/common/name_management/test_http_endpoints.py new file mode 100644 index 00000000000..b56a00201ea --- /dev/null +++ b/update-server/tests/common/name_management/test_http_endpoints.py @@ -0,0 +1,50 @@ +import asyncio +from unittest.mock import MagicMock + +import pytest +from aiohttp import web + +from otupdate.common.name_management import NameManager + + +@pytest.fixture +def mock_name_manager(): + return MagicMock(spec=NameManager) + + +async def test_get_name(test_cli, monkeypatch, mock_name_manager) -> None: + mock_name_manager.get_name.return_value = "the returned name" + monkeypatch.setattr(NameManager, "from_request", lambda request: mock_name_manager) + + response = await (test_cli[0].get("/server/name")) + assert response.status == 200 + body = await response.json() + assert body["name"] == "the returned name" + + +async def test_set_name_valid(test_cli, monkeypatch, mock_name_manager) -> None: + async def mock_set_name(new_name: str) -> str: + return "the returned name" + + mock_name_manager.set_name = mock_set_name + monkeypatch.setattr(NameManager, "from_request", lambda request: mock_name_manager) + + response = await test_cli[0].post("/server/name", json={"name": "the input name"}) + assert response.status == 200 + body = await response.json() + assert body["name"] == "the returned name" + + +async def test_set_name_not_json(test_cli) -> None: + response = await test_cli[0].post("/server/name", data="bada bing bada boom") + assert response.status == 400 + + +async def test_set_name_field_missing(test_cli) -> None: + response = await test_cli[0].post("/server/name", json={}) + assert response.status == 400 + + +async def test_set_name_field_not_a_str(test_cli) -> None: + response = await test_cli[0].post("/server/name", json={"name": 2}) + assert response.status == 400 From 19fe6c2d08ecc37fb5407b21926d1319e448f0a1 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 00:49:33 -0400 Subject: [PATCH 02/41] Lint. --- update-server/otupdate/buildroot/__init__.py | 35 +++++++++---------- update-server/otupdate/buildroot/__main__.py | 2 +- .../otupdate/common/name_management/avahi.py | 7 ++-- .../otupdate/openembedded/__init__.py | 5 +-- .../name_management/test_http_endpoints.py | 2 -- 5 files changed, 23 insertions(+), 28 deletions(-) diff --git a/update-server/otupdate/buildroot/__init__.py b/update-server/otupdate/buildroot/__init__.py index 10ff3de35e6..76957c8808d 100644 --- a/update-server/otupdate/buildroot/__init__.py +++ b/update-server/otupdate/buildroot/__init__.py @@ -1,19 +1,18 @@ """ update-server implementation for buildroot systems """ import asyncio import logging -import textwrap import json from typing import Any, AsyncGenerator, Mapping, Optional from aiohttp import web from otupdate.common import ( config, + constants, control, - ssh_key_management, name_management, - constants, - update, + ssh_key_management, systemd, + update, ) from . import update_actions @@ -44,7 +43,7 @@ def get_app( config_file_override: Optional[str] = None, name_override: Optional[str] = None, boot_id_override: Optional[str] = None, - loop: Optional[asyncio.AbstractEventLoop] = None, + loop: Optional[asyncio.AbstractEventLoop] = None, # TODO: Remove? ) -> web.Application: """Build and return the aiohttp.web.Application that runs the server @@ -59,31 +58,32 @@ def get_app( app = web.Application(middlewares=[log_error_middleware]) - async def _setup_and_cleanup_ctx( - app: web.Application - ) -> AsyncGenerator[None, None]: + async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, None]: # Stuff everything inside here so that: # - Getting the order right is more foolproof # - We can log it all together - # FIX BEFORE MERGE: Do name override. - app[config.CONFIG_VARNAME] = config_obj app[constants.RESTART_LOCK_NAME] = asyncio.Lock() app[constants.DEVICE_BOOT_ID_NAME] = boot_id update_actions.OT2UpdateActions.build_and_insert(app) - async with name_management.build_and_insert(app): - name = name_management.NameManager.from_app(app).get_name() + async with name_management.build_name_manager( + name_override=name_override + ) as name_manager: + name_manager.install_on_app(app) + initial_name = name_manager.get_name() + LOG.info( "Setup: " + "\n\t".join( [ - f"Device name: {name}", + f"Device name: {initial_name}", "Buildroot version: " f'{version.get("buildroot_version", "unknown")}', - "\t(from git sha " f'{version.get("buildroot_sha", "unknown")}', + "\t(from git sha " + f'{version.get("buildroot_sha", "unknown")}', "API version: " f'{version.get("opentrons_api_version", "unknown")}', "\t(from git sha " @@ -100,13 +100,9 @@ async def _setup_and_cleanup_ctx( LOG.info(f"Notifying {systemd.SOURCE} that service is up.") systemd.notify_up() - LOG.info(f"Serving requests.") yield - LOG.info("Running teardown code.") - LOG.info("Done running teardown code.") - - app.cleanup_ctx.append(_setup_and_cleanup_ctx) + app.cleanup_ctx.append(set_up_and_tear_down) app.router.add_routes( [ @@ -128,6 +124,7 @@ async def _setup_and_cleanup_ctx( web.get("/server/name", name_management.get_name_endpoint), ] ) + return app diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index 19f841d58d6..df94ff9feb2 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -4,7 +4,7 @@ import asyncio import logging -from . import get_app, constants +from . import get_app from otupdate.common import name_management, cli, systemd from aiohttp import web diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 0339d66fb7f..b1c93acb0ba 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -3,11 +3,10 @@ from __future__ import annotations -import abc import asyncio import contextlib import logging -from typing import AsyncGenerator, Awaitable, Callable, Optional, cast +from typing import AsyncGenerator, Awaitable, Callable, cast _COLLISION_POLL_INTERVAL = 5 @@ -171,7 +170,7 @@ def start_advertising(self, service_name: str) -> None: domainname = self._server.GetDomainName() self._entry_group.Reset() - _log.info(f"Reset entry group.") + _log.info("Reset entry group.") # TODO(mm, 2022-05-06): This isn't exception-safe. # We've already reset the entry group, so if this fails @@ -194,7 +193,7 @@ def start_advertising(self, service_name: str) -> None: self._entry_group.Commit() - _log.info(f"Committed.") + _log.info("Committed.") def alternative_service_name(self, current_service_name: str) -> str: result = self._server.GetAlternativeServiceName(current_service_name) diff --git a/update-server/otupdate/openembedded/__init__.py b/update-server/otupdate/openembedded/__init__.py index 82b4fa3a7ed..a1bdc36c9f0 100644 --- a/update-server/otupdate/openembedded/__init__.py +++ b/update-server/otupdate/openembedded/__init__.py @@ -3,14 +3,15 @@ import logging import json from aiohttp import web -from typing import Optional, Mapping, Any +from typing import AsyncGenerator, Optional, Mapping, Any from otupdate.common import ( config, constants, control, - ssh_key_management, name_management, + ssh_key_management, + systemd, update, ) diff --git a/update-server/tests/common/name_management/test_http_endpoints.py b/update-server/tests/common/name_management/test_http_endpoints.py index b56a00201ea..a6f914765c4 100644 --- a/update-server/tests/common/name_management/test_http_endpoints.py +++ b/update-server/tests/common/name_management/test_http_endpoints.py @@ -1,8 +1,6 @@ -import asyncio from unittest.mock import MagicMock import pytest -from aiohttp import web from otupdate.common.name_management import NameManager From 722475fc77e9fe162168d55c81c4a98be60016c7 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 00:57:13 -0400 Subject: [PATCH 03/41] Delete unused `loop` arguments. --- update-server/otupdate/buildroot/__init__.py | 1 - update-server/otupdate/openembedded/__init__.py | 5 ----- update-server/tests/buildroot/conftest.py | 7 ++----- update-server/tests/common/conftest.py | 5 ++--- update-server/tests/openembedded/conftest.py | 7 ++----- 5 files changed, 6 insertions(+), 19 deletions(-) diff --git a/update-server/otupdate/buildroot/__init__.py b/update-server/otupdate/buildroot/__init__.py index 76957c8808d..e2e6512ebb4 100644 --- a/update-server/otupdate/buildroot/__init__.py +++ b/update-server/otupdate/buildroot/__init__.py @@ -43,7 +43,6 @@ def get_app( config_file_override: Optional[str] = None, name_override: Optional[str] = None, boot_id_override: Optional[str] = None, - loop: Optional[asyncio.AbstractEventLoop] = None, # TODO: Remove? ) -> web.Application: """Build and return the aiohttp.web.Application that runs the server diff --git a/update-server/otupdate/openembedded/__init__.py b/update-server/otupdate/openembedded/__init__.py index a1bdc36c9f0..308772b9c09 100644 --- a/update-server/otupdate/openembedded/__init__.py +++ b/update-server/otupdate/openembedded/__init__.py @@ -49,17 +49,12 @@ def get_app( config_file_override: Optional[str] = None, name_override: Optional[str] = None, boot_id_override: Optional[str] = None, - loop: Optional[asyncio.AbstractEventLoop] = None, ) -> web.Application: """Build and return the aiohttp.web.Application that runs the server""" if not system_version_file: system_version_file = OE_BUILTIN_VERSION_FILE version = get_version_dict(system_version_file) - - if not loop: - loop = asyncio.get_event_loop() - config_obj = config.load(config_file_override) app = web.Application(middlewares=[log_error_middleware]) diff --git a/update-server/tests/buildroot/conftest.py b/update-server/tests/buildroot/conftest.py index cfbbb0535b9..609a89e92da 100644 --- a/update-server/tests/buildroot/conftest.py +++ b/update-server/tests/buildroot/conftest.py @@ -10,9 +10,7 @@ @pytest.fixture -async def test_cli( - aiohttp_client, loop, otupdate_config, monkeypatch, version_file_path -): +async def test_cli(aiohttp_client, otupdate_config, monkeypatch, version_file_path): """ Build an app using dummy versions, then build a test client and return it """ @@ -21,9 +19,8 @@ async def test_cli( config_file_override=otupdate_config, name_override="opentrons-test", boot_id_override="dummy-boot-id-abc123", - loop=loop, ) - client = await loop.create_task(aiohttp_client(app)) + client = await aiohttp_client(app) return client diff --git a/update-server/tests/common/conftest.py b/update-server/tests/common/conftest.py index 97a2edb8de0..fd409ed02cd 100644 --- a/update-server/tests/common/conftest.py +++ b/update-server/tests/common/conftest.py @@ -23,7 +23,7 @@ @pytest.fixture(params=[openembedded, buildroot]) async def test_cli( - aiohttp_client, loop, otupdate_config, request, version_file_path + aiohttp_client, otupdate_config, request, version_file_path ) -> Tuple[TestClient, str]: """ Build an app using dummy versions, then build a test client and return it @@ -34,9 +34,8 @@ async def test_cli( config_file_override=otupdate_config, name_override="opentrons-test", boot_id_override="dummy-boot-id-abc123", - loop=loop, ) - client = await loop.create_task(aiohttp_client(app)) + client = await aiohttp_client(app) return client, cli_client_pkg.__name__ diff --git a/update-server/tests/openembedded/conftest.py b/update-server/tests/openembedded/conftest.py index 81bd12dbf5c..d9cf5b720b2 100644 --- a/update-server/tests/openembedded/conftest.py +++ b/update-server/tests/openembedded/conftest.py @@ -26,9 +26,7 @@ def mock_update_actions_interface( @pytest.fixture -async def test_cli( - aiohttp_client, loop, otupdate_config, version_file_path -) -> TestClient: +async def test_cli(aiohttp_client, otupdate_config, version_file_path) -> TestClient: """ Build an app using dummy versions, then build a test client and return it """ @@ -37,9 +35,8 @@ async def test_cli( config_file_override=otupdate_config, name_override="opentrons-test", boot_id_override="dummy-boot-id-abc123", - loop=loop, ) - client = await loop.create_task(aiohttp_client(app)) + client = await aiohttp_client(app) return client From faa12445652e61f0ffe094f4db1400ae1b2add0d Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 00:58:31 -0400 Subject: [PATCH 04/41] More hacking. --- .../common/name_management/__init__.py | 4 +- .../common/name_management/name_manager.py | 73 ++++++++++++----- .../otupdate/openembedded/__init__.py | 78 +++++++++++-------- 3 files changed, 102 insertions(+), 53 deletions(-) diff --git a/update-server/otupdate/common/name_management/__init__.py b/update-server/otupdate/common/name_management/__init__.py index 7ef56d92035..1f685e69a91 100644 --- a/update-server/otupdate/common/name_management/__init__.py +++ b/update-server/otupdate/common/name_management/__init__.py @@ -47,7 +47,7 @@ from aiohttp import web -from .name_manager import NameManager, build_and_insert +from .name_manager import NameManager, build_name_manager from .static_hostname import set_up_static_hostname @@ -114,7 +114,7 @@ async def get_name_endpoint(request: web.Request) -> web.Response: __all__ = [ "NameManager", - "build_and_insert", + "build_name_manager", "set_up_static_hostname", "get_name_endpoint", "set_name_endpoint", diff --git a/update-server/otupdate/common/name_management/name_manager.py b/update-server/otupdate/common/name_management/name_manager.py index 4a0e071d3ea..364f2ca8dc7 100644 --- a/update-server/otupdate/common/name_management/name_manager.py +++ b/update-server/otupdate/common/name_management/name_manager.py @@ -1,8 +1,9 @@ from __future__ import annotations +from abc import ABC, abstractmethod from contextlib import asynccontextmanager from logging import getLogger -from typing import AsyncGenerator +from typing import AsyncGenerator, Optional from aiohttp import web @@ -15,7 +16,32 @@ _log = getLogger(__name__) -class NameManager: +class NameManager(ABC): + @classmethod + def from_request(cls, request: web.Request) -> NameManager: + return cls.from_app(request.app) + + @staticmethod + def from_app(app: web.Application) -> NameManager: + name_manager = app.get(_NAME_MANAGER_VARNAME, None) + assert isinstance( + name_manager, NameManager + ), f"Unexpected type {type(name_manager)}. Incorrect Application setup?" + return name_manager + + def install_on_app(self, app: web.Application) -> None: + app[_NAME_MANAGER_VARNAME] = self + + @abstractmethod + async def set_name(self, new_name: str) -> str: + pass + + @abstractmethod + def get_name(self) -> str: + pass + + +class RealNameManager(NameManager): def __init__(self, avahi_client: AvahiClient) -> None: """For internal use by this class only.""" self._avahi_client = avahi_client @@ -25,23 +51,11 @@ def __init__(self, avahi_client: AvahiClient) -> None: async def build( cls, avahi_client: AvahiClient ) -> AsyncGenerator[NameManager, None]: - name_manager = NameManager(avahi_client) + name_manager = cls(avahi_client) async with avahi_client.collision_callback(name_manager._on_avahi_collision): await avahi_client.start_advertising(service_name=name_manager.get_name()) yield name_manager - @classmethod - def from_request(cls, request: web.Request) -> NameManager: - return cls.from_app(request.app) - - @staticmethod - def from_app(app: web.Application) -> NameManager: - name_manager = app.get(_NAME_MANAGER_VARNAME, None) - assert isinstance( - name_manager, NameManager - ), f"Unexpected type {type(name_manager)}. Incorrect Application setup?" - return name_manager - async def set_name(self, new_name: str) -> str: """See `set_name_endpoint()`.""" await self._avahi_client.start_advertising(service_name=new_name) @@ -69,9 +83,28 @@ async def _on_avahi_collision(self) -> None: await self.set_name(new_name=alternative_name) +class FakeNameManager(NameManager): + def __init__(self, name_override: str) -> None: + self._name_override = name_override + + async def set_name(self, new_name: str) -> str: + raise NotImplementedError( + "Can't change the name when it's been overridden for testing." + ) + + def get_name(self) -> str: + return self._name_override + + @asynccontextmanager -async def build_and_insert(app: web.Application) -> AsyncGenerator[None, None]: - avahi_client = await AvahiClient.connect() - async with NameManager.build(avahi_client=avahi_client) as name_manager: - app[_NAME_MANAGER_VARNAME] = name_manager - yield +async def build_name_manager( + name_override: Optional[str], +) -> AsyncGenerator[NameManager, None]: + if name_override is None: + avahi_client = await AvahiClient.connect() + async with RealNameManager.build( + avahi_client=avahi_client + ) as real_name_manager: + yield real_name_manager + else: + yield FakeNameManager(name_override=name_override) diff --git a/update-server/otupdate/openembedded/__init__.py b/update-server/otupdate/openembedded/__init__.py index 308772b9c09..ca7339ed39f 100644 --- a/update-server/otupdate/openembedded/__init__.py +++ b/update-server/otupdate/openembedded/__init__.py @@ -55,40 +55,55 @@ def get_app( system_version_file = OE_BUILTIN_VERSION_FILE version = get_version_dict(system_version_file) + boot_id = boot_id_override or control.get_boot_id() config_obj = config.load(config_file_override) app = web.Application(middlewares=[log_error_middleware]) - name = name_override or name_management.get_pretty_hostname() - boot_id = boot_id_override or control.get_boot_id() - app[config.CONFIG_VARNAME] = config_obj - app[constants.RESTART_LOCK_NAME] = asyncio.Lock() - app[constants.DEVICE_BOOT_ID_NAME] = boot_id - # FIX BEFORE MERGE - # app[constants.DEVICE_NAME_VARNAME] = name - - LOG.info( - "Setup: " - + "\n\t".join( - [ - f"Device name: {name}", - "Buildroot version: " - f'{version.get("buildroot_version", "unknown")}', - "\t(from git sha " f'{version.get("buildroot_sha", "unknown")}', - "API version: " - f'{version.get("opentrons_api_version", "unknown")}', - "\t(from git sha " - f'{version.get("opentrons_api_sha", "unknown")}', - "Update server version: " - f'{version.get("update_server_version", "unknown")}', - "\t(from git sha " - f'{version.get("update_server_sha", "unknown")}', - ] - ) - ) - rfs = RootFSInterface() - part_mgr = PartitionManager() - updater = Updater(rfs, part_mgr) - app[FILE_ACTIONS_VARNAME] = updater + + async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, None]: + app[config.CONFIG_VARNAME] = config_obj + app[constants.RESTART_LOCK_NAME] = asyncio.Lock() + app[constants.DEVICE_BOOT_ID_NAME] = boot_id + + rfs = RootFSInterface() + part_mgr = PartitionManager() + updater = Updater(rfs, part_mgr) + app[FILE_ACTIONS_VARNAME] = updater + + async with name_management.build_name_manager( + name_override=name_override + ) as name_manager: + name_manager.install_on_app(app) + initial_name = name_manager.get_name() + + LOG.info( + "Setup: " + + "\n\t".join( + [ + f"Device name: {initial_name}", + "Buildroot version: " + f'{version.get("buildroot_version", "unknown")}', + "\t(from git sha " + f'{version.get("buildroot_sha", "unknown")}', + "API version: " + f'{version.get("opentrons_api_version", "unknown")}', + "\t(from git sha " + f'{version.get("opentrons_api_sha", "unknown")}', + "Update server version: " + f'{version.get("update_server_version", "unknown")}', + "\t(from git sha " + f'{version.get("update_server_sha", "unknown")}', + ] + ) + ) + + LOG.info(f"Notifying {systemd.SOURCE} that service is up.") + systemd.notify_up() + + yield + + app.cleanup_ctx.append(set_up_and_tear_down) + app.router.add_routes( [ web.get( @@ -109,6 +124,7 @@ def get_app( web.get("/server/name", name_management.get_name_endpoint), ] ) + return app From 6cb33698a952f7b6eff89df514fdd76e203cf50c Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:29:29 -0400 Subject: [PATCH 05/41] Refactor and document `collision_callback()`. --- .../otupdate/common/name_management/avahi.py | 48 ++++++++++++++----- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index b1c93acb0ba..595f32a8885 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -72,29 +72,51 @@ async def alternative_service_name(self, current_service_name: str) -> str: async def collision_callback( self, callback: CollisionCallback ) -> AsyncGenerator[None, None]: - """ - To document: - * Potentially duplicate collision events if resolving a collision takes too long - * May cancel callback when this exits - """ + """Be informed of name collisions. - async def _poll_and_call_back() -> None: - _log.info("Beginning polling.") - while True: - if await self._is_collided(): - await callback() # TODO: Log exception? - await asyncio.sleep(_COLLISION_POLL_INTERVAL) + Background: + The Avahi service name and the static hostname that this client advertises + must be unique on the network. When Avahi detects a collision, it will stop + advertising until we fix the conflict by giving it a different name. - background_task = asyncio.create_task(_poll_and_call_back()) + While this context manager is entered, `callback()` will be called + whenever a collision happens. You should then call `start_advertising()` + with a new name to resume advertising. + + If `callback()` raises an exception, it's logged but otherwise ignored. + """ + # Ideally, instead of polling, we'd listen to the EntryGroup.StateChanged + # signal. But receiving signals through the dbus library requires a 3rd-party + # event loop like GLib's. + background_task = asyncio.create_task( + self._poll_infinitely_for_collision(callback=callback) + ) try: yield - finally: background_task.cancel() with contextlib.suppress(asyncio.CancelledError): await background_task + async def _poll_infinitely_for_collision(self, callback: CollisionCallback) -> None: + while True: + is_collided = await self._is_collided() + + if is_collided: + try: + await callback() + except asyncio.CancelledError: + # If it raises CancelledError, this task is being cancelled. + # Let the CancelledError propagate and break the loop + # so we don't accidentally suppress the cancellation. + raise + except Exception: + # If it raises any other Exception, it's unexpected. + # Log it and keep polling. + _log.exception("Exception while handling Avahi name collision.") + + await asyncio.sleep(_COLLISION_POLL_INTERVAL) async def _is_collided(self) -> bool: async with self._lock: return await asyncio.get_running_loop().run_in_executor( From 970350c18ee41664ef5adca178baac7b53ee8bcc Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:30:02 -0400 Subject: [PATCH 06/41] Document `alternative_service_name()`. --- .../otupdate/common/name_management/avahi.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 595f32a8885..fbcc4fffb13 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -61,12 +61,26 @@ async def start_advertising(self, service_name: str) -> None: ) async def alternative_service_name(self, current_service_name: str) -> str: + """Return an alternative to the given Avahi service name. + + This is useful for fixing name collisions with other things on the network. + For example: + + alternative_service_name("Foo") == "Foo #2" + alternative_service_name("Foo #2") == "Foo #3" + + Appending incrementing integers like this, instead of using something like + the machine's serial number, follows a recommendation in the DNS-SD spec: + https://datatracker.ietf.org/doc/html/rfc6763#appendix-D + """ + # We delegate to the Avahi daemon to find the alternative name for us. + # We could compute it ourselves, but getting Avahi to do it is convenient + # because it handles edge cases like appending "#2" to a name that already has + # the maximum length. async with self._lock: return await asyncio.get_running_loop().run_in_executor( None, self._sync_client.alternative_service_name, current_service_name ) - # Also document why we ask Avahi to do this for us. - # Test whether Avahi gracefully handles overlong names. @contextlib.asynccontextmanager async def collision_callback( From e79e3a43b0d3fc10c410cbccda5fbd455fcf9f9c Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:31:56 -0400 Subject: [PATCH 07/41] Note why we persist service names across reboots. --- .../otupdate/common/name_management/name_manager.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/update-server/otupdate/common/name_management/name_manager.py b/update-server/otupdate/common/name_management/name_manager.py index 364f2ca8dc7..d410620aaec 100644 --- a/update-server/otupdate/common/name_management/name_manager.py +++ b/update-server/otupdate/common/name_management/name_manager.py @@ -80,6 +80,13 @@ async def _on_avahi_collision(self) -> None: _log.info( f"Name collision detected by Avahi. Changing name to {alternative_name}." ) + + # Setting the new name includes persisting it for the next boot. + # + # Persisting the new name is recommended in the mDNS spec + # (https://datatracker.ietf.org/doc/html/rfc6762#section-9). + # It prevents two machines with the same name from flipping + # which one is #1 and which one is #2 every time they reboot. await self.set_name(new_name=alternative_name) From 02f3216ed3125737617f559c4f3e18f0a87fe256 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:32:10 -0400 Subject: [PATCH 08/41] Document `restart_daemon()`. --- update-server/otupdate/common/name_management/avahi.py | 1 + 1 file changed, 1 insertion(+) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index fbcc4fffb13..008f1660334 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -16,6 +16,7 @@ async def restart_daemon() -> None: + """Restart the system's Avahi daemon and wait for it to come back up.""" proc = await asyncio.create_subprocess_exec( "systemctl", "restart", From fe5654ae95856734f1bf116d71564507ccd451e5 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:41:49 -0400 Subject: [PATCH 09/41] Dedent `SyncClient`. --- .../otupdate/common/name_management/avahi.py | 187 +++++++++--------- 1 file changed, 89 insertions(+), 98 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 008f1660334..18a5713eb3e 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -8,6 +8,13 @@ import logging from typing import AsyncGenerator, Awaitable, Callable, cast +try: + import dbus + + _DBUS_AVAILABLE = True +except ImportError: + _DBUS_AVAILABLE = False + _COLLISION_POLL_INTERVAL = 5 @@ -142,112 +149,96 @@ async def _is_collided(self) -> bool: CollisionCallback = Callable[[], Awaitable[None]] -try: - import dbus -except ImportError: - _avahi_available = False -else: - _avahi_available = True +class _SyncClient: + """A non-async wrapper around Avahi's D-Bus API. - class _SyncClient: - """ - To document: - Does I/O and is not thread-safe. - dbus module doesn't natively support async/await. - """ + Since methods of this class do blocking I/O, they should be offloaded to a thread + and not called directly inside an async event loop. But they're not safe to call + concurrently from multiple threads, so the calls should be serialized with a lock. + """ - # For semantics of the methods we're calling, see Avahi's API docs. - # For example: https://www.avahi.org/doxygen/html/index.html#good_publish - # It's mostly in terms of the C API, but the semantics should be the same. - # - # For exact method names and argument types, see Avahi's D-Bus bindings, - # which they specify across several machine-readable files. For example: - # https://github.com/lathiat/avahi/blob/v0.7/avahi-daemon/org.freedesktop.Avahi.EntryGroup.xml - def __init__( - self, - bus: dbus.SystemBus, - server: dbus.Interface, - entry_group: dbus.Interface, - ) -> None: - """For internal use by this class only. Use `connect()` instead. - - Args: - bus: The system bus instance. - server: An org.freedesktop.Avahi.Server interface. - entry_group: An org.freedesktop.Avahi.EntryGroup interface. - """ - self._bus = bus - self._server = server - self._entry_group = entry_group - - @classmethod - def connect(cls) -> _SyncClient: - _log.info("Connecting to Avahi daemon.") - - bus = dbus.SystemBus() - server_obj = bus.get_object("org.freedesktop.Avahi", "/") - server_if = dbus.Interface(server_obj, "org.freedesktop.Avahi.Server") - entry_group_path = server_if.EntryGroupNew() - entry_group_obj = bus.get_object("org.freedesktop.Avahi", entry_group_path) - entry_group_if = dbus.Interface( - entry_group_obj, "org.freedesktop.Avahi.EntryGroup" - ) - return cls( - bus=bus, - server=server_if, - entry_group=entry_group_if, - ) + # For general semantics of the methods we're calling on dbus proxies, + # see Avahi's API docs. For example: + # https://www.avahi.org/doxygen/html/index.html#good_publish + # It's mostly in terms of the C API, but the semantics should be the same. - def start_advertising(self, service_name: str) -> None: - _log.info(f"Starting advertising with name {service_name}.") - # TODO(mm, 2022-05-26): Can we leave these as the empty string? - # The avahi_entry_group_add_service() C API recommends passing NULL - # to let the daemon decide these values. - hostname = self._server.GetHostName() - domainname = self._server.GetDomainName() - - self._entry_group.Reset() - _log.info("Reset entry group.") - - # TODO(mm, 2022-05-06): This isn't exception-safe. - # We've already reset the entry group, so if this fails - # (for example because Avahi doesn't like the new name), - # we'll be left with no entry group, - # and Avahi will stop advertising the machine. - self._entry_group.AddService( - dbus.Int32(-1), # avahi.IF_UNSPEC - dbus.Int32(-1), # avahi.PROTO_UNSPEC - dbus.UInt32(0), # flags - service_name, # sname - "_http._tcp", # stype - domainname, # sdomain (.local) - f"{hostname}.{domainname}", # shost (hostname.local) - dbus.UInt16(31950), # port - dbus.Array([], signature="ay"), - ) + # For exact method names and argument types, see Avahi's D-Bus bindings, + # which they specify across several XML files. For example: + # https://github.com/lathiat/avahi/blob/v0.7/avahi-daemon/org.freedesktop.Avahi.EntryGroup.xml + + def __init__( + self, + bus: dbus.SystemBus, + server: dbus.Interface, + entry_group: dbus.Interface, + ) -> None: + """For internal use by this class only. Use `connect()` instead. - _log.info(f"Added service with {hostname} {domainname}.") + Args: + bus: The system bus instance. + server: An org.freedesktop.Avahi.Server interface. + entry_group: An org.freedesktop.Avahi.EntryGroup interface. + """ + self._bus = bus + self._server = server + self._entry_group = entry_group - self._entry_group.Commit() + @classmethod + def connect(cls) -> _SyncClient: + bus = dbus.SystemBus() + server_obj = bus.get_object("org.freedesktop.Avahi", "/") + server_if = dbus.Interface(server_obj, "org.freedesktop.Avahi.Server") + entry_group_path = server_if.EntryGroupNew() + entry_group_obj = bus.get_object("org.freedesktop.Avahi", entry_group_path) + entry_group_if = dbus.Interface( + entry_group_obj, "org.freedesktop.Avahi.EntryGroup" + ) + return cls( + bus=bus, + server=server_if, + entry_group=entry_group_if, + ) - _log.info("Committed.") + def start_advertising(self, service_name: str) -> None: + # TODO(mm, 2022-05-26): Can we leave these as the empty string? + # The avahi_entry_group_add_service() C API recommends passing NULL + # to let the daemon decide these values. + hostname = self._server.GetHostName() + domainname = self._server.GetDomainName() + + self._entry_group.Reset() + + # TODO(mm, 2022-05-06): Can this be made exception-safe? + # We've already reset the entry group, so if this fails + # (for example because Avahi doesn't like the new name), + # we'll be left with no entry group, + # and Avahi will stop advertising the machine. + self._entry_group.AddService( + dbus.Int32(-1), # avahi.IF_UNSPEC + dbus.Int32(-1), # avahi.PROTO_UNSPEC + dbus.UInt32(0), # flags + service_name, # sname + "_http._tcp", # stype + domainname, # sdomain (.local) + f"{hostname}.{domainname}", # shost (hostname.local) + dbus.UInt16(31950), # port + dbus.Array([], signature="ay"), + ) - def alternative_service_name(self, current_service_name: str) -> str: - result = self._server.GetAlternativeServiceName(current_service_name) - assert isinstance(result, str) - return result + self._entry_group.Commit() - def is_collided(self) -> bool: - """ - To document: - Will be the case if another device on the network has - the same service name or host. - """ + def alternative_service_name(self, current_service_name: str) -> str: + result = self._server.GetAlternativeServiceName(current_service_name) + assert isinstance(result, str) + return result - state = self._entry_group.GetState() + def is_collided(self) -> bool: + state = self._entry_group.GetState() - # The value 3 comes from: - # https://github.com/lathiat/avahi/blob/v0.8/avahi-common/defs.h#L234 - avahi_entry_group_collision = dbus.Int32(3) + # "3" comes from AVAHI_ENTRY_GROUP_COLLISION being index 3 in this enum: + # https://github.com/lathiat/avahi/blob/v0.8/avahi-common/defs.h#L234 + avahi_entry_group_collision = dbus.Int32(3) - return cast(bool, state == avahi_entry_group_collision) + # Cast for when type-checking is run on dev machines without dbus, + # where the type-checker will see this expression as `Any == Any`. + return cast(bool, state == avahi_entry_group_collision) From 1462dde029579039e926e536e183270e5e76dc48 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:42:09 -0400 Subject: [PATCH 10/41] Fixup --- update-server/otupdate/common/name_management/avahi.py | 1 + 1 file changed, 1 insertion(+) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 18a5713eb3e..1b3c0ef702a 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -139,6 +139,7 @@ async def _poll_infinitely_for_collision(self, callback: CollisionCallback) -> N _log.exception("Exception while handling Avahi name collision.") await asyncio.sleep(_COLLISION_POLL_INTERVAL) + async def _is_collided(self) -> bool: async with self._lock: return await asyncio.get_running_loop().run_in_executor( From 216d9a4d2a78ff299cc0176a20493ebf450a9eea Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:42:33 -0400 Subject: [PATCH 11/41] Remove todo about handling when dbus isn't available. --- update-server/otupdate/common/name_management/avahi.py | 1 - 1 file changed, 1 deletion(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 1b3c0ef702a..bcaedcab230 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -49,7 +49,6 @@ def __init__(self, sync_client: _SyncClient) -> None: @classmethod async def connect(cls) -> AvahiClient: - # TODO: Handle case when dbus isn't available. sync_client = await asyncio.get_running_loop().run_in_executor( executor=None, func=_SyncClient.connect, From c05a0e0ff1fd0fe48bad0d35c209de21fa1f6b87 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 20:43:15 -0400 Subject: [PATCH 12/41] Document `start_advertising()`. --- .../otupdate/common/name_management/avahi.py | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index bcaedcab230..c0b2426ae75 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -56,11 +56,23 @@ async def connect(cls) -> AvahiClient: return cls(sync_client=sync_client) async def start_advertising(self, service_name: str) -> None: - """ - To document: - * Picks up current static hostname and domain - * Replaces an existing advertisement if there is one - * Advertisement will automatically stop upon collision + """Start advertising the machine over mDNS + DNS-SD. + + Since the Avahi service name corresponds to the DNS-SD instance name, + it's a human-readable string of mostly arbitrary Unicode, + at most 63 octets (not 63 code points or 63 characters!) long. + (See: https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1) + Avahi will raise an exception through this method if it thinks + the new service name is invalid. + + Avahi will stop advertising the machine when either of these happen: + + * This process dies. + * We have a name collision with another device on the network. + See `collision_callback()`. + + It's safe to call this more than once. If we're already advertising, + the existing service name will be replaced with the new one. """ async with self._lock: await asyncio.get_running_loop().run_in_executor( From bf4cbb1134e62bba43f548f3ddb43baa4b9308f4 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 21:50:53 -0400 Subject: [PATCH 13/41] Rename `NameManager` -> `NameSynchronizer`. --- update-server/otupdate/buildroot/__init__.py | 8 ++-- update-server/otupdate/buildroot/__main__.py | 2 +- update-server/otupdate/common/control.py | 4 +- .../common/name_management/__init__.py | 14 +++--- .../{name_manager.py => name_synchronizer.py} | 48 ++++++++++--------- .../otupdate/openembedded/__init__.py | 8 ++-- .../name_management/test_http_endpoints.py | 22 +++++---- 7 files changed, 57 insertions(+), 49 deletions(-) rename update-server/otupdate/common/name_management/{name_manager.py => name_synchronizer.py} (70%) diff --git a/update-server/otupdate/buildroot/__init__.py b/update-server/otupdate/buildroot/__init__.py index e2e6512ebb4..1fb0bd77b4c 100644 --- a/update-server/otupdate/buildroot/__init__.py +++ b/update-server/otupdate/buildroot/__init__.py @@ -68,11 +68,11 @@ async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, Non update_actions.OT2UpdateActions.build_and_insert(app) - async with name_management.build_name_manager( + async with name_management.build_name_synchronizer( name_override=name_override - ) as name_manager: - name_manager.install_on_app(app) - initial_name = name_manager.get_name() + ) as name_synchronizer: + name_synchronizer.install_on_app(app) + initial_name = name_synchronizer.get_name() LOG.info( "Setup: " diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index df94ff9feb2..a48593c24aa 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -19,7 +19,7 @@ def main() -> None: systemd.configure_logging(getattr(logging, args.log_level.upper())) # Because this involves restarting Avahi, this must happen early, - # before the NameManager starts up and connects to Avahi. + # before the NameSynchronizer starts up and connects to Avahi. LOG.info("Setting static hostname") static_hostname = loop.run_until_complete(name_management.set_up_static_hostname()) LOG.info(f"Set static hostname to {static_hostname}") diff --git a/update-server/otupdate/common/control.py b/update-server/otupdate/common/control.py index 73cf324aeb7..f0897ff075e 100644 --- a/update-server/otupdate/common/control.py +++ b/update-server/otupdate/common/control.py @@ -13,7 +13,7 @@ from aiohttp import web from .constants import RESTART_LOCK_NAME, DEVICE_BOOT_ID_NAME -from .name_management import NameManager +from .name_management import NameSynchronizer LOG = logging.getLogger(__name__) @@ -42,7 +42,7 @@ async def health(request: web.Request) -> web.Response: { **health_response, **{ - "name": NameManager.from_request(request).get_name(), + "name": NameSynchronizer.from_request(request).get_name(), "serialNumber": get_serial(), "bootId": request.app[DEVICE_BOOT_ID_NAME], }, diff --git a/update-server/otupdate/common/name_management/__init__.py b/update-server/otupdate/common/name_management/__init__.py index 1f685e69a91..bee80687144 100644 --- a/update-server/otupdate/common/name_management/__init__.py +++ b/update-server/otupdate/common/name_management/__init__.py @@ -47,7 +47,7 @@ from aiohttp import web -from .name_manager import NameManager, build_name_manager +from .name_synchronizer import NameSynchronizer, build_name_synchronizer from .static_hostname import set_up_static_hostname @@ -90,8 +90,8 @@ def build_400(msg: str) -> web.Response: if not isinstance(name_to_set, str): return build_400('"name" key is not a string"') - name_manager = NameManager.from_request(request) - new_name = await name_manager.set_name(new_name=name_to_set) + name_synchronizer = NameSynchronizer.from_request(request) + new_name = await name_synchronizer.set_name(new_name=name_to_set) return web.json_response( # type: ignore[no-untyped-call,no-any-return] data={"name": new_name}, status=200 @@ -106,15 +106,15 @@ async def get_name_endpoint(request: web.Request) -> web.Response: GET /server/name -> 200 OK, {'name': robot name} """ - name_manager = NameManager.from_request(request) + name_synchronizer = NameSynchronizer.from_request(request) return web.json_response( # type: ignore[no-untyped-call,no-any-return] - data={"name": name_manager.get_name()}, status=200 + data={"name": name_synchronizer.get_name()}, status=200 ) __all__ = [ - "NameManager", - "build_name_manager", + "NameSynchronizer", + "build_name_synchronizer", "set_up_static_hostname", "get_name_endpoint", "set_name_endpoint", diff --git a/update-server/otupdate/common/name_management/name_manager.py b/update-server/otupdate/common/name_management/name_synchronizer.py similarity index 70% rename from update-server/otupdate/common/name_management/name_manager.py rename to update-server/otupdate/common/name_management/name_synchronizer.py index d410620aaec..e51c5142dfe 100644 --- a/update-server/otupdate/common/name_management/name_manager.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -12,25 +12,25 @@ from .pretty_hostname import get_pretty_hostname, persist_pretty_hostname -_NAME_MANAGER_VARNAME = APP_VARIABLE_PREFIX + "name_manager" +_name_synchronizer_VARNAME = APP_VARIABLE_PREFIX + "name_synchronizer" _log = getLogger(__name__) -class NameManager(ABC): +class NameSynchronizer(ABC): @classmethod - def from_request(cls, request: web.Request) -> NameManager: + def from_request(cls, request: web.Request) -> NameSynchronizer: return cls.from_app(request.app) @staticmethod - def from_app(app: web.Application) -> NameManager: - name_manager = app.get(_NAME_MANAGER_VARNAME, None) + def from_app(app: web.Application) -> NameSynchronizer: + name_synchronizer = app.get(_name_synchronizer_VARNAME, None) assert isinstance( - name_manager, NameManager - ), f"Unexpected type {type(name_manager)}. Incorrect Application setup?" - return name_manager + name_synchronizer, NameSynchronizer + ), f"Unexpected type {type(name_synchronizer)}. Incorrect Application setup?" + return name_synchronizer def install_on_app(self, app: web.Application) -> None: - app[_NAME_MANAGER_VARNAME] = self + app[_name_synchronizer_VARNAME] = self @abstractmethod async def set_name(self, new_name: str) -> str: @@ -41,7 +41,7 @@ def get_name(self) -> str: pass -class RealNameManager(NameManager): +class RealNameSynchronizer(NameSynchronizer): def __init__(self, avahi_client: AvahiClient) -> None: """For internal use by this class only.""" self._avahi_client = avahi_client @@ -50,11 +50,15 @@ def __init__(self, avahi_client: AvahiClient) -> None: @asynccontextmanager async def build( cls, avahi_client: AvahiClient - ) -> AsyncGenerator[NameManager, None]: - name_manager = cls(avahi_client) - async with avahi_client.collision_callback(name_manager._on_avahi_collision): - await avahi_client.start_advertising(service_name=name_manager.get_name()) - yield name_manager + ) -> AsyncGenerator[NameSynchronizer, None]: + name_synchronizer = cls(avahi_client) + async with avahi_client.collision_callback( + name_synchronizer._on_avahi_collision + ): + await avahi_client.start_advertising( + service_name=name_synchronizer.get_name() + ) + yield name_synchronizer async def set_name(self, new_name: str) -> str: """See `set_name_endpoint()`.""" @@ -90,7 +94,7 @@ async def _on_avahi_collision(self) -> None: await self.set_name(new_name=alternative_name) -class FakeNameManager(NameManager): +class FakeNameSynchronizer(NameSynchronizer): def __init__(self, name_override: str) -> None: self._name_override = name_override @@ -104,14 +108,14 @@ def get_name(self) -> str: @asynccontextmanager -async def build_name_manager( +async def build_name_synchronizer( name_override: Optional[str], -) -> AsyncGenerator[NameManager, None]: +) -> AsyncGenerator[NameSynchronizer, None]: if name_override is None: avahi_client = await AvahiClient.connect() - async with RealNameManager.build( + async with RealNameSynchronizer.build( avahi_client=avahi_client - ) as real_name_manager: - yield real_name_manager + ) as real_name_synchronizer: + yield real_name_synchronizer else: - yield FakeNameManager(name_override=name_override) + yield FakeNameSynchronizer(name_override=name_override) diff --git a/update-server/otupdate/openembedded/__init__.py b/update-server/otupdate/openembedded/__init__.py index ca7339ed39f..31e15d22fa1 100644 --- a/update-server/otupdate/openembedded/__init__.py +++ b/update-server/otupdate/openembedded/__init__.py @@ -70,11 +70,11 @@ async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, Non updater = Updater(rfs, part_mgr) app[FILE_ACTIONS_VARNAME] = updater - async with name_management.build_name_manager( + async with name_management.build_name_synchronizer( name_override=name_override - ) as name_manager: - name_manager.install_on_app(app) - initial_name = name_manager.get_name() + ) as name_synchronizer: + name_synchronizer.install_on_app(app) + initial_name = name_synchronizer.get_name() LOG.info( "Setup: " diff --git a/update-server/tests/common/name_management/test_http_endpoints.py b/update-server/tests/common/name_management/test_http_endpoints.py index a6f914765c4..3343a71f62c 100644 --- a/update-server/tests/common/name_management/test_http_endpoints.py +++ b/update-server/tests/common/name_management/test_http_endpoints.py @@ -2,17 +2,19 @@ import pytest -from otupdate.common.name_management import NameManager +from otupdate.common.name_management import NameSynchronizer @pytest.fixture -def mock_name_manager(): - return MagicMock(spec=NameManager) +def mock_name_synchronizer(): + return MagicMock(spec=NameSynchronizer) -async def test_get_name(test_cli, monkeypatch, mock_name_manager) -> None: - mock_name_manager.get_name.return_value = "the returned name" - monkeypatch.setattr(NameManager, "from_request", lambda request: mock_name_manager) +async def test_get_name(test_cli, monkeypatch, mock_name_synchronizer) -> None: + mock_name_synchronizer.get_name.return_value = "the returned name" + monkeypatch.setattr( + NameSynchronizer, "from_request", lambda request: mock_name_synchronizer + ) response = await (test_cli[0].get("/server/name")) assert response.status == 200 @@ -20,12 +22,14 @@ async def test_get_name(test_cli, monkeypatch, mock_name_manager) -> None: assert body["name"] == "the returned name" -async def test_set_name_valid(test_cli, monkeypatch, mock_name_manager) -> None: +async def test_set_name_valid(test_cli, monkeypatch, mock_name_synchronizer) -> None: async def mock_set_name(new_name: str) -> str: return "the returned name" - mock_name_manager.set_name = mock_set_name - monkeypatch.setattr(NameManager, "from_request", lambda request: mock_name_manager) + mock_name_synchronizer.set_name = mock_set_name + monkeypatch.setattr( + NameSynchronizer, "from_request", lambda request: mock_name_synchronizer + ) response = await test_cli[0].post("/server/name", json={"name": "the input name"}) assert response.status == 200 From db1ba296f03f189425babb7f4c51e3991d7555e1 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 22:23:58 -0400 Subject: [PATCH 14/41] Document `NameSynchronizer`. --- .../name_management/name_synchronizer.py | 54 ++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index e51c5142dfe..3693bf26494 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -17,28 +17,70 @@ class NameSynchronizer(ABC): + """Keep the machine's human-readable names in sync with each other. + + This ties the pretty hostname and the Avahi service name together, + so they always have the same value. + + The `set_name()` and `get_name()` methods are intended for use by HTTP + endpoints, which makes for a total of three names tied together, + if you also count the name available over HTTP. + + See the `name_management` package docstring for an overview of these various names. + + Tying all of these names together... + + * Is important to avoid confusing the client-side discovery client, + at least at the time of writing. + https://github.com/Opentrons/opentrons/issues/10199 + + * Helps maintain a conceptually simple interface. + There is one name accessible in three separate ways, + rather than three separate names. + + * Implements the DNS-SD spec's recommendation to make the DNS-SD instance name + configurable. https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1 + """ + @classmethod def from_request(cls, request: web.Request) -> NameSynchronizer: - return cls.from_app(request.app) + """Return the server's singleton NameSynchronizer from a request. - @staticmethod - def from_app(app: web.Application) -> NameSynchronizer: - name_synchronizer = app.get(_name_synchronizer_VARNAME, None) + The singleton NameSynchronizer is expected to have been installed on the + aiohttp.Application already via `install_on_app()`. + """ + name_synchronizer = request.app.get(_name_synchronizer_VARNAME, None) assert isinstance( name_synchronizer, NameSynchronizer ), f"Unexpected type {type(name_synchronizer)}. Incorrect Application setup?" return name_synchronizer def install_on_app(self, app: web.Application) -> None: + """Install this NameSynchronizer on `app` for later retrieval via + `from_request()`. + + This should be done as part of server startup. + """ app[_name_synchronizer_VARNAME] = self @abstractmethod async def set_name(self, new_name: str) -> str: - pass + """Set the machine's human-readable name. + + This first sets thhe Avahi service name, and then persists it + as the pretty hostname. + + Returns the new name. This is normally the same as the requested name, + but it it might be different if it had to be truncated, sanitized, etc. + """ @abstractmethod def get_name(self) -> str: - pass + """Return the machine's current human-readable name. + + Note that this can change even if you haven't called `set_name()`, + if it was necessary to avoid conflicts with other devices on the network. + """ class RealNameSynchronizer(NameSynchronizer): From ac042ebf0cefe759f467614d35560102102c8592 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 22:24:08 -0400 Subject: [PATCH 15/41] Document `FakeNameSynchronizer`. --- .../otupdate/common/name_management/name_synchronizer.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 3693bf26494..aaa1091c530 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -137,6 +137,12 @@ async def _on_avahi_collision(self) -> None: class FakeNameSynchronizer(NameSynchronizer): + """A dummy implementation of NameSynchronizer to use in integration tests. + + update-server's integration tests run on systems where we don't have access to + an Avahi daemon. So RealNameSynchronizer wouldn't work. + """ + def __init__(self, name_override: str) -> None: self._name_override = name_override From 15ae12d8b0c0652aea98c0b69055ae5561294916 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 22:38:36 -0400 Subject: [PATCH 16/41] Readability tweak. --- .../otupdate/common/name_management/name_synchronizer.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index aaa1091c530..1e1acdd500e 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -28,17 +28,17 @@ class NameSynchronizer(ABC): See the `name_management` package docstring for an overview of these various names. - Tying all of these names together... + We tie all of these names together because - * Is important to avoid confusing the client-side discovery client, + * It's important to avoid confusing the client-side discovery client, at least at the time of writing. https://github.com/Opentrons/opentrons/issues/10199 - * Helps maintain a conceptually simple interface. + * It helps maintain a conceptually simple interface. There is one name accessible in three separate ways, rather than three separate names. - * Implements the DNS-SD spec's recommendation to make the DNS-SD instance name + * It implements the DNS-SD spec's recommendation to make the DNS-SD instance name configurable. https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1 """ From 488a11cee0cf27fed36cc357b42927ad35d76d60 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 22:38:47 -0400 Subject: [PATCH 17/41] Fix find/replace error. --- .../otupdate/common/name_management/name_synchronizer.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 1e1acdd500e..2c0e43d7856 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -12,7 +12,7 @@ from .pretty_hostname import get_pretty_hostname, persist_pretty_hostname -_name_synchronizer_VARNAME = APP_VARIABLE_PREFIX + "name_synchronizer" +_NAME_SYNCHRONIZER_VARNAME = APP_VARIABLE_PREFIX + "name_synchronizer" _log = getLogger(__name__) @@ -49,7 +49,7 @@ def from_request(cls, request: web.Request) -> NameSynchronizer: The singleton NameSynchronizer is expected to have been installed on the aiohttp.Application already via `install_on_app()`. """ - name_synchronizer = request.app.get(_name_synchronizer_VARNAME, None) + name_synchronizer = request.app.get(_NAME_SYNCHRONIZER_VARNAME, None) assert isinstance( name_synchronizer, NameSynchronizer ), f"Unexpected type {type(name_synchronizer)}. Incorrect Application setup?" @@ -61,7 +61,7 @@ def install_on_app(self, app: web.Application) -> None: This should be done as part of server startup. """ - app[_name_synchronizer_VARNAME] = self + app[_NAME_SYNCHRONIZER_VARNAME] = self @abstractmethod async def set_name(self, new_name: str) -> str: From 1406d596cea0e84e12e9849fc4e3c812d37b7cf0 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 22:38:59 -0400 Subject: [PATCH 18/41] Document `RealNameSynchronizer` and `build_name_synchronizer()`. --- .../name_management/name_synchronizer.py | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 2c0e43d7856..60d769c48c8 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -84,8 +84,10 @@ def get_name(self) -> str: class RealNameSynchronizer(NameSynchronizer): + """A functioning implementation of NameSynchronizer, to run in production.""" + def __init__(self, avahi_client: AvahiClient) -> None: - """For internal use by this class only.""" + """For internal use by this class only. Use `build()` instead.""" self._avahi_client = avahi_client @classmethod @@ -93,6 +95,18 @@ def __init__(self, avahi_client: AvahiClient) -> None: async def build( cls, avahi_client: AvahiClient ) -> AsyncGenerator[NameSynchronizer, None]: + """Build a RealNameSynchronizer and keep it running in the background. + + Avahi advertisements will start as soon as this context manager is entered. + The pretty hostname will be used as the Avahi service name. + + While this context manager remains entered, Avahi will be monitored in the + background to see if this device's name ever collides with another device on + the network. If that ever happens, a new name will be chosen automatically, + which will be visible through `get_name()`. + + Collision monitoring will stop when this context manager exits. + """ name_synchronizer = cls(avahi_client) async with avahi_client.collision_callback( name_synchronizer._on_avahi_collision @@ -103,7 +117,6 @@ async def build( yield name_synchronizer async def set_name(self, new_name: str) -> str: - """See `set_name_endpoint()`.""" await self._avahi_client.start_advertising(service_name=new_name) # Setting the Avahi service name can fail if Avahi doesn't like the new name. # Persist only after it succeeds, so we don't persist something invalid. @@ -115,16 +128,16 @@ def get_name(self) -> str: async def _on_avahi_collision(self) -> None: current_name = self.get_name() + # Assume that the service name was the thing that collided. # Theoretically it also could have been the static hostname, # but our static hostnames are unique in practice, so that's unlikely. - - # TODO: Is this some kind of race condition? alternative_name = await self._avahi_client.alternative_service_name( current_name ) _log.info( - f"Name collision detected by Avahi. Changing name to {alternative_name}." + f"Name collision detected by Avahi." + f" Changing name from {current_name} to {alternative_name}." ) # Setting the new name includes persisting it for the next boot. @@ -159,6 +172,7 @@ def get_name(self) -> str: async def build_name_synchronizer( name_override: Optional[str], ) -> AsyncGenerator[NameSynchronizer, None]: + """Return a RealNameSynchronizer for production or FakeNameManager for testing.""" if name_override is None: avahi_client = await AvahiClient.connect() async with RealNameSynchronizer.build( From 8bef330932dd10b63846ea3e5931d31cf58cf005 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 23:01:53 -0400 Subject: [PATCH 19/41] Delete half-assed explanation. --- update-server/otupdate/buildroot/__init__.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/update-server/otupdate/buildroot/__init__.py b/update-server/otupdate/buildroot/__init__.py index 1fb0bd77b4c..0637bfd0934 100644 --- a/update-server/otupdate/buildroot/__init__.py +++ b/update-server/otupdate/buildroot/__init__.py @@ -58,10 +58,6 @@ def get_app( app = web.Application(middlewares=[log_error_middleware]) async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, None]: - # Stuff everything inside here so that: - # - Getting the order right is more foolproof - # - We can log it all together - app[config.CONFIG_VARNAME] = config_obj app[constants.RESTART_LOCK_NAME] = asyncio.Lock() app[constants.DEVICE_BOOT_ID_NAME] = boot_id From 9ecc3361e03c9f2111e41ad4885c088bc0dd4c6a Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 23:09:43 -0400 Subject: [PATCH 20/41] Remove too-early `systemd.notify_up()`s. --- update-server/otupdate/buildroot/__main__.py | 3 --- update-server/otupdate/openembedded/__main__.py | 2 -- 2 files changed, 5 deletions(-) diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index a48593c24aa..c39940adf99 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -27,9 +27,6 @@ def main() -> None: LOG.info("Building buildroot update server") app = get_app(args.version_file, args.config_file) - LOG.info(f"Notifying {systemd.SOURCE} that service is up") - systemd.notify_up() - LOG.info(f"Starting buildroot update server on http://{args.host}:{args.port}") web.run_app(app, host=args.host, port=args.port) # type: ignore[no-untyped-call] diff --git a/update-server/otupdate/openembedded/__main__.py b/update-server/otupdate/openembedded/__main__.py index 3a34637c58c..249a6cba3b4 100644 --- a/update-server/otupdate/openembedded/__main__.py +++ b/update-server/otupdate/openembedded/__main__.py @@ -25,8 +25,6 @@ def main() -> None: LOG.info("Building openembedded update server") app = get_app(args.version_file, args.config_file) - systemd.notify_up() - LOG.info(f"Starting openembedded update server on http://{args.host}:{args.port}") web.run_app(app, host=args.host, port=args.port) # type: ignore[no-untyped-call] From 40bc918b82b4de561f8d092708a84966196d21bd Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 1 Jun 2022 23:10:08 -0400 Subject: [PATCH 21/41] Format `buildroot/main.py` and `openembedded/main.py` identically for diffing. --- update-server/otupdate/buildroot/__main__.py | 1 + update-server/otupdate/openembedded/__main__.py | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index c39940adf99..8535b9474e6 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -16,6 +16,7 @@ def main() -> None: parser = cli.build_root_parser() args = parser.parse_args() loop = asyncio.get_event_loop() + systemd.configure_logging(getattr(logging, args.log_level.upper())) # Because this involves restarting Avahi, this must happen early, diff --git a/update-server/otupdate/openembedded/__main__.py b/update-server/otupdate/openembedded/__main__.py index 249a6cba3b4..b14c7ed6669 100644 --- a/update-server/otupdate/openembedded/__main__.py +++ b/update-server/otupdate/openembedded/__main__.py @@ -3,8 +3,9 @@ """ import asyncio import logging -import logging.config + from . import get_app + from otupdate.common import name_management, cli, systemd from aiohttp import web @@ -18,9 +19,11 @@ def main() -> None: systemd.configure_logging(getattr(logging, args.log_level.upper())) - LOG.info("Setting hostname") - hostname = loop.run_until_complete(name_management.set_up_static_hostname()) - LOG.info(f"Set hostname to {hostname}") + # Because this involves restarting Avahi, this must happen early, + # before the NameSynchronizer starts up and connects to Avahi. + LOG.info("Setting static hostname") + static_hostname = loop.run_until_complete(name_management.set_up_static_hostname()) + LOG.info(f"Set static hostname to {static_hostname}") LOG.info("Building openembedded update server") app = get_app(args.version_file, args.config_file) From 2c166895aa312be2b71f00613d97d12ccd9d5868 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 3 Jun 2022 14:27:25 -0400 Subject: [PATCH 22/41] Add dependency on Decoy. --- update-server/Pipfile | 4 +++- update-server/Pipfile.lock | 10 +++++++++- update-server/mypy.ini | 1 + update-server/pytest.ini | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/update-server/Pipfile b/update-server/Pipfile index e7d01f0fc7c..50ff797de6c 100644 --- a/update-server/Pipfile +++ b/update-server/Pipfile @@ -18,12 +18,14 @@ pytest-watch = "~=4.2.0" pytest-cov = "==2.10.1" pytest-aiohttp = "==0.3.0" coverage = "==5.1" -# pytest dependencies on windows, spec'd here to force lockfile inclusion +# atomicwrites and colorama are pytest dependencies on windows, +# spec'd here to force lockfile inclusion # https://github.com/pypa/pipenv/issues/4408#issuecomment-668324177 atomicwrites = {version="==1.4.0", sys_platform="== 'win32'"} colorama = {version="==0.4.4", sys_platform="== 'win32'"} mypy = "==0.940" black = "==22.3.0" +decoy = "~=1.10" [requires] python_version = "3.7" diff --git a/update-server/Pipfile.lock b/update-server/Pipfile.lock index 26b52e105aa..f75c700b742 100644 --- a/update-server/Pipfile.lock +++ b/update-server/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "c1f36f5bd77121139aa590708ed72923fd199df3b0c32569a9870e0d8c8162ca" + "sha256": "62e7113800829936d95ee7f0b54803dfb95e2040c6f1b6b84a4c0b3876d2d6c8" }, "pipfile-spec": 6, "requires": { @@ -328,6 +328,14 @@ "index": "pypi", "version": "==5.1" }, + "decoy": { + "hashes": [ + "sha256:599d6f4b6e67b74a3d8edf7a432b0b7d5d3c0296f7733fe96f2f4be4360bbfec", + "sha256:d84bd95816339ab2fda65aa6ced3291a0739bbc83ca82344c8543a801a082d56" + ], + "index": "pypi", + "version": "==1.10.3" + }, "docopt": { "hashes": [ "sha256:49b3a825280bd66b3aa83585ef59c4a8c82f2c8a522dbe754a8bc8d08c85c491" diff --git a/update-server/mypy.ini b/update-server/mypy.ini index 4dc4bd7f78e..33f0a4990f3 100644 --- a/update-server/mypy.ini +++ b/update-server/mypy.ini @@ -1,6 +1,7 @@ [mypy] strict = True show_error_codes = True +plugins = decoy.mypy # The dbus and systemd packages will not be installed in non-Linux dev environments. diff --git a/update-server/pytest.ini b/update-server/pytest.ini index dd19983da9a..073cb34a9a4 100644 --- a/update-server/pytest.ini +++ b/update-server/pytest.ini @@ -11,4 +11,4 @@ markers = exclude_rootfs_ext4: check if theres no rootfs bad_cert_path: bad local cert for code signing no_cert_path: no local cert for code signing - bad_rootfs_ext4_hash: bad rootfs hash \ No newline at end of file + bad_rootfs_ext4_hash: bad rootfs hash From 75b429db71ca26770b09d7517dea90e9842931c5 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 3 Jun 2022 15:43:16 -0400 Subject: [PATCH 23/41] Test `RealNameSynchronizer`'s collision handling. --- .../name_management/test_name_synchronizer.py | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 update-server/tests/common/name_management/test_name_synchronizer.py diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py new file mode 100644 index 00000000000..157cb97d7d9 --- /dev/null +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -0,0 +1,111 @@ +import asyncio +from contextlib import asynccontextmanager +from typing import Callable + +import pytest +from decoy import Decoy, matchers + +from otupdate.common.name_management import name_synchronizer +from otupdate.common.name_management.name_synchronizer import RealNameSynchronizer +from otupdate.common.name_management.avahi import AvahiClient +from otupdate.common.name_management.pretty_hostname import ( + get_pretty_hostname as real_get_pretty_hostname, + persist_pretty_hostname as real_persist_pretty_hostname, +) + + +@pytest.fixture +def mock_get_pretty_hostname(decoy: Decoy): + return decoy.mock(func=real_get_pretty_hostname) + + +@pytest.fixture +def monkeypatch_get_pretty_hostname(monkeypatch, mock_get_pretty_hostname): + monkeypatch.setattr( + name_synchronizer, "get_pretty_hostname", mock_get_pretty_hostname + ) + + +@pytest.fixture +def mock_persist_pretty_hostname(decoy: Decoy): + return decoy.mock(func=real_persist_pretty_hostname) + + +@pytest.fixture +def monkeypatch_persist_pretty_hostname(monkeypatch, mock_persist_pretty_hostname): + monkeypatch.setattr( + name_synchronizer, "persist_pretty_hostname", mock_persist_pretty_hostname + ) + + +@pytest.fixture +def mock_avahi_client(decoy: Decoy) -> AvahiClient: + return decoy.mock(cls=AvahiClient) + + +def test_build_and_startup() -> None: + # It should start advertising with the current pretty hostname + # as the Avahi service name. + raise NotImplementedError() + # decoy.verify(await mock_avahi_client.start_advertising("initial name")) + + +async def test_collision_handling( + mock_get_pretty_hostname: Callable[[], str], + monkeypatch_get_pretty_hostname: None, + mock_persist_pretty_hostname: Callable[[str], str], + monkeypatch_persist_pretty_hostname: None, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + """It should resolve naming collisions reported by the AvahiClient. + + When notified of a collision, it should: + + 1. Get a new service name. + 2. Start advertising with it. + 3. Persist it as the pretty hostname. + """ + decoy.when(mock_get_pretty_hostname()).then_return("initial name") + decoy.when( + await mock_avahi_client.alternative_service_name("initial name") + ).then_return("alternative name") + + # Expect the subject to do: + # + # with avahi_client.collsion_callback(some_callback_func): + # ... + # + # When it does, save the function that it provided as `some_callback_func` + # into `collision_callback_captor.value`. + collision_callback_captor = matchers.Captor() + decoy.when( + mock_avahi_client.collision_callback(collision_callback_captor) + ).then_enter_with( + # https://github.com/mcous/decoy/issues/135 + "" + ) + + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): + captured_collision_callback = collision_callback_captor.value + # Prompt the subject to run its collision-handling logic. + await captured_collision_callback() + # Exit this context manager to wait for the subject to wrap up cleanly, + # ensuring its collision handling has run to completion before we assert stuff. + + decoy.verify( + # Asserting this exact order is one way to make sure the subject avoids + # persisting invalid names that can't be advertised. + # https://github.com/Opentrons/opentrons/issues/9960. + await mock_avahi_client.start_advertising("alternative name"), + await mock_persist_pretty_hostname("alternative name"), + ) + + +def test_set() -> None: + # Test when given a name that Avahi doesn't like. + raise NotImplementedError() + + +def test_get() -> None: + raise NotImplementedError() From 9b8ba06e4cc556a151a51c36faf8a9c7614d23a9 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 3 Jun 2022 16:49:54 -0400 Subject: [PATCH 24/41] Fill out other `RealNameSynchronizer` tests. --- .../name_management/test_name_synchronizer.py | 110 ++++++++++++++++-- 1 file changed, 100 insertions(+), 10 deletions(-) diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index 157cb97d7d9..89c4e72a3bd 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -43,11 +43,24 @@ def mock_avahi_client(decoy: Decoy) -> AvahiClient: return decoy.mock(cls=AvahiClient) -def test_build_and_startup() -> None: - # It should start advertising with the current pretty hostname - # as the Avahi service name. - raise NotImplementedError() - # decoy.verify(await mock_avahi_client.start_advertising("initial name")) +async def test_advertises_name( + mock_get_pretty_hostname: Callable[[], str], + monkeypatch_get_pretty_hostname: None, + mock_persist_pretty_hostname: Callable[[str], str], + monkeypatch_persist_pretty_hostname: None, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + decoy.when(mock_get_pretty_hostname()).then_return("initial name") + decoy.when( + mock_avahi_client.collision_callback(matchers.Anything()) + ).then_enter_with( + # https://github.com/mcous/decoy/issues/135 + "" + ) + + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + decoy.verify(await mock_avahi_client.start_advertising("initial name")) async def test_collision_handling( @@ -102,10 +115,87 @@ async def test_collision_handling( ) -def test_set() -> None: - # Test when given a name that Avahi doesn't like. - raise NotImplementedError() +async def test_set( + mock_get_pretty_hostname: Callable[[], str], + monkeypatch_get_pretty_hostname: None, + mock_persist_pretty_hostname: Callable[[str], str], + monkeypatch_persist_pretty_hostname: None, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + """It should set the new name as the Avahi service name and then as the pretty + hostname. + """ + decoy.when(mock_get_pretty_hostname()).then_return("initial name") + decoy.when( + mock_avahi_client.collision_callback(matchers.Anything()) + ).then_enter_with( + # https://github.com/mcous/decoy/issues/135 + "" + ) + + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + await subject.set_name("new name") + + decoy.verify( + await mock_avahi_client.start_advertising("new name"), + await mock_persist_pretty_hostname("new name"), + ) + + +async def test_set_does_not_persist_invalid_avahi_service_name( + mock_get_pretty_hostname: Callable[[], str], + monkeypatch_get_pretty_hostname: None, + mock_persist_pretty_hostname: Callable[[str], str], + monkeypatch_persist_pretty_hostname: None, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + """It should not persist any name that's not valid as an Avahi service name. + + Covers this bug: + https://github.com/Opentrons/opentrons/issues/9960 + """ + + decoy.when(mock_get_pretty_hostname()).then_return("initial name, no danger") + decoy.when( + mock_avahi_client.collision_callback(matchers.Anything()) + ).then_enter_with( + # https://github.com/mcous/decoy/issues/135 + "" + ) + + decoy.when( + await mock_avahi_client.start_advertising("danger!") + ).then_raise(Exception("oh the humanity")) + + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + with pytest.raises(Exception, match="oh the humanity"): + await subject.set_name("danger!") + + decoy.verify( + await mock_persist_pretty_hostname(matchers.Anything()), + times=0 + ) + + +async def test_get( + mock_get_pretty_hostname: Callable[[], str], + monkeypatch_get_pretty_hostname: None, + mock_persist_pretty_hostname: Callable[[str], str], + monkeypatch_persist_pretty_hostname: None, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + decoy.when(mock_get_pretty_hostname()).then_return("initial name, no danger") + decoy.when( + mock_avahi_client.collision_callback(matchers.Anything()) + ).then_enter_with( + # https://github.com/mcous/decoy/issues/135 + "" + ) + decoy.when(mock_get_pretty_hostname()).then_return("the current name") -def test_get() -> None: - raise NotImplementedError() + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + assert subject.get_name() == "the current name" From 4b53b357ee640d0534b2f4ed1261d3956c7fc72e Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 3 Jun 2022 17:19:31 -0400 Subject: [PATCH 25/41] Remove unnecessary `async` for `persist_pretty_hostname()`. --- .../otupdate/common/name_management/name_synchronizer.py | 2 +- .../otupdate/common/name_management/pretty_hostname.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 60d769c48c8..134145eaacf 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -120,7 +120,7 @@ async def set_name(self, new_name: str) -> str: await self._avahi_client.start_advertising(service_name=new_name) # Setting the Avahi service name can fail if Avahi doesn't like the new name. # Persist only after it succeeds, so we don't persist something invalid. - persisted_pretty_hostname = await persist_pretty_hostname(new_name) + persisted_pretty_hostname = persist_pretty_hostname(new_name) return persisted_pretty_hostname def get_name(self) -> str: diff --git a/update-server/otupdate/common/name_management/pretty_hostname.py b/update-server/otupdate/common/name_management/pretty_hostname.py index 1be093af9c1..1a958a25e72 100644 --- a/update-server/otupdate/common/name_management/pretty_hostname.py +++ b/update-server/otupdate/common/name_management/pretty_hostname.py @@ -31,7 +31,7 @@ def get_pretty_hostname(default: str = "no name set") -> str: return default -async def persist_pretty_hostname(name: str) -> str: +def persist_pretty_hostname(name: str) -> str: """Change the robot's pretty hostname. Writes the new name to /etc/machine-info so it persists across reboots. From 239b15c010ef6aa4680c5bfe36978134802103a4 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 3 Jun 2022 17:28:56 -0400 Subject: [PATCH 26/41] Clean up `NameSynchronizer` tests. --- .../name_management/name_synchronizer.py | 2 +- .../name_management/test_name_synchronizer.py | 207 +++++++++--------- 2 files changed, 107 insertions(+), 102 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 134145eaacf..ff4bf810437 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -94,7 +94,7 @@ def __init__(self, avahi_client: AvahiClient) -> None: @asynccontextmanager async def build( cls, avahi_client: AvahiClient - ) -> AsyncGenerator[NameSynchronizer, None]: + ) -> AsyncGenerator[RealNameSynchronizer, None]: """Build a RealNameSynchronizer and keep it running in the background. Avahi advertisements will start as soon as this context manager is entered. diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index 89c4e72a3bd..166a80e90a8 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -1,6 +1,5 @@ import asyncio -from contextlib import asynccontextmanager -from typing import Callable +from typing import Any, AsyncGenerator, Callable import pytest from decoy import Decoy, matchers @@ -14,25 +13,37 @@ ) +_GET_PRETTY_HOSTNAME_SIGNATURE = Callable[[], str] +_PERSIST_PRETTY_HOSTNAME_SIGNATURE = Callable[[str], str] + + @pytest.fixture -def mock_get_pretty_hostname(decoy: Decoy): +def mock_get_pretty_hostname(decoy: Decoy) -> _GET_PRETTY_HOSTNAME_SIGNATURE: + """Return a mock in the shape of the get_pretty_hostname() function.""" return decoy.mock(func=real_get_pretty_hostname) @pytest.fixture -def monkeypatch_get_pretty_hostname(monkeypatch, mock_get_pretty_hostname): +def monkeypatch_get_pretty_hostname( + monkeypatch: Any, mock_get_pretty_hostname: _GET_PRETTY_HOSTNAME_SIGNATURE +) -> None: + """Replace the real get_pretty_hostname() with our mock.""" monkeypatch.setattr( name_synchronizer, "get_pretty_hostname", mock_get_pretty_hostname ) @pytest.fixture -def mock_persist_pretty_hostname(decoy: Decoy): +def mock_persist_pretty_hostname(decoy: Decoy) -> _PERSIST_PRETTY_HOSTNAME_SIGNATURE: + """Return a mock in the shape of the persist_pretty_hostname() function.""" return decoy.mock(func=real_persist_pretty_hostname) @pytest.fixture -def monkeypatch_persist_pretty_hostname(monkeypatch, mock_persist_pretty_hostname): +def monkeypatch_persist_pretty_hostname( + monkeypatch: Any, mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE +) -> None: + """Replace the real persist_pretty_hostname with our mock.""" monkeypatch.setattr( name_synchronizer, "persist_pretty_hostname", mock_persist_pretty_hostname ) @@ -40,33 +51,113 @@ def monkeypatch_persist_pretty_hostname(monkeypatch, mock_persist_pretty_hostnam @pytest.fixture def mock_avahi_client(decoy: Decoy) -> AvahiClient: + """Return a mock in the shape of an AvahiClient.""" return decoy.mock(cls=AvahiClient) -async def test_advertises_name( - mock_get_pretty_hostname: Callable[[], str], +@pytest.fixture +async def started_up_subject( + monkeypatch_get_pretty_hostname: None, + monkeypatch_persist_pretty_hostname: None, + mock_avahi_client: AvahiClient, + decoy: Decoy, + loop: asyncio.AbstractEventLoop, # Required by aiohttp for async fixtures. +) -> AsyncGenerator[RealNameSynchronizer, None]: + """Return a subject RealNameSynchronizer that's set up with mock dependencies, + and that's already started up and running. + + Tests that need to operate in the subject's startup phase itself + should build the subject manually instead of using this fixture. + """ + # RealNameSynchronizer.build() will call mock_avahi_client.collision_callback() + # and expect to be able to enter its result as a context manager. + decoy.when( + mock_avahi_client.collision_callback(matchers.Anything()) + ).then_enter_with( + # https://github.com/mcous/decoy/issues/135 + "" # type: ignore[arg-type] + ) + + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + yield subject + + +async def test_set( + started_up_subject: RealNameSynchronizer, + mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + """It should set the new name as the Avahi service name and then as the pretty + hostname. + """ + await started_up_subject.set_name("new name") + + decoy.verify( + await mock_avahi_client.start_advertising("new name"), + mock_persist_pretty_hostname("new name"), + ) + + +async def test_set_does_not_persist_invalid_avahi_service_name( + started_up_subject: RealNameSynchronizer, + mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, + mock_avahi_client: AvahiClient, + decoy: Decoy, +) -> None: + """It should not persist any name that's not valid as an Avahi service name. + + Covers this bug: + https://github.com/Opentrons/opentrons/issues/9960 + """ + decoy.when(await mock_avahi_client.start_advertising("danger!")).then_raise( + Exception("oh the humanity") + ) + + with pytest.raises(Exception, match="oh the humanity"): + await started_up_subject.set_name("danger!") + + decoy.verify(mock_persist_pretty_hostname(matchers.Anything()), times=0) + + +async def test_get( + started_up_subject: RealNameSynchronizer, + mock_get_pretty_hostname: _GET_PRETTY_HOSTNAME_SIGNATURE, + decoy: Decoy, +) -> None: + decoy.when(mock_get_pretty_hostname()).then_return("the current name") + assert started_up_subject.get_name() == "the current name" + + +async def test_advertises_initial_name( + started_up_subject: RealNameSynchronizer, + mock_get_pretty_hostname: _GET_PRETTY_HOSTNAME_SIGNATURE, monkeypatch_get_pretty_hostname: None, - mock_persist_pretty_hostname: Callable[[str], str], + mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, monkeypatch_persist_pretty_hostname: None, mock_avahi_client: AvahiClient, decoy: Decoy, ) -> None: + """It should immediately start advertising the existing pretty hostname + as the Avahi service name, when it's started up. + """ + decoy.when(mock_get_pretty_hostname()).then_return("initial name") decoy.when( mock_avahi_client.collision_callback(matchers.Anything()) ).then_enter_with( # https://github.com/mcous/decoy/issues/135 - "" + "" # type: ignore[arg-type] ) - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): decoy.verify(await mock_avahi_client.start_advertising("initial name")) async def test_collision_handling( - mock_get_pretty_hostname: Callable[[], str], + mock_get_pretty_hostname: _GET_PRETTY_HOSTNAME_SIGNATURE, monkeypatch_get_pretty_hostname: None, - mock_persist_pretty_hostname: Callable[[str], str], + mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, monkeypatch_persist_pretty_hostname: None, mock_avahi_client: AvahiClient, decoy: Decoy, @@ -96,7 +187,7 @@ async def test_collision_handling( mock_avahi_client.collision_callback(collision_callback_captor) ).then_enter_with( # https://github.com/mcous/decoy/issues/135 - "" + "" # type: ignore[arg-type] ) async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): @@ -111,91 +202,5 @@ async def test_collision_handling( # persisting invalid names that can't be advertised. # https://github.com/Opentrons/opentrons/issues/9960. await mock_avahi_client.start_advertising("alternative name"), - await mock_persist_pretty_hostname("alternative name"), + mock_persist_pretty_hostname("alternative name"), ) - - -async def test_set( - mock_get_pretty_hostname: Callable[[], str], - monkeypatch_get_pretty_hostname: None, - mock_persist_pretty_hostname: Callable[[str], str], - monkeypatch_persist_pretty_hostname: None, - mock_avahi_client: AvahiClient, - decoy: Decoy, -) -> None: - """It should set the new name as the Avahi service name and then as the pretty - hostname. - """ - decoy.when(mock_get_pretty_hostname()).then_return("initial name") - decoy.when( - mock_avahi_client.collision_callback(matchers.Anything()) - ).then_enter_with( - # https://github.com/mcous/decoy/issues/135 - "" - ) - - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: - await subject.set_name("new name") - - decoy.verify( - await mock_avahi_client.start_advertising("new name"), - await mock_persist_pretty_hostname("new name"), - ) - - -async def test_set_does_not_persist_invalid_avahi_service_name( - mock_get_pretty_hostname: Callable[[], str], - monkeypatch_get_pretty_hostname: None, - mock_persist_pretty_hostname: Callable[[str], str], - monkeypatch_persist_pretty_hostname: None, - mock_avahi_client: AvahiClient, - decoy: Decoy, -) -> None: - """It should not persist any name that's not valid as an Avahi service name. - - Covers this bug: - https://github.com/Opentrons/opentrons/issues/9960 - """ - - decoy.when(mock_get_pretty_hostname()).then_return("initial name, no danger") - decoy.when( - mock_avahi_client.collision_callback(matchers.Anything()) - ).then_enter_with( - # https://github.com/mcous/decoy/issues/135 - "" - ) - - decoy.when( - await mock_avahi_client.start_advertising("danger!") - ).then_raise(Exception("oh the humanity")) - - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: - with pytest.raises(Exception, match="oh the humanity"): - await subject.set_name("danger!") - - decoy.verify( - await mock_persist_pretty_hostname(matchers.Anything()), - times=0 - ) - - -async def test_get( - mock_get_pretty_hostname: Callable[[], str], - monkeypatch_get_pretty_hostname: None, - mock_persist_pretty_hostname: Callable[[str], str], - monkeypatch_persist_pretty_hostname: None, - mock_avahi_client: AvahiClient, - decoy: Decoy, -) -> None: - decoy.when(mock_get_pretty_hostname()).then_return("initial name, no danger") - decoy.when( - mock_avahi_client.collision_callback(matchers.Anything()) - ).then_enter_with( - # https://github.com/mcous/decoy/issues/135 - "" - ) - - decoy.when(mock_get_pretty_hostname()).then_return("the current name") - - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: - assert subject.get_name() == "the current name" From c3467d9a5bd2a6470f79f89caafb43e427f7080c Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 11:25:49 -0400 Subject: [PATCH 27/41] Update Decoy to get the fix for mcous/decoy#135. --- update-server/Pipfile.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/update-server/Pipfile.lock b/update-server/Pipfile.lock index f75c700b742..cced41f3d19 100644 --- a/update-server/Pipfile.lock +++ b/update-server/Pipfile.lock @@ -330,11 +330,11 @@ }, "decoy": { "hashes": [ - "sha256:599d6f4b6e67b74a3d8edf7a432b0b7d5d3c0296f7733fe96f2f4be4360bbfec", - "sha256:d84bd95816339ab2fda65aa6ced3291a0739bbc83ca82344c8543a801a082d56" + "sha256:6d2088d95a4c020cc546e36f551145aa765bf54c73779d818b9e9cc9e970723e", + "sha256:8c5a960f35976f1365aa859355214b2cfb7a7fbc2064c7d32a5156d051e8f2f1" ], "index": "pypi", - "version": "==1.10.3" + "version": "==1.11.0" }, "docopt": { "hashes": [ From 3bf799aa2c54bdcdd911e3029971bb43f67aa787 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 11:36:15 -0400 Subject: [PATCH 28/41] Remove todo for mcous/decoy#135. --- .../tests/common/name_management/test_name_synchronizer.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index 166a80e90a8..b36202dbebc 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -73,10 +73,7 @@ async def started_up_subject( # and expect to be able to enter its result as a context manager. decoy.when( mock_avahi_client.collision_callback(matchers.Anything()) - ).then_enter_with( - # https://github.com/mcous/decoy/issues/135 - "" # type: ignore[arg-type] - ) + ).then_enter_with(None) async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: yield subject From b4cfb3ec2487cbdd51e1c3474f41442e899b9be6 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 11:35:32 -0400 Subject: [PATCH 29/41] Fix tests not asserting that context manager was entered. --- .../name_management/test_name_synchronizer.py | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index b36202dbebc..979b3ff45dc 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -140,15 +140,17 @@ async def test_advertises_initial_name( """ decoy.when(mock_get_pretty_hostname()).then_return("initial name") - decoy.when( - mock_avahi_client.collision_callback(matchers.Anything()) - ).then_enter_with( - # https://github.com/mcous/decoy/issues/135 - "" # type: ignore[arg-type] + mock_collision_subscription_context_manager = decoy.mock() + decoy.when(mock_avahi_client.collision_callback(matchers.Anything())).then_return( + mock_collision_subscription_context_manager ) async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): - decoy.verify(await mock_avahi_client.start_advertising("initial name")) + decoy.verify( + # It should only start advertising after subscribing to collisions. + await mock_collision_subscription_context_manager.__aenter__(), + await mock_avahi_client.start_advertising("initial name"), + ) async def test_collision_handling( @@ -179,12 +181,12 @@ async def test_collision_handling( # # When it does, save the function that it provided as `some_callback_func` # into `collision_callback_captor.value`. + mock_collision_subscription_context_manager = decoy.mock() collision_callback_captor = matchers.Captor() decoy.when( mock_avahi_client.collision_callback(collision_callback_captor) - ).then_enter_with( - # https://github.com/mcous/decoy/issues/135 - "" # type: ignore[arg-type] + ).then_return( + mock_collision_subscription_context_manager ) async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): @@ -195,9 +197,11 @@ async def test_collision_handling( # ensuring its collision handling has run to completion before we assert stuff. decoy.verify( - # Asserting this exact order is one way to make sure the subject avoids - # persisting invalid names that can't be advertised. - # https://github.com/Opentrons/opentrons/issues/9960. + await mock_collision_subscription_context_manager.__aenter__(), + await mock_avahi_client.start_advertising("initial name"), + # Asserting that the subject advertised the alternative name before persisting + # it is one way to ensurethat it doesn't persist invalid names that can't be + # advertised. https://github.com/Opentrons/opentrons/issues/9960. await mock_avahi_client.start_advertising("alternative name"), mock_persist_pretty_hostname("alternative name"), ) From 7c3e707e3749bf0bd198d6146041b5240a2d7b90 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 12:37:12 -0400 Subject: [PATCH 30/41] Initialize `NameSynchronizer` outside app and inject a mock in tests. --- update-server/otupdate/buildroot/__init__.py | 72 ++++++-------- update-server/otupdate/buildroot/__main__.py | 26 +++-- update-server/otupdate/common/control.py | 4 +- .../common/name_management/__init__.py | 13 ++- .../name_management/name_synchronizer.py | 96 +++++++------------ .../otupdate/common/run_application.py | 35 +++++++ .../otupdate/openembedded/__init__.py | 77 +++++++-------- .../otupdate/openembedded/__main__.py | 28 ++++-- update-server/tests/buildroot/conftest.py | 10 +- update-server/tests/buildroot/test_control.py | 13 ++- update-server/tests/common/conftest.py | 4 +- .../name_management/test_http_endpoints.py | 31 ++---- .../name_management/test_name_synchronizer.py | 26 +++-- update-server/tests/conftest.py | 10 ++ update-server/tests/openembedded/conftest.py | 6 +- .../tests/openembedded/test_control.py | 13 ++- 16 files changed, 244 insertions(+), 220 deletions(-) create mode 100644 update-server/otupdate/common/run_application.py diff --git a/update-server/otupdate/buildroot/__init__.py b/update-server/otupdate/buildroot/__init__.py index 0637bfd0934..b562d4e4ff0 100644 --- a/update-server/otupdate/buildroot/__init__.py +++ b/update-server/otupdate/buildroot/__init__.py @@ -2,7 +2,8 @@ import asyncio import logging import json -from typing import Any, AsyncGenerator, Mapping, Optional +from typing import Any, Mapping, Optional + from aiohttp import web from otupdate.common import ( @@ -11,7 +12,6 @@ control, name_management, ssh_key_management, - systemd, update, ) from . import update_actions @@ -39,6 +39,7 @@ def get_version(version_file: str) -> Mapping[str, str]: def get_app( + name_synchronizer: name_management.NameSynchronizer, system_version_file: Optional[str] = None, config_file_override: Optional[str] = None, name_override: Optional[str] = None, @@ -57,47 +58,11 @@ def get_app( app = web.Application(middlewares=[log_error_middleware]) - async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, None]: - app[config.CONFIG_VARNAME] = config_obj - app[constants.RESTART_LOCK_NAME] = asyncio.Lock() - app[constants.DEVICE_BOOT_ID_NAME] = boot_id - - update_actions.OT2UpdateActions.build_and_insert(app) - - async with name_management.build_name_synchronizer( - name_override=name_override - ) as name_synchronizer: - name_synchronizer.install_on_app(app) - initial_name = name_synchronizer.get_name() - - LOG.info( - "Setup: " - + "\n\t".join( - [ - f"Device name: {initial_name}", - "Buildroot version: " - f'{version.get("buildroot_version", "unknown")}', - "\t(from git sha " - f'{version.get("buildroot_sha", "unknown")}', - "API version: " - f'{version.get("opentrons_api_version", "unknown")}', - "\t(from git sha " - f'{version.get("opentrons_api_sha", "unknown")}', - "Update server version: " - f'{version.get("update_server_version", "unknown")}', - "\t(from git sha " - f'{version.get("update_server_sha", "unknown")}', - "Smoothie firmware version: TODO", - ] - ) - ) - - LOG.info(f"Notifying {systemd.SOURCE} that service is up.") - systemd.notify_up() - - yield - - app.cleanup_ctx.append(set_up_and_tear_down) + app[config.CONFIG_VARNAME] = config_obj + app[constants.RESTART_LOCK_NAME] = asyncio.Lock() + app[constants.DEVICE_BOOT_ID_NAME] = boot_id + update_actions.OT2UpdateActions.build_and_insert(app) + name_management.install_name_synchronizer(name_synchronizer, app) app.router.add_routes( [ @@ -120,6 +85,27 @@ async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, Non ] ) + LOG.info( + "Setup: " + + "\n\t".join( + [ + f"Device name: {name_synchronizer.get_name()}", + "Buildroot version: " + f'{version.get("buildroot_version", "unknown")}', + "\t(from git sha " f'{version.get("buildroot_sha", "unknown")}', + "API version: " + f'{version.get("opentrons_api_version", "unknown")}', + "\t(from git sha " + f'{version.get("opentrons_api_sha", "unknown")}', + "Update server version: " + f'{version.get("update_server_version", "unknown")}', + "\t(from git sha " + f'{version.get("update_server_sha", "unknown")}', + "Smoothie firmware version: TODO", + ] + ) + ) + return app diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index 8535b9474e6..92dc9d91106 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -3,34 +3,42 @@ """ import asyncio import logging +from typing import NoReturn from . import get_app from otupdate.common import name_management, cli, systemd -from aiohttp import web +from otupdate.common.run_application import run_application + LOG = logging.getLogger(__name__) -def main() -> None: +async def main() -> NoReturn: parser = cli.build_root_parser() args = parser.parse_args() - loop = asyncio.get_event_loop() systemd.configure_logging(getattr(logging, args.log_level.upper())) # Because this involves restarting Avahi, this must happen early, # before the NameSynchronizer starts up and connects to Avahi. LOG.info("Setting static hostname") - static_hostname = loop.run_until_complete(name_management.set_up_static_hostname()) + static_hostname = await name_management.set_up_static_hostname() LOG.info(f"Set static hostname to {static_hostname}") - LOG.info("Building buildroot update server") - app = get_app(args.version_file, args.config_file) + async with name_management.build_name_synchronizer( + name_override=None + ) as name_synchronizer: + LOG.info("Building buildroot update server") + app = get_app( + system_version_file=args.version_file, + config_file_override=args.config_file, + name_synchronizer=name_synchronizer, + ) - LOG.info(f"Starting buildroot update server on http://{args.host}:{args.port}") - web.run_app(app, host=args.host, port=args.port) # type: ignore[no-untyped-call] + LOG.info(f"Starting buildroot update server on http://{args.host}:{args.port}") + await run_application(app=app, host=args.host, port=args.port) if __name__ == "__main__": - main() + asyncio.run(main()) diff --git a/update-server/otupdate/common/control.py b/update-server/otupdate/common/control.py index f0897ff075e..5b31221e722 100644 --- a/update-server/otupdate/common/control.py +++ b/update-server/otupdate/common/control.py @@ -13,7 +13,7 @@ from aiohttp import web from .constants import RESTART_LOCK_NAME, DEVICE_BOOT_ID_NAME -from .name_management import NameSynchronizer +from .name_management import get_name_synchronizer LOG = logging.getLogger(__name__) @@ -42,7 +42,7 @@ async def health(request: web.Request) -> web.Response: { **health_response, **{ - "name": NameSynchronizer.from_request(request).get_name(), + "name": get_name_synchronizer(request).get_name(), "serialNumber": get_serial(), "bootId": request.app[DEVICE_BOOT_ID_NAME], }, diff --git a/update-server/otupdate/common/name_management/__init__.py b/update-server/otupdate/common/name_management/__init__.py index bee80687144..13472547132 100644 --- a/update-server/otupdate/common/name_management/__init__.py +++ b/update-server/otupdate/common/name_management/__init__.py @@ -47,7 +47,12 @@ from aiohttp import web -from .name_synchronizer import NameSynchronizer, build_name_synchronizer +from .name_synchronizer import ( + NameSynchronizer, + build_name_synchronizer, + install_name_synchronizer, + get_name_synchronizer, +) from .static_hostname import set_up_static_hostname @@ -90,7 +95,7 @@ def build_400(msg: str) -> web.Response: if not isinstance(name_to_set, str): return build_400('"name" key is not a string"') - name_synchronizer = NameSynchronizer.from_request(request) + name_synchronizer = get_name_synchronizer(request) new_name = await name_synchronizer.set_name(new_name=name_to_set) return web.json_response( # type: ignore[no-untyped-call,no-any-return] @@ -106,7 +111,7 @@ async def get_name_endpoint(request: web.Request) -> web.Response: GET /server/name -> 200 OK, {'name': robot name} """ - name_synchronizer = NameSynchronizer.from_request(request) + name_synchronizer = get_name_synchronizer(request) return web.json_response( # type: ignore[no-untyped-call,no-any-return] data={"name": name_synchronizer.get_name()}, status=200 ) @@ -115,6 +120,8 @@ async def get_name_endpoint(request: web.Request) -> web.Response: __all__ = [ "NameSynchronizer", "build_name_synchronizer", + "install_name_synchronizer", + "get_name_synchronizer", "set_up_static_hostname", "get_name_endpoint", "set_name_endpoint", diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index ff4bf810437..6062e665108 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -1,6 +1,5 @@ from __future__ import annotations -from abc import ABC, abstractmethod from contextlib import asynccontextmanager from logging import getLogger from typing import AsyncGenerator, Optional @@ -16,7 +15,7 @@ _log = getLogger(__name__) -class NameSynchronizer(ABC): +class NameSynchronizer: """Keep the machine's human-readable names in sync with each other. This ties the pretty hostname and the Avahi service name together, @@ -42,50 +41,6 @@ class NameSynchronizer(ABC): configurable. https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1 """ - @classmethod - def from_request(cls, request: web.Request) -> NameSynchronizer: - """Return the server's singleton NameSynchronizer from a request. - - The singleton NameSynchronizer is expected to have been installed on the - aiohttp.Application already via `install_on_app()`. - """ - name_synchronizer = request.app.get(_NAME_SYNCHRONIZER_VARNAME, None) - assert isinstance( - name_synchronizer, NameSynchronizer - ), f"Unexpected type {type(name_synchronizer)}. Incorrect Application setup?" - return name_synchronizer - - def install_on_app(self, app: web.Application) -> None: - """Install this NameSynchronizer on `app` for later retrieval via - `from_request()`. - - This should be done as part of server startup. - """ - app[_NAME_SYNCHRONIZER_VARNAME] = self - - @abstractmethod - async def set_name(self, new_name: str) -> str: - """Set the machine's human-readable name. - - This first sets thhe Avahi service name, and then persists it - as the pretty hostname. - - Returns the new name. This is normally the same as the requested name, - but it it might be different if it had to be truncated, sanitized, etc. - """ - - @abstractmethod - def get_name(self) -> str: - """Return the machine's current human-readable name. - - Note that this can change even if you haven't called `set_name()`, - if it was necessary to avoid conflicts with other devices on the network. - """ - - -class RealNameSynchronizer(NameSynchronizer): - """A functioning implementation of NameSynchronizer, to run in production.""" - def __init__(self, avahi_client: AvahiClient) -> None: """For internal use by this class only. Use `build()` instead.""" self._avahi_client = avahi_client @@ -94,7 +49,7 @@ def __init__(self, avahi_client: AvahiClient) -> None: @asynccontextmanager async def build( cls, avahi_client: AvahiClient - ) -> AsyncGenerator[RealNameSynchronizer, None]: + ) -> AsyncGenerator[NameSynchronizer, None]: """Build a RealNameSynchronizer and keep it running in the background. Avahi advertisements will start as soon as this context manager is entered. @@ -117,6 +72,14 @@ async def build( yield name_synchronizer async def set_name(self, new_name: str) -> str: + """Set the machine's human-readable name. + + This first sets thhe Avahi service name, and then persists it + as the pretty hostname. + + Returns the new name. This is normally the same as the requested name, + but it it might be different if it had to be truncated, sanitized, etc. + """ await self._avahi_client.start_advertising(service_name=new_name) # Setting the Avahi service name can fail if Avahi doesn't like the new name. # Persist only after it succeeds, so we don't persist something invalid. @@ -124,6 +87,11 @@ async def set_name(self, new_name: str) -> str: return persisted_pretty_hostname def get_name(self) -> str: + """Return the machine's current human-readable name. + + Note that this can change even if you haven't called `set_name()`, + if it was necessary to avoid conflicts with other devices on the network. + """ return get_pretty_hostname() async def _on_avahi_collision(self) -> None: @@ -149,23 +117,28 @@ async def _on_avahi_collision(self) -> None: await self.set_name(new_name=alternative_name) -class FakeNameSynchronizer(NameSynchronizer): - """A dummy implementation of NameSynchronizer to use in integration tests. +def install_name_synchronizer( + name_synchronizer: NameSynchronizer, app: web.Application +) -> None: + """Install a NameSynchronizer on `app` for later retrieval + via get_name_synchronizer(). - update-server's integration tests run on systems where we don't have access to - an Avahi daemon. So RealNameSynchronizer wouldn't work. + This should be done as part of server startup. """ + app[_NAME_SYNCHRONIZER_VARNAME] = name_synchronizer - def __init__(self, name_override: str) -> None: - self._name_override = name_override - async def set_name(self, new_name: str) -> str: - raise NotImplementedError( - "Can't change the name when it's been overridden for testing." - ) +def get_name_synchronizer(request: web.Request) -> NameSynchronizer: + """Return the server's singleton NameSynchronizer from a request. - def get_name(self) -> str: - return self._name_override + The singleton NameSynchronizer is expected to have been installed on the + aiohttp.Application already via install_name_synchronizer(). + """ + name_synchronizer = request.app.get(_NAME_SYNCHRONIZER_VARNAME, None) + assert isinstance( + name_synchronizer, NameSynchronizer + ), f"Unexpected type {type(name_synchronizer)}. Incorrect Application setup?" + return name_synchronizer @asynccontextmanager @@ -175,9 +148,10 @@ async def build_name_synchronizer( """Return a RealNameSynchronizer for production or FakeNameManager for testing.""" if name_override is None: avahi_client = await AvahiClient.connect() - async with RealNameSynchronizer.build( + async with NameSynchronizer.build( avahi_client=avahi_client ) as real_name_synchronizer: yield real_name_synchronizer else: - yield FakeNameSynchronizer(name_override=name_override) + assert False # TODO + # yield FakeNameSynchronizer(name_override=name_override) diff --git a/update-server/otupdate/common/run_application.py b/update-server/otupdate/common/run_application.py new file mode 100644 index 00000000000..5acb47ef6e6 --- /dev/null +++ b/update-server/otupdate/common/run_application.py @@ -0,0 +1,35 @@ +import asyncio +from typing import NoReturn + +from aiohttp import web + +from . import systemd + + +async def run_application(app: web.Application, host: str, port: int) -> NoReturn: + """ + To document: + + * Never returns until the task is cancelled (process is shut down) + * Unlike standard run(), useful if we need to do async stuff outside of the + server + * Rename to mention notifying up + """ + runner = web.AppRunner(app) # type: ignore[no-untyped-call] + + await runner.setup() # type: ignore[no-untyped-call] + + try: + site = web.TCPSite( # type: ignore[no-untyped-call] + runner=runner, host=host, port=port + ) + await site.start() # type: ignore[no-untyped-call] + systemd.notify_up() + while True: + # It seems like there should be a better way of doing this, + # but this is what the docs recommend. + # https://docs.aiohttp.org/en/stable/web_advanced.html#application-runners + await asyncio.sleep(3600) + + finally: + await runner.cleanup() # type: ignore[no-untyped-call] diff --git a/update-server/otupdate/openembedded/__init__.py b/update-server/otupdate/openembedded/__init__.py index 31e15d22fa1..89dcf0cacd9 100644 --- a/update-server/otupdate/openembedded/__init__.py +++ b/update-server/otupdate/openembedded/__init__.py @@ -3,7 +3,7 @@ import logging import json from aiohttp import web -from typing import AsyncGenerator, Optional, Mapping, Any +from typing import Optional, Mapping, Any from otupdate.common import ( config, @@ -11,7 +11,6 @@ control, name_management, ssh_key_management, - systemd, update, ) @@ -45,6 +44,7 @@ def get_version_dict(version_file: Optional[str]) -> Mapping[str, str]: def get_app( + name_synchronizer: name_management.NameSynchronizer, system_version_file: Optional[str] = None, config_file_override: Optional[str] = None, name_override: Optional[str] = None, @@ -60,49 +60,16 @@ def get_app( app = web.Application(middlewares=[log_error_middleware]) - async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, None]: - app[config.CONFIG_VARNAME] = config_obj - app[constants.RESTART_LOCK_NAME] = asyncio.Lock() - app[constants.DEVICE_BOOT_ID_NAME] = boot_id - - rfs = RootFSInterface() - part_mgr = PartitionManager() - updater = Updater(rfs, part_mgr) - app[FILE_ACTIONS_VARNAME] = updater - - async with name_management.build_name_synchronizer( - name_override=name_override - ) as name_synchronizer: - name_synchronizer.install_on_app(app) - initial_name = name_synchronizer.get_name() - - LOG.info( - "Setup: " - + "\n\t".join( - [ - f"Device name: {initial_name}", - "Buildroot version: " - f'{version.get("buildroot_version", "unknown")}', - "\t(from git sha " - f'{version.get("buildroot_sha", "unknown")}', - "API version: " - f'{version.get("opentrons_api_version", "unknown")}', - "\t(from git sha " - f'{version.get("opentrons_api_sha", "unknown")}', - "Update server version: " - f'{version.get("update_server_version", "unknown")}', - "\t(from git sha " - f'{version.get("update_server_sha", "unknown")}', - ] - ) - ) - - LOG.info(f"Notifying {systemd.SOURCE} that service is up.") - systemd.notify_up() - - yield - - app.cleanup_ctx.append(set_up_and_tear_down) + app[config.CONFIG_VARNAME] = config_obj + app[constants.RESTART_LOCK_NAME] = asyncio.Lock() + app[constants.DEVICE_BOOT_ID_NAME] = boot_id + + rfs = RootFSInterface() + part_mgr = PartitionManager() + updater = Updater(rfs, part_mgr) + app[FILE_ACTIONS_VARNAME] = updater + + name_management.install_name_synchronizer(name_synchronizer, app) app.router.add_routes( [ @@ -125,6 +92,26 @@ async def set_up_and_tear_down(app: web.Application) -> AsyncGenerator[None, Non ] ) + LOG.info( + "Setup: " + + "\n\t".join( + [ + f"Device name: {name_synchronizer.get_name()}", + "Buildroot version: " + f'{version.get("buildroot_version", "unknown")}', + "\t(from git sha " f'{version.get("buildroot_sha", "unknown")}', + "API version: " + f'{version.get("opentrons_api_version", "unknown")}', + "\t(from git sha " + f'{version.get("opentrons_api_sha", "unknown")}', + "Update server version: " + f'{version.get("update_server_version", "unknown")}', + "\t(from git sha " + f'{version.get("update_server_sha", "unknown")}', + ] + ) + ) + return app diff --git a/update-server/otupdate/openembedded/__main__.py b/update-server/otupdate/openembedded/__main__.py index b14c7ed6669..9266e768fcc 100644 --- a/update-server/otupdate/openembedded/__main__.py +++ b/update-server/otupdate/openembedded/__main__.py @@ -3,34 +3,44 @@ """ import asyncio import logging +from typing import NoReturn from . import get_app from otupdate.common import name_management, cli, systemd -from aiohttp import web +from otupdate.common.run_application import run_application + LOG = logging.getLogger(__name__) -def main() -> None: +async def main() -> NoReturn: parser = cli.build_root_parser() args = parser.parse_args() - loop = asyncio.get_event_loop() systemd.configure_logging(getattr(logging, args.log_level.upper())) # Because this involves restarting Avahi, this must happen early, # before the NameSynchronizer starts up and connects to Avahi. LOG.info("Setting static hostname") - static_hostname = loop.run_until_complete(name_management.set_up_static_hostname()) + static_hostname = await name_management.set_up_static_hostname() LOG.info(f"Set static hostname to {static_hostname}") - LOG.info("Building openembedded update server") - app = get_app(args.version_file, args.config_file) + async with name_management.build_name_synchronizer( + name_override=None + ) as name_synchronizer: + LOG.info("Building openembedded update server") + app = get_app( + system_version_file=args.version_file, + config_file_override=args.config_file, + name_synchronizer=name_synchronizer, + ) - LOG.info(f"Starting openembedded update server on http://{args.host}:{args.port}") - web.run_app(app, host=args.host, port=args.port) # type: ignore[no-untyped-call] + LOG.info( + f"Starting openembedded update server on http://{args.host}:{args.port}" + ) + await run_application(app=app, host=args.host, port=args.port) if __name__ == "__main__": - main() + asyncio.run(main()) diff --git a/update-server/tests/buildroot/conftest.py b/update-server/tests/buildroot/conftest.py index 609a89e92da..d31e699ef76 100644 --- a/update-server/tests/buildroot/conftest.py +++ b/update-server/tests/buildroot/conftest.py @@ -10,14 +10,20 @@ @pytest.fixture -async def test_cli(aiohttp_client, otupdate_config, monkeypatch, version_file_path): +async def test_cli( + aiohttp_client, + otupdate_config, + monkeypatch, + version_file_path, + mock_name_synchronizer, +): """ Build an app using dummy versions, then build a test client and return it """ app = buildroot.get_app( + name_synchronizer=mock_name_synchronizer, system_version_file=version_file_path, config_file_override=otupdate_config, - name_override="opentrons-test", boot_id_override="dummy-boot-id-abc123", ) client = await aiohttp_client(app) diff --git a/update-server/tests/buildroot/test_control.py b/update-server/tests/buildroot/test_control.py index b56c3149588..15e2afbf46b 100644 --- a/update-server/tests/buildroot/test_control.py +++ b/update-server/tests/buildroot/test_control.py @@ -2,14 +2,23 @@ from typing import Dict from aiohttp.test_utils import TestClient +from decoy import Decoy +from otupdate.common.name_management import NameSynchronizer -async def test_health(test_cli: TestClient, version_dict: Dict[str, str]): + +async def test_health( + test_cli: TestClient, + version_dict: Dict[str, str], + mock_name_synchronizer: NameSynchronizer, + decoy: Decoy, +): + decoy.when(mock_name_synchronizer.get_name()).then_return("test name") resp = await test_cli.get("/server/update/health") assert resp.status == 200 body = await resp.json() assert body == { - "name": "opentrons-test", + "name": "test name", "updateServerVersion": version_dict["update_server_version"], "apiServerVersion": version_dict["opentrons_api_version"], "smoothieVersion": "unimplemented", diff --git a/update-server/tests/common/conftest.py b/update-server/tests/common/conftest.py index fd409ed02cd..1a3b1795cbd 100644 --- a/update-server/tests/common/conftest.py +++ b/update-server/tests/common/conftest.py @@ -23,16 +23,16 @@ @pytest.fixture(params=[openembedded, buildroot]) async def test_cli( - aiohttp_client, otupdate_config, request, version_file_path + aiohttp_client, otupdate_config, request, version_file_path, mock_name_synchronizer ) -> Tuple[TestClient, str]: """ Build an app using dummy versions, then build a test client and return it """ cli_client_pkg = request.param app = cli_client_pkg.get_app( + name_synchronizer=mock_name_synchronizer, system_version_file=version_file_path, config_file_override=otupdate_config, - name_override="opentrons-test", boot_id_override="dummy-boot-id-abc123", ) client = await aiohttp_client(app) diff --git a/update-server/tests/common/name_management/test_http_endpoints.py b/update-server/tests/common/name_management/test_http_endpoints.py index 3343a71f62c..497078b5ecd 100644 --- a/update-server/tests/common/name_management/test_http_endpoints.py +++ b/update-server/tests/common/name_management/test_http_endpoints.py @@ -1,38 +1,21 @@ -from unittest.mock import MagicMock - -import pytest - -from otupdate.common.name_management import NameSynchronizer - - -@pytest.fixture -def mock_name_synchronizer(): - return MagicMock(spec=NameSynchronizer) - - -async def test_get_name(test_cli, monkeypatch, mock_name_synchronizer) -> None: - mock_name_synchronizer.get_name.return_value = "the returned name" - monkeypatch.setattr( - NameSynchronizer, "from_request", lambda request: mock_name_synchronizer - ) +async def test_get_name(test_cli, mock_name_synchronizer, decoy) -> None: + decoy.when(mock_name_synchronizer.get_name()).then_return("the returned name") response = await (test_cli[0].get("/server/name")) assert response.status == 200 + body = await response.json() assert body["name"] == "the returned name" -async def test_set_name_valid(test_cli, monkeypatch, mock_name_synchronizer) -> None: - async def mock_set_name(new_name: str) -> str: - return "the returned name" - - mock_name_synchronizer.set_name = mock_set_name - monkeypatch.setattr( - NameSynchronizer, "from_request", lambda request: mock_name_synchronizer +async def test_set_name_valid(test_cli, mock_name_synchronizer, decoy) -> None: + decoy.when(await mock_name_synchronizer.set_name("the input name")).then_return( + "the returned name" ) response = await test_cli[0].post("/server/name", json={"name": "the input name"}) assert response.status == 200 + body = await response.json() assert body["name"] == "the returned name" diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index 979b3ff45dc..b4e5eddd9a2 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -5,7 +5,7 @@ from decoy import Decoy, matchers from otupdate.common.name_management import name_synchronizer -from otupdate.common.name_management.name_synchronizer import RealNameSynchronizer +from otupdate.common.name_management.name_synchronizer import NameSynchronizer from otupdate.common.name_management.avahi import AvahiClient from otupdate.common.name_management.pretty_hostname import ( get_pretty_hostname as real_get_pretty_hostname, @@ -62,25 +62,25 @@ async def started_up_subject( mock_avahi_client: AvahiClient, decoy: Decoy, loop: asyncio.AbstractEventLoop, # Required by aiohttp for async fixtures. -) -> AsyncGenerator[RealNameSynchronizer, None]: - """Return a subject RealNameSynchronizer that's set up with mock dependencies, +) -> AsyncGenerator[NameSynchronizer, None]: + """Return a subject NameSynchronizer that's set up with mock dependencies, and that's already started up and running. Tests that need to operate in the subject's startup phase itself should build the subject manually instead of using this fixture. """ - # RealNameSynchronizer.build() will call mock_avahi_client.collision_callback() + # NameSynchronizer.build() will call mock_avahi_client.collision_callback() # and expect to be able to enter its result as a context manager. decoy.when( mock_avahi_client.collision_callback(matchers.Anything()) ).then_enter_with(None) - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + async with NameSynchronizer.build(avahi_client=mock_avahi_client) as subject: yield subject async def test_set( - started_up_subject: RealNameSynchronizer, + started_up_subject: NameSynchronizer, mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, mock_avahi_client: AvahiClient, decoy: Decoy, @@ -97,7 +97,7 @@ async def test_set( async def test_set_does_not_persist_invalid_avahi_service_name( - started_up_subject: RealNameSynchronizer, + started_up_subject: NameSynchronizer, mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, mock_avahi_client: AvahiClient, decoy: Decoy, @@ -118,7 +118,7 @@ async def test_set_does_not_persist_invalid_avahi_service_name( async def test_get( - started_up_subject: RealNameSynchronizer, + started_up_subject: NameSynchronizer, mock_get_pretty_hostname: _GET_PRETTY_HOSTNAME_SIGNATURE, decoy: Decoy, ) -> None: @@ -127,7 +127,7 @@ async def test_get( async def test_advertises_initial_name( - started_up_subject: RealNameSynchronizer, + started_up_subject: NameSynchronizer, mock_get_pretty_hostname: _GET_PRETTY_HOSTNAME_SIGNATURE, monkeypatch_get_pretty_hostname: None, mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, @@ -145,7 +145,7 @@ async def test_advertises_initial_name( mock_collision_subscription_context_manager ) - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): + async with NameSynchronizer.build(avahi_client=mock_avahi_client): decoy.verify( # It should only start advertising after subscribing to collisions. await mock_collision_subscription_context_manager.__aenter__(), @@ -185,11 +185,9 @@ async def test_collision_handling( collision_callback_captor = matchers.Captor() decoy.when( mock_avahi_client.collision_callback(collision_callback_captor) - ).then_return( - mock_collision_subscription_context_manager - ) + ).then_return(mock_collision_subscription_context_manager) - async with RealNameSynchronizer.build(avahi_client=mock_avahi_client): + async with NameSynchronizer.build(avahi_client=mock_avahi_client): captured_collision_callback = collision_callback_captor.value # Prompt the subject to run its collision-handling logic. await captured_collision_callback() diff --git a/update-server/tests/conftest.py b/update-server/tests/conftest.py index 9c1e76a982d..319d608b252 100644 --- a/update-server/tests/conftest.py +++ b/update-server/tests/conftest.py @@ -8,6 +8,10 @@ from typing import Dict import pytest +from decoy import Decoy + +from otupdate.common.name_management import NameSynchronizer + HERE = os.path.abspath(os.path.dirname(__file__)) @@ -143,3 +147,9 @@ def otupdate_config(request, tmpdir, testing_cert): conf.update({"update_cert_path": testing_cert}) json.dump(conf, open(path, "w")) return path + + +@pytest.fixture +def mock_name_synchronizer(decoy: Decoy) -> NameSynchronizer: + """Return a mock in the shape of a NameSynchronizer.""" + return decoy.mock(cls=NameSynchronizer) diff --git a/update-server/tests/openembedded/conftest.py b/update-server/tests/openembedded/conftest.py index d9cf5b720b2..b2db3b9c23c 100644 --- a/update-server/tests/openembedded/conftest.py +++ b/update-server/tests/openembedded/conftest.py @@ -26,14 +26,16 @@ def mock_update_actions_interface( @pytest.fixture -async def test_cli(aiohttp_client, otupdate_config, version_file_path) -> TestClient: +async def test_cli( + aiohttp_client, otupdate_config, version_file_path, mock_name_synchronizer +) -> TestClient: """ Build an app using dummy versions, then build a test client and return it """ app = openembedded.get_app( + name_synchronizer=mock_name_synchronizer, system_version_file=version_file_path, config_file_override=otupdate_config, - name_override="opentrons-test", boot_id_override="dummy-boot-id-abc123", ) client = await aiohttp_client(app) diff --git a/update-server/tests/openembedded/test_control.py b/update-server/tests/openembedded/test_control.py index e76afc29f49..4112371a7d6 100644 --- a/update-server/tests/openembedded/test_control.py +++ b/update-server/tests/openembedded/test_control.py @@ -3,14 +3,23 @@ from typing import Dict from aiohttp.test_utils import TestClient +from decoy import Decoy +from otupdate.common.name_management import NameSynchronizer -async def test_health(test_cli: TestClient, version_dict: Dict[str, str]): + +async def test_health( + test_cli: TestClient, + version_dict: Dict[str, str], + mock_name_synchronizer: NameSynchronizer, + decoy: Decoy, +): + decoy.when(mock_name_synchronizer.get_name()).then_return("test name") resp = await test_cli.get("/server/update/health") assert resp.status == 200 body = await resp.json() assert body == { - "name": "opentrons-test", + "name": "test name", "updateServerVersion": version_dict["update_server_version"], "apiServerVersion": version_dict["opentrons_api_version"], "systemVersion": version_dict["openembedded_version"], From 6ff5a9ba76ec3e1fd83bed5bb68167756e56368c Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 17:40:35 -0400 Subject: [PATCH 31/41] Simplify interface for starting `NameSynchronizer`. --- update-server/otupdate/buildroot/__main__.py | 4 +-- .../common/name_management/__init__.py | 2 -- .../name_management/name_synchronizer.py | 34 +++++++------------ .../otupdate/openembedded/__main__.py | 4 +-- .../name_management/test_name_synchronizer.py | 8 ++--- 5 files changed, 19 insertions(+), 33 deletions(-) diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index 92dc9d91106..9d4a11d25a6 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -26,9 +26,7 @@ async def main() -> NoReturn: static_hostname = await name_management.set_up_static_hostname() LOG.info(f"Set static hostname to {static_hostname}") - async with name_management.build_name_synchronizer( - name_override=None - ) as name_synchronizer: + async with name_management.NameSynchronizer.start() as name_synchronizer: LOG.info("Building buildroot update server") app = get_app( system_version_file=args.version_file, diff --git a/update-server/otupdate/common/name_management/__init__.py b/update-server/otupdate/common/name_management/__init__.py index 13472547132..0daecc0d573 100644 --- a/update-server/otupdate/common/name_management/__init__.py +++ b/update-server/otupdate/common/name_management/__init__.py @@ -49,7 +49,6 @@ from .name_synchronizer import ( NameSynchronizer, - build_name_synchronizer, install_name_synchronizer, get_name_synchronizer, ) @@ -119,7 +118,6 @@ async def get_name_endpoint(request: web.Request) -> web.Response: __all__ = [ "NameSynchronizer", - "build_name_synchronizer", "install_name_synchronizer", "get_name_synchronizer", "set_up_static_hostname", diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 6062e665108..ead3fcd0cad 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -27,7 +27,7 @@ class NameSynchronizer: See the `name_management` package docstring for an overview of these various names. - We tie all of these names together because + We tie all of these names together because: * It's important to avoid confusing the client-side discovery client, at least at the time of writing. @@ -42,15 +42,15 @@ class NameSynchronizer: """ def __init__(self, avahi_client: AvahiClient) -> None: - """For internal use by this class only. Use `build()` instead.""" + """For internal use by this class only. Use `start()` instead.""" self._avahi_client = avahi_client @classmethod @asynccontextmanager - async def build( - cls, avahi_client: AvahiClient + async def start( + cls, avahi_client: Optional[AvahiClient] = None ) -> AsyncGenerator[NameSynchronizer, None]: - """Build a RealNameSynchronizer and keep it running in the background. + """Build a NameSynchronizer and keep it running in the background. Avahi advertisements will start as soon as this context manager is entered. The pretty hostname will be used as the Avahi service name. @@ -61,7 +61,15 @@ async def build( which will be visible through `get_name()`. Collision monitoring will stop when this context manager exits. + + Args: + avahi_client: The interface for communicating with Avahi. + Changeable for testing this class; should normally be left as + the default. """ + if avahi_client is None: + avahi_client = await AvahiClient.connect() + name_synchronizer = cls(avahi_client) async with avahi_client.collision_callback( name_synchronizer._on_avahi_collision @@ -139,19 +147,3 @@ def get_name_synchronizer(request: web.Request) -> NameSynchronizer: name_synchronizer, NameSynchronizer ), f"Unexpected type {type(name_synchronizer)}. Incorrect Application setup?" return name_synchronizer - - -@asynccontextmanager -async def build_name_synchronizer( - name_override: Optional[str], -) -> AsyncGenerator[NameSynchronizer, None]: - """Return a RealNameSynchronizer for production or FakeNameManager for testing.""" - if name_override is None: - avahi_client = await AvahiClient.connect() - async with NameSynchronizer.build( - avahi_client=avahi_client - ) as real_name_synchronizer: - yield real_name_synchronizer - else: - assert False # TODO - # yield FakeNameSynchronizer(name_override=name_override) diff --git a/update-server/otupdate/openembedded/__main__.py b/update-server/otupdate/openembedded/__main__.py index 9266e768fcc..4f4474011c5 100644 --- a/update-server/otupdate/openembedded/__main__.py +++ b/update-server/otupdate/openembedded/__main__.py @@ -26,9 +26,7 @@ async def main() -> NoReturn: static_hostname = await name_management.set_up_static_hostname() LOG.info(f"Set static hostname to {static_hostname}") - async with name_management.build_name_synchronizer( - name_override=None - ) as name_synchronizer: + async with name_management.NameSynchronizer.start() as name_synchronizer: LOG.info("Building openembedded update server") app = get_app( system_version_file=args.version_file, diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index b4e5eddd9a2..aa3b990cea7 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -69,13 +69,13 @@ async def started_up_subject( Tests that need to operate in the subject's startup phase itself should build the subject manually instead of using this fixture. """ - # NameSynchronizer.build() will call mock_avahi_client.collision_callback() + # NameSynchronizer.start() will call mock_avahi_client.collision_callback() # and expect to be able to enter its result as a context manager. decoy.when( mock_avahi_client.collision_callback(matchers.Anything()) ).then_enter_with(None) - async with NameSynchronizer.build(avahi_client=mock_avahi_client) as subject: + async with NameSynchronizer.start(avahi_client=mock_avahi_client) as subject: yield subject @@ -145,7 +145,7 @@ async def test_advertises_initial_name( mock_collision_subscription_context_manager ) - async with NameSynchronizer.build(avahi_client=mock_avahi_client): + async with NameSynchronizer.start(avahi_client=mock_avahi_client): decoy.verify( # It should only start advertising after subscribing to collisions. await mock_collision_subscription_context_manager.__aenter__(), @@ -187,7 +187,7 @@ async def test_collision_handling( mock_avahi_client.collision_callback(collision_callback_captor) ).then_return(mock_collision_subscription_context_manager) - async with NameSynchronizer.build(avahi_client=mock_avahi_client): + async with NameSynchronizer.start(avahi_client=mock_avahi_client): captured_collision_callback = collision_callback_captor.value # Prompt the subject to run its collision-handling logic. await captured_collision_callback() From 1b9a7e42713a94ac5d26e9e3a0ecb6896e22e636 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 17:48:06 -0400 Subject: [PATCH 32/41] Rename `collision_callback()` -> `listen_for_collisions()`. --- .../otupdate/common/name_management/avahi.py | 4 ++-- .../common/name_management/name_synchronizer.py | 4 ++-- .../common/name_management/test_name_synchronizer.py | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index c0b2426ae75..4979943c610 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -69,7 +69,7 @@ async def start_advertising(self, service_name: str) -> None: * This process dies. * We have a name collision with another device on the network. - See `collision_callback()`. + See `listen_for_collisions()`. It's safe to call this more than once. If we're already advertising, the existing service name will be replaced with the new one. @@ -102,7 +102,7 @@ async def alternative_service_name(self, current_service_name: str) -> str: ) @contextlib.asynccontextmanager - async def collision_callback( + async def listen_for_collisions( self, callback: CollisionCallback ) -> AsyncGenerator[None, None]: """Be informed of name collisions. diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index ead3fcd0cad..10280fa7736 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -71,8 +71,8 @@ async def start( avahi_client = await AvahiClient.connect() name_synchronizer = cls(avahi_client) - async with avahi_client.collision_callback( - name_synchronizer._on_avahi_collision + async with avahi_client.listen_for_collisions( + callback=name_synchronizer._on_avahi_collision ): await avahi_client.start_advertising( service_name=name_synchronizer.get_name() diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index aa3b990cea7..1b740ae94d2 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -69,10 +69,10 @@ async def started_up_subject( Tests that need to operate in the subject's startup phase itself should build the subject manually instead of using this fixture. """ - # NameSynchronizer.start() will call mock_avahi_client.collision_callback() + # NameSynchronizer.start() will call mock_avahi_client.listen_for_collisions() # and expect to be able to enter its result as a context manager. decoy.when( - mock_avahi_client.collision_callback(matchers.Anything()) + mock_avahi_client.listen_for_collisions(matchers.Anything()) ).then_enter_with(None) async with NameSynchronizer.start(avahi_client=mock_avahi_client) as subject: @@ -141,9 +141,9 @@ async def test_advertises_initial_name( decoy.when(mock_get_pretty_hostname()).then_return("initial name") mock_collision_subscription_context_manager = decoy.mock() - decoy.when(mock_avahi_client.collision_callback(matchers.Anything())).then_return( - mock_collision_subscription_context_manager - ) + decoy.when( + mock_avahi_client.listen_for_collisions(matchers.Anything()) + ).then_return(mock_collision_subscription_context_manager) async with NameSynchronizer.start(avahi_client=mock_avahi_client): decoy.verify( @@ -184,7 +184,7 @@ async def test_collision_handling( mock_collision_subscription_context_manager = decoy.mock() collision_callback_captor = matchers.Captor() decoy.when( - mock_avahi_client.collision_callback(collision_callback_captor) + mock_avahi_client.listen_for_collisions(collision_callback_captor) ).then_return(mock_collision_subscription_context_manager) async with NameSynchronizer.start(avahi_client=mock_avahi_client): From d1a1bb56616843a715e8846576364e651dacb8f6 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 18:01:20 -0400 Subject: [PATCH 33/41] Log name changes. --- .../otupdate/common/name_management/name_synchronizer.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 10280fa7736..8996a520c82 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -92,6 +92,10 @@ async def set_name(self, new_name: str) -> str: # Setting the Avahi service name can fail if Avahi doesn't like the new name. # Persist only after it succeeds, so we don't persist something invalid. persisted_pretty_hostname = persist_pretty_hostname(new_name) + _log.info( + f"Changed name to {repr(new_name)}" + f" (persisted {repr(persisted_pretty_hostname)})." + ) return persisted_pretty_hostname def get_name(self) -> str: @@ -113,7 +117,7 @@ async def _on_avahi_collision(self) -> None: ) _log.info( f"Name collision detected by Avahi." - f" Changing name from {current_name} to {alternative_name}." + f" Changing name from {repr(current_name)} to {repr(alternative_name)}." ) # Setting the new name includes persisting it for the next boot. From cc549ca4cf92739df21716bbaff786b1a46fa138 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 18:01:26 -0400 Subject: [PATCH 34/41] Fix up comments. --- .../otupdate/common/name_management/name_synchronizer.py | 2 +- .../tests/common/name_management/test_name_synchronizer.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 8996a520c82..7c69f47ad7c 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -82,7 +82,7 @@ async def start( async def set_name(self, new_name: str) -> str: """Set the machine's human-readable name. - This first sets thhe Avahi service name, and then persists it + This first sets the Avahi service name, and then persists it as the pretty hostname. Returns the new name. This is normally the same as the requested name, diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index 1b740ae94d2..c2ddd068db6 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -197,9 +197,10 @@ async def test_collision_handling( decoy.verify( await mock_collision_subscription_context_manager.__aenter__(), await mock_avahi_client.start_advertising("initial name"), - # Asserting that the subject advertised the alternative name before persisting - # it is one way to ensurethat it doesn't persist invalid names that can't be - # advertised. https://github.com/Opentrons/opentrons/issues/9960. + # The subject should only persist the alternative name *after* + # the Avahi client accepts it for advertisement, + # just in case the alternative name turns out to be invalid in some way. + # https://github.com/Opentrons/opentrons/issues/9960 await mock_avahi_client.start_advertising("alternative name"), mock_persist_pretty_hostname("alternative name"), ) From 197303fad458da6f97e94713d8ebab9310e508e3 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 18:11:05 -0400 Subject: [PATCH 35/41] Rename `run_application()` -> `run_and_notify_up()` and document. --- update-server/otupdate/buildroot/__main__.py | 4 ++-- update-server/otupdate/common/run_application.py | 16 +++++++++------- update-server/otupdate/openembedded/__main__.py | 4 ++-- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/update-server/otupdate/buildroot/__main__.py b/update-server/otupdate/buildroot/__main__.py index 9d4a11d25a6..8d763cf3538 100644 --- a/update-server/otupdate/buildroot/__main__.py +++ b/update-server/otupdate/buildroot/__main__.py @@ -8,7 +8,7 @@ from . import get_app from otupdate.common import name_management, cli, systemd -from otupdate.common.run_application import run_application +from otupdate.common.run_application import run_and_notify_up LOG = logging.getLogger(__name__) @@ -35,7 +35,7 @@ async def main() -> NoReturn: ) LOG.info(f"Starting buildroot update server on http://{args.host}:{args.port}") - await run_application(app=app, host=args.host, port=args.port) + await run_and_notify_up(app=app, host=args.host, port=args.port) if __name__ == "__main__": diff --git a/update-server/otupdate/common/run_application.py b/update-server/otupdate/common/run_application.py index 5acb47ef6e6..393372d6b84 100644 --- a/update-server/otupdate/common/run_application.py +++ b/update-server/otupdate/common/run_application.py @@ -6,14 +6,14 @@ from . import systemd -async def run_application(app: web.Application, host: str, port: int) -> NoReturn: - """ - To document: +async def run_and_notify_up(app: web.Application, host: str, port: int) -> NoReturn: + """Run an aiohttp application. + + Once the application is up and running and serving requests, + notify systemd that this service has completed its startup. - * Never returns until the task is cancelled (process is shut down) - * Unlike standard run(), useful if we need to do async stuff outside of the - server - * Rename to mention notifying up + This method will only return once the task has been canceled, + such as if this process has been signaled to stop. """ runner = web.AppRunner(app) # type: ignore[no-untyped-call] @@ -24,7 +24,9 @@ async def run_application(app: web.Application, host: str, port: int) -> NoRetur runner=runner, host=host, port=port ) await site.start() # type: ignore[no-untyped-call] + systemd.notify_up() + while True: # It seems like there should be a better way of doing this, # but this is what the docs recommend. diff --git a/update-server/otupdate/openembedded/__main__.py b/update-server/otupdate/openembedded/__main__.py index 4f4474011c5..1c912805b77 100644 --- a/update-server/otupdate/openembedded/__main__.py +++ b/update-server/otupdate/openembedded/__main__.py @@ -8,7 +8,7 @@ from . import get_app from otupdate.common import name_management, cli, systemd -from otupdate.common.run_application import run_application +from otupdate.common.run_application import run_and_notify_up LOG = logging.getLogger(__name__) @@ -37,7 +37,7 @@ async def main() -> NoReturn: LOG.info( f"Starting openembedded update server on http://{args.host}:{args.port}" ) - await run_application(app=app, host=args.host, port=args.port) + await run_and_notify_up(app=app, host=args.host, port=args.port) if __name__ == "__main__": From 7d0c529f81c7306d1298d99cb8264637a89f4a34 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 6 Jun 2022 18:14:28 -0400 Subject: [PATCH 36/41] Fix up mock names. --- .../common/name_management/test_name_synchronizer.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index c2ddd068db6..ba0e1618fe4 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -176,16 +176,16 @@ async def test_collision_handling( # Expect the subject to do: # - # with avahi_client.collsion_callback(some_callback_func): + # with avahi_client.listen_for_collisions(some_callback_func): # ... # # When it does, save the function that it provided as `some_callback_func` # into `collision_callback_captor.value`. - mock_collision_subscription_context_manager = decoy.mock() + mock_listen_context_manager = decoy.mock() collision_callback_captor = matchers.Captor() decoy.when( mock_avahi_client.listen_for_collisions(collision_callback_captor) - ).then_return(mock_collision_subscription_context_manager) + ).then_return(mock_listen_context_manager) async with NameSynchronizer.start(avahi_client=mock_avahi_client): captured_collision_callback = collision_callback_captor.value @@ -195,7 +195,7 @@ async def test_collision_handling( # ensuring its collision handling has run to completion before we assert stuff. decoy.verify( - await mock_collision_subscription_context_manager.__aenter__(), + await mock_listen_context_manager.__aenter__(), await mock_avahi_client.start_advertising("initial name"), # The subject should only persist the alternative name *after* # the Avahi client accepts it for advertisement, From 7ac8ec8849d14f9b04b0e8942bd5be25da9642b6 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 8 Jun 2022 14:20:45 -0400 Subject: [PATCH 37/41] Stub out a custom `alternative_service_name()` implementation. --- .../otupdate/common/name_management/avahi.py | 60 ++++++++++--------- .../name_management/name_synchronizer.py | 6 +- .../name_management/test_name_synchronizer.py | 30 ++++++++-- 3 files changed, 61 insertions(+), 35 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 4979943c610..b7e1dc9ea96 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -79,28 +79,6 @@ async def start_advertising(self, service_name: str) -> None: None, self._sync_client.start_advertising, service_name ) - async def alternative_service_name(self, current_service_name: str) -> str: - """Return an alternative to the given Avahi service name. - - This is useful for fixing name collisions with other things on the network. - For example: - - alternative_service_name("Foo") == "Foo #2" - alternative_service_name("Foo #2") == "Foo #3" - - Appending incrementing integers like this, instead of using something like - the machine's serial number, follows a recommendation in the DNS-SD spec: - https://datatracker.ietf.org/doc/html/rfc6763#appendix-D - """ - # We delegate to the Avahi daemon to find the alternative name for us. - # We could compute it ourselves, but getting Avahi to do it is convenient - # because it handles edge cases like appending "#2" to a name that already has - # the maximum length. - async with self._lock: - return await asyncio.get_running_loop().run_in_executor( - None, self._sync_client.alternative_service_name, current_service_name - ) - @contextlib.asynccontextmanager async def listen_for_collisions( self, callback: CollisionCallback @@ -161,6 +139,39 @@ async def _is_collided(self) -> bool: CollisionCallback = Callable[[], Awaitable[None]] +# TODO(mm, 2022-06-08): Delegate to Avahi instead of implementing this ourselves +# when it becomes safe for our names to have spaces and number signs. +# See code history on https://github.com/Opentrons/opentrons/pull/10559. +def alternative_service_name(current_service_name: str) -> str: + """Return an alternative to the given Avahi service name. + + This is useful for fixing name collisions with other things on the network. + For example: + + alternative_service_name("Foo") == "FooNum2" + alternative_service_name("FooNum2") == "FooNum3" + + Appending incrementing integers like this, instead of using something like + the machine's serial number, follows a recommendation in the DNS-SD spec: + https://datatracker.ietf.org/doc/html/rfc6763#appendix-D + + We use "Num" as a separator because: + + * Having some separator is good to avoid accidentally "incrementing" names that are + serial numbers; we don't want OT2CEP20200827B10 to become OT2CEP20200827B11. + + * The Opentrons App is temporarily limiting user-input names to alphanumeric + characters while other server-side bugs are being resolved. + So, the names that we generate here should also be alphanumeric. + https://github.com/Opentrons/opentrons/issues/10214 + + * The number sign (#) character in particular breaks the Opentrons App, + so we especially can't use that. + https://github.com/Opentrons/opentrons/issues/10672 + """ + raise NotImplementedError() + + class _SyncClient: """A non-async wrapper around Avahi's D-Bus API. @@ -239,11 +250,6 @@ def start_advertising(self, service_name: str) -> None: self._entry_group.Commit() - def alternative_service_name(self, current_service_name: str) -> str: - result = self._server.GetAlternativeServiceName(current_service_name) - assert isinstance(result, str) - return result - def is_collided(self) -> bool: state = self._entry_group.GetState() diff --git a/update-server/otupdate/common/name_management/name_synchronizer.py b/update-server/otupdate/common/name_management/name_synchronizer.py index 7c69f47ad7c..c2e061c4a40 100644 --- a/update-server/otupdate/common/name_management/name_synchronizer.py +++ b/update-server/otupdate/common/name_management/name_synchronizer.py @@ -7,7 +7,7 @@ from aiohttp import web from otupdate.common.constants import APP_VARIABLE_PREFIX -from .avahi import AvahiClient +from .avahi import AvahiClient, alternative_service_name from .pretty_hostname import get_pretty_hostname, persist_pretty_hostname @@ -112,9 +112,7 @@ async def _on_avahi_collision(self) -> None: # Assume that the service name was the thing that collided. # Theoretically it also could have been the static hostname, # but our static hostnames are unique in practice, so that's unlikely. - alternative_name = await self._avahi_client.alternative_service_name( - current_name - ) + alternative_name = alternative_service_name(current_name) _log.info( f"Name collision detected by Avahi." f" Changing name from {repr(current_name)} to {repr(alternative_name)}." diff --git a/update-server/tests/common/name_management/test_name_synchronizer.py b/update-server/tests/common/name_management/test_name_synchronizer.py index ba0e1618fe4..ba185ca584d 100644 --- a/update-server/tests/common/name_management/test_name_synchronizer.py +++ b/update-server/tests/common/name_management/test_name_synchronizer.py @@ -6,7 +6,10 @@ from otupdate.common.name_management import name_synchronizer from otupdate.common.name_management.name_synchronizer import NameSynchronizer -from otupdate.common.name_management.avahi import AvahiClient +from otupdate.common.name_management.avahi import ( + AvahiClient, + alternative_service_name as real_alternative_service_name, +) from otupdate.common.name_management.pretty_hostname import ( get_pretty_hostname as real_get_pretty_hostname, persist_pretty_hostname as real_persist_pretty_hostname, @@ -15,6 +18,7 @@ _GET_PRETTY_HOSTNAME_SIGNATURE = Callable[[], str] _PERSIST_PRETTY_HOSTNAME_SIGNATURE = Callable[[str], str] +_ALTERNATIVE_SERVICE_NAME_SIGNATURE = Callable[[str], str] @pytest.fixture @@ -49,6 +53,22 @@ def monkeypatch_persist_pretty_hostname( ) +@pytest.fixture +def mock_alternative_service_name(decoy: Decoy) -> _ALTERNATIVE_SERVICE_NAME_SIGNATURE: + """Return a mock in the shape of the alternative_service_name() function.""" + return decoy.mock(func=real_alternative_service_name) + + +@pytest.fixture +def monkeypatch_alternative_service_name( + monkeypatch: Any, mock_alternative_service_name: _ALTERNATIVE_SERVICE_NAME_SIGNATURE +) -> None: + """Replace the real alternative_service_name() with our mock.""" + monkeypatch.setattr( + name_synchronizer, "alternative_service_name", mock_alternative_service_name + ) + + @pytest.fixture def mock_avahi_client(decoy: Decoy) -> AvahiClient: """Return a mock in the shape of an AvahiClient.""" @@ -158,6 +178,8 @@ async def test_collision_handling( monkeypatch_get_pretty_hostname: None, mock_persist_pretty_hostname: _PERSIST_PRETTY_HOSTNAME_SIGNATURE, monkeypatch_persist_pretty_hostname: None, + mock_alternative_service_name: _ALTERNATIVE_SERVICE_NAME_SIGNATURE, + monkeypatch_alternative_service_name: None, mock_avahi_client: AvahiClient, decoy: Decoy, ) -> None: @@ -170,9 +192,9 @@ async def test_collision_handling( 3. Persist it as the pretty hostname. """ decoy.when(mock_get_pretty_hostname()).then_return("initial name") - decoy.when( - await mock_avahi_client.alternative_service_name("initial name") - ).then_return("alternative name") + decoy.when(mock_alternative_service_name("initial name")).then_return( + "alternative name" + ) # Expect the subject to do: # From 888754286a3dc0f2dc986b65023d3cf55ba7c14b Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 8 Jun 2022 16:35:03 -0400 Subject: [PATCH 38/41] Implement custom `alternative_service_name()`. --- .../otupdate/common/name_management/avahi.py | 75 ++++++++++++++++++- .../common/name_management/test_avahi.py | 65 ++++++++++++++++ 2 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 update-server/tests/common/name_management/test_avahi.py diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index b7e1dc9ea96..216c11dcd85 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -6,6 +6,7 @@ import asyncio import contextlib import logging +import re from typing import AsyncGenerator, Awaitable, Callable, cast try: @@ -18,6 +19,16 @@ _COLLISION_POLL_INTERVAL = 5 +SERVICE_NAME_MAXIMUM_OCTETS = 63 +"""The maximum length of an Avahi service name. + +Measured in octets (bytes) -- not code points or characters! + +This comes from the DNS-SD specification on instance names, +which Avahi service names correspond to, under the hood. +https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1 +""" + _log = logging.getLogger(__name__) @@ -60,8 +71,8 @@ async def start_advertising(self, service_name: str) -> None: Since the Avahi service name corresponds to the DNS-SD instance name, it's a human-readable string of mostly arbitrary Unicode, - at most 63 octets (not 63 code points or 63 characters!) long. - (See: https://datatracker.ietf.org/doc/html/rfc6763#section-4.1.1) + at most SERVICE_NAME_MAXIMUM_OCTETS long. + Avahi will raise an exception through this method if it thinks the new service name is invalid. @@ -169,7 +180,65 @@ def alternative_service_name(current_service_name: str) -> str: so we especially can't use that. https://github.com/Opentrons/opentrons/issues/10672 """ - raise NotImplementedError() + separator = "Num" + + pattern = f"(?P.*){separator}(?P[0-9]+)" + match = re.fullmatch( + pattern=pattern, string=current_service_name, flags=re.DOTALL + ) + if match is None: + base_name = current_service_name + next_sequence_number = 2 + else: + base_name = match["base_name"] + next_sequence_number = int(match["sequence_number"]) + 1 + + new_suffix = f"{separator}{next_sequence_number}" + + octets_needed_for_suffix = len(new_suffix.encode("utf-8")) + octets_available_for_base_name = ( + SERVICE_NAME_MAXIMUM_OCTETS - octets_needed_for_suffix + ) + + if octets_available_for_base_name >= 0: + truncated_base_name = _truncate_by_utf8_octets( + s=base_name, maximum_utf8_octets=octets_available_for_base_name + ) + result = f"{truncated_base_name}{new_suffix}" + else: + # Edge case: The sequence number was already so high, incrementing it added a + # new digit that we don't have room for. Roll back to zero instead. + result = f"{separator}0" + + assert len(result.encode("utf-8")) <= SERVICE_NAME_MAXIMUM_OCTETS + return result + + +def _truncate_by_utf8_octets(s: str, maximum_utf8_octets: int) -> str: + """Truncate a string so it's no longer than the given number of octets when + encoded as UTF-8. + + This takes care to avoid truncating in the middle of a multi-byte code point. + So the output is always valid Unicode. + + However, it may split up multi-code-point sequences + that a human would inituitively see as a single character. + For example, it's possible to encode "é" as two code points: + + 1. U+0061 Latin Small Letter E + 2. U+0306 Combining Acute Accent + + And this may truncate in between them, leaving an unaccented "e" in the output. + (If we care to fix this, we should look into preserving grapheme clusters: + https://unicode.org/reports/tr29/) + """ + encoded = s.encode("utf-8")[:maximum_utf8_octets] + return encoded.decode( + "utf-8", + # Silently discard any trailing junk left over + # from truncating in the middle of a code point. + errors="ignore", + ) class _SyncClient: diff --git a/update-server/tests/common/name_management/test_avahi.py b/update-server/tests/common/name_management/test_avahi.py new file mode 100644 index 00000000000..007f112b9f3 --- /dev/null +++ b/update-server/tests/common/name_management/test_avahi.py @@ -0,0 +1,65 @@ +import pytest +from otupdate.common.name_management.avahi import ( + alternative_service_name, + SERVICE_NAME_MAXIMUM_OCTETS, +) + + +def test_alternative_service_name_sequencing() -> None: + """It should sequence through incrementing integers. + + "BaseName" -> "BaseNameNum2" -> "BaseNameNum3" etc. + """ + base_name = "BaseName" + current_name = base_name + for sequence_number in range(2, 150): + current_name = alternative_service_name(current_name) + assert current_name == f"{base_name}Num{sequence_number}" + + +@pytest.mark.parametrize( + ("current_name", "expected_alternative_name"), + [ + ( + # 63 ASCII bytes, exactly at the maximum. + "123456789012345678901234567890123456789012345678901234567890123", + # 59 bytes from the first 59 characters + # + 4 bytes for the new "Num2" suffix + # = 63 bytes total, exactly at the maximum. + "12345678901234567890123456789012345678901234567890123456789Num2", + ), + ( + # 21 kanji + # * 3 UTF-8 bytes per kanji + # = 63 UTF-8 bytes, exactly at the maximum. + "名名名名名名名名名名名名名名名名名名名名名", + # 57 bytes from the first 19 kanji + # + 4 bytes for the new "Num2" suffix + # = 61 bytes total, just below the maximum of 63. + # + # It's correct for it to be below the maximum because an additional 3-byte + # kanji code point would put us over the limit, and we shouldn't truncate + # in the middle of a code point. + "名名名名名名名名名名名名名名名名名名名Num2", + ), + ( + # 63 ASCII bytes, exactly at the maximum, + # and no room left to increment the sequence number. + # Pathological, but maybe someone "clever" set the name to this manually. + "Num999999999999999999999999999999999999999999999999999999999999", + # Incremented the sequence number and truncated the trailing zero. + # This is an arbitrary solution and it can change according to whatever's + # easiest to implement. + "Num0", + ), + + ], +) +def test_alternative_service_name_truncation( + current_name: str, + expected_alternative_name: str, +) -> None: + """When appending a sequence number, it should not let the name become too long.""" + alternative_name = alternative_service_name(current_name) + assert alternative_name == expected_alternative_name + assert len(alternative_name.encode("utf-8")) <= SERVICE_NAME_MAXIMUM_OCTETS From c6d938d00dd492e0394620b9903d1aff205752ce Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 8 Jun 2022 16:38:17 -0400 Subject: [PATCH 39/41] Lint. --- update-server/otupdate/common/name_management/avahi.py | 4 +--- update-server/tests/common/name_management/test_avahi.py | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 216c11dcd85..438e7a92b08 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -183,9 +183,7 @@ def alternative_service_name(current_service_name: str) -> str: separator = "Num" pattern = f"(?P.*){separator}(?P[0-9]+)" - match = re.fullmatch( - pattern=pattern, string=current_service_name, flags=re.DOTALL - ) + match = re.fullmatch(pattern=pattern, string=current_service_name, flags=re.DOTALL) if match is None: base_name = current_service_name next_sequence_number = 2 diff --git a/update-server/tests/common/name_management/test_avahi.py b/update-server/tests/common/name_management/test_avahi.py index 007f112b9f3..95153fe9e73 100644 --- a/update-server/tests/common/name_management/test_avahi.py +++ b/update-server/tests/common/name_management/test_avahi.py @@ -52,7 +52,6 @@ def test_alternative_service_name_sequencing() -> None: # easiest to implement. "Num0", ), - ], ) def test_alternative_service_name_truncation( From fdac8ff5744fb331740311e2dbd81cc8e0704490 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 8 Jun 2022 16:50:26 -0400 Subject: [PATCH 40/41] Specify what kind of octets. --- update-server/otupdate/common/name_management/avahi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/update-server/otupdate/common/name_management/avahi.py b/update-server/otupdate/common/name_management/avahi.py index 438e7a92b08..b2afdd8fd41 100644 --- a/update-server/otupdate/common/name_management/avahi.py +++ b/update-server/otupdate/common/name_management/avahi.py @@ -22,7 +22,7 @@ SERVICE_NAME_MAXIMUM_OCTETS = 63 """The maximum length of an Avahi service name. -Measured in octets (bytes) -- not code points or characters! +Measured in UTF-8 octets (bytes) -- not code points or characters! This comes from the DNS-SD specification on instance names, which Avahi service names correspond to, under the hood. From fb04bb6ca82f1fa1fdf20f41cecced41b40fb278 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 8 Jun 2022 16:52:48 -0400 Subject: [PATCH 41/41] Fix outdated test case comment. --- update-server/tests/common/name_management/test_avahi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/update-server/tests/common/name_management/test_avahi.py b/update-server/tests/common/name_management/test_avahi.py index 95153fe9e73..c5f3fe16e6f 100644 --- a/update-server/tests/common/name_management/test_avahi.py +++ b/update-server/tests/common/name_management/test_avahi.py @@ -47,7 +47,7 @@ def test_alternative_service_name_sequencing() -> None: # and no room left to increment the sequence number. # Pathological, but maybe someone "clever" set the name to this manually. "Num999999999999999999999999999999999999999999999999999999999999", - # Incremented the sequence number and truncated the trailing zero. + # Rolled back to 0. # This is an arbitrary solution and it can change according to whatever's # easiest to implement. "Num0",