Skip to content

Commit

Permalink
Clean up symlink handling
Browse files Browse the repository at this point in the history 
(Plus messy merge with cache manager.)
  • Loading branch information
@dbutenhof
dbutenhof committed Oct 12, 2023
1 parent 1d449fe commit 9182f08
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 79 deletions.
2 changes: 1 addition & 1 deletion lib/pbench/server/api/resources/datasets_contents.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def _get(

dataset: Dataset = params.uri["dataset"]
target = params.uri.get("target")
path = Path("" if target in ("/", None) else target)
path = Path("." if target in ("/", None) else target)

cache_m = CacheManager(self.config, current_app.logger)
try:
Expand Down
58 changes: 32 additions & 26 deletions lib/pbench/server/cache_manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
from pbench.common import MetadataLog, selinux
from pbench.server import JSONOBJECT, OperationCode, PathLike, PbenchServerConfig
from pbench.server.database.models.audit import Audit, AuditStatus, AuditType
from pbench.server import JSONOBJECT, OperationCode, PathLike, PbenchServerConfig
from pbench.server.database.models.audit import Audit, AuditStatus, AuditType
from pbench.server.database.models.datasets import Dataset
from pbench.server.utils import get_tarball_md5

Expand Down Expand Up @@ -73,7 +71,6 @@ class MetadataError(CacheManagerError):
def __init__(self, tarball: Path, error: Exception):
super().__init__(
f"A problem occurred processing metadata.log from {tarball}: {str(error)!r}"
f"A problem occurred processing metadata.log from {tarball}: {str(error)!r}"
)
self.tarball = tarball
self.error = str(error)
Expand All @@ -100,11 +97,23 @@ def __init__(self, tarball: Path, error: str):


class CacheType(Enum):
FILE = auto()

"""A directory"""

DIRECTORY = auto()
SYMLINK = auto()

"""A regular file"""
FILE = auto()

"""An invalid symlink (absolute or outside tarball)"""
BROKEN = auto()

"""A device or mount point or other unsupported inode type"""
OTHER = auto()

"""A relative symbolic link within the tarball"""
SYMLINK = auto()


@dataclass
class CacheObject:
Expand Down Expand Up @@ -144,21 +153,23 @@ def create(cls, root: Path, path: Path) -> "CacheObject":
if path.is_symlink():
ftype = CacheType.SYMLINK
link_path = path.readlink()
try:
if link_path.is_absolute():
raise ValueError("symlink path is absolute")
r_path = path.resolve(strict=True)
resolve_path = r_path.relative_to(root)
except (FileNotFoundError, ValueError):
if link_path.is_absolute():
resolve_path = link_path
resolve_type = CacheType.OTHER
resolve_type = CacheType.BROKEN
else:
if r_path.is_dir():
resolve_type = CacheType.DIRECTORY
elif r_path.is_file():
resolve_type = CacheType.FILE
try:
r_path = path.resolve(strict=True)
resolve_path = r_path.relative_to(root)
except (FileNotFoundError, ValueError):
resolve_path = link_path
resolve_type = CacheType.BROKEN
else:
resolve_type = CacheType.OTHER
if r_path.is_dir():
resolve_type = CacheType.DIRECTORY
elif r_path.is_file():
resolve_type = CacheType.FILE
else:
resolve_type = CacheType.OTHER
elif path.is_file():
ftype = CacheType.FILE
size = path.stat().st_size
Expand All @@ -169,7 +180,7 @@ def create(cls, root: Path, path: Path) -> "CacheObject":

return cls(
name="" if path == root else path.name,
location=Path("") if path == root else path.relative_to(root),
location=path.relative_to(root),
resolve_path=resolve_path,
resolve_type=resolve_type,
size=size,
Expand Down Expand Up @@ -314,7 +325,6 @@ def __init__(
self,
stream: IO[bytes],
lock: Optional[LockRef] = None,
lock: Optional[LockRef] = None,
subproc: Optional[subprocess.Popen] = None,
):
"""Construct an instance to track extracted inventory
Expand All @@ -326,8 +336,6 @@ def __init__(
stream: the data stream of a specific tarball member
lock: a cache lock reference
subproc: a Popen object to clean up on close
lock: a cache lock reference
subproc: a Popen object to clean up on close
"""
self.stream = stream
self.lock = lock
Expand Down Expand Up @@ -356,7 +364,6 @@ def close(self):
while self.subproc.stderr.read(4096):
pass
self.subproc.wait(60.0)
except Exception as e:
except Exception as e:
# Release our reference to the subprocess.Popen object so that the
# object can be reclaimed.
Expand Down Expand Up @@ -470,7 +477,7 @@ def __init__(self, path: Path, resource_id: str, controller: "Controller"):
self.cache.mkdir(parents=True, exist_ok=True)

# Record hierarchy of a Tar ball
self.cachemap: Optional[CacheMap] = None
self.cachemap: Optional[CacheMapEntry] = None

# Record the base of the unpacked files for cache management, which
# is (self.cache / self.name) and will be None when the cache is
Expand Down Expand Up @@ -646,7 +653,7 @@ def build_map(self):
dir_queue = deque(((self.unpacked, cmap),))
while dir_queue:
unpacked, parent_map = dir_queue.popleft()
curr: CacheMapEntry = {}
curr: CacheMap = {}
for path in unpacked.glob("*"):
details = CacheObject.create(self.unpacked, path)
curr[path.name] = {"details": details}
Expand Down Expand Up @@ -682,7 +689,7 @@ def find_entry(self, path: Path) -> CacheMapEntry:
return self.cachemap

path_parts = path.parts[:-1]
node: CacheMapEntry = self.cachemap["children"]
node: CacheMap = self.cachemap["children"]

try:
for dir in path_parts:
Expand Down Expand Up @@ -958,7 +965,6 @@ def get_results(self, lock: LockManager) -> Path:
root=audit,
status=AuditStatus.FAILURE if error else AuditStatus.SUCCESS,
attributes=attributes,
attributes=attributes,
)
lock.downgrade()

Expand Down
2 changes: 1 addition & 1 deletion lib/pbench/test/functional/server/test_datasets.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ def test_list_filter_typed(self, server_client: PbenchServerClient, login_user):
class TestInventory:
"""Validate APIs involving tarball inventory"""

@pytest.mark.dependency(name="contents", depends=["index"], scope="session")
@pytest.mark.dependency(name="contents", depends=["upload"], scope="session")
def test_contents(self, server_client: PbenchServerClient, login_user):
"""Check that we can retrieve the root directory TOC
Expand Down
89 changes: 43 additions & 46 deletions lib/pbench/test/unit/server/test_cache_manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

import pytest

from pbench.server import JSONOBJECT, OperationCode
from pbench.server import JSONOBJECT, OperationCode
from pbench.server.cache_manager import (
BadDirpath,
Expand All @@ -24,7 +23,6 @@
DuplicateTarball,
Inventory,
LockManager,
LockManager,
LockRef,
MetadataError,
Tarball,
Expand Down Expand Up @@ -453,7 +451,7 @@ def generate_test_result_tree(tmp_path: Path, dir_name: str) -> Path:
f1415_sym -> ./f1412_sym
f1416_sym -> ../../subdir12/f122_sym
f11.txt
f12_sym -> ..
f12_sym -> ../../..
f1.json
metadata.log
Expand Down Expand Up @@ -518,24 +516,38 @@ def generate_test_result_tree(tmp_path: Path, dir_name: str) -> Path:
parents=True, exist_ok=True
)
(sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1411.txt").touch()
sym_file = sub_dir / "subdir1" / "f12_sym"
os.symlink(Path(".."), sym_file)
sym_file = sub_dir / "subdir1" / "subdir12" / "f121_sym"
os.symlink(Path("../..") / "subdir1" / "subdir15", sym_file)
sym_file = sub_dir / "subdir1" / "subdir12" / "f122_sym"
os.symlink(Path(".") / "bad_subdir" / "nonexistent_file.txt", sym_file)
sym_file = sub_dir / "subdir1" / "subdir13" / "f131_sym"
os.symlink(Path("/etc") / "passwd", sym_file)
sym_file = sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1412_sym"
os.symlink(sub_dir / "subdir1" / "f11.txt", sym_file)
sym_file = sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1413_sym"
os.symlink(Path("..") / "subdir141", sym_file)
sym_file = sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1414_sym"
os.symlink(Path(".") / "f1411.txt", sym_file)
sym_file = sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1415_sym"
os.symlink(Path(".") / "f1412_sym", sym_file)
sym_file = sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1416_sym"
os.symlink(Path("../..") / "subdir12" / "f122_sym", sym_file)
os.symlink(Path("../../.."), sub_dir / "subdir1" / "f12_sym")
os.symlink(
Path("../..") / "subdir1" / "subdir15",
sub_dir / "subdir1" / "subdir12" / "f121_sym",
)
os.symlink(
Path(".") / "bad_subdir" / "nonexistent_file.txt",
sub_dir / "subdir1" / "subdir12" / "f122_sym",
)
os.symlink(
Path("/etc/passwd"), sub_dir / "subdir1" / "subdir13" / "f131_sym"
)
os.symlink(
sub_dir / "subdir1" / "f11.txt",
sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1412_sym",
)
os.symlink(
Path("..") / "subdir141",
sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1413_sym",
)
os.symlink(
Path(".") / "f1411.txt",
sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1414_sym",
)
os.symlink(
Path(".") / "f1412_sym",
sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1415_sym",
)
os.symlink(
Path("../..") / "subdir12" / "f122_sym",
sub_dir / "subdir1" / "subdir14" / "subdir141" / "f1416_sym",
)

return sub_dir

Expand Down Expand Up @@ -642,7 +654,7 @@ def mock_run(args, **_kwargs):
args, returncode=0, stdout="Successfully Unpacked!", stderr=None
)

def mock_resolve(_path, _strict=False):
def mock_resolve(_path, strict: bool = False):
"""In this scenario, there are no symlinks,
so resolve() should never be called."""
raise AssertionError("Unexpected call to Path.resolve()")
Expand Down Expand Up @@ -831,8 +843,8 @@ def fake_enter(_s):
"subdir1/f12_sym",
"subdir1/f12_sym",
"f12_sym",
Path("."),
CacheType.DIRECTORY,
Path("../../.."),
CacheType.BROKEN,
None,
CacheType.SYMLINK,
),
Expand All @@ -841,7 +853,7 @@ def fake_enter(_s):
"subdir1/subdir12/f121_sym",
"f121_sym",
Path("../../subdir1/subdir15"),
CacheType.OTHER,
CacheType.BROKEN,
None,
CacheType.SYMLINK,
),
Expand All @@ -850,7 +862,7 @@ def fake_enter(_s):
"subdir1/subdir12/f122_sym",
"f122_sym",
Path("bad_subdir/nonexistent_file.txt"),
CacheType.OTHER,
CacheType.BROKEN,
None,
CacheType.SYMLINK,
),
Expand All @@ -859,7 +871,7 @@ def fake_enter(_s):
"subdir1/subdir13/f131_sym",
"f131_sym",
Path("/etc/passwd"),
CacheType.OTHER,
CacheType.BROKEN,
None,
CacheType.SYMLINK,
),
Expand All @@ -886,7 +898,7 @@ def fake_enter(_s):
"subdir1/subdir14/subdir141/f1412_sym",
"f1412_sym",
Path("/mock_absolute_path/subdir1/f11.txt"),
CacheType.OTHER,
CacheType.BROKEN,
None,
CacheType.SYMLINK,
),
Expand Down Expand Up @@ -922,7 +934,7 @@ def fake_enter(_s):
"subdir1/subdir14/subdir141/f1416_sym",
"f1416_sym",
Path("../../subdir12/f122_sym"),
CacheType.OTHER,
CacheType.BROKEN,
None,
CacheType.SYMLINK,
),
Expand Down Expand Up @@ -967,15 +979,8 @@ def test_find_entry(

# test traverse with random path
c_map = tb.find_entry(Path(file_path))
if file_type is CacheType.DIRECTORY:
assert sorted(c_map.keys()) == [
"children",
"details",
], "Directory should have children and details"
else:
assert sorted(c_map.keys()) == [
"details"
], "Non-directory should have only details"
assert "details" in c_map
assert (file_type is not CacheType.DIRECTORY) ^ ("children" in c_map)
assert c_map["details"].location == Path(location)
assert c_map["details"].name == name
assert c_map["details"].resolve_path == resolve_path
Expand Down Expand Up @@ -1893,12 +1898,6 @@ def test_lockmanager(self, monkeypatch):
("upgrade"),
("downgrade"),
]
assert FakeLockRef.operations == [
assert FakeLockRef.operations == [
("acquire", False, True),
("upgrade"),
("downgrade"),
]
assert FakeLockRef.operations == [
("acquire", False, True),
("upgrade"),
Expand All @@ -1923,8 +1922,6 @@ def test_lockmanager(self, monkeypatch):
assert not lock.wait
assert FakeLockRef.operations == [("acquire", False, False)]
assert FakeLockRef.operations == [("acquire", False, False), ("release")]
assert FakeLockRef.operations == [("acquire", False, False)]
assert FakeLockRef.operations == [("acquire", False, False), ("release")]

# keep option
FakeLockRef.reset()
Expand Down
12 changes: 7 additions & 5 deletions lib/pbench/test/unit/server/test_indexing_tarballs.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from pathlib import Path
from signal import SIGHUP
import time
from typing import Any, Dict, List, Optional, Union
from typing import Any, Dict, List, Optional

import pytest

Expand Down Expand Up @@ -292,13 +292,15 @@ def __exit__(self, *exc):

def upgrade(self):
"""Upgrade a shared lock to exclusive"""
if not self.exclusive:
self.exclusive = True
self.exclusive = True

def downgrade(self):
"""Downgrade an exclusive lock to shared"""
if self.exclusive:
self.exclusive = False
self.exclusive = False

def release(self):
"""Release a lock kept past its expiration date"""
self.exclusive = False


class FakeController:
Expand Down

0 comments on commit 9182f08

Please sign in to comment.