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

Martin Kolman mkolman at redhat.com
Mon Jun 15 11:56:36 UTC 2015


On Fri, 2015-06-12 at 10:01 +0200, Vratislav Podzimek wrote:
> 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.
Yes - this is targeted to the rhel7-branch, the last standing bastion
on yumpayload. :) As for xprogressive_delay - I would rather like to
keep it the way it is now on the rhel7-branch, as it is already used in
two other places in the yum transaction script and it seems to be
working fine so far.

But I'm definitely open to making it more flexible on the Master
branch.

>
> 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.
>
I've checked how the dnfpayload handles retries and as far as I can
tell:
* repository metadata downloads are handled in dnfpayload and no
attempt is made to retry on error
* batch package download is handled in dnf and exactly one retry
attempt is made (it tries to download all the packages that failed to
download on the first try) and no delay is introduced between attempts

So I'll probably send a patch similar to this one (retry metadata
downloads) for dnfpayload and a patch to dnf to add multiple-retry &
retry delay support for batch package downloads.

BTW, dnf/librepo also seems to be able to handle delta rpm downloads,
which could also be potentially useful for us (reducing the amount of
downloaded data when installing from a medium with a repo & updates
repo enabled). 


More information about the anaconda-patches mailing list