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

Anne Mulhern amulhern at redhat.com
Thu Jun 11 13:54:28 UTC 2015


I think you should consider making xprogressive_delay a little more useful.




----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Thursday, June 11, 2015 7:43:14 AM
> Subject: Re: [anaconda][rhel7-branch][PATCH] Retry package repository	metadata downloads (#1177366)
> 
> 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'.

Right now, xprogressive_delay/xdelay isn't doing much that is useful. You could just
as well lose it entirely, and change the below line to time.sleep(0.25 * (2 ** retry_count)).
That way, you don't have two generators doing essentially the same computation and just
marching along in step.

Alternatively, and probably better, you could make xprogressive_delay a bit more useful,
something like:

def xprogressive_delay(factor, max_iterations):
    """ Generator that yields a sequence of delays, each an order of magnitude more than previous.

        :param factor: the factor by which to multiply the delay
        :param int max_iterations: maximum number of iterations
        :rtype: same as the type of factor

        The base sequence is just zero followed by the powers of 2 starting from 2.
    """
    yield 0
    counter = 1
    while counter < max_iterations:
        yield factor * (2 ** counter)
        counter += 1

and then use it like:

for delay in xprogressive_delay(0.25, MAX_METADATA_DOWNLOAD_RETRIES):
    time.sleep(delay)
    # and so forth

else:
    # run out of retries
    # and so forth
    
Also, xrange is not defined in Python 3.

- mulhern

> 
> > +                            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
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 


More information about the anaconda-patches mailing list