🐛Dynamic-sidecar stops testing dependencies after 30 seconds instead of relying on docker healthcheck (#8686)

Author: sanderegg

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/docker_api/_core.py

@@ -47,7 +47,7 @@
     ServiceState.RUNNING,
 }
 
-log = logging.getLogger(__name__)
+_logger = logging.getLogger(__name__)
 
 
 async def get_swarm_network(simcore_services_network_name: DockerNetworkName) -> dict:
@@ -122,14 +122,10 @@ async def create_service_and_get_id(
             }
             kwargs["registry"] = registry_settings.resolved_registry_url
 
-        logging.debug("Creating service with\n%s", json_dumps(kwargs, indent=1))
+        # convert to a list if networks instead of target: id/name
+        _logger.debug("Creating service with\n%s", json_dumps(kwargs, indent=1))
         service_start_result = await client.services.create(**kwargs)
-
-        log.debug(
-            "Started service %s with\n%s",
-            service_start_result,
-            json_dumps(kwargs, indent=1),
-        )
+        _logger.debug("Started service %s", service_start_result)
 
     if "ID" not in service_start_result:
         msg = f"Error while starting service: {service_start_result!s}"
@@ -284,10 +280,7 @@ async def are_sidecar_and_proxy_services_present(
             swarm_stack_name=swarm_stack_name,
             return_only_sidecars=False,
         )
-    if len(stack_services) != _NUM_SIDECAR_STACK_SERVICES:
-        return False
-
-    return True
+    return len(stack_services) == _NUM_SIDECAR_STACK_SERVICES
 
 
 async def _list_docker_services(
@@ -352,7 +345,7 @@ async def remove_dynamic_sidecar_network(network_name: str) -> bool:
             "Docker takes some time to establish that the network has no more "
             "containers attached to it."
         )
-        log.warning(message)
+        _logger.warning(message)
         return False
 
 
@@ -386,7 +379,7 @@ async def _get_id_from_name(client, network_name: str) -> str:
 
     async with docker_client() as client:
         existing_networks_names = {x["Name"] for x in await client.networks.list()}
-        log.debug("existing_networks_names=%s", existing_networks_names)
+        _logger.debug("existing_networks_names=%s", existing_networks_names)
 
         # create networks if missing
         for network in networks:
@@ -409,7 +402,7 @@ async def _get_id_from_name(client, network_name: str) -> str:
                     # multiple calls to this function can be processed in parallel
                     # this will cause creation to fail, it is OK to assume it already
                     # exist an raise an error (see below)
-                    log.info(
+                    _logger.info(
                         "Network %s might already exist, skipping creation", network
                     )
 
@@ -456,7 +449,7 @@ async def try_to_remove_network(network_name: str) -> None:
         try:
             await network.delete()
         except aiodocker.exceptions.DockerError:
-            log.warning("Could not remove network %s", network_name)
+            _logger.warning("Could not remove network %s", network_name)
 
 
 async def _update_service_spec(
@@ -523,7 +516,7 @@ async def update_scheduler_data_label(scheduler_data: SchedulerData) -> None:
         )
     except GenericDockerError as e:
         if e.original_exception.status == status.HTTP_404_NOT_FOUND:
-            log.info(
+            _logger.info(
                 "Skipped labels update for service '%s' which could not be found.",
                 scheduler_data.service_name,
             )
@@ -540,4 +533,4 @@ async def constrain_service_to_node(
             }
         },
     )
-    log.info("Constraining service %s to node %s", service_name, docker_node_id)
+    _logger.info("Constraining service %s to node %s", service_name, docker_node_id)

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/docker_service_specs/proxy.py

@@ -127,7 +127,10 @@ def get_dynamic_proxy_spec(
             cpu_limit=float(CPU_10_PERCENT) / 1e9,
         ).to_simcore_runtime_docker_labels(),
         "name": scheduler_data.proxy_service_name,
-        "networks": [swarm_network_id, dynamic_sidecar_network_id],
+        "networks": [  # NOTE: this is deprecated in docker v1.44 and is replaced by task_template/Networks
+            swarm_network_id,
+            dynamic_sidecar_network_id,
+        ],
         "task_template": {
             "ContainerSpec": {
                 "Env": {},
@@ -153,6 +156,10 @@ def get_dynamic_proxy_spec(
                 ],
                 "Mounts": mounts,
             },
+            "Networks": [
+                {"Target": swarm_network_id},
+                {"Target": dynamic_sidecar_network_id},
+            ],
             "Placement": {
                 "Constraints": [
                     "node.platform.os == linux",

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/docker_service_specs/sidecar.py

@@ -504,7 +504,7 @@ async def get_dynamic_sidecar_spec(  # pylint:disable=too-many-arguments# noqa:
         "endpoint_spec": {"Ports": ports} if ports else {},
         "labels": service_labels,
         "name": scheduler_data.service_name,
-        "networks": [
+        "networks": [  # NOTE: this is deprecated in docker v1.44 and is replaced by task_template/Networks
             {"Target": swarm_network_id},
             *get_prometheus_monitoring_networks(
                 dynamic_services_scheduler_settings.DYNAMIC_SIDECAR_PROMETHEUS_MONITORING_NETWORKS,
@@ -549,6 +549,13 @@ async def get_dynamic_sidecar_spec(  # pylint:disable=too-many-arguments# noqa:
                     else None
                 ),
             },
+            "Networks": [
+                {"Target": swarm_network_id},
+                *get_prometheus_monitoring_networks(
+                    dynamic_services_scheduler_settings.DYNAMIC_SIDECAR_PROMETHEUS_MONITORING_NETWORKS,
+                    scheduler_data.callbacks_mapping,
+                ),
+            ],
             "Placement": {"Constraints": placement_constraints},
             "RestartPolicy": DOCKER_CONTAINER_SPEC_RESTART_POLICY_DEFAULTS,
             # this will get overwritten

services/director-v2/tests/unit/with_dbs/test_modules_dynamic_sidecar_docker_service_specs.py

@@ -393,6 +393,7 @@ def expected_dynamic_sidecar_spec(
                     },
                 ],
             },
+            "Networks": [{"Target": "mocked_swarm_network_id"}],
             "Placement": {
                 "Constraints": [
                     f"node.labels.{DOCKER_TASK_EC2_INSTANCE_TYPE_PLACEMENT_CONSTRAINT_KEY}=={hardware_info.aws_ec2_instances[0]}",
@@ -568,7 +569,7 @@ async def test_merge_dynamic_sidecar_specs_with_user_specific_specs(
     expected_dynamic_sidecar_spec_dict = AioDockerServiceSpec.model_validate(
         expected_dynamic_sidecar_spec
     ).model_dump()
-    # ensure some entries are sorted the same to prevent flakyness
+    # ensure some entries are sorted the same to prevent flakiness
     for sorted_dict in [dynamic_sidecar_spec_dict, expected_dynamic_sidecar_spec_dict]:
         for key in ["DY_SIDECAR_STATE_EXCLUDE", "DY_SIDECAR_STATE_PATHS"]:
             # this is a json of a list

services/director/src/simcore_service_director/producer.py

@@ -231,6 +231,9 @@ async def _create_docker_service_params(
         "name": service_name,
         "task_template": {
             "ContainerSpec": container_spec,
+            "Networks": (
+                [{"Target": internal_network_id}] if internal_network_id else []
+            ),
             "Placement": {"Constraints": ([])},
             "RestartPolicy": {
                 "Condition": "on-failure",
@@ -277,7 +280,9 @@ async def _create_docker_service_params(
             f"traefik.http.routers.{service_name}.middlewares": f"{app_settings.DIRECTOR_SWARM_STACK_NAME}_gzip@swarm",
         }
         | app_settings.DIRECTOR_SERVICES_CUSTOM_LABELS,
-        "networks": [internal_network_id] if internal_network_id else [],
+        "networks": (
+            [internal_network_id] if internal_network_id else []
+        ),  # NOTE: this is deprecated in docker v1.44 and is replaced by task_template/Networks
     }
     if app_settings.DIRECTOR_SERVICES_CUSTOM_PLACEMENT_CONSTRAINTS:
         _logger.debug(
@@ -427,6 +432,7 @@ async def _create_docker_service_params(
     swarm_network_id = swarm_network["Id"]
     swarm_network_name = swarm_network["Name"]
     docker_params["networks"].append(swarm_network_id)
+    docker_params["task_template"]["Networks"].append({"Target": swarm_network_id})
     docker_params["labels"]["traefik.swarm.network"] = swarm_network_name
 
     # set labels for CPU and Memory limits

services/dynamic-sidecar/Dockerfile

@@ -100,7 +100,7 @@ ENV PATH="${VIRTUAL_ENV}/bin:$PATH"
 # volumes between itself and the spawned containers
 ENV DYNAMIC_SIDECAR_DY_VOLUMES_MOUNT_DIR="/dy-volumes"
 
-# create direcotry to persist SharedStore data accessiable
+# create directory to persist SharedStore data accessible
 # between dynamic-sidecar reboots
 ENV DYNAMIC_SIDECAR_SHARED_STORE_DIR="${DYNAMIC_SIDECAR_DY_VOLUMES_MOUNT_DIR}/shared-store"
 RUN mkdir -p "${DYNAMIC_SIDECAR_SHARED_STORE_DIR}" && \
@@ -186,14 +186,11 @@ COPY --chown=scu:scu --from=prod-only-deps  ${DYNAMIC_SIDECAR_DY_VOLUMES_MOUNT_D
 COPY --chown=scu:scu services/dynamic-sidecar/docker services/dynamic-sidecar/docker
 RUN chmod +x services/dynamic-sidecar/docker/*.sh
 
-# NOTE: the start period of 3 minutes is to allow the dynamic-sidecar
-# enough time to connect to the externald dependencies. some times the docker
-# networks take time to get created
-# https://docs.docker.com/reference/dockerfile/#healthcheck
+# NOTE: sometimes docker network may take some time to resolve DNS, thus the longer start-period
 HEALTHCHECK \
   --interval=10s \
   --timeout=5s \
-  --start-period=180s \
+  --start-period=64s \
   --start-interval=1s \
   --retries=5 \
   CMD ["python3", "services/dynamic-sidecar/docker/healthcheck.py", "http://localhost:8000/health"]
@@ -206,7 +203,7 @@ CMD ["/bin/sh", "services/dynamic-sidecar/docker/boot.sh"]
 
 # --------------------------Development stage -------------------
 # Source code accessible in host but runs in container
-# Runs as myu with same gid/uid as host
+# Runs as my with same gid/uid as host
 # Placed at the end to speed-up the build if images targeting production
 #
 #  + /devel         WORKDIR

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/service_liveness.py

@@ -5,15 +5,14 @@
 from typing import Final
 
 from common_library.errors_classes import OsparcErrorMixin
-from tenacity import AsyncRetrying, RetryCallState, TryAgain
+from tenacity import AsyncRetrying, RetryCallState, TryAgain, stop_never
 from tenacity.stop import stop_after_delay
 from tenacity.wait import wait_fixed
 
 _logger = logging.getLogger(__name__)
 
 
 _DEFAULT_CHECK_INTERVAL: Final[timedelta] = timedelta(seconds=1)
-_DEFAULT_TIMEOUT_INTERVAL: Final[timedelta] = timedelta(seconds=30)
 
 
 class CouldNotReachServiceError(OsparcErrorMixin, Exception):
@@ -46,12 +45,16 @@ async def _attempt_to_wait_for_handler(
     service_name: str,
     endpoint: str,
     check_interval: timedelta,
-    timeout: timedelta,
+    max_delay: timedelta | None,
     **kwargs,
 ) -> None:
     async for attempt in AsyncRetrying(
         wait=wait_fixed(check_interval),
-        stop=stop_after_delay(timeout.total_seconds()),
+        stop=(
+            stop_never
+            if max_delay is None
+            else stop_after_delay(max_delay.total_seconds())
+        ),
         before_sleep=_before_sleep_log(_logger, service_name, endpoint),
         reraise=True,
     ):
@@ -66,7 +69,7 @@ async def wait_for_service_liveness(
     service_name: str,
     endpoint: str,
     check_interval: timedelta | None = None,
-    timeout: timedelta | None = None,
+    max_delay: timedelta | None = None,
     **kwargs,
 ) -> None:
     """waits for async_handler to return ``True`` or ``None`` instead of
@@ -79,17 +82,15 @@ async def wait_for_service_liveness(
 
     Keyword Arguments:
         check_interval -- interval at which service check is ran (default: {_DEFAULT_CHECK_INTERVAL})
-        timeout -- stops trying to contact service and raises ``CouldNotReachServiceError``
-            (default: {_DEFAULT_TIMEOUT_INTERVAL})
+        max_delay -- stops trying to contact service after max_delay and raises ``CouldNotReachServiceError``
+            (default: None - no timeout)
 
     Raises:
         CouldNotReachServiceError: if it was not able to contact the service in time
     """
 
     if check_interval is None:
         check_interval = _DEFAULT_CHECK_INTERVAL
-    if timeout is None:
-        timeout = _DEFAULT_TIMEOUT_INTERVAL
 
     try:
         start = time.time()
@@ -99,7 +100,7 @@ async def wait_for_service_liveness(
             service_name=service_name,
             endpoint=endpoint,
             check_interval=check_interval,
-            timeout=timeout,
+            max_delay=max_delay,
             **kwargs,
         )
         elapsed_ms = (time.time() - start) * 1000

services/dynamic-sidecar/tests/unit/test_core_external_dependencies.py

@@ -16,16 +16,36 @@
 from simcore_service_dynamic_sidecar.core.external_dependencies import (
     CouldNotReachExternalDependenciesError,
 )
+from simcore_service_dynamic_sidecar.modules.service_liveness import (
+    wait_for_service_liveness as original_wait_for_service_liveness,
+)
 
 _LONG_STARTUP_SHUTDOWN_TIMEOUT: Final[NonNegativeFloat] = 60
 
 
 @pytest.fixture
 def mock_liveness_timeout(mocker: MockerFixture) -> None:
-    mocker.patch(
-        "simcore_service_dynamic_sidecar.modules.service_liveness._DEFAULT_TIMEOUT_INTERVAL",
-        new=timedelta(seconds=0.1),
-    )
+    # Wrap wait_for_service_liveness to inject a short timeout
+
+    async def _wait_with_timeout(*args, **kwargs):
+        # Force a short timeout for all liveness checks
+        kwargs["max_delay"] = timedelta(seconds=0.5)
+        kwargs["check_interval"] = timedelta(seconds=0.1)
+        return await original_wait_for_service_liveness(  # pylint: disable=missing-kwoa
+            *args, **kwargs
+        )
+
+    # Patch in all modules that import it
+    for module in [
+        "simcore_service_dynamic_sidecar.modules.database",
+        "simcore_service_dynamic_sidecar.core.storage",
+        "simcore_service_dynamic_sidecar.core.rabbitmq",
+        "simcore_service_dynamic_sidecar.core.registry",
+    ]:
+        mocker.patch(
+            f"{module}.wait_for_service_liveness",
+            side_effect=_wait_with_timeout,
+        )
 
 
 @pytest.fixture

…-sidecar/tests/unit/test_core_stroage.py …-sidecar/tests/unit/test_core_storage.pyservices/dynamic-sidecar/tests/unit/test_core_stroage.py renamed to services/dynamic-sidecar/tests/unit/test_core_storage.py services/dynamic-sidecar/tests/unit/test_core_stroage.py renamed to services/dynamic-sidecar/tests/unit/test_core_storage.py

@@ -3,7 +3,6 @@
 
 import multiprocessing
 from collections.abc import AsyncIterable
-from datetime import timedelta
 from typing import Annotated, Final
 from unittest.mock import Mock
 
@@ -13,7 +12,6 @@
 from fastapi import Depends, FastAPI, HTTPException, status
 from fastapi.security import HTTPBasic, HTTPBasicCredentials
 from pydantic import TypeAdapter
-from pytest_mock import MockerFixture
 from settings_library.node_ports import StorageAuthSettings
 from simcore_service_dynamic_sidecar.core.storage import (
     _get_url,
@@ -105,17 +103,9 @@ def _run_server(app):
     process.kill()
 
 
-@pytest.fixture
-def mock_liveness_timeout(mocker: MockerFixture) -> None:
-    mocker.patch(
-        "simcore_service_dynamic_sidecar.modules.service_liveness._DEFAULT_TIMEOUT_INTERVAL",
-        new=timedelta(seconds=2),
-    )
-
-
 @pytest.fixture
 def mock_dynamic_sidecar_app(
-    mock_liveness_timeout: None, storage_auth_settings: StorageAuthSettings
+    storage_auth_settings: StorageAuthSettings,
 ) -> Mock:
     mock = Mock()
     mock.state.settings.NODE_PORTS_STORAGE_AUTH = storage_auth_settings

services/dynamic-sidecar/tests/unit/test_service_liveness.py

@@ -31,7 +31,7 @@ async def _ok_handler() -> bool | None:
         service_name="test_service",
         endpoint="http://fake.endpoint_string",
         check_interval=check_interval,
-        timeout=timeout,
+        max_delay=timeout,
     )
 
 
@@ -52,6 +52,6 @@ async def _failing_handler() -> bool:
             service_name="test_service",
             endpoint="http://fake.endpoint_string",
             check_interval=check_interval,
-            timeout=timeout,
+            max_delay=timeout,
         )
     assert "Could not contact service" in f"{exc_info.value}"