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

Anne Mulhern amulhern at redhat.com
Mon Jun 15 12:17:46 UTC 2015





----- Original Message -----
> From: "Martin Kolman" <mkolman at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Monday, June 15, 2015 7:56:36 AM
> Subject: Re: [anaconda][rhel7-branch][PATCH] Retry package repository	metadata downloads (#1177366)
> 
> 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.
> >

It looks like xprogressive_delay is actually not used on master branch
at all (because yum-stuff went away). Last use gone in commit
8ff443d43e79862e33411882de9cdb870632ce3a. Of course, there's
always, as you point out, dnf...

It still looks to me like using xprogressive-delay in this code
just increases the complexity and like for-else could be your
friend and save you a level of indentation or two, regardless
of whether you try to rewrite xprogressive-delay or not on this
branch.

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

- mulhern


More information about the anaconda-patches mailing list