fix(executors): run backups off the event loop
The scheduler serves its own REST API from the same loop that runs executors, and the config backup spends ~21 minutes inside tarfile and zlib. Called inline that starves the loop for the whole window: the service was unreachable 03:05-03:25 every night, and again at 07:39 today when the job was triggered by hand to prove the T-69 report path. asyncio.to_thread is the fix -- zlib releases the GIL while compressing, so the loop is scheduled normally. The outage was invisible for as long as it existed. The hourly health check fires at :35 and the outage runs 03:05-03:25, so no sample ever landed inside it. A fixed-phase hourly probe cannot see a 20-minute event; that is aliasing, not bad luck, and it would have stayed hidden indefinitely. health_report.report_async joins it: psycopg2 is a blocking driver, so reporting from the loop held it for the connect and insert -- up to connect_timeout seconds precisely when the database is unreachable, which is when a report matters most. Both backup executors use it now. Caveat worth knowing: the engine wraps executors in asyncio.wait_for and a thread cannot be cancelled, so on timeout the task is recorded failed while the tar runs to completion. Still strictly better than blocking everything, and the configured 3600s is well clear of the observed 1263s. Seven tests in this file had been red since the initial commit -- they came over with the portainer-core extraction, patched the Path class wholesale, asserted "backed up" against a function returning "Backup completed: ...", and one wrapped its call in except Exception: pass with its only assertion commented out. There is no CI test gate here, so nothing reported it. Replaced with tests that build real archives in tmp_path and assert on their contents. The new loop test is the one that matters and it is mutation-checked: with to_thread reverted it counts 0 heartbeat ticks, with it ~40. Their structural demands (_create_tar_filter, a sync _cleanup_old_backups) were adopted because threading wanted that shape anyway. Their exclude semantics were not. The tests assert fnmatch behaviour and the deployed config is written against substring matching -- it excludes logs as ".log", and paths as unanchored fragments like "ollama/models/*" against members named "docker-data/ollama/...". Under fnmatch neither matches, and the nightly archive would silently gain many GB of model blobs instead of shrinking. That is now pinned by tests naming the consequence, so the "improvement" fails loudly. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Claude
@@ -2,6 +2,20 @@
|
||||
Config Backup Executor
|
||||
Backs up Docker container configs and host-based service configs.
|
||||
Replicates functionality of maintenance container's backup-configs.sh
|
||||
|
||||
The work runs in a worker thread. `execute` is awaited by the scheduling engine
|
||||
on the same event loop that serves `/health` and the whole REST API, and this
|
||||
job spends ~21 minutes inside tarfile and zlib. Called inline it starves the
|
||||
loop for that entire window: on 2026-08-11 the API was unreachable 03:05-03:25
|
||||
nightly and again at 07:39 when the job was triggered by hand, which is T-74.
|
||||
The hourly health check fires at :35 and had never once sampled the outage.
|
||||
|
||||
`asyncio.to_thread` is the whole fix — zlib releases the GIL while compressing,
|
||||
so the loop gets scheduled normally. One caveat worth knowing: the engine wraps
|
||||
executors in `asyncio.wait_for`, and a thread cannot be cancelled. On timeout
|
||||
the task is recorded as failed while the tar keeps running to completion. That
|
||||
is still strictly better than blocking everything, and the configured timeout
|
||||
(3600s) is well clear of the observed 1263s.
|
||||
"""
|
||||
import asyncio
|
||||
import logging
|
||||
@@ -9,7 +23,7 @@ import tarfile
|
||||
import tempfile
|
||||
from datetime import datetime, timedelta
|
||||
from pathlib import Path
|
||||
from typing import List, Dict, Any
|
||||
from typing import Any, Callable, Dict, List, Optional
|
||||
|
||||
from src.config import Settings
|
||||
from src.executors import health_report
|
||||
@@ -17,125 +31,42 @@ from src.executors import health_report
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
async def _run(config: dict, settings: Settings) -> str:
|
||||
def _create_tar_filter(excludes: List[str]) -> Callable[[Any], Optional[Any]]:
|
||||
"""Build the tarfile filter for a set of exclude patterns.
|
||||
|
||||
Matching is **substring**, not glob, and that is deliberate: a pattern is
|
||||
reduced to its literal core by dropping leading `*/` and trailing `/*`, and
|
||||
a member is excluded when that core appears anywhere in its name.
|
||||
|
||||
It looks like a half-finished glob and the temptation is to "fix" it with
|
||||
`fnmatch`. Doing so would break the deployed configuration badly, because
|
||||
that configuration is written against these semantics:
|
||||
|
||||
".log" fnmatch would match only a file named exactly `.log`,
|
||||
so every log file starts being archived instead.
|
||||
"ollama/models/*" fnmatch anchors at the start of the name, and members
|
||||
are named `docker-data/ollama/models/...`, so nothing
|
||||
matches and many GB of model blobs enter the archive.
|
||||
|
||||
Same for `amp/Versions/*`, `qdrant/storage/*` and the rest — every one is an
|
||||
unanchored mid-path fragment. The nightly archive would grow, not shrink.
|
||||
`tests/test_config_backup_executor.py` pins both cases so this cannot be
|
||||
changed silently.
|
||||
"""
|
||||
Execute config backup task.
|
||||
cores = [p.replace('*/', '').replace('/*', '') for p in excludes]
|
||||
|
||||
Config schema:
|
||||
{
|
||||
"sources": [
|
||||
{
|
||||
"path": "/data/docker-data",
|
||||
"name": "docker-data",
|
||||
"excludes": ["*/cache/*", "*/temp/*", "*.log"]
|
||||
}
|
||||
],
|
||||
"backup_dir": "/backups/docker-configs",
|
||||
"retention_days": 30,
|
||||
"compress": true
|
||||
}
|
||||
def tar_filter(tarinfo):
|
||||
for core in cores:
|
||||
if core in tarinfo.name:
|
||||
logger.debug(f"Excluding: {tarinfo.name}")
|
||||
return None
|
||||
return tarinfo
|
||||
|
||||
Args:
|
||||
config: Backup configuration
|
||||
settings: Global scheduler settings
|
||||
|
||||
Returns:
|
||||
Summary of backup operation
|
||||
|
||||
Raises:
|
||||
Exception: On backup failure
|
||||
"""
|
||||
sources = config.get('sources', [])
|
||||
backup_dir = Path(config.get('backup_dir', '/backups/docker-configs'))
|
||||
retention_days = config.get('retention_days', 30)
|
||||
compress = config.get('compress', True)
|
||||
|
||||
if not sources:
|
||||
raise ValueError("No backup sources configured")
|
||||
|
||||
timestamp = datetime.now().strftime('%Y%m%d-%H%M%S')
|
||||
backup_filename = f"docker-configs-{timestamp}.tar.gz"
|
||||
backup_file = backup_dir / backup_filename
|
||||
|
||||
logger.info(f"Starting Docker configs backup: {backup_filename}")
|
||||
|
||||
# Create backup directory
|
||||
backup_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
# Create temporary directory for staging
|
||||
with tempfile.TemporaryDirectory(prefix='backup-') as temp_dir:
|
||||
temp_path = Path(temp_dir)
|
||||
results = []
|
||||
|
||||
# Backup each source
|
||||
for source in sources:
|
||||
source_path = Path(source['path'])
|
||||
source_name = source['name']
|
||||
excludes = source.get('excludes', [])
|
||||
|
||||
if not source_path.exists():
|
||||
logger.warning(f"Source path does not exist: {source_path}")
|
||||
continue
|
||||
|
||||
logger.info(f"Backing up {source_name} from {source_path}")
|
||||
|
||||
# Create tar for this source
|
||||
source_tar = temp_path / f"{source_name}.tar.gz"
|
||||
|
||||
def tar_filter(tarinfo):
|
||||
"""Filter function to exclude patterns."""
|
||||
for pattern in excludes:
|
||||
# Simple pattern matching (could be enhanced with fnmatch)
|
||||
if pattern.replace('*/', '').replace('/*', '') in tarinfo.name:
|
||||
logger.debug(f"Excluding: {tarinfo.name}")
|
||||
return None
|
||||
return tarinfo
|
||||
|
||||
with tarfile.open(source_tar, 'w:gz') as tar:
|
||||
tar.add(
|
||||
source_path,
|
||||
arcname=source_name,
|
||||
filter=tar_filter,
|
||||
recursive=True
|
||||
)
|
||||
|
||||
source_size = source_tar.stat().st_size / (1024 * 1024) # MB
|
||||
results.append(f"{source_name}: {source_size:.2f}MB")
|
||||
logger.info(f"Backed up {source_name}: {source_size:.2f}MB")
|
||||
|
||||
# Combine all source backups into final archive
|
||||
logger.info("Creating combined backup archive...")
|
||||
with tarfile.open(backup_file, 'w:gz') as final_tar:
|
||||
for item in temp_path.glob('*.tar.gz'):
|
||||
final_tar.add(item, arcname=item.name)
|
||||
|
||||
# Verify backup created
|
||||
if not backup_file.exists():
|
||||
raise Exception("Backup file was not created")
|
||||
|
||||
backup_size = backup_file.stat().st_size / (1024 * 1024) # MB
|
||||
logger.info(f"Backup created successfully: {backup_size:.2f}MB")
|
||||
|
||||
# Clean up old backups
|
||||
await cleanup_old_backups(backup_dir, retention_days)
|
||||
|
||||
# Count remaining backups
|
||||
backup_count = len(list(backup_dir.glob('docker-configs-*.tar.gz')))
|
||||
total_size = sum(f.stat().st_size for f in backup_dir.glob('docker-configs-*.tar.gz'))
|
||||
total_size_mb = total_size / (1024 * 1024)
|
||||
|
||||
output = (
|
||||
f"Backup completed: {backup_filename} ({backup_size:.2f}MB). "
|
||||
f"Sources: {', '.join(results)}. "
|
||||
f"Retention: {backup_count} backups, {total_size_mb:.2f}MB total."
|
||||
)
|
||||
|
||||
logger.info(output)
|
||||
return output
|
||||
return tar_filter
|
||||
|
||||
|
||||
async def cleanup_old_backups(backup_dir: Path, retention_days: int):
|
||||
"""Remove backups older than retention period."""
|
||||
def _cleanup_old_backups(backup_dir: Path, retention_days: int) -> None:
|
||||
"""Remove backups older than the retention period."""
|
||||
cutoff_date = datetime.now() - timedelta(days=retention_days)
|
||||
removed_count = 0
|
||||
removed_size = 0
|
||||
@@ -143,7 +74,6 @@ async def cleanup_old_backups(backup_dir: Path, retention_days: int):
|
||||
logger.info(f"Cleaning up backups older than {retention_days} days...")
|
||||
|
||||
for backup_file in backup_dir.glob('docker-configs-*.tar.gz'):
|
||||
# Get file modification time
|
||||
file_mtime = datetime.fromtimestamp(backup_file.stat().st_mtime)
|
||||
|
||||
if file_mtime < cutoff_date:
|
||||
@@ -160,6 +90,101 @@ async def cleanup_old_backups(backup_dir: Path, retention_days: int):
|
||||
logger.info("No old backups to remove")
|
||||
|
||||
|
||||
def _backup(config: dict) -> str:
|
||||
"""The blocking body of the backup. Runs in a worker thread, never on the loop.
|
||||
|
||||
Config schema:
|
||||
{
|
||||
"sources": [
|
||||
{
|
||||
"path": "/data/docker-data",
|
||||
"name": "docker-data",
|
||||
"excludes": ["*/cache/*", "*/temp/*", "*.log"]
|
||||
}
|
||||
],
|
||||
"backup_dir": "/backups/docker-configs",
|
||||
"retention_days": 30,
|
||||
"compress": true
|
||||
}
|
||||
|
||||
Returns a one-line summary. Raises on failure.
|
||||
"""
|
||||
sources = config.get('sources', [])
|
||||
backup_dir = Path(config.get('backup_dir', '/backups/docker-configs'))
|
||||
retention_days = config.get('retention_days', 30)
|
||||
|
||||
if not sources:
|
||||
raise ValueError("No backup sources configured")
|
||||
|
||||
timestamp = datetime.now().strftime('%Y%m%d-%H%M%S')
|
||||
backup_filename = f"docker-configs-{timestamp}.tar.gz"
|
||||
backup_file = backup_dir / backup_filename
|
||||
|
||||
logger.info(f"Starting Docker configs backup: {backup_filename}")
|
||||
|
||||
backup_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
with tempfile.TemporaryDirectory(prefix='backup-') as temp_dir:
|
||||
temp_path = Path(temp_dir)
|
||||
results = []
|
||||
|
||||
for source in sources:
|
||||
source_path = Path(source['path'])
|
||||
source_name = source['name']
|
||||
excludes = source.get('excludes', [])
|
||||
|
||||
if not source_path.exists():
|
||||
logger.warning(f"Source path does not exist: {source_path}")
|
||||
continue
|
||||
|
||||
logger.info(f"Backing up {source_name} from {source_path}")
|
||||
|
||||
source_tar = temp_path / f"{source_name}.tar.gz"
|
||||
|
||||
with tarfile.open(source_tar, 'w:gz') as tar:
|
||||
tar.add(
|
||||
source_path,
|
||||
arcname=source_name,
|
||||
filter=_create_tar_filter(excludes),
|
||||
recursive=True
|
||||
)
|
||||
|
||||
source_size = source_tar.stat().st_size / (1024 * 1024) # MB
|
||||
results.append(f"{source_name}: {source_size:.2f}MB")
|
||||
logger.info(f"Backed up {source_name}: {source_size:.2f}MB")
|
||||
|
||||
logger.info("Creating combined backup archive...")
|
||||
with tarfile.open(backup_file, 'w:gz') as final_tar:
|
||||
for item in temp_path.glob('*.tar.gz'):
|
||||
final_tar.add(item, arcname=item.name)
|
||||
|
||||
if not backup_file.exists():
|
||||
raise Exception("Backup file was not created")
|
||||
|
||||
backup_size = backup_file.stat().st_size / (1024 * 1024) # MB
|
||||
logger.info(f"Backup created successfully: {backup_size:.2f}MB")
|
||||
|
||||
_cleanup_old_backups(backup_dir, retention_days)
|
||||
|
||||
backup_count = len(list(backup_dir.glob('docker-configs-*.tar.gz')))
|
||||
total_size = sum(f.stat().st_size for f in backup_dir.glob('docker-configs-*.tar.gz'))
|
||||
total_size_mb = total_size / (1024 * 1024)
|
||||
|
||||
output = (
|
||||
f"Backup completed: {backup_filename} ({backup_size:.2f}MB). "
|
||||
f"Sources: {', '.join(results)}. "
|
||||
f"Retention: {backup_count} backups, {total_size_mb:.2f}MB total."
|
||||
)
|
||||
|
||||
logger.info(output)
|
||||
return output
|
||||
|
||||
|
||||
async def _run(config: dict, settings: Settings) -> str:
|
||||
"""Await the backup without holding the event loop. See the module docstring."""
|
||||
return await asyncio.to_thread(_backup, config)
|
||||
|
||||
|
||||
async def execute(config: dict, settings: Settings) -> str:
|
||||
"""Run the backup and report its own outcome to check_history (D-33, T-69).
|
||||
|
||||
@@ -172,7 +197,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
try:
|
||||
output = await _run(config, settings)
|
||||
except Exception as exc:
|
||||
health_report.report(
|
||||
await health_report.report_async(
|
||||
settings,
|
||||
domain="backup",
|
||||
status=health_report.CRITICAL,
|
||||
@@ -180,7 +205,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
metrics={"job": "scheduler/config_backup_executor", "error": str(exc)[:400]},
|
||||
)
|
||||
raise
|
||||
health_report.report(
|
||||
await health_report.report_async(
|
||||
settings,
|
||||
domain="backup",
|
||||
status=health_report.OK,
|
||||
|
||||
@@ -28,6 +28,7 @@ Three consequences that are load-bearing here:
|
||||
broken reporter produces — so check for rows, not for errors.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
from datetime import datetime, timezone
|
||||
@@ -122,6 +123,25 @@ def report(
|
||||
conn.close()
|
||||
|
||||
|
||||
async def report_async(
|
||||
settings: Any,
|
||||
domain: str,
|
||||
status: str,
|
||||
source: str,
|
||||
metrics: Optional[Dict[str, Any]] = None,
|
||||
) -> bool:
|
||||
"""`report` for callers on the event loop. Prefer this one inside executors.
|
||||
|
||||
psycopg2 is a blocking driver, so calling `report` directly from an
|
||||
`async def` holds the loop for the length of the connect and insert — up to
|
||||
`connect_timeout` seconds if the database is unreachable, which is exactly
|
||||
when a report is most likely to be attempted. The scheduler serves its own
|
||||
`/health` from that loop, so the cost of a slow report is the whole service
|
||||
appearing down (T-74).
|
||||
"""
|
||||
return await asyncio.to_thread(report, settings, domain, status, source, metrics)
|
||||
|
||||
|
||||
def _host() -> str:
|
||||
"""The host a row is attributed to.
|
||||
|
||||
|
||||
@@ -153,7 +153,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
try:
|
||||
output = await _run(config, settings)
|
||||
except Exception as exc:
|
||||
health_report.report(
|
||||
await health_report.report_async(
|
||||
settings,
|
||||
domain="backup",
|
||||
status=health_report.CRITICAL,
|
||||
@@ -161,7 +161,7 @@ async def execute(config: dict, settings: Settings) -> str:
|
||||
metrics={"job": "scheduler/portainer_backup_executor", "error": str(exc)[:400]},
|
||||
)
|
||||
raise
|
||||
health_report.report(
|
||||
await health_report.report_async(
|
||||
settings,
|
||||
domain="backup",
|
||||
status=health_report.OK,
|
||||
|
||||
@@ -1,214 +1,241 @@
|
||||
"""Tests for the config backup executor.
|
||||
|
||||
These replace a set that arrived with the portainer-core extraction and had
|
||||
never passed in this repo: they patched the `Path` class wholesale, asserted
|
||||
`"backed up" in result` against a function that returns `"Backup completed: …"`,
|
||||
and one wrapped its only call in `except Exception: pass` with its assertion
|
||||
commented out. Seven were red from the initial commit onward, and there is no CI
|
||||
test gate here to notice (see CLAUDE.md).
|
||||
|
||||
The replacements use real directories and real archives under `tmp_path`. A
|
||||
backup executor's whole job is what ends up inside the tar, and mocking
|
||||
`tarfile` means nothing is checked.
|
||||
"""
|
||||
Tests for the config backup executor.
|
||||
"""
|
||||
import pytest
|
||||
import asyncio
|
||||
import os
|
||||
import tarfile
|
||||
import time
|
||||
from pathlib import Path
|
||||
from unittest.mock import AsyncMock, patch, MagicMock, mock_open
|
||||
from src.executors import config_backup_executor
|
||||
|
||||
import pytest
|
||||
|
||||
from src.config import Settings
|
||||
from src.executors import config_backup_executor
|
||||
|
||||
|
||||
def _make_source(root: Path) -> Path:
|
||||
"""A source tree with one file per exclude pattern the deployment uses."""
|
||||
src = root / "docker-data"
|
||||
(src / "ollama" / "models" / "blobs").mkdir(parents=True)
|
||||
(src / "svc" / "cache").mkdir(parents=True)
|
||||
(src / "svc" / "logs").mkdir(parents=True)
|
||||
|
||||
(src / "keep.conf").write_text("keep me")
|
||||
(src / "svc" / "settings.json").write_text("keep me too")
|
||||
(src / "ollama" / "models" / "blobs" / "sha256-abc").write_text("many GB in reality")
|
||||
(src / "svc" / "cache" / "junk.bin").write_text("disposable")
|
||||
(src / "svc" / "logs" / "app.log").write_text("noisy")
|
||||
return src
|
||||
|
||||
|
||||
def _members(archive: Path) -> set:
|
||||
"""Names inside the inner per-source tar of a combined backup archive."""
|
||||
with tarfile.open(archive, "r:gz") as outer:
|
||||
inner_name = outer.getnames()[0]
|
||||
fh = outer.extractfile(inner_name)
|
||||
with tarfile.open(fileobj=fh, mode="r:gz") as inner:
|
||||
return set(inner.getnames())
|
||||
|
||||
|
||||
@pytest.mark.executor
|
||||
@pytest.mark.unit
|
||||
class TestConfigBackupExecutor:
|
||||
"""Tests for config_backup_executor module."""
|
||||
class TestConfigBackup:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_executor_requires_config_fields(self, test_settings: Settings):
|
||||
"""Test that executor validates required config."""
|
||||
incomplete_config = {
|
||||
"sources": []
|
||||
# Missing backup_dir
|
||||
}
|
||||
async def test_no_sources_is_rejected(self, test_settings: Settings, tmp_path: Path):
|
||||
with pytest.raises(ValueError, match="No backup sources"):
|
||||
await config_backup_executor._run({"backup_dir": str(tmp_path)}, test_settings)
|
||||
|
||||
with pytest.raises((ValueError, KeyError)):
|
||||
await config_backup_executor.execute(incomplete_config, test_settings)
|
||||
def test_archive_contains_the_source(self, tmp_path: Path):
|
||||
src = _make_source(tmp_path)
|
||||
out = tmp_path / "backups"
|
||||
summary = config_backup_executor._backup({
|
||||
"sources": [{"path": str(src), "name": "docker-data", "excludes": []}],
|
||||
"backup_dir": str(out),
|
||||
})
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_executor_with_minimal_config(self, test_settings: Settings, tmp_path: Path):
|
||||
"""Test executor with minimal valid configuration."""
|
||||
backup_dir = tmp_path / "backups"
|
||||
backup_dir.mkdir()
|
||||
assert "Backup completed" in summary
|
||||
archives = list(out.glob("docker-configs-*.tar.gz"))
|
||||
assert len(archives) == 1
|
||||
assert "docker-data/keep.conf" in _members(archives[0])
|
||||
|
||||
source_dir = tmp_path / "source"
|
||||
source_dir.mkdir()
|
||||
(source_dir / "test.txt").write_text("test content")
|
||||
|
||||
config = {
|
||||
def test_excludes_keep_matching_members_out(self, tmp_path: Path):
|
||||
"""The patterns here are the ones the deployed task actually carries."""
|
||||
src = _make_source(tmp_path)
|
||||
out = tmp_path / "backups"
|
||||
config_backup_executor._backup({
|
||||
"sources": [{
|
||||
"path": str(source_dir),
|
||||
"name": "test-source",
|
||||
"excludes": []
|
||||
"path": str(src),
|
||||
"name": "docker-data",
|
||||
"excludes": ["*/cache/*", ".log", "ollama/models/*"],
|
||||
}],
|
||||
"backup_dir": str(backup_dir),
|
||||
"compress": True,
|
||||
"retention_days": 30
|
||||
}
|
||||
"backup_dir": str(out),
|
||||
})
|
||||
|
||||
with patch('src.executors.config_backup_executor.Path') as mock_path_cls:
|
||||
# Setup path mocking
|
||||
mock_source = MagicMock()
|
||||
mock_source.exists.return_value = True
|
||||
mock_source.is_dir.return_value = True
|
||||
mock_source.iterdir.return_value = [MagicMock(name="test.txt")]
|
||||
names = _members(next(iter(out.glob("docker-configs-*.tar.gz"))))
|
||||
assert "docker-data/keep.conf" in names
|
||||
assert "docker-data/svc/settings.json" in names
|
||||
assert "docker-data/svc/cache/junk.bin" not in names
|
||||
assert "docker-data/svc/logs/app.log" not in names
|
||||
assert "docker-data/ollama/models/blobs/sha256-abc" not in names
|
||||
|
||||
mock_backup = MagicMock()
|
||||
mock_backup.mkdir = MagicMock()
|
||||
def test_missing_source_is_skipped_not_fatal(self, tmp_path: Path):
|
||||
src = _make_source(tmp_path)
|
||||
out = tmp_path / "backups"
|
||||
summary = config_backup_executor._backup({
|
||||
"sources": [
|
||||
{"path": str(tmp_path / "does-not-exist"), "name": "gone", "excludes": []},
|
||||
{"path": str(src), "name": "docker-data", "excludes": []},
|
||||
],
|
||||
"backup_dir": str(out),
|
||||
})
|
||||
assert "docker-data" in summary
|
||||
assert "gone" not in summary
|
||||
|
||||
def path_side_effect(p):
|
||||
if str(p) == str(source_dir):
|
||||
return mock_source
|
||||
elif str(p) == str(backup_dir):
|
||||
return mock_backup
|
||||
return MagicMock()
|
||||
def test_cleanup_removes_only_expired_backups(self, tmp_path: Path):
|
||||
out = tmp_path / "backups"
|
||||
out.mkdir()
|
||||
old = out / "docker-configs-20200101-000000.tar.gz"
|
||||
recent = out / "docker-configs-20991231-000000.tar.gz"
|
||||
unrelated = out / "notes.txt"
|
||||
for f in (old, recent, unrelated):
|
||||
f.write_text("x")
|
||||
|
||||
mock_path_cls.side_effect = path_side_effect
|
||||
long_ago = time.time() - (30 * 86400)
|
||||
os.utime(old, (long_ago, long_ago))
|
||||
|
||||
with patch('tarfile.open'), \
|
||||
patch('src.executors.config_backup_executor._cleanup_old_backups'):
|
||||
config_backup_executor._cleanup_old_backups(out, retention_days=7)
|
||||
|
||||
result = await config_backup_executor.execute(config, test_settings)
|
||||
|
||||
assert "backed up" in result.lower() or "success" in result.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_executor_excludes_patterns(self, test_settings: Settings, tmp_path: Path):
|
||||
"""Test that executor respects exclude patterns."""
|
||||
backup_dir = tmp_path / "backups"
|
||||
source_dir = tmp_path / "source"
|
||||
|
||||
config = {
|
||||
"sources": [{
|
||||
"path": str(source_dir),
|
||||
"name": "test",
|
||||
"excludes": ["*.log", "cache/*"]
|
||||
}],
|
||||
"backup_dir": str(backup_dir),
|
||||
"compress": True
|
||||
}
|
||||
|
||||
with patch('src.executors.config_backup_executor.Path'), \
|
||||
patch('tarfile.open') as mock_tar, \
|
||||
patch('src.executors.config_backup_executor._cleanup_old_backups'):
|
||||
|
||||
# Mock tarfile
|
||||
mock_tar_obj = MagicMock()
|
||||
mock_tar.return_value.__enter__ = MagicMock(return_value=mock_tar_obj)
|
||||
mock_tar.return_value.__exit__ = MagicMock(return_value=None)
|
||||
|
||||
try:
|
||||
await config_backup_executor.execute(config, test_settings)
|
||||
except Exception:
|
||||
# May fail due to mocking complexity, but that's ok
|
||||
pass
|
||||
|
||||
# Should have attempted to create tarfile
|
||||
# assert mock_tar.called # Would check if it was actually called
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_executor_handles_missing_source(self, test_settings: Settings, tmp_path: Path):
|
||||
"""Test executor handles missing source directory."""
|
||||
backup_dir = tmp_path / "backups"
|
||||
backup_dir.mkdir()
|
||||
|
||||
config = {
|
||||
"sources": [{
|
||||
"path": "/nonexistent/path",
|
||||
"name": "missing",
|
||||
"excludes": []
|
||||
}],
|
||||
"backup_dir": str(backup_dir),
|
||||
"compress": True
|
||||
}
|
||||
|
||||
with patch('src.executors.config_backup_executor.Path') as mock_path_cls:
|
||||
mock_source = MagicMock()
|
||||
mock_source.exists.return_value = False
|
||||
|
||||
mock_path_cls.return_value = mock_source
|
||||
|
||||
result = await config_backup_executor.execute(config, test_settings)
|
||||
|
||||
# Should skip non-existent sources
|
||||
assert "skipped" in result.lower() or "not found" in result.lower() or "0" in result
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cleanup_old_backups(self, tmp_path: Path):
|
||||
"""Test cleanup of old backup files."""
|
||||
backup_dir = tmp_path / "backups"
|
||||
backup_dir.mkdir()
|
||||
|
||||
# Create some "old" backup files
|
||||
old_backup = backup_dir / "backup-2020-01-01.tar.gz"
|
||||
old_backup.write_text("old")
|
||||
|
||||
recent_backup = backup_dir / "backup-2025-12-01.tar.gz"
|
||||
recent_backup.write_text("recent")
|
||||
|
||||
with patch('src.executors.config_backup_executor.Path') as mock_path_cls:
|
||||
mock_backup_dir = MagicMock()
|
||||
mock_old_file = MagicMock()
|
||||
mock_old_file.name = "backup-2020-01-01.tar.gz"
|
||||
mock_old_file.stat.return_value.st_mtime = 0 # Very old
|
||||
|
||||
mock_recent_file = MagicMock()
|
||||
mock_recent_file.name = "backup-2025-12-01.tar.gz"
|
||||
mock_recent_file.stat.return_value.st_mtime = 999999999999 # Recent
|
||||
|
||||
mock_backup_dir.glob.return_value = [mock_old_file, mock_recent_file]
|
||||
mock_path_cls.return_value = mock_backup_dir
|
||||
|
||||
config_backup_executor._cleanup_old_backups(mock_backup_dir, retention_days=7)
|
||||
|
||||
# Old file should be removed
|
||||
mock_old_file.unlink.assert_called_once()
|
||||
assert not old.exists()
|
||||
assert recent.exists()
|
||||
assert unrelated.exists(), "cleanup must only touch files it wrote"
|
||||
|
||||
|
||||
@pytest.mark.executor
|
||||
@pytest.mark.unit
|
||||
class TestConfigBackupHelpers:
|
||||
"""Tests for helper functions."""
|
||||
class TestEventLoopIsNotBlocked:
|
||||
"""T-74. The scheduler serves its own API from the loop that runs executors.
|
||||
|
||||
def test_tar_filter_excludes_cache(self):
|
||||
"""Test that tar filter excludes cache directories."""
|
||||
excludes = ["*/cache/*", "*.log"]
|
||||
filter_func = config_backup_executor._create_tar_filter(excludes)
|
||||
This job spends ~21 minutes in tarfile and zlib, so calling it inline made
|
||||
the whole service unreachable 03:05-03:25 every night. The hourly health
|
||||
check runs at :35 and so never once observed it — the outage was invisible
|
||||
for as long as it existed.
|
||||
"""
|
||||
|
||||
# Mock tarinfo for cache file
|
||||
cache_tarinfo = MagicMock()
|
||||
cache_tarinfo.name = "data/cache/temp.txt"
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_yields_to_the_loop_while_backing_up(
|
||||
self, test_settings: Settings, monkeypatch
|
||||
):
|
||||
blocked_for = 0.4
|
||||
monkeypatch.setattr(
|
||||
config_backup_executor, "_backup",
|
||||
lambda config: (time.sleep(blocked_for), "Backup completed: fake")[1],
|
||||
)
|
||||
|
||||
result = filter_func(cache_tarinfo)
|
||||
assert result is None # Should exclude
|
||||
ticks = 0
|
||||
|
||||
def test_tar_filter_includes_normal_files(self):
|
||||
"""Test that tar filter includes normal files."""
|
||||
excludes = ["*/cache/*"]
|
||||
filter_func = config_backup_executor._create_tar_filter(excludes)
|
||||
async def heartbeat():
|
||||
nonlocal ticks
|
||||
while True:
|
||||
await asyncio.sleep(0.01)
|
||||
ticks += 1
|
||||
|
||||
# Mock tarinfo for normal file
|
||||
normal_tarinfo = MagicMock()
|
||||
normal_tarinfo.name = "data/config.json"
|
||||
hb = asyncio.create_task(heartbeat())
|
||||
try:
|
||||
result = await config_backup_executor._run({}, test_settings)
|
||||
finally:
|
||||
hb.cancel()
|
||||
|
||||
result = filter_func(normal_tarinfo)
|
||||
assert result == normal_tarinfo # Should include
|
||||
assert result == "Backup completed: fake"
|
||||
# Held inline, the loop gets no scheduling opportunity at all and this is
|
||||
# 0. Off the loop it is ~40. The bar is low on purpose: the distinction
|
||||
# being drawn is "the loop ran" versus "the loop was dead", and a loaded
|
||||
# CI box should not turn that into a flake.
|
||||
assert ticks >= 5, f"event loop starved during backup: {ticks} ticks"
|
||||
|
||||
def test_tar_filter_with_wildcard_patterns(self):
|
||||
"""Test tar filter with various wildcard patterns."""
|
||||
excludes = ["*.log", "*.tmp", "temp/*"]
|
||||
filter_func = config_backup_executor._create_tar_filter(excludes)
|
||||
|
||||
# Log file
|
||||
log_tarinfo = MagicMock()
|
||||
log_tarinfo.name = "app.log"
|
||||
assert filter_func(log_tarinfo) is None
|
||||
@pytest.mark.executor
|
||||
@pytest.mark.unit
|
||||
class TestExcludeMatching:
|
||||
"""Substring matching, and why it must stay that way.
|
||||
|
||||
# Temp file
|
||||
tmp_tarinfo = MagicMock()
|
||||
tmp_tarinfo.name = "cache.tmp"
|
||||
assert filter_func(tmp_tarinfo) is None
|
||||
`_create_tar_filter` reduces each pattern to a literal core and asks whether
|
||||
it appears anywhere in the member name. That reads like an unfinished glob,
|
||||
and the obvious "improvement" is `fnmatch`. These tests exist to make that
|
||||
change fail loudly, because the deployed config is written against these
|
||||
semantics and `fnmatch` would silently stop excluding the largest things in
|
||||
the tree.
|
||||
"""
|
||||
|
||||
# Normal file
|
||||
normal_tarinfo = MagicMock()
|
||||
normal_tarinfo.name = "config.json"
|
||||
assert filter_func(normal_tarinfo) == normal_tarinfo
|
||||
def test_mid_path_fragment_matches_anywhere(self):
|
||||
"""`ollama/models/*` must exclude a member named `docker-data/ollama/...`.
|
||||
|
||||
Under fnmatch the pattern anchors at the start of the name, does not
|
||||
match, and many GB of model blobs enter the nightly archive.
|
||||
"""
|
||||
f = config_backup_executor._create_tar_filter(["ollama/models/*"])
|
||||
|
||||
class TI:
|
||||
name = "docker-data/ollama/models/blobs/sha256-abc"
|
||||
|
||||
assert f(TI()) is None
|
||||
|
||||
def test_bare_suffix_matches_every_file_carrying_it(self):
|
||||
"""The deployment excludes logs by the bare string `.log`, not `*.log`.
|
||||
|
||||
Under fnmatch this matches only a file named exactly `.log`, so every
|
||||
real log file starts being archived.
|
||||
"""
|
||||
f = config_backup_executor._create_tar_filter([".log"])
|
||||
|
||||
class TI:
|
||||
name = "docker-data/svc/logs/app.log"
|
||||
|
||||
assert f(TI()) is None
|
||||
|
||||
def test_wrapped_pattern_is_reduced_to_its_core(self):
|
||||
f = config_backup_executor._create_tar_filter(["*/cache/*"])
|
||||
|
||||
class Cache:
|
||||
name = "docker-data/svc/cache/junk.bin"
|
||||
|
||||
class Normal:
|
||||
name = "docker-data/svc/settings.json"
|
||||
|
||||
normal = Normal()
|
||||
assert f(Cache()) is None
|
||||
assert f(normal) is normal
|
||||
|
||||
def test_a_glob_star_is_not_interpreted(self):
|
||||
"""`*.log` is a literal here — it is not a suffix match.
|
||||
|
||||
This is the sharp edge of substring matching and the reason the deployed
|
||||
config spells the pattern `.log`. Pinned so the behaviour is documented
|
||||
rather than discovered.
|
||||
"""
|
||||
f = config_backup_executor._create_tar_filter(["*.log"])
|
||||
|
||||
class TI:
|
||||
name = "app.log"
|
||||
|
||||
ti = TI()
|
||||
assert f(ti) is ti
|
||||
|
||||
def test_no_excludes_keeps_everything(self):
|
||||
f = config_backup_executor._create_tar_filter([])
|
||||
|
||||
class TI:
|
||||
name = "anything/at/all"
|
||||
|
||||
ti = TI()
|
||||
assert f(ti) is ti
|
||||
|
||||
Reference in New Issue
Block a user