[anaconda][rhel7-branch][PATCH] Retry package repository metadata downloads (#1177366)

Vratislav Podzimek vpodzime at redhat.com
Thu Jun 11 11:43:14 UTC 2015


On Thu, 2015-06-11 at 13:11 +0200, Martin Kolman wrote:
> Anaconda already retries package downloads and package set population,
> but until now it did not retry package repository metadata downloads.
> 
> So retry metadata downloads up to 10 times per repository with a
> progressively longer delay between retries. This should make
> network installations on unreliable networks more likely to succeed.
> 
> Resolves: rhbz#1177366
> Signed-off-by: Martin Kolman <mkolman at redhat.com>
> ---
>  pyanaconda/packaging/yumpayload.py | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
> index 6574f5e..127ca4e 100644
> --- a/pyanaconda/packaging/yumpayload.py
> +++ b/pyanaconda/packaging/yumpayload.py
> @@ -68,7 +68,7 @@ from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, IS
>  from pyanaconda.flags import flags
>  
>  from pyanaconda import iutil
> -from pyanaconda.iutil import ProxyString, ProxyStringError
> +from pyanaconda.iutil import ProxyString, ProxyStringError, xprogressive_delay
>  from pyanaconda.i18n import _
>  from pyanaconda.nm import nm_is_connected
>  from pyanaconda.product import isFinal
> @@ -90,6 +90,8 @@ YUM_PLUGINS = ["fastestmirror", "langpacks"]
>  YUM_REPOS_DIR = "/etc/yum.repos.d/"
>  BASE_REPO_NAMES = [BASE_REPO_NAME] + PackagePayload.DEFAULT_REPOS
>  
> +MAX_METADATA_DOWNLOAD_RETRIES = 10
> +
>  import inspect
>  import threading
>  _private_yum_lock = threading.RLock()
> @@ -629,13 +631,31 @@ reposdir=%s
>              with _yum_lock:
>                  repo = self._yum.repos.getRepo(repo_id)
>                  if repo.enabled:
> -                    try:
> -                        log.info("gathering repo metadata for %s", repo_id)
> -                        self._getRepoMetadata(repo)
> -                    except PayloadError as e:
> -                        log.error("failed to grab repo metadata for %s: %s",
> -                                  repo_id, e)
> -                        self.disableRepo(repo_id)
> +                    # retry metadata downloads with a progressively longer pause,
> +                    # so that unattended installations on unreliable networks have
> +                    # a higher chance of finishing successfully
> +                    xdelay = xprogressive_delay()
> +                    for retry_count in xrange(0, MAX_METADATA_DOWNLOAD_RETRIES + 1):
> +                        # retry count == 0 -> first attempt
> +                        # retry count > 0  -> retry
> +                        if retry_count:
If you use 'if retry_count > 0' here, you can throw away the comment
above it while keeping things clear, I think. If you feel like it, you
can add the '# first attempt' comment to the inner block of the 'if'.

> +                            time.sleep(next(xdelay))  # wait a bit before retry
> +                        try:
> +                            log.info("gathering repo metadata for %s", repo_id)
> +                            self._getRepoMetadata(repo)
> +                            log.info("gathered repo metadata for %s", repo_id)
> +                            break
> +                        except PayloadError as e:
> +                            log.error("failed to grab repo metadata for %s: %s", repo_id, e)
> +                            if retry_count < MAX_METADATA_DOWNLOAD_RETRIES:
> +                                # retry
> +                                log.error("retrying metadata download for repo %s, retrying (%d/%d)",
> +                                          repo_id, retry_count + 1, MAX_METADATA_DOWNLOAD_RETRIES)
This doesn't seem to me like an error message. log.info() would make
more sense to me here.

> +                            else:
> +                                # run out of retries
> +                                log.error("metadata download for repo %s failed after %d retries",
> +                                          repo_id, retry_count)
> +                                self.disableRepo(repo_id)
>                  else:
>                      log.info("skipping disabled repo %s", repo_id)
Other than the minor things above this looks good to me.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list