review @pcrespov

Author: matusdrobuliak66

services/efs-guardian/src/simcore_service_efs_guardian/services/background_tasks.py

@@ -8,7 +8,7 @@
 
 
 async def removal_policy_task(app: FastAPI) -> None:
-    _logger.info("Removal policy task started")
+    _logger.info("FAKE Removal policy task started (not yet implemented)")
 
     # After X days of inactivity remove data from EFS
     # Probably use `last_modified_data` in the project DB table

services/efs-guardian/src/simcore_service_efs_guardian/services/background_tasks_setup.py

@@ -27,7 +27,7 @@ class EfsGuardianBackgroundTask(TypedDict):
 ]
 
 
-def on_app_startup(app: FastAPI) -> Callable[[], Awaitable[None]]:
+def _on_app_startup(app: FastAPI) -> Callable[[], Awaitable[None]]:
     async def _startup() -> None:
         with log_context(
             _logger, logging.INFO, msg="Efs Guardian startup.."
@@ -49,7 +49,7 @@ async def _startup() -> None:
     return _startup
 
 
-def on_app_shutdown(
+def _on_app_shutdown(
     _app: FastAPI,
 ) -> Callable[[], Awaitable[None]]:
     async def _stop() -> None:
@@ -69,5 +69,5 @@ async def _stop() -> None:
 
 
 def setup(app: FastAPI) -> None:
-    app.add_event_handler("startup", on_app_startup(app))
-    app.add_event_handler("shutdown", on_app_shutdown(app))
+    app.add_event_handler("startup", _on_app_startup(app))
+    app.add_event_handler("shutdown", _on_app_shutdown(app))

services/efs-guardian/src/simcore_service_efs_guardian/services/efs_manager.py

@@ -77,8 +77,7 @@ async def get_project_node_data_size(
             / f"{node_id}"
         )
 
-        service_size = await efs_manager_utils.get_size_bash_async(_dir_path)
-        return service_size
+        return await efs_manager_utils.get_size_bash_async(_dir_path)
 
     async def remove_project_node_data_write_permissions(
         self, project_id: ProjectID, node_id: NodeID

services/efs-guardian/src/simcore_service_efs_guardian/services/process_messages.py

@@ -3,6 +3,7 @@
 from fastapi import FastAPI
 from models_library.rabbitmq_messages import DynamicServiceRunningMessage
 from pydantic import parse_raw_as
+from servicelib.logging_utils import log_context
 from simcore_service_efs_guardian.services.modules.redis import get_redis_lock_client
 
 from ..core.settings import get_application_settings
@@ -50,22 +51,16 @@ async def process_dynamic_service_running_message(app: FastAPI, data: bytes) ->
     )
 
     if size > settings.EFS_DEFAULT_USER_SERVICE_SIZE_BYTES:
-        _logger.warning(
-            "Removing write permissions inside of EFS starts for project ID: %s, node ID: %s, current user: %s, size: %s, upper limit: %s",
-            rabbit_message.project_id,
-            rabbit_message.node_id,
-            rabbit_message.user_id,
-            size,
-            settings.EFS_DEFAULT_USER_SERVICE_SIZE_BYTES,
-        )
-        redis = get_redis_lock_client(app)
-        async with redis.lock_context(
-            f"efs_remove_write_permissions-{rabbit_message.project_id=}-{rabbit_message.node_id=}",
-            blocking=True,
-            blocking_timeout_s=10,
-        ):
-            await efs_manager.remove_project_node_data_write_permissions(
-                project_id=rabbit_message.project_id, node_id=rabbit_message.node_id
-            )
+        msg = f"Removing write permissions inside of EFS starts for project ID: {rabbit_message.project_id}, node ID: {rabbit_message.node_id}, current user: {rabbit_message.user_id}, size: {size}, upper limit: {settings.EFS_DEFAULT_USER_SERVICE_SIZE_BYTES}"
+        with log_context(_logger, logging.WARNING, msg=msg):
+            redis = get_redis_lock_client(app)
+            async with redis.lock_context(
+                f"efs_remove_write_permissions-{rabbit_message.project_id=}-{rabbit_message.node_id=}",
+                blocking=True,
+                blocking_timeout_s=10,
+            ):
+                await efs_manager.remove_project_node_data_write_permissions(
+                    project_id=rabbit_message.project_id, node_id=rabbit_message.node_id
+                )
 
     return True

services/efs-guardian/src/simcore_service_efs_guardian/services/process_messages_setup.py

@@ -15,7 +15,11 @@
 _logger = logging.getLogger(__name__)
 
 
-_RUT_MESSAGE_TTL_IN_MS = 2 * 60 * 60 * 1000  # 2 hours
+_SEC = 1000  # in ms
+_MIN = 60 * _SEC  # in ms
+_HOUR = 60 * _MIN  # in ms
+
+_EFS_MESSAGE_TTL_IN_MS = 2 * _HOUR
 
 
 async def _subscribe_to_rabbitmq(app) -> str:
@@ -27,12 +31,12 @@ async def _subscribe_to_rabbitmq(app) -> str:
                 process_dynamic_service_running_message, app
             ),
             exclusive_queue=False,
-            message_ttl=_RUT_MESSAGE_TTL_IN_MS,
+            message_ttl=_EFS_MESSAGE_TTL_IN_MS,
         )
         return subscribed_queue
 
 
-def on_app_startup(app: FastAPI) -> Callable[[], Awaitable[None]]:
+def _on_app_startup(app: FastAPI) -> Callable[[], Awaitable[None]]:
     async def _startup() -> None:
         with log_context(
             _logger, logging.INFO, msg="setup resource tracker"
@@ -48,7 +52,7 @@ async def _startup() -> None:
     return _startup
 
 
-def on_app_shutdown(
+def _on_app_shutdown(
     _app: FastAPI,
 ) -> Callable[[], Awaitable[None]]:
     async def _stop() -> None:
@@ -58,5 +62,5 @@ async def _stop() -> None:
 
 
 def setup(app: FastAPI) -> None:
-    app.add_event_handler("startup", on_app_startup(app))
-    app.add_event_handler("shutdown", on_app_shutdown(app))
+    app.add_event_handler("startup", _on_app_startup(app))
+    app.add_event_handler("shutdown", _on_app_shutdown(app))

services/efs-guardian/tests/unit/conftest.py

@@ -2,7 +2,10 @@
 # pylint:disable=unused-argument
 # pylint:disable=redefined-outer-name
 
+import os
 import re
+import shutil
+import stat
 from collections.abc import AsyncIterator, Callable
 from pathlib import Path
 from typing import Awaitable
@@ -18,6 +21,7 @@
 from pytest_mock import MockerFixture
 from pytest_simcore.helpers.monkeypatch_envs import EnvVarsDict, setenvs_from_dict
 from servicelib.rabbitmq import RabbitMQRPCClient
+from settings_library.efs import AwsEfsSettings
 from settings_library.rabbit import RabbitSettings
 from simcore_service_efs_guardian.core.application import create_app
 from simcore_service_efs_guardian.core.settings import ApplicationSettings
@@ -28,6 +32,7 @@
     "pytest_simcore.docker_registry",
     "pytest_simcore.docker_swarm",
     "pytest_simcore.environment_configs",
+    "pytest_simcore.faker_projects_data",
     "pytest_simcore.pydantic_models",
     "pytest_simcore.pytest_global_environs",
     "pytest_simcore.rabbit_service",
@@ -148,3 +153,22 @@ async def rpc_client(
 async def mocked_redis_server(mocker: MockerFixture) -> None:
     mock_redis = FakeRedis()
     mocker.patch("redis.asyncio.from_url", return_value=mock_redis)
+
+
+@pytest.fixture
+async def cleanup(app: FastAPI):
+
+    yield
+
+    aws_efs_settings: AwsEfsSettings = app.state.settings.EFS_GUARDIAN_AWS_EFS_SETTINGS
+    _dir_path = Path(aws_efs_settings.EFS_MOUNTED_PATH)
+    if _dir_path.exists():
+        for root, dirs, files in os.walk(_dir_path):
+            for name in dirs + files:
+                file_path = Path(root, name)
+                # Get the current permissions of the file or directory
+                current_permissions = Path.stat(file_path).st_mode
+                # Add write permission for the owner (user)
+                Path.chmod(file_path, current_permissions | stat.S_IWUSR)
+
+        shutil.rmtree(_dir_path)

services/efs-guardian/tests/unit/test_api_health.py

@@ -30,7 +30,7 @@ def app_environment(
 
 async def test_healthcheck(
     rabbit_service: RabbitSettings,
-    mocked_redis_server,
+    mocked_redis_server: None,
     client: httpx.AsyncClient,
 ):
     response = await client.get("/")

services/efs-guardian/tests/unit/test_efs_guardian_rpc.py

@@ -10,6 +10,8 @@
 import pytest
 from faker import Faker
 from fastapi import FastAPI
+from models_library.projects import ProjectID
+from models_library.projects_nodes import NodeID
 from pytest_simcore.helpers.monkeypatch_envs import setenvs_from_dict
 from pytest_simcore.helpers.typing_env import EnvVarsDict
 from servicelib.rabbitmq import RabbitMQRPCClient
@@ -40,20 +42,21 @@ async def test_rpc_create_project_specific_data_dir(
     rpc_client: RabbitMQRPCClient,
     faker: Faker,
     app: FastAPI,
+    project_id: ProjectID,
+    node_id: NodeID,
+    cleanup: None,
 ):
     aws_efs_settings: AwsEfsSettings = app.state.settings.EFS_GUARDIAN_AWS_EFS_SETTINGS
 
-    _project_id = faker.uuid4()
-    _node_id = faker.uuid4()
-    _storage_directory_name = faker.name()
+    _storage_directory_name = faker.word()
 
     with patch(
         "simcore_service_efs_guardian.services.efs_manager.os.chown"
     ) as mocked_chown:
         result = await efs_manager.create_project_specific_data_dir(
             rpc_client,
-            project_id=_project_id,
-            node_id=_node_id,
+            project_id=project_id,
+            node_id=node_id,
             storage_directory_name=_storage_directory_name,
         )
         mocked_chown.assert_called_once()
@@ -62,8 +65,8 @@ async def test_rpc_create_project_specific_data_dir(
     _expected_path = (
         aws_efs_settings.EFS_MOUNTED_PATH
         / aws_efs_settings.EFS_PROJECT_SPECIFIC_DATA_DIRECTORY
-        / _project_id
-        / _node_id
+        / f"{project_id}"
+        / f"{node_id}"
         / _storage_directory_name
     )
     assert _expected_path == result

services/efs-guardian/tests/unit/test_efs_manager.py

@@ -4,8 +4,6 @@
 # pylint: disable=unused-argument
 # pylint: disable=unused-variable
 
-import os
-import shutil
 import stat
 from pathlib import Path
 from unittest.mock import patch
@@ -41,32 +39,13 @@ def app_environment(
     )
 
 
-@pytest.fixture
-async def cleanup(app: FastAPI):
-
-    yield
-
-    aws_efs_settings: AwsEfsSettings = app.state.settings.EFS_GUARDIAN_AWS_EFS_SETTINGS
-    _dir_path = Path(aws_efs_settings.EFS_MOUNTED_PATH)
-    if Path.exists(_dir_path):
-        for root, dirs, files in os.walk(_dir_path):
-            for name in dirs + files:
-                file_path = os.path.join(root, name)
-                # Get the current permissions of the file or directory
-                current_permissions = os.stat(file_path).st_mode
-                # Add write permission for the owner (user)
-                os.chmod(file_path, current_permissions | stat.S_IWUSR)
-
-        shutil.rmtree(_dir_path)
-
-
 def assert_permissions(
     file_path: Path,
     expected_readable: bool,
     expected_writable: bool,
     expected_executable: bool,
 ):
-    file_stat = os.stat(file_path)
+    file_stat = Path.stat(file_path)
     file_permissions = file_stat.st_mode
     is_readable = bool(file_permissions & stat.S_IRUSR)
     is_writable = bool(file_permissions & stat.S_IWUSR)
@@ -88,25 +67,25 @@ async def test_remove_write_access_rights(
     mocked_redis_server: None,
     app: FastAPI,
     cleanup: None,
+    project_id: ProjectID,
+    node_id: NodeID,
 ):
     aws_efs_settings: AwsEfsSettings = app.state.settings.EFS_GUARDIAN_AWS_EFS_SETTINGS
 
-    _project_id = ProjectID(faker.uuid4())
-    _node_id = NodeID(faker.uuid4())
     _storage_directory_name = faker.word()
     _dir_path = (
         aws_efs_settings.EFS_MOUNTED_PATH
         / aws_efs_settings.EFS_PROJECT_SPECIFIC_DATA_DIRECTORY
-        / f"{_project_id}"
-        / f"{_node_id}"
+        / f"{project_id}"
+        / f"{node_id}"
         / f"{_storage_directory_name}"
     )
 
     efs_manager: EfsManager = app.state.efs_manager
 
     assert (
         await efs_manager.check_project_node_data_directory_exits(
-            project_id=_project_id, node_id=_node_id
+            project_id=project_id, node_id=node_id
         )
         is False
     )
@@ -115,31 +94,31 @@ async def test_remove_write_access_rights(
         "simcore_service_efs_guardian.services.efs_manager.os.chown"
     ) as mocked_chown:
         await efs_manager.create_project_specific_data_dir(
-            project_id=_project_id,
-            node_id=_node_id,
+            project_id=project_id,
+            node_id=node_id,
             storage_directory_name=_storage_directory_name,
         )
+        assert mocked_chown.called
 
     assert (
         await efs_manager.check_project_node_data_directory_exits(
-            project_id=_project_id, node_id=_node_id
+            project_id=project_id, node_id=node_id
         )
         is True
     )
 
     size_before = await efs_manager.get_project_node_data_size(
-        project_id=_project_id, node_id=_node_id
+        project_id=project_id, node_id=node_id
     )
 
     file_paths = []
     for i in range(3):  # Let's create 3 small files for testing
         file_path = Path(_dir_path, f"test_file_{i}.txt")
-        with open(file_path, "w") as f:
-            f.write(f"This is file {i}")
+        file_path.write_text(f"This is file {i}")
         file_paths.append(file_path)
 
     size_after = await efs_manager.get_project_node_data_size(
-        project_id=_project_id, node_id=_node_id
+        project_id=project_id, node_id=node_id
     )
     assert size_after > size_before
 
@@ -153,7 +132,7 @@ async def test_remove_write_access_rights(
         )
 
     await efs_manager.remove_project_node_data_write_permissions(
-        project_id=_project_id, node_id=_node_id
+        project_id=project_id, node_id=node_id
     )
 
     for file_path in file_paths: