Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Proxmox mirror sync wrapper to safely synchronize /debian and /images via tsumugu, while handling the non-indexed /iso page with a custom HTML parser/downloader to avoid incorrect parsing and unsafe deletions.
Changes:
- Replace prior apt-sync/lftp approach with
tsumugu syncscoped per subtree for safe cleanup behavior. - Add an embedded Python
/isosync stage that discovers artifacts from the custom HTML page, performs HEAD metadata checks, downloads atomically, and deletes stale files with a bounded limit. - Add extensive in-script rationale documenting why
/isomust be handled separately.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fddb84e to
2ea1ec0
Compare
|
Reworked from production experience: the previous single proxmox.sh could not run in either stock image (tunathu/tsumugu has no python3, tunathu/tunasync-scripts has no tsumugu). Now split into two jobs (2ea1ec0): proxmox-deb-img.sh syncs /debian/ and /images/ with tsumugu, proxmox-iso.py scrapes the custom /iso/ page with stdlib-only python (atomic .tmp+rename downloads, stale deletion capped by TUNASYNC_PROXMOX_ISO_MAXDELETE). Both have been running in production on mirror.nju.edu.cn. |
|
Copilot review addressed:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The entrypoint and required orchestration are missing, reporting is incomplete, and parser safety issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (5)
Resolved since last review (7)
download.proxmox.comas the only allowed host, butbaseis derived…download.proxmox.comas the only allowed host, butbaseis derived…HEAD(e.g., 405/403) but allowsGET, the initial sync…HEAD(e.g., 405/403) but allowsGET, the initial sync…USERAGENT(possibly including the localtsumugu --version) and passes it to…set -eudropspipefail, but the script uses pipelines (e.g., inUSERAGENT). Without `set -o…set -eudropspipefail, but the script uses pipelines (e.g., inUSERAGENT). Without `set -o…
happyaron
left a comment
happyaron
left a comment
There was a problem hiding this comment.
Thanks for working on this — the old script clearly doesn't cope with the current upstream layout. A few things need addressing before this can go in:
1. The PR description doesn't match the diff. It describes a single rewritten proxmox.sh (42 → 219 lines), but the diff deletes proxmox.sh and adds two separate jobs (proxmox-deb-img.sh + proxmox-iso.py). It also says /pmg, /pve, /devel are siblings of /iso (they're under debian/), that size reporting via REPO_SIZE_FILE / helpers/size-sum.sh is preserved (that helper doesn't exist in this repo, and neither new script reports size), and that tsumugu handles /iso with a parser hint (the PR adds a Python scraper precisely because it doesn't). Please update it so reviewers know what they're approving.
2. Deleting proxmox.sh breaks existing deployments. Every mirror whose tunasync config points at proxmox.sh will start failing after pulling this. The replacement also requires config changes that aren't documented anywhere: two jobs, two different images (tunathu/tsumugu and tunathu/tunasync-scripts), sharing one working dir. Please either keep proxmox.sh as the entry point or document the new job setup. See also the inline comment on scope/size.
3. /iso is no longer synced recursively. The old lftp mirror was recursive; the new scraper only reads the top-level page. Fine while /iso/ stays flat, but worth stating explicitly.
| --exclude '/pmg/dists/.+changelog$' \ | ||
| --exclude '^/dists/trixie/pve-test/binary-arm64/' \ | ||
| --exclude '^/pve/dists/trixie/pve-test/binary-arm64/' \ | ||
| "${UPSTREAM%/}/debian/" "$WORKDIR/debian" |
There was a problem hiding this comment.
This mirrors all of /debian/, which is a big scope increase over the old script (pve / pbs / pbs-client / pmg only, current Debian release only, amd64 only). Upstream /debian/ currently also includes ceph-octopus … ceph-tentacle, corosync-3, pdm, devel, and bullseye/bookworm/trixie across all architectures.
On top of that, on a fresh install the pve packages get stored twice: upstream /debian/dists/ is a real directory holding the .debs themselves (Packages has Filename: dists/trixie/pve-no-subscription/binary-amd64/…deb). Existing mirrors are unaffected because tsumugu skips the local debian/dists -> pve/dists symlink left by the old script (and keeps it in the delete pass), but a new deployment has no such symlink and will download a full second copy.
Could you state whether the wider scope is intended and give the expected disk usage vs. the old script? Summing Size: from the Packages indices (deduped by Filename:) gives a close estimate. If not intended, restrict with include/exclude rules; for dists, either create the symlink before running tsumugu or exclude ^/dists/.
There was a problem hiding this comment.
The wider scope was not intended — the default is now restricted back to the legacy apt-sync coverage in b83cb08: repos pve/pbs/pbs-client/pmg, all suites upstream publishes for them (currently bullseye/bookworm/trixie, the same set as apt-sync.py's @debian-current), amd64 only, plus the top-level files, via ordered --exclusion-v2 rules. PROXMOX_DEB_ALL=1 opts back into the full /debian/ tree. Measured with tsumugu sync --dry-run against live upstream: restricted scope = ~48.5k objects / ~398 GiB apparent; the excluded extras (ceph-*, corosync-3, pdm, devel, arm64 trees) add roughly 90+ GiB. The fresh-install duplicate is fixed by pre-creating debian/dists -> pve/dists before syncing (verified in the dry-run: tsumugu records the symlink in its keep-set and never descends into it). One operational caveat, now also in the script comment: tsumugu treats excluded remote paths as absent, so narrowing scope on an existing full mirror deletes the extra local trees — bounded by --max-delete, which aborts the run when exceeded.
| WORKDIR="${TUNASYNC_WORKING_DIR:-/data/mirrors/proxmox}" | ||
| UPSTREAM="${TUNASYNC_UPSTREAM_URL:-http://download.proxmox.com/}" | ||
| MAXDELETE="${TUNASYNC_TSUMUGU_MAXDELETE:-10000}" | ||
| THREADS="${TUNASYNC_TSUMUGU_THREADS:-1}" | ||
| USERAGENT="${TUNASYNC_TSUMUGU_USERAGENT:-tsumugu}" | ||
|
|
||
| tsumugu sync \ | ||
| --timezone 0 --user-agent "$USERAGENT" --max-delete "$MAXDELETE" \ |
There was a problem hiding this comment.
This duplicates tsumugu.sh and drifts from its defaults:
WORKDIRdefaults to a site-specific/data/mirrors/proxmox; other scripts requireTUNASYNC_WORKING_DIR.- User agent is bare
tsumugurather than the versionedtsumugu/<ver>; threads default 1 instead of 2;NO_COLOR=1isn't exported. --timezone 0is hardcoded.
Could this reuse tsumugu.sh (called twice) or at least its variable handling?
There was a problem hiding this comment.
Direct reuse isn't possible in our deployment: production bind-mounts only this single script into the tsumugu image, so tsumugu.sh isn't present beside it to call. b83cb08 instead inlines its variable handling and fixes the drift you listed: TUNASYNC_WORKING_DIR is now required (no site-specific default), threads default 2, maxdelete default 1000, versioned tsumugu/<ver> user agent, NO_COLOR=1 exported. --timezone 0 stays, now with a comment: it disables tsumugu's recursive-HEAD timezone guessing, which has panicked on other mirrors (zabbix-app), and the upstream listings are UTC.
| tsumugu sync \ | ||
| --timezone 0 --user-agent "$USERAGENT" --max-delete "$MAXDELETE" \ | ||
| --parser nginx --threads "$THREADS" \ | ||
| --ignore-status 503 \ |
There was a problem hiding this comment.
Why --ignore-status 503 here? It can mask a genuine upstream outage on /images/. Please add a comment explaining which paths return 503 and why that's safe to ignore.
There was a problem hiding this comment.
Removed in b83cb08. I probed /images/ today (root, mail/, system/, sample files — all 200) and the flag has no documented origin in our history (it appeared in the job-split commit; the pre-split script didn't have it), so I can't name a path that needs it — keeping it would only mask a genuine outage. If a specific path ever turns out to 503 intermittently we can re-add it scoped, with the comment you asked for.
| --exclude '^/dists/trixie/pve-test/binary-arm64/' \ | ||
| --exclude '^/pve/dists/trixie/pve-test/binary-arm64/' \ |
There was a problem hiding this comment.
These trixie pve-test/binary-arm64 excludes are hardcoded to a single suite and will go stale. Please add a comment on why they're needed (and whether a suite-agnostic regex would do).
There was a problem hiding this comment.
Replaced by the suite-agnostic --exclude '/binary-arm64/' in b83cb08, with a comment. Background: those trees returned 401 when listed upstream at the time the rules were added (they have since been fixed), and the legacy scope is amd64-only anyway — a generic rule can't go stale when the next Debian suite appears.
| print(f'proxmox iso: metadata probe failed for missing {name}: {e}', file=sys.stderr, flush=True) | ||
| errors.append(f'{name}: probe {e}') | ||
| continue | ||
| if target.exists() and size >= 0 and target.stat().st_size == size: |
There was a problem hiding this comment.
Size-only comparison. tsumugu and the old lftp --only-newer also compare mtime, so a same-size replacement (e.g. re-signed .asc, re-spun ISO) will never be re-fetched here. Since Last-Modified is already fetched, comparing it against the local mtime would be cheap.
Related: when Content-Length is absent, size is -1 (lines 106/113), so this condition is false and the file is re-downloaded on every run.
There was a problem hiding this comment.
Fixed in b83cb08: up_to_date() now requires every available signal to match — size when Content-Length was sent AND Last-Modified compared against the local mtime when sent — so same-size replacements (re-signed .asc, re-spun ISO) are re-fetched. Your related point is fixed too: when Content-Length is absent, a matching Last-Modified now suffices to skip instead of re-downloading every run; with neither signal the file is still re-downloaded. Covered by integration tests (same-size/newer-mtime refetch, no-Content-Length skip/refetch).
|
|
||
| # Delete only stale regular files directly under /iso/. This cannot affect | ||
| # /debian/ or /images/ because this stage never traverses outside work. | ||
| stale = [p for p in work.iterdir() if p.is_file() and not p.name.startswith('.tmp.') and p.name not in remote_names] |
There was a problem hiding this comment.
.tmp.* files are excluded from stale cleanup, but if tunasync kills the job mid-download the partial file is left behind. It's only overwritten if the same name reappears, so a partial ISO for a file later removed upstream stays on disk forever. Consider deleting stray .tmp.* at startup (or including them in cleanup).
There was a problem hiding this comment.
Done in b83cb08: leftover .tmp.* files under the iso workdir are removed at startup. Downloads always restart from scratch and rename atomically, so this is safe, and a partial file whose remote name later disappears no longer stays on disk forever.
| name = urllib.parse.unquote(Path(parsed.path).name) | ||
| if not name or name in ('.', '..') or name.endswith('/'): |
There was a problem hiding this comment.
After Path(...).name, name.endswith('/') and name in ('.', '..') can never be true. A link like ./ or /iso/ would become a file named iso, and /iso/sub/x.iso would be flattened to x.iso. Checking parsed.path.endswith('/') and requiring no further / after base_path would make the guard actually work. (The current page has no such links, so this is defensive.)
There was a problem hiding this comment.
Fixed in b83cb08: the guard now works on parsed.path — trailing-slash links are rejected before Path().name, and any path still containing / after the base_path prefix (i.e. a subdirectory link) is skipped instead of being flattened. Integration tests cover ./, /iso/ and /iso/sub/x.iso links.
| # been processed, and only if the count is <= TUNASYNC_PROXMOX_ISO_MAXDELETE. | ||
| # | ||
| # This stage deliberately does not verify SHA256 itself; however the mirror was | ||
| # separately reviewed after implementation and all official *.sha256 files |
There was a problem hiding this comment.
This note records a one-off manual check on one deployment; it doesn't describe the code's behaviour and will age badly. Suggest either verifying .sha256 in the script or dropping the sentence.
There was a problem hiding this comment.
Went with real verification in b83cb08: .sha256 files are fetched first, and every payload this run fetched (or whose .sha256 changed) is hashed and compared. A mismatch removes both sides of the pair (so the next run re-fetches a consistent set) and fails the job. The one-off note is gone.
|
Round-2 Copilot review addressed in 65f12f8:
Verified end-to-end against a local test server (cross-host redirect link refused and not fetched; On the lingering USERAGENT finding from round 1: resolved by construction — both stages now read |
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| _here=$(dirname "$(realpath "$0")") |
There was a problem hiding this comment.
proxmox.sh is restored as a forwarding wrapper in b83cb08: it runs each stage the current image supports (deb/images when tsumugu is present, iso when python3 is), prints a loud stderr warning for every skipped stage, and exits non-zero when no stage can run. A single wrapper can't span both images, so the updated PR description documents the two-job deployment (one shared working dir, tunathu/tsumugu + tunathu/tunasync-scripts) with a config example.
| if [ -f "${_here}/proxmox-iso.py" ] && command -v python3 >/dev/null 2>&1; then | ||
| python3 "${_here}/proxmox-iso.py" | ||
| fi |
There was a problem hiding this comment.
The conditional call is gone in b83cb08: proxmox-deb-img.sh no longer invokes the ISO stage at all — it is a separate job with its own tunasync status, so a broken image fails that job instead of silently publishing without ISOs. The compat proxmox.sh wrapper likewise exits non-zero when no stage can run and warns on stderr for each skipped stage.
| WORKDIR="${TUNASYNC_WORKING_DIR:-/data/mirrors/proxmox}" | ||
| UPSTREAM="${TUNASYNC_UPSTREAM_URL:-http://download.proxmox.com/}" |
There was a problem hiding this comment.
Fixed in b83cb08, though slightly differently: both scripts now require TUNASYNC_WORKING_DIR with a clear error instead of keeping two fallbacks that could diverge (tunasync's command provider always sets it — worker/cmd_provider.go), so the shell-default/Python-KeyError mismatch is gone. The Python side now exits with a readable message instead of a traceback.
| final_netloc = urllib.parse.urlparse(resp.geturl()).netloc | ||
| if final_netloc != allowed_netloc: | ||
| resp.close() | ||
| raise ValueError(f'redirect to unexpected host: {final_netloc}') |
There was a problem hiding this comment.
Fixed in b83cb08: a redirect target must keep an http(s) scheme, the allowed netloc, AND a path under base_path, otherwise the response is refused before any bytes are consumed. An integration test confirms a redirect from /iso/ to /outside/ is rejected and the file is not saved.
|
Thanks for the thorough review — all three points are addressed in b83cb08 and the rewritten PR description: 1. Description vs. diff. The description now matches the diff: the old 2. Deleted entry point. 3. Recursive /iso/. Now stated explicitly in the script header and the description: only the top-level Scope note from the inline thread: the |
Delete the legacy apt-sync/lftp based proxmox.sh and split the mirror into two stages with different runtime requirements: - proxmox-deb-img.sh (new): syncs /debian/ and /images/ with tsumugu. The default scope matches the legacy script — repos pve, pbs, pbs-client, pmg; every suite upstream publishes for them (currently bullseye, bookworm, trixie); amd64 only; plus top-level key files. PROXMOX_DEB_ALL=1 widens it to the complete /debian/ tree. The debian/dists symlink is created before syncing so tsumugu keeps it in its keep-set and never downloads the duplicate tree. Size accounting follows the repo convention (+<bytes> in REPO_SIZE_FILE, summed by helpers/size-sum.sh, which already exists in-tree). - proxmox-iso.py (new): scrapes the custom top-level /iso/ HTML page, accepts only same-host /iso/ links with conservative filenames, and validates redirect targets against the same allowlist. Files are compared by size and Last-Modified, and freshly downloaded .iso files are verified against the upstream .sha256 files. Per-round cleanup removes stale .tmp files and vanished artifacts. - proxmox.sh (kept): backward-compatible wrapper for legacy single-job setups. It runs every stage the current image supports (tsumugu -> proxmox-deb-img.sh, python3 -> proxmox-iso.py), warns loudly about skipped stages, and exits non-zero when no stage can run. Deployment: two tunasync jobs sharing one working directory, one per image, each invoking its stage script directly (config example in the PR description). Test evidence: tsumugu --dry-run over the default scope matches the legacy mirror (~48.5k objects, ~398 GiB, debian/dists kept as symlink); stub-based integration tests for proxmox-iso.py cover 18 cases (fresh download, size/mtime skip, changed size, changed mtime, sha256 mismatch abort, redirect allowlist, .tmp cleanup, ...); the wrapper was exercised in 5 cases (both stages, deb-only, iso-only, neither, failure propagation).
b83cb08 to
6684841
Compare
|
This branch has been squashed into a single commit for merge-readiness: Review items → resolution (all verified against the final code at @happyaron's review body:
@happyaron's inline comments:
Copilot (final round):
Test evidence on the final code: Some earlier in-thread replies described intermediate states; the current diff and this summary are authoritative. |
Summary
Delete the legacy apt-sync + lftp based
proxmox.shimplementation and split the mirror into two stages with different runtime requirements:proxmox-deb-img.sh(new) — mirrors/debian/and/images/with tsumugu (runs in thetunathu/tsumuguimage).proxmox-iso.py(new) — stdlib-only Python scraper for the custom/iso/HTML page (runs in thetunathu/tunasync-scriptsimage).proxmox.sh(kept) — backward-compatible wrapper: it runs every stage the current image provides (tsumugu → deb/images, python3 → iso), prints a loud stderr warning for each skipped stage, and exits non-zero when no stage can run.Why
/iso/no longer serves a machine-readable autoindex — it is a custom HTML download page, solftp mirrorcannot enumerate it. tsumugu's nginx/apache parsers do not parse it either, and its fallback parser reports ISO sizes as 0, so/iso/gets a small purpose-built scraper instead.tunathu/tsumuguhas no python3,tunathu/tunasync-scriptshas no tsumugu).Layout
Workdir layout is unchanged:
debian/,images/,iso/sit underTUNASYNC_WORKING_DIR. (pmg,pve,develare repositories underdebian/, not siblings ofiso/.)Deployment: two jobs, one shared workdir
Both jobs must share the same
TUNASYNC_WORKING_DIR(the mirror root; do not pointmirror_dirat theiso/subdirectory). Legacy single-job configs invokingproxmox.shkeep working via the wrapper: it runs every stage the image supports, warns on stderr for each skipped stage, and exits non-zero when no stage can run./debian/ scope
The default scope matches the legacy apt-sync script: repos
pve,pbs,pbs-client,pmg(all suites published for them — currently bullseye, bookworm, trixie, the same set as apt-sync.py's@debian-current), amd64 only, plus the top-level files (key.asc,*.gpg).PROXMOX_DEB_ALL=1opts into the full/debian/tree (adds ceph-*, corosync-3, pdm, devel, all architectures). Note that tsumugu treats excluded remote paths as absent upstream, so narrowing the scope on an existing full mirror deletes the extra local trees (bounded by--max-delete, which aborts the run when exceeded) — dry-run first viaTUNASYNC_TSUMUGU_OPTIONS=--dry-run.debian/dists -> pve/distsis created as a symlink before syncing (as the legacy script did): upstream/debian/dists/is a real directory duplicating the pve.debs, and creating the symlink first lets tsumugu record it in its keep-set so fresh installs never download a second copy./iso/ stage
/iso/is flat today; subdirectory links are skipped and the scraper would need extending if upstream ever adds them (the old lftp script mirrored recursively)..asc, re-spun ISO) are re-fetched. HEAD-rejecting servers are handled with a 1-byte ranged GET fallback..tmp.<name>then rename atomically; leftover.tmp.*is removed at startup; the local mtime is set to the upstream Last-Modified..sha256(fetched first); a mismatch removes both sides of the pair and fails the run.TUNASYNC_PROXMOX_ISO_MAXDELETE(default 100) and only run after a fully successful pass./iso/. The allowlist is derived fromTUNASYNC_UPSTREAM_URL, so pointing the job at another mirror/proxy host works.Size reporting
New for this mirror (the legacy script reported none): the deb-img stage sums the trees into
REPO_SIZE_FILEand prints thesize-sum:line viahelpers/size-sum.sh— that helper already exists in this repo and is shared by other scripts (bazel-apt.sh, chef.sh, cvmfs.sh, …); production images that mount only this single script fall back to a directnumfmtprint.Testing
tsumugu sync --dry-runagainst live upstream with the default scope: ~48.5k objects, ~398 GiB apparent;debian/distskept as a symlink, never descended into.proxmox-iso.py: 18 cases — fresh download, skip on matching size+mtime, changed size, changed mtime, sha256 mismatch abort, redirect allowlist (cross-host, non-http scheme, path escape), leftover.tmpcleanup, stale-delete cap, HEAD-rejecting server, path-prefixed upstream, …