From ca2bf4630a9d6a567f5a6798c83fe9adb5354ae7 Mon Sep 17 00:00:00 2001 From: Xylar Asay-Davis Date: Fri, 15 May 2026 15:03:09 +0200 Subject: [PATCH] Add support for group-writable roots to managed directories This is needed for directory used by NCO to point to the latest E3SM-Unified pixi environment. --- mache/deploy/run.py | 42 +++++-- mache/deploy/shared.py | 84 +++++++++++++- mache/deploy/templates/config.yaml.j2.j2 | 7 ++ mache/deploy/templates/hooks.py.j2 | 10 +- tests/test_deploy_run.py | 118 ++++++++++++++++++++ tests/test_deploy_shared.py | 133 ++++++++++++++++++++++- 6 files changed, 380 insertions(+), 14 deletions(-) diff --git a/mache/deploy/run.py b/mache/deploy/run.py index fa52de15..1a7feec7 100644 --- a/mache/deploy/run.py +++ b/mache/deploy/run.py @@ -1058,6 +1058,10 @@ def _apply_deploy_permissions( managed_paths.extend( _normalize_permission_path(path) for path in spack_paths ) + managed_paths.extend( + _normalize_permission_path(path) + for path in shared_artifacts.managed_recursive_dirs + ) managed_paths.extend( _normalize_permission_path(path) for path in shared_artifacts.managed_files @@ -1072,17 +1076,35 @@ def _apply_deploy_permissions( managed_paths = list(dict.fromkeys(managed_paths)) - if not managed_paths: - return + if managed_paths: + update_permissions( + managed_paths, + group, + show_progress=True, + group_writable=False, + other_readable=world_readable, + recursive=True, + ) - update_permissions( - managed_paths, - group, - show_progress=True, - group_writable=False, - other_readable=world_readable, - recursive=True, - ) + root_group_writable_dirs = [ + path + for path in ( + _normalize_permission_path(path) + for path in shared_artifacts.root_group_writable_dirs + ) + if path is not None + ] + root_group_writable_dirs = list(dict.fromkeys(root_group_writable_dirs)) + + if root_group_writable_dirs: + update_permissions( + root_group_writable_dirs, + group, + show_progress=True, + group_writable=True, + other_readable=world_readable, + recursive=False, + ) def _normalize_permission_path(path: str | None) -> str | None: diff --git a/mache/deploy/shared.py b/mache/deploy/shared.py index 972a9542..566f57ff 100644 --- a/mache/deploy/shared.py +++ b/mache/deploy/shared.py @@ -12,6 +12,8 @@ class SharedDeployArtifacts: base_path: str | None = None managed_dirs: list[str] = field(default_factory=list) managed_files: list[str] = field(default_factory=list) + managed_recursive_dirs: list[str] = field(default_factory=list) + root_group_writable_dirs: list[str] = field(default_factory=list) def create_shared_deploy_artifacts( @@ -29,11 +31,15 @@ def create_shared_deploy_artifacts( repo_root=repo_root, field_name='shared.base_path', ) - managed_dirs = _normalize_path_entries( + ( + managed_recursive_dirs, + root_group_writable_dirs, + ) = _normalize_managed_directory_entries( shared_cfg.get('managed_directories'), repo_root=repo_root, field_name='shared.managed_directories', ) + managed_dirs: list[str] = [] managed_files = _normalize_path_entries( shared_cfg.get('managed_files'), repo_root=repo_root, @@ -81,10 +87,16 @@ def create_shared_deploy_artifacts( dest_link.symlink_to(target_path) managed_dirs.append(str(dest_link.parent)) + managed_dirs = _dedupe_existing_paths(managed_dirs) + return SharedDeployArtifacts( base_path=base_path, - managed_dirs=_dedupe_existing_paths(managed_dirs), + managed_dirs=managed_dirs, managed_files=_dedupe_existing_paths(managed_files), + managed_recursive_dirs=_dedupe_existing_paths(managed_recursive_dirs), + root_group_writable_dirs=_dedupe_existing_paths( + root_group_writable_dirs + ), ) @@ -133,6 +145,54 @@ def _normalize_path_entries( return entries +def _normalize_managed_directory_entries( + value: Any, + *, + repo_root: str, + field_name: str, +) -> tuple[list[str], list[str]]: + if value is None: + return [], [] + if not isinstance(value, list): + raise ValueError(f'{field_name} must be a list if provided') + + managed_entries: list[str] = [] + root_group_writable_entries: list[str] = [] + for index, item in enumerate(value): + item_field_name = f'{field_name}[{index}]' + path_value: Any + root_group_writable: bool + if isinstance(item, str): + path_value = item + root_group_writable = False + elif isinstance(item, dict): + path_value = item.get('path') + raw_root_group_writable = _coerce_optional_bool( + item.get('root_group_writable'), + field_name=f'{item_field_name}.root_group_writable', + ) + root_group_writable = ( + False + if raw_root_group_writable is None + else raw_root_group_writable + ) + else: + raise ValueError( + f'{item_field_name} must be a string or mapping with a path' + ) + + path = _resolve_path( + value=path_value, + repo_root=repo_root, + field_name=f'{item_field_name}.path', + ) + managed_entries.append(path) + if root_group_writable: + root_group_writable_entries.append(path) + + return managed_entries, root_group_writable_entries + + def _normalize_load_script_copy_entries( value: Any, *, @@ -215,6 +275,26 @@ def _normalize_load_script_symlink_entries( return list(deduped.values()) +def _coerce_optional_bool( + value: Any, + *, + field_name: str, +) -> bool | None: + if value is None: + return None + if isinstance(value, bool): + return value + if isinstance(value, str): + candidate = value.strip().lower() + if candidate in ('true', 'yes', 'on', '1'): + return True + if candidate in ('false', 'no', 'off', '0'): + return False + if candidate in ('', 'none', 'null', 'dynamic'): + return None + raise ValueError(f'{field_name} must be a boolean if provided') + + def _resolve_path( *, value: Any, diff --git a/mache/deploy/templates/config.yaml.j2.j2 b/mache/deploy/templates/config.yaml.j2.j2 index 19945751..d59301cc 100644 --- a/mache/deploy/templates/config.yaml.j2.j2 +++ b/mache/deploy/templates/config.yaml.j2.j2 @@ -194,6 +194,13 @@ shared: # post-deploy permission update step. This is helpful when a downstream # pre_publish hook creates additional shared artifacts that mache itself # does not create. + # `managed_directories` entries may be either: + # - "/absolute/or/relative/path/to/directory" + # - {path: "/path/to/directory", root_group_writable: true} + # + # All managed directories are updated recursively without group-write + # permission. Use `root_group_writable: true` to apply group write only to + # that directory path itself after other permission updates. managed_directories: [] managed_files: [] diff --git a/mache/deploy/templates/hooks.py.j2 b/mache/deploy/templates/hooks.py.j2 index 7386aade..3e7831b3 100644 --- a/mache/deploy/templates/hooks.py.j2 +++ b/mache/deploy/templates/hooks.py.j2 @@ -94,7 +94,9 @@ def pre_pixi(ctx: DeployContext) -> dict[str, Any] | None: # - runtime["shared"]["base_path"]: shared base directory to update # permissions on recursively before path-specific updates # - runtime["shared"]["managed_directories"]: extra shared directories to - # include in the permission update step + # include in the permission update step. Entries may be strings or + # mappings like: + # {"path": "/shared/latest", "root_group_writable": True} # - runtime["shared"]["managed_files"]: extra shared files to include in # the permission update step # @@ -114,6 +116,12 @@ def pre_pixi(ctx: DeployContext) -> dict[str, Any] | None: # updates.setdefault("shared", {})["load_script_copies"] = [ # "/shared/load_my_software.sh", # ] + # updates.setdefault("shared", {})["managed_directories"] = [ + # { + # "path": "/shared/latest", + # "root_group_writable": True, + # }, + # ] return updates diff --git a/tests/test_deploy_run.py b/tests/test_deploy_run.py index c6d05e09..0db4dbfd 100644 --- a/tests/test_deploy_run.py +++ b/tests/test_deploy_run.py @@ -1195,6 +1195,124 @@ def _fake_update_permissions(*args, **kwargs): assert third_kwargs['recursive'] is True +def test_apply_deploy_permissions_applies_recursive_dir_and_writable_root( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + prefix = tmp_path / 'prefix' + prefix.mkdir() + + managed_dir = tmp_path / 'shared' / 'latest' + managed_dir.mkdir(parents=True) + + calls = [] + + def _fake_update_permissions(*args, **kwargs): + calls.append((args, kwargs)) + + monkeypatch.setattr( + deploy_run, 'update_permissions', _fake_update_permissions + ) + + logger = deploy_run.logging.getLogger( + 'test-apply-deploy-permissions-recursive-writable-root' + ) + logger.handlers = [deploy_run.logging.NullHandler()] + logger.propagate = False + + deploy_run._apply_deploy_permissions( + prefix=str(prefix), + extra_prefixes=None, + load_script_paths=[], + spack_paths=[], + shared_artifacts=SharedDeployArtifacts( + managed_recursive_dirs=[str(managed_dir)], + root_group_writable_dirs=[str(managed_dir)], + ), + group='e3sm', + world_readable=True, + logger=logger, + ) + + assert len(calls) == 3 + + first_args, first_kwargs = calls[0] + assert first_args == (str(prefix), 'e3sm') + assert first_kwargs['group_writable'] is False + assert first_kwargs['recursive'] is False + + second_args, second_kwargs = calls[1] + assert second_args == ([str(managed_dir)], 'e3sm') + assert second_kwargs['group_writable'] is False + assert second_kwargs['recursive'] is True + + third_args, third_kwargs = calls[2] + assert third_args == ([str(managed_dir)], 'e3sm') + assert third_kwargs['group_writable'] is True + assert third_kwargs['other_readable'] is True + assert third_kwargs['recursive'] is False + + +def test_apply_deploy_permissions_applies_writable_root_under_shared_base( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + prefix = tmp_path / 'prefix' + prefix.mkdir() + + shared_base = tmp_path / 'shared' + shared_base.mkdir() + writable_dir = shared_base / 'latest' + writable_dir.mkdir() + + calls = [] + + def _fake_update_permissions(*args, **kwargs): + calls.append((args, kwargs)) + + monkeypatch.setattr( + deploy_run, 'update_permissions', _fake_update_permissions + ) + + logger = deploy_run.logging.getLogger( + 'test-apply-deploy-permissions-group-writable' + ) + logger.handlers = [deploy_run.logging.NullHandler()] + logger.propagate = False + + deploy_run._apply_deploy_permissions( + prefix=str(prefix), + extra_prefixes=None, + load_script_paths=[], + spack_paths=[], + shared_artifacts=SharedDeployArtifacts( + base_path=str(shared_base), + managed_recursive_dirs=[str(writable_dir)], + root_group_writable_dirs=[str(writable_dir)], + managed_files=[], + ), + group='e3sm', + world_readable=True, + logger=logger, + ) + + assert len(calls) == 3 + + first_args, first_kwargs = calls[0] + assert first_args == (str(shared_base), 'e3sm') + assert first_kwargs['group_writable'] is False + assert first_kwargs['recursive'] is True + + second_args, second_kwargs = calls[1] + assert second_args == (str(prefix), 'e3sm') + assert second_kwargs['group_writable'] is False + assert second_kwargs['recursive'] is False + + third_args, third_kwargs = calls[2] + assert third_args == ([str(writable_dir)], 'e3sm') + assert third_kwargs['group_writable'] is True + assert third_kwargs['other_readable'] is True + assert third_kwargs['recursive'] is False + + def test_apply_deploy_permissions_updates_shared_base_first( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ): diff --git a/tests/test_deploy_shared.py b/tests/test_deploy_shared.py index dca48a95..5c507792 100644 --- a/tests/test_deploy_shared.py +++ b/tests/test_deploy_shared.py @@ -59,13 +59,15 @@ def test_create_shared_deploy_artifacts_copies_load_scripts_and_links( assert artifacts == SharedDeployArtifacts( base_path=None, managed_dirs=[ - str(extra_dir), str(copied_script.parent), ], managed_files=[ str(extra_file), str(copied_script), ], + managed_recursive_dirs=[ + str(extra_dir), + ], ) @@ -90,6 +92,135 @@ def test_create_shared_deploy_artifacts_resolves_runtime_base_path( ) +def test_create_shared_deploy_artifacts_marks_writable_roots( + tmp_path: Path, +): + repo_root = tmp_path / 'repo' + repo_root.mkdir() + + readonly_dir = repo_root / 'shared' / 'readonly' + writable_dir = repo_root / 'shared' / 'writable' + duplicate_dir = repo_root / 'shared' / 'duplicate' + runtime_dir = repo_root / 'shared' / 'runtime' + for path in [ + readonly_dir, + writable_dir, + duplicate_dir, + runtime_dir, + ]: + path.mkdir(parents=True) + + artifacts = create_shared_deploy_artifacts( + config={ + 'shared': { + 'managed_directories': [ + 'shared/readonly', + 'shared/duplicate', + { + 'path': 'shared/duplicate', + 'root_group_writable': True, + }, + { + 'path': 'shared/writable', + 'root_group_writable': 'true', + }, + ], + } + }, + runtime={}, + repo_root=str(repo_root), + load_script_paths=[], + logger=_logger(), + ) + + assert artifacts == SharedDeployArtifacts( + managed_recursive_dirs=[ + str(readonly_dir), + str(duplicate_dir), + str(writable_dir), + ], + root_group_writable_dirs=[ + str(duplicate_dir), + str(writable_dir), + ], + ) + + runtime_artifacts = create_shared_deploy_artifacts( + config={ + 'shared': { + 'managed_directories': ['shared/readonly'], + } + }, + runtime={ + 'shared': { + 'managed_directories': [ + { + 'path': 'shared/runtime', + 'root_group_writable': True, + } + ], + } + }, + repo_root=str(repo_root), + load_script_paths=[], + logger=_logger(), + ) + + assert runtime_artifacts == SharedDeployArtifacts( + managed_recursive_dirs=[str(runtime_dir)], + root_group_writable_dirs=[str(runtime_dir)], + ) + + +def test_create_shared_deploy_artifacts_validates_managed_directory_entries( + tmp_path: Path, +): + with pytest.raises( + ValueError, + match='shared.managed_directories\\[0\\] must be a string', + ): + create_shared_deploy_artifacts( + config={'shared': {'managed_directories': [42]}}, + runtime={}, + repo_root=str(tmp_path), + load_script_paths=[], + logger=_logger(), + ) + + with pytest.raises( + ValueError, + match='shared.managed_directories\\[0\\].path must not be null', + ): + create_shared_deploy_artifacts( + config={'shared': {'managed_directories': [{}]}}, + runtime={}, + repo_root=str(tmp_path), + load_script_paths=[], + logger=_logger(), + ) + + with pytest.raises( + ValueError, + match='shared.managed_directories\\[0\\].root_group_writable', + ): + create_shared_deploy_artifacts( + config={ + 'shared': { + 'managed_directories': [ + { + 'path': 'shared/writable', + 'root_group_writable': 'maybe', + } + ] + } + }, + runtime={}, + repo_root=str(tmp_path), + load_script_paths=[], + logger=_logger(), + ) + + def test_create_shared_deploy_artifacts_requires_single_load_script( tmp_path: Path, ):