[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