named caches: move instead of symlinking

client/named_cache.py

Index: client/named_cache.py
diff --git a/client/named_cache.py b/client/named_cache.py
index e3bf5e81c865e62c1235b0d4cb1f13917159c237..93fece2fe494f68334c0b2c7dbcfbc104242b037 100644
--- a/client/named_cache.py
+++ b/client/named_cache.py
@@ -53,9 +53,9 @@ class CacheManager(object):
 
   @contextlib.contextmanager
   def open(self, time_fn=None):
-    """Opens NamedCaches for mutation operations, such as request or trim.
+    """Opens NamedCaches for mutation operations, such as install.
 
-    Only on caller can open the cache manager at a time. If the same thread
+    Only one caller can open the cache manager at a time. If the same thread
     calls this function after opening it earlier, the call will deadlock.
 
     time_fn is a function that returns timestamp (float) and used to take
@@ -86,48 +86,14 @@ class CacheManager(object):
   def __len__(self):
     """Returns number of items in the cache.
 
-    Requires NamedCache to be open.
+    NamedCache must be open.
     """
     return len(self._lru)
 
-  def request(self, name):
-    """Returns an absolute path to the directory of the named cache.
-
-    Creates a cache directory if it does not exist yet.
-
-    Requires NamedCache to be open.
-    """
-    self._lock.assert_locked()
-    assert isinstance(name, basestring), name
-    path = self._lru.get(name)
-    create_named_link = False
-    if path is None:
-      path = self._allocate_dir()
-      create_named_link = True
-      logging.info('Created %r for %r', path, name)
-    abs_path = os.path.join(self.root_dir, path)
-
-    # TODO(maruel): That's weird, it should exist already.
-    file_path.ensure_tree(abs_path)
-    self._lru.add(name, path)
-
-    if create_named_link:
-      # Create symlink <root_dir>/<named>/<name> -> <root_dir>/<short name>
-      # for user convenience.
-      named_path = self._get_named_path(name)
-      if os.path.exists(named_path):
-        file_path.remove(named_path)
-      else:
-        file_path.ensure_tree(os.path.dirname(named_path))
-      logging.info('Symlink %r to %r', named_path, abs_path)
-      fs.symlink(abs_path, named_path)
-
-    return abs_path
-
   def get_oldest(self):
     """Returns name of the LRU cache or None.
 
-    Requires NamedCache to be open.
+    NamedCache must be open..

[M-A Ruel, 2017/05/09 18:35:14]

one dot

[nodir, 2017/05/09 23:20:09]

Done.

     """
     self._lock.assert_locked()
     try:
@@ -138,7 +104,7 @@ class CacheManager(object):
   def get_timestamp(self, name):
     """Returns timestamp of last use of an item.
 
-    Requires NamedCache to be open.
+    NamedCache must be open.
 
     Raises KeyError if cache is not found.
     """
@@ -146,46 +112,101 @@ class CacheManager(object):
     assert isinstance(name, basestring), name
     return self._lru.get_timestamp(name)
 
-  @contextlib.contextmanager
-  def create_symlinks(self, root, named_caches):
-    """Creates symlinks in |root| for the specified named_caches.
+  @property
+  def available(self):
+    """Returns a set of names of available caches.
+
+    NamedCache must be open.
+    """
+    self._lock.assert_locked()
+    return self._lru.keys_set()
+
+  def install(self, root, named_caches):

[M-A Ruel, 2017/05/09 18:35:14]

I'd prefer to have this function accept one named_cache, the loop should be
outside.

[nodir, 2017/05/09 23:20:09]

do'h! Yeah, this is much simpler

+    """Moves directories of the specified named caches to |root|.
 
     named_caches must be a list of (name, path) tuples.
 
-    Requires NamedCache to be open.
+    NamedCache must be open..

[M-A Ruel, 2017/05/09 18:35:14]

same

[nodir, 2017/05/09 23:20:09]

Done.

 
     Raises Error if cannot create a symlink.
     """
     self._lock.assert_locked()
-    for name, path in named_caches:
-      logging.info('Named cache %r -> %r', name, path)
+    for name, rel_install in named_caches:
+      logging.info('Installing named cache %r to %r', name, rel_install)
       try:
-        _validate_named_cache_path(path)
-        symlink_path = os.path.abspath(os.path.join(root, path))
-        file_path.ensure_tree(os.path.dirname(symlink_path))
-        requested = self.request(name)
-        logging.info('Symlink %r to %r', symlink_path, requested)
-        fs.symlink(requested, symlink_path)
+        _validate_named_cache_path(rel_install)
+        rel_install = unicode(rel_install)
+        abs_install = os.path.abspath(os.path.join(root, rel_install))
+
+        rel_cache = self._lru.get(name)
+        if rel_cache:
+          abs_cache = os.path.join(self.root_dir, rel_cache)
+          if os.path.isdir(abs_cache):
+            logging.info('Moving %r to %r', abs_cache, abs_install)
+            file_path.ensure_tree(os.path.dirname(abs_install))
+            fs.rename(abs_cache, abs_install)
+            self._remove(name)
+            continue
+
+          logging.warning('directory for named cache %r does not exist', name)
+          self._remove(name)
+
+        # The named cache does not exist.
+        # Just create an empty directory in the root.
+        # When uninstalling, we will move it back to the cache and create an
+        # an entry.
+        file_path.ensure_tree(abs_install)
       except (OSError, Error) as ex:
         raise Error(
-            'cannot create a symlink for cache named "%s" at "%s": %s' % (
-              name, symlink_path, ex))
+            'cannot install cache named %r at %r: %s' % (
+              name, abs_install, ex))
 
-  def delete_symlinks(self, root, named_caches):
-    """Deletes symlinks from |root| for the specified named_caches.
+  def uninstall(self, root, named_caches):

[M-A Ruel, 2017/05/09 18:35:14]

same about one call per named cache. Makes the function more readable.

[nodir, 2017/05/09 23:20:09]

Done.

+    """Moves directories of the specified named caches back from |root|.
 
     named_caches must be a list of (name, path) tuples.
     """
-    for name, path in named_caches:
-      logging.info('Unlinking named cache "%s"', name)
+    for name, rel_install in named_caches:
+      logging.info('Uninstalling named cache %r from %r', name, rel_install)
       try:
-        _validate_named_cache_path(path)
-        symlink_path = os.path.abspath(os.path.join(root, path))
-        fs.unlink(symlink_path)
+        _validate_named_cache_path(rel_install)
+        rel_install = unicode(rel_install)

[M-A Ruel, 2017/05/09 18:35:14]

don't, make sure caller is fixed.

[nodir, 2017/05/09 23:20:09]

Done.

+        abs_install = os.path.abspath(os.path.join(root, rel_install))
+        if not os.path.isdir(abs_install):
+          logging.warning(
+              'Directory %r does not exist anymore. Cache lost.', abs_install)
+          continue
+
+        rel_cache = self._lru.get(name)
+        if rel_cache:
+          # Do not crash because cache already exists.
+          logging.warning('overwriting an existing named cache %r', name)
+          create_named_link = False
+        else:
+          rel_cache = self._allocate_dir()
+          create_named_link = True
+
+        # Move the dir and create an entry for the named cache.
+        abs_cache = os.path.join(self.root_dir, rel_cache)
+        logging.info('Moving %r to %r', abs_install, abs_cache)
+        file_path.ensure_tree(os.path.dirname(abs_cache))
+        fs.rename(abs_install, abs_cache)
+        self._lru.add(name, rel_cache)
+
+        if create_named_link:
+          # Create symlink <root_dir>/<named>/<name> -> <root_dir>/<short name>
+          # for user convenience.
+          named_path = self._get_named_path(name)
+          if os.path.exists(named_path):
+            file_path.remove(named_path)
+          else:
+            file_path.ensure_tree(os.path.dirname(named_path))
+          fs.symlink(abs_cache, named_path)
+          logging.info('Created symlink %r for %r', named_path, name)
       except (OSError, Error) as ex:
         raise Error(
-            'cannot unlink cache named "%s" at "%s": %s' % (
-              name, symlink_path, ex))
+            'cannot uninstall cache named %r at %r: %s' % (
+              name, abs_install, ex))
 
   def trim(self, min_free_space):
     """Purges cache.
@@ -195,7 +216,7 @@ class CacheManager(object):
 
     If min_free_space is None, disk free space is not checked.
 
-    Requires NamedCache to be open.
+    NamedCache must be open..

[M-A Ruel, 2017/05/09 18:35:14]

same

 
     Returns:
       Number of caches deleted.
@@ -211,22 +232,16 @@ class CacheManager(object):
     while ((min_free_space and free_space < min_free_space)
            or len(self._lru) > MAX_CACHE_SIZE):
       logging.info(
-          'Making space for named cache %s > %s or %s > %s',
+          'Making space for named cache %d > %d or %d > %d',
           free_space, min_free_space, len(self._lru), MAX_CACHE_SIZE)
       try:
         name, (path, _) = self._lru.get_oldest()
       except KeyError:
         return total
-      named_dir = self._get_named_path(name)
-      if fs.islink(named_dir):
-        fs.unlink(named_dir)
-      path_abs = os.path.join(self.root_dir, path)
-      if os.path.isdir(path_abs):
-        logging.info('Removing named cache %s', path_abs)
-        file_path.rmtree(path_abs)
+      logging.info('Removing named cache %r', name)
+      self._remove(name)
       if min_free_space:
         free_space = file_path.get_free_space(self.root_dir)
-      self._lru.pop(name)
       total += 1
     return total
 
@@ -251,6 +266,28 @@ class CacheManager(object):
       tried.add(rel_path)
     raise Error('could not allocate a new cache dir, too many cache dirs')
 
+  def _remove(self, name):
+    """Removes a cache directory and entry.
+
+    NamedCache must be open.
+
+    Returns:
+      Number of caches deleted.
+    """
+    self._lock.assert_locked()
+    rel_path = self._lru.get(name)
+    if not rel_path:
+      return
+
+    named_dir = self._get_named_path(name)
+    if fs.islink(named_dir):
+      fs.unlink(named_dir)
+
+    abs_path = os.path.join(self.root_dir, rel_path)
+    if os.path.isdir(abs_path):
+      file_path.rmtree(abs_path)
+    self._lru.pop(name)
+
   def _get_named_path(self, name):
     return os.path.join(self.root_dir, 'named', name)
 
@@ -281,7 +318,7 @@ def process_named_cache_options(parser, options):
   for name, path in options.named_caches:
     if not CACHE_NAME_RE.match(name):
       parser.error(
-          'cache name "%s" does not match %s' % (name, CACHE_NAME_RE.pattern))
+          'cache name %r does not match %r' % (name, CACHE_NAME_RE.pattern))
     if not path:
       parser.error('cache path cannot be empty')
   if options.named_cache_root:
@@ -290,6 +327,8 @@ def process_named_cache_options(parser, options):
 
 
 def _validate_named_cache_path(path):
+  if not isinstance(path, basestring):
+    raise Error('named cache path must be a string')
   if os.path.isabs(path):
     raise Error('named cache path must not be absolute')
   if '..' in path.split(os.path.sep):