diff --git a/napalm_proxmox/vm_provision_mixin.py b/napalm_proxmox/vm_provision_mixin.py index 7c9935e..928fcee 100644 --- a/napalm_proxmox/vm_provision_mixin.py +++ b/napalm_proxmox/vm_provision_mixin.py @@ -3,6 +3,7 @@ from __future__ import annotations import base64 +import hashlib import logging import time import yaml @@ -74,38 +75,63 @@ class ProxmoxVMProvisionMixin: """ Download image_url to the node's image cache dir if not already present. - Returns the local path on the hypervisor node. Verifies image_checksum - (format ":", e.g. "sha256:abc123...") if given, re-downloading - is left to the caller's next attempt if verification fails. + Returns the local path on the hypervisor node. The cache filename is + prefixed with a hash of the *full* URL, not just its basename — + Ubuntu (and others) publish per-build URLs that change daily under a + stable basename (e.g. .../release-20260713/ubuntu-26.04-server- + cloudimg-amd64.img), so keying the cache on the basename alone let a + stale previous-day build satisfy the "already cached" check and fail + checksum verification against today's expected hash. + + Verifies image_checksum (format ":", e.g. "sha256:abc123...") + if given. On mismatch, removes the bad file and retries the download + once (covers a corrupted/partial transfer or a stale same-keyed file) + before raising. """ filename = image_url.rstrip("/").rsplit("/", 1)[-1] - local_path = f"{_IMAGE_CACHE_DIR}/{filename}" + url_hash = hashlib.sha256(image_url.encode()).hexdigest()[:12] + local_path = f"{_IMAGE_CACHE_DIR}/{url_hash}-{filename}" - exists = self._run_node_command( - f"mkdir -p {_IMAGE_CACHE_DIR} && test -f {local_path} && echo EXISTS || echo MISSING", - timeout=30, - ) - if "EXISTS" not in exists: - _logger.info(f"Downloading cloud image {image_url} -> {local_path}") - self._run_node_command( - f"wget -q -O {local_path}.tmp '{image_url}' && mv {local_path}.tmp {local_path}", - timeout=timeout, + algo, _, expected = (image_checksum or "").partition(":") + algo = (algo or "sha256").lower() + + max_attempts = 2 + for attempt in range(1, max_attempts + 1): + exists = self._run_node_command( + f"mkdir -p {_IMAGE_CACHE_DIR} && test -f {local_path} " + f"&& echo EXISTS || echo MISSING", + timeout=30, ) + if "EXISTS" not in exists: + _logger.info(f"Downloading cloud image {image_url} -> {local_path}") + self._run_node_command( + f"wget -q -O {local_path}.tmp '{image_url}' " + f"&& mv {local_path}.tmp {local_path}", + timeout=timeout, + ) + + if not image_checksum: + return local_path - if image_checksum: - algo, _, expected = image_checksum.partition(":") - algo = (algo or "sha256").lower() actual = self._run_node_command( f"{algo}sum {local_path} | awk '{{print $1}}'", timeout=60 ) - if actual.lower() != expected.lower(): - # Remove the bad file so a retry re-downloads instead of reusing it. - self._run_node_command(f"rm -f {local_path}", timeout=30) + if actual.lower() == expected.lower(): + return local_path + + # Remove the bad file so the next attempt re-downloads instead of + # reusing it. + self._run_node_command(f"rm -f {local_path}", timeout=30) + if attempt == max_attempts: raise RuntimeError( f"Checksum mismatch for {image_url}: expected {expected}, got {actual}" ) + _logger.warning( + f"Checksum mismatch for {image_url} on attempt {attempt}/{max_attempts} " + "— retrying download" + ) - return local_path + raise AssertionError("unreachable") # loop always returns or raises above def _find_default_image_storage(self) -> str: """Find a storage suitable for VM root disks (content includes 'images'). diff --git a/tests/test_vm_provision_mixin.py b/tests/test_vm_provision_mixin.py index 14eb99f..0e5a487 100644 --- a/tests/test_vm_provision_mixin.py +++ b/tests/test_vm_provision_mixin.py @@ -678,10 +678,73 @@ def test_download_cloud_image_verifies_matching_checksum(): assert path.endswith("debian-12-genericcloud-amd64.qcow2") -def test_download_cloud_image_checksum_mismatch_raises_and_removes_file(): +def test_download_cloud_image_cache_key_differs_for_same_basename_different_url(): + """Regression: Ubuntu's per-build URLs change daily under a stable + basename (ubuntu-26.04-server-cloudimg-amd64.img) — a cache key derived + from just the basename let a stale build from a previous day collide + with today's cache lookup, skip re-downloading, and fail checksum + verification against today's expected hash. Gitea issue filed for this.""" + mixin = ProxmoxVMProvisionMixin() + mixin._run_node_command = MagicMock(return_value="MISSING") + + url_a = ( + "https://cloud-images.ubuntu.com/releases/server/releases/resolute/" + "release-20260713/ubuntu-26.04-server-cloudimg-amd64.img" + ) + url_b = ( + "https://cloud-images.ubuntu.com/releases/server/releases/resolute/" + "release-20260714/ubuntu-26.04-server-cloudimg-amd64.img" + ) + + path_a = mixin._download_cloud_image(url_a, None, timeout=300) + path_b = mixin._download_cloud_image(url_b, None, timeout=300) + + assert path_a != path_b + assert path_a.endswith("ubuntu-26.04-server-cloudimg-amd64.img") + assert path_b.endswith("ubuntu-26.04-server-cloudimg-amd64.img") + + +def test_download_cloud_image_checksum_mismatch_retries_and_succeeds(): + """The reported bug's actual scenario: a stale same-named cache entry + fails checksum, gets removed, and the retry re-downloads + verifies + successfully instead of failing the whole provisioning job outright.""" mixin = ProxmoxVMProvisionMixin() mixin._run_node_command = MagicMock( - side_effect=["EXISTS", "wrong-checksum", ""] # existence, checksum, rm + side_effect=[ + "EXISTS", # attempt 1: stale file already present + "wrong-checksum", # attempt 1: checksum against stale content + "", # rm -f + "MISSING", # attempt 2: file gone, re-download + "", # wget + "abc123", # attempt 2: checksum against fresh content + ] + ) + + path = mixin._download_cloud_image( + "https://cloud-images.ubuntu.com/releases/server/releases/resolute/" + "release-20260713/ubuntu-26.04-server-cloudimg-amd64.img", + "sha256:abc123", + timeout=300, + ) + + assert path.endswith("ubuntu-26.04-server-cloudimg-amd64.img") + commands = [c[0][0] for c in mixin._run_node_command.call_args_list] + assert sum(1 for cmd in commands if "wget" in cmd) == 1 + assert sum(1 for cmd in commands if cmd.startswith("rm -f")) == 1 + + +def test_download_cloud_image_checksum_mismatch_persists_raises_after_retry(): + mixin = ProxmoxVMProvisionMixin() + mixin._run_node_command = MagicMock( + side_effect=[ + "EXISTS", + "wrong-checksum", + "", # rm -f (attempt 1) + "MISSING", + "", # wget + "still-wrong", + "", # rm -f (attempt 2) + ] ) with pytest.raises(RuntimeError, match="Checksum mismatch"): @@ -692,7 +755,7 @@ def test_download_cloud_image_checksum_mismatch_raises_and_removes_file(): ) commands = [c[0][0] for c in mixin._run_node_command.call_args_list] - assert any(cmd.startswith("rm -f") for cmd in commands) + assert sum(1 for cmd in commands if cmd.startswith("rm -f")) == 2 # ---------------------------------------------------------------------------