From 9d5151ba25394b35ff7fc4b06b858c53e638ec5d Mon Sep 17 00:00:00 2001 From: Jonathon Anderson <17242663+blue42u@users.noreply.github.com> Date: Tue, 25 Oct 2022 13:36:12 -0500 Subject: [PATCH] BinaryCacheIndex: track update failures with cooldown (#33509) #32137 added an option to update() a BinaryCacheIndex with a cooldown: repeated attempts within this cooldown would not actually retry. However, the cooldown was not properly tracked for failures (which is common when the mirror does not store any binaries and therefore has no index.json). This commit ensures that update(..., with_cooldown=True) will also skip the update even if a failure has occurred within the cooldown period. --- lib/spack/spack/binary_distribution.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index 3f74d91a9e..d8ac0bb1fa 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/lib/spack/spack/binary_distribution.py @@ -107,7 +107,8 @@ def __init__(self, cache_root): # cache (_mirrors_for_spec) self._specs_already_associated = set() - # mapping from mirror urls to the time.time() of the last index fetch. + # mapping from mirror urls to the time.time() of the last index fetch and a bool indicating + # whether the fetch succeeded or not. self._last_fetch_times = {} # _mirrors_for_spec is a dictionary mapping DAG hashes to lists of @@ -346,21 +347,25 @@ def update(self, with_cooldown=False): with_cooldown and ttl > 0 and cached_mirror_url in self._last_fetch_times - and now - self._last_fetch_times[cached_mirror_url] < ttl + and now - self._last_fetch_times[cached_mirror_url][0] < ttl ): - # The fetch worked last time, so don't error - all_methods_failed = False + # We're in the cooldown period, don't try to fetch again + # If the fetch succeeded last time, consider this update a success, otherwise + # re-report the error here + if self._last_fetch_times[cached_mirror_url][1]: + all_methods_failed = False else: # May need to fetch the index and update the local caches try: needs_regen = self._fetch_and_cache_index( cached_mirror_url, expect_hash=cached_index_hash ) - self._last_fetch_times[cached_mirror_url] = now + self._last_fetch_times[cached_mirror_url] = (now, True) all_methods_failed = False except FetchCacheError as fetch_error: needs_regen = False fetch_errors.extend(fetch_error.errors) + self._last_fetch_times[cached_mirror_url] = (now, False) # The need to regenerate implies a need to clear as well. spec_cache_clear_needed |= needs_regen spec_cache_regenerate_needed |= needs_regen @@ -392,11 +397,12 @@ def update(self, with_cooldown=False): # Need to fetch the index and update the local caches try: needs_regen = self._fetch_and_cache_index(mirror_url) - self._last_fetch_times[mirror_url] = now + self._last_fetch_times[mirror_url] = (now, True) all_methods_failed = False except FetchCacheError as fetch_error: fetch_errors.extend(fetch_error.errors) needs_regen = False + self._last_fetch_times[mirror_url] = (now, False) # Generally speaking, a new mirror wouldn't imply the need to # clear the spec cache, so leave it as is. if needs_regen: