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

Vratislav Podzimek vpodzime at redhat.com
Fri Jun 12 08:01:08 UTC 2015


On Thu, 2015-06-11 at 09:54 -0400, Anne Mulhern wrote:
> 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.
Neither is the yumpayload. ;) This is the RHEL 7 version of the patch, I
think.

But I agree xprogressive_delay could be made more useful like above at
least on the master branch. And probably renamed to progressive_delay as
the 'x' prefix is often not used in Python 3 where iterators/generators
are defaults.

-- 
Vratislav Podzimek

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



More information about the anaconda-patches mailing list