[PATCH 1/3] Use cache for base repo if possible

Vratislav Podzimek vpodzime at redhat.com
Tue Oct 22 07:31:58 UTC 2013


On Mon, 2013-10-21 at 09:44 -0400, Samantha N. Bueno wrote:
> On Wed, Oct 16, 2013 at 03:26:21PM +0200, Vratislav Podzimek wrote:
> > The baseRepo property is tricky, because it needs yum_lock to be evaluated.
> > However, in many cases, nothing changes in the configuration and we can simply
> > return the same value as in the previous call. If there is some change that can
> > potentially change the value, the cache needs to be made invalid and refreshed.
> > 
> > This speeds things up and makes the getBaseRepo method cheap enough to be used
> > in additional places that need it.
> > 
> > Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> > ---
> >  pyanaconda/constants.py              |  1 +
> >  pyanaconda/packaging/__init__.py     | 21 +++++++--
> >  pyanaconda/packaging/dnfpayload.py   |  4 +-
> >  pyanaconda/packaging/yumpayload.py   | 87 +++++++++++++++++++++++++++++++++---
> >  pyanaconda/ui/gui/spokes/network.py  |  4 +-
> >  pyanaconda/ui/gui/spokes/software.py |  2 +-
> >  pyanaconda/ui/gui/spokes/source.py   | 12 ++---
> >  pyanaconda/ui/gui/spokes/storage.py  |  2 +-
> >  pyanaconda/ui/tui/spokes/software.py |  2 +-
> >  pyanaconda/ui/tui/spokes/source.py   |  8 ++--
> >  10 files changed, 116 insertions(+), 27 deletions(-)
> > 
> > diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py
> > index 7af5014..115ef28 100644
> > --- a/pyanaconda/constants.py
> > +++ b/pyanaconda/constants.py
> > @@ -108,6 +108,7 @@ THREAD_GEOLOCATION_REFRESH = "AnaGeolocationRefreshThread"
> >  THREAD_DATE_TIME = "AnaDateTimeThread"
> >  THREAD_TIME_INIT = "AnaTimeInitThread"
> >  THREAD_XKL_WRAPPER_INIT = "AnaXklWrapperInitThread"
> > +THREAD_REFRESH_BASE_REPO = "AnaBaseRepoRefreshThread"
> >  
> >  # Geolocation constants
> >  
> > diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
> > index 74adf7f..a5bcb01 100644
> > --- a/pyanaconda/packaging/__init__.py
> > +++ b/pyanaconda/packaging/__init__.py
> > @@ -64,6 +64,8 @@ from pyanaconda.product import productName, productVersion
> >  import urlgrabber
> >  urlgrabber.grabber.default_grabber.opts.user_agent = "%s (anaconda)/%s" %(productName, productVersion)
> >  
> > +REPO_NOT_SET = False
> > +
> >  ###
> >  ### ERROR HANDLING
> >  ###
> > @@ -145,9 +147,22 @@ class Payload(object):
> >          """ A list of addon repo identifiers. """
> >          return [r.name for r in self.data.repo.dataList()]
> >  
> > -    @property
> > -    def baseRepo(self):
> > -        """ The identifier of the current base repo. """
> > +    def getBaseRepo(self, wait=True, callback=None):
> > +        """
> > +        Get the identifier of the current base repo.
> > +
> > +        :param wait: whether to block until the identifier is derived from the
> > +                     configuration (may take a long time) or just try to use
> > +                     the cached value and return REPO_NOT_SET if there is none
> > +        :type wait: bool
> > +        :param callback: callback that will be called once the indentifier is
> > +                         derived from the configuration (gets the repo ID as
> > +                         the first argument) if it is not returned instantly
> > +        :type callback: str -> None
> > +        :returns: id of the current base repo or None if wait=True is used or
> > +                  REPO_NOT_SET if wait=False is used and the value is not cached
> > +
> > +        """
> >          return None
> >  
> >      @property
> > diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py
> > index d0bda57..3e5e01b 100644
> > --- a/pyanaconda/packaging/dnfpayload.py
> > +++ b/pyanaconda/packaging/dnfpayload.py
> > @@ -217,8 +217,8 @@ class DNFPayload(packaging.PackagePayload):
> >          # addon repos via kickstart
> >          return [r.name for r in self.data.repo.dataList()]
> >  
> > -    @property
> > -    def baseRepo(self):
> > +    def getBaseRepo(self, wait=True, callback=None):
> > +        # is any locking needed here as in the yumpayload?
> >          repo_names = [constants.BASE_REPO_NAME] + DEFAULT_REPOS
> >          for repo in self._base.repos.iter_enabled():
> >              if repo.id in repo_names:
> > diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
> > index 0eb5cf1..4225649 100644
> > --- a/pyanaconda/packaging/yumpayload.py
> > +++ b/pyanaconda/packaging/yumpayload.py
> > @@ -61,7 +61,7 @@ except ImportError:
> >      log.error("import of yum failed")
> >      yum = None
> >  
> > -from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, ISO_DIR, MOUNT_DIR, ROOT_PATH
> > +from pyanaconda.constants import BASE_REPO_NAME, DRACUT_ISODIR, INSTALL_TREE, ISO_DIR, MOUNT_DIR, ROOT_PATH, THREAD_REFRESH_BASE_REPO
> >  from pyanaconda.flags import flags
> >  
> >  from pyanaconda import iutil
> > @@ -77,7 +77,9 @@ from pyanaconda.errors import ERROR_RAISE, errorHandler
> >  from pyanaconda.packaging import DependencyError, MetadataError, NoNetworkError, NoSuchGroup, \
> >                                   NoSuchPackage, PackagePayload, PayloadError, PayloadInstallError, \
> >                                   PayloadSetupError
> > +from pyanaconda.packaging import REPO_NOT_SET
> >  from pyanaconda.progress import progressQ
> > +from pyanaconda.threads import threadMgr, AnacondaThread
> >  
> >  from pyanaconda.localization import langcode_matches_locale
> >  
> > @@ -135,6 +137,13 @@ class YumPayload(PackagePayload):
> >          self._requiredPackages = []
> >          self._requiredGroups = []
> >  
> > +        # base repo caching
> > +        self._cached_base_repo = None
> > +        self._br_cache_valid = False
> > +        self._br_callbacks = []
> > +        self._br_cache_valid_lock = threading.Lock()
> > +        self._br_refresh_lock = threading.Lock()
> 
> Two variables assigned the same value, seems like it should just be one.
The answer here applies to both your questions/comments. The reason for
two locks and also the nested with blocks here is granularity -- if we
had only one lock acquired for the whole refresh process, for checking
cache validity and taking the value, we would end up with just another
name for the _yum_lock.

This way the two locks just 

a) make sure nobody can check cache validity and then get invalidated
value (the test-and-get and test-and-set operations need to be done
atomically) and

b) make sure no callbacks are enqueued while callbacks are already being
run (that could cause some callback to be left out never called)

and since they are both released ASAP, calls with wait=False should
never take long time.

-- 
Vratislav Podzimek

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



More information about the anaconda-patches mailing list