diff --git a/fractal_server/app/models/user_settings.py b/fractal_server/app/models/user_settings.py index 807a2006c6..db816d2dd6 100644 --- a/fractal_server/app/models/user_settings.py +++ b/fractal_server/app/models/user_settings.py @@ -5,6 +5,17 @@ class UserSettings(SQLModel, table=True): + """ + Comprehensive list of user settings. + + Attributes: + ssh_host: SSH-reachable host where a SLURM client is available. + ssh_username: User on `ssh_host`. + ssh_private_key_path: Path of private SSH key for `ssh_username`. + ssh_tasks_dir: Task-venvs base folder on `ssh_host`. + ssh_jobs_dir: Jobs base folder on `ssh_host`. + """ + __tablename__ = "user_settings" id: Optional[int] = Field(default=None, primary_key=True) diff --git a/fractal_server/app/routes/api/v2/_aux_functions.py b/fractal_server/app/routes/api/v2/_aux_functions.py index c3280162bf..1ac43a6452 100644 --- a/fractal_server/app/routes/api/v2/_aux_functions.py +++ b/fractal_server/app/routes/api/v2/_aux_functions.py @@ -23,6 +23,10 @@ from ....schemas.v2 import JobStatusTypeV2 from fractal_server.app.models import UserOAuth from fractal_server.images import Filters +from fractal_server.logger import set_logger + + +logger = set_logger(__name__) async def _get_project_check_owner( diff --git a/fractal_server/app/routes/api/v2/submit.py b/fractal_server/app/routes/api/v2/submit.py index 7ba57f65b9..0a8d0b9f84 100644 --- a/fractal_server/app/routes/api/v2/submit.py +++ b/fractal_server/app/routes/api/v2/submit.py @@ -27,6 +27,7 @@ from ....schemas.v2 import JobCreateV2 from ....schemas.v2 import JobReadV2 from ....schemas.v2 import JobStatusTypeV2 +from ...aux.validate_user_settings import validate_user_settings from ._aux_functions import _get_dataset_check_owner from ._aux_functions import _get_workflow_check_owner from ._aux_functions import clean_app_job_list_v2 @@ -109,8 +110,14 @@ async def apply_workflow( ), ) - # If backend is SLURM, check that the user has required attributes + # Validate user settings (which will eventually replace the block below, + # where check required user attributes) FRACTAL_RUNNER_BACKEND = settings.FRACTAL_RUNNER_BACKEND + await validate_user_settings( + user=user, backend=settings.FRACTAL_RUNNER_BACKEND, db=db + ) + + # If backend is SLURM, check that the user has required attributes if FRACTAL_RUNNER_BACKEND == "slurm": if not user.slurm_user: raise HTTPException( diff --git a/fractal_server/app/routes/api/v2/task_collection.py b/fractal_server/app/routes/api/v2/task_collection.py index 5bc1f575bf..d829bf87b3 100644 --- a/fractal_server/app/routes/api/v2/task_collection.py +++ b/fractal_server/app/routes/api/v2/task_collection.py @@ -25,6 +25,7 @@ from ....schemas.v2 import CollectionStatusV2 from ....schemas.v2 import TaskCollectPipV2 from ....schemas.v2 import TaskReadV2 +from ...aux.validate_user_settings import validate_user_settings from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth import current_active_user from fractal_server.app.routes.auth import current_active_verified_user @@ -41,7 +42,6 @@ from fractal_server.tasks.v2.endpoint_operations import inspect_package from fractal_server.tasks.v2.utils import get_python_interpreter_v2 - router = APIRouter() logger = set_logger(__name__) @@ -107,6 +107,11 @@ async def collect_tasks_pip( detail=f"Invalid task-collection object. Original error: {e}", ) + # Validate user settings (backend-specific) + await validate_user_settings( + user=user, backend=settings.FRACTAL_RUNNER_BACKEND, db=db + ) + # END of SSH/non-SSH common part if settings.FRACTAL_RUNNER_BACKEND == "slurm_ssh": diff --git a/fractal_server/app/routes/aux/validate_user_settings.py b/fractal_server/app/routes/aux/validate_user_settings.py new file mode 100644 index 0000000000..d25c3223fd --- /dev/null +++ b/fractal_server/app/routes/aux/validate_user_settings.py @@ -0,0 +1,41 @@ +from fastapi import HTTPException +from fastapi import status +from pydantic import ValidationError + +from fractal_server.app.db import AsyncSession +from fractal_server.app.models import UserOAuth +from fractal_server.app.models import UserSettings +from fractal_server.app.routes.api.v2._aux_functions import logger +from fractal_server.user_settings import SlurmSshUserSettings + + +async def validate_user_settings( + *, user: UserOAuth, backend: str, db: AsyncSession +): + """ + FIXME docstring + """ + # First: check that the foreign-key exists. TODO: remove this check, + # after this column is made required + if user.user_settings_id is None: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Error: user '{user.email}' has no settings.", + ) + + user_settings = await db.get(UserSettings, user.user_settings_id) + + if backend == "slurm_ssh": + try: + SlurmSshUserSettings(**user_settings.model_dump()) + except ValidationError as e: + error_msg = ( + "User settings are not valid for " + f"FRACTAL_RUNNER_BACKEND='{backend}'. " + f"Original error: {str(e)}" + ) + logger.warning(error_msg) + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=error_msg, + ) diff --git a/fractal_server/user_settings.py b/fractal_server/user_settings.py new file mode 100644 index 0000000000..22ad31e47e --- /dev/null +++ b/fractal_server/user_settings.py @@ -0,0 +1,22 @@ +# TODO: move this file to the appropriate path +from pydantic import BaseModel + + +class SlurmSshUserSettings(BaseModel): + """ + Subset of user settings which must be present for task collection and job + execution when using the Slurm-SSH runner. + + Attributes: + ssh_host: SSH-reachable host where a SLURM client is available. + ssh_username: User on `ssh_host`. + ssh_private_key_path: Path of private SSH key for `ssh_username`. + ssh_tasks_dir: Task-venvs base folder on `ssh_host`. + ssh_jobs_dir: Jobs base folder on `ssh_host`. + """ + + ssh_host: str + ssh_username: str + ssh_private_key_path: str + ssh_tasks_dir: str + ssh_jobs_dir: str diff --git a/tests/fixtures_server.py b/tests/fixtures_server.py index a3680ef5b7..46e86c082e 100644 --- a/tests/fixtures_server.py +++ b/tests/fixtures_server.py @@ -232,7 +232,8 @@ async def MockCurrentUser(app, db): from fractal_server.app.routes.auth import current_active_verified_user from fractal_server.app.routes.auth import current_active_user from fractal_server.app.routes.auth import current_active_superuser - from fractal_server.app.routes.auth import UserOAuth + from fractal_server.app.models import UserOAuth + from fractal_server.app.models import UserSettings def _random_email(): return f"{random.randint(0, 100000000)}@example.org" @@ -243,8 +244,8 @@ class _MockCurrentUser: Context managed user override """ - name: str = "User Name" user_kwargs: Optional[dict[str, Any]] = None + user_settings_dict: Optional[dict[str, Any]] = None email: Optional[str] = field(default_factory=_random_email) previous_dependencies: dict = field(default_factory=dict) @@ -259,32 +260,40 @@ async def __aenter__(self): f"User with id {self.user_kwargs['id']} doesn't exist" ) self.user = db_user + # Removing objects from test db session, so that we can operate + # on them from other sessions + db.expunge(self.user) else: # Create new user - defaults = dict( + user_attributes = dict( email=self.email, hashed_password="fake_hashed_password", slurm_user="test01", ) - if self.user_kwargs: - defaults.update(self.user_kwargs) - self.user = UserOAuth(name=self.name, **defaults) + if self.user_kwargs is not None: + user_attributes.update(self.user_kwargs) + self.user = UserOAuth(**user_attributes) + + # Create new user_settings object and associate it to user + user_settings_dict = self.user_settings_dict or {} + user_settings = UserSettings(**user_settings_dict) + self.user.settings = user_settings try: db.add(self.user) await db.commit() - await db.refresh(self.user) except IntegrityError: # Safety net, in case of non-unique email addresses await db.rollback() self.user.email = _random_email() db.add(self.user) await db.commit() - await db.refresh(self.user) + await db.refresh(self.user) - # Removing object from test db session, so that we can operate - # on user from other sessions - db.expunge(self.user) + # Removing objects from test db session, so that we can operate + # on them from other sessions + db.expunge(user_settings) + db.expunge(self.user) # Find out which dependencies should be overridden, and store their # pre-override value diff --git a/tests/no_version/test_unit_user_settings.py b/tests/no_version/test_unit_user_settings.py index 8628dfc522..ba9419fac4 100644 --- a/tests/no_version/test_unit_user_settings.py +++ b/tests/no_version/test_unit_user_settings.py @@ -1,7 +1,12 @@ +import pytest from devtools import debug +from fastapi import HTTPException from fractal_server.app.models import UserOAuth from fractal_server.app.models import UserSettings +from fractal_server.app.routes.aux.validate_user_settings import ( + validate_user_settings, +) async def test_unit_link_user_to_settings(db): @@ -80,3 +85,64 @@ async def test_unit_link_user_to_settings(db): assert user_D.settings is not None # FIXME: Test delete cascade + + +async def test_validate_user_settings(db): + common_attributes = dict( + hashed_password="xxx", + is_active=True, + is_superuser=False, + is_verified=True, + ) + + # Prepare users + user_without_settings = UserOAuth(email="a@a.a", **common_attributes) + db.add(user_without_settings) + await db.commit() + await db.refresh(user_without_settings) + + invalid_settings = UserSettings() + user_with_invalid_settings = UserOAuth( + email="b@b.b", + **common_attributes, + settings=invalid_settings, + ) + db.add(user_with_invalid_settings) + await db.commit() + await db.refresh(user_with_invalid_settings) + + valid_settings = UserSettings( + ssh_host="x", + ssh_jobs_dir="/x", + ssh_private_key_path="/x", + ssh_tasks_dir="/x", + ssh_username="x", + ) + user_with_valid_settings = UserOAuth( + email="c@c.c", + **common_attributes, + settings=valid_settings, + ) + db.add(user_with_valid_settings) + await db.commit() + await db.refresh(user_with_valid_settings) + + debug(user_without_settings) + debug(user_with_invalid_settings) + debug(user_with_valid_settings) + + with pytest.raises(HTTPException, match="has no settings"): + await validate_user_settings( + user=user_without_settings, backend="slurm_ssh", db=db + ) + + with pytest.raises( + HTTPException, match="validation errors for SlurmSshUserSettings" + ): + await validate_user_settings( + user=user_with_invalid_settings, backend="slurm_ssh", db=db + ) + + await validate_user_settings( + user=user_with_valid_settings, backend="slurm_ssh", db=db + ) diff --git a/tests/v2/03_api/test_api_task_collection_ssh.py b/tests/v2/03_api/test_api_task_collection_ssh.py index 424929953d..c3478a25b1 100644 --- a/tests/v2/03_api/test_api_task_collection_ssh.py +++ b/tests/v2/03_api/test_api_task_collection_ssh.py @@ -7,6 +7,7 @@ PREFIX = "api/v2/task" +SLURM_USER = "test01" async def test_task_collection_ssh_from_pypi( @@ -19,6 +20,8 @@ async def test_task_collection_ssh_from_pypi( tmp777_path: Path, fractal_ssh: FractalSSH, current_py_version: str, + slurmlogin_ip, + ssh_keys, ): # Define and create remote working directory @@ -39,7 +42,18 @@ async def test_task_collection_ssh_from_pypi( } override_settings_factory(**settings_overrides) - async with MockCurrentUser(user_kwargs=dict(is_verified=True)): + user_settings_dict = dict( + ssh_host=slurmlogin_ip, + ssh_username=SLURM_USER, + ssh_private_key_path=ssh_keys["private"], + ssh_tasks_dir=(tmp777_path / "tasks").as_posix(), + ssh_jobs_dir=(tmp777_path / "artifacts").as_posix(), + ) + + async with MockCurrentUser( + user_kwargs=dict(is_verified=True), + user_settings_dict=user_settings_dict, + ): # CASE 1: successful collection diff --git a/tests/v2/08_full_workflow/common_functions.py b/tests/v2/08_full_workflow/common_functions.py index f764bf0283..1dfcc47aa5 100644 --- a/tests/v2/08_full_workflow/common_functions.py +++ b/tests/v2/08_full_workflow/common_functions.py @@ -497,6 +497,7 @@ async def workflow_with_non_python_task( tmp777_path: Path, additional_user_kwargs=None, this_should_fail: bool = False, + user_settings_dict: Optional[dict] = None, ) -> str: """ Run a non-python-task Fractal job. @@ -510,7 +511,9 @@ async def workflow_with_non_python_task( user_kwargs.update(additional_user_kwargs) debug(user_kwargs) - async with MockCurrentUser(user_kwargs=user_kwargs) as user: + async with MockCurrentUser( + user_kwargs=user_kwargs, user_settings_dict=user_settings_dict + ) as user: # Create project project = await project_factory_v2(user) project_id = project.id diff --git a/tests/v2/08_full_workflow/test_full_workflow_slurm_ssh_v2.py b/tests/v2/08_full_workflow/test_full_workflow_slurm_ssh_v2.py index cbd0d41718..7f71379edc 100644 --- a/tests/v2/08_full_workflow/test_full_workflow_slurm_ssh_v2.py +++ b/tests/v2/08_full_workflow/test_full_workflow_slurm_ssh_v2.py @@ -39,6 +39,14 @@ async def test_workflow_with_non_python_task_slurm_ssh( FRACTAL_SLURM_CONFIG_FILE=testdata_path / "slurm_config.json", ) + user_settings_dict = dict( + ssh_host=slurmlogin_ip, + ssh_username=SLURM_USER, + ssh_private_key_path=ssh_keys["private"], + ssh_tasks_dir=(tmp777_path / "tasks").as_posix(), + ssh_jobs_dir=(tmp777_path / "artifacts").as_posix(), + ) + connection = get_ssh_connection() app.state.fractal_ssh = FractalSSH(connection=connection) @@ -46,6 +54,7 @@ async def test_workflow_with_non_python_task_slurm_ssh( await workflow_with_non_python_task( MockCurrentUser=MockCurrentUser, + user_settings_dict=user_settings_dict, client=client, testdata_path=testdata_path, project_factory_v2=project_factory_v2, @@ -91,6 +100,14 @@ async def test_workflow_with_non_python_task_slurm_ssh_fail( FRACTAL_SLURM_CONFIG_FILE=testdata_path / "slurm_config.json", ) + user_settings_dict = dict( + ssh_host=slurmlogin_ip, + ssh_username=SLURM_USER, + ssh_private_key_path="/invalid/path", + ssh_tasks_dir=(tmp777_path / "tasks").as_posix(), + ssh_jobs_dir=(tmp777_path / "artifacts").as_posix(), + ) + connection = get_ssh_connection() app.state.fractal_ssh = FractalSSH(connection=connection) @@ -99,6 +116,7 @@ async def test_workflow_with_non_python_task_slurm_ssh_fail( job_logs = await workflow_with_non_python_task( MockCurrentUser=MockCurrentUser, client=client, + user_settings_dict=user_settings_dict, testdata_path=testdata_path, project_factory_v2=project_factory_v2, dataset_factory_v2=dataset_factory_v2,