[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