[Notes] [Git][BuildStream/buildstream][chandan/import-is-not-buildelemen

Chandan Singh pushed to branch chandan/import-is-not-buildelement at BuildStream / buildstream

Commits:

c9ce89d2

by Tristan Van Berkom at 2019-01-18T19:36:26Z

_cas/cascache.py: Cleanup directories when removing refs

With out this, empty directories in the refs/heads directory just
grow unconditionally.

c536ab6a

by Tristan Van Berkom at 2019-01-18T19:36:26Z

tests/artifactcache/expiry.py: Test refs directory cleanup

Enhance the test which checks removal of the extract directories
with an additional check that the ref directories are cleaned up
when removing artifacts.

99699ffc

by Tristan Van Berkom at 2019-01-18T19:36:26Z

utils.py: Added _tempnamedfile()

When used in a child process in BuildStream, this should be
used instead of tempfile.NamedTemporaryFile() directly, otherwise
we fail to cleanup the file on SIGTERM.

8ce483d4

by Tristan Van Berkom at 2019-01-18T19:36:26Z

_cas/cascache.py: Use utils._tempdir() and utils._tempnamedfile()

The direct usage of tempfile.TemporaryDirectory() and
tempfile.NamedTemporaryFile() here causes leakage of any temporary
data when the process operating on temporary data is terminated
with SIGTERM.

Using the utilities ensures that trash is not left behind in
~/.cache/buildstream/artifacts/tmp when the user terminates
BuildStream with ^C.

73c7252d

by Tristan Van Berkom at 2019-01-18T20:57:42Z

Merge branch 'tristan/cas-cleanup-improve' into 'master'

CASCache cleanup improvements

See merge request BuildStream/buildstream!1087

173c30ed

by Chandan Singh at 2019-01-18T21:25:36Z

Derive import plugin from Element instead of BuildElement

The `import` element is not really a build element. The main purpose of
the `BuildElement` class is to allow users to run `build-commands`,
`install-commands` etc. But, `import` does not run such commands.
Moreover, we already override all the methods provided by
`BuildElement`. So it only makes it confusing to have it derived from
`BuildElement` class when it is not a build element.

So, derive it from the base `Element` class instead.

Changes:

buildstream/_cas/cascache.py

@@ -21,7 +21,7 @@ import hashlib
  import itertools
  import os
  import stat
 -import tempfile
 +import errno
  import uuid
  import contextlib
@@ -129,7 +129,7 @@ class CASCache():
              else:
                  return dest
 -        with tempfile.TemporaryDirectory(prefix='tmp', dir=self.tmpdir) as tmpdir:
 +        with utils._tempdir(prefix='tmp', dir=self.tmpdir) as tmpdir:
              checkoutdir = os.path.join(tmpdir, ref)
              self._checkout(checkoutdir, tree)
@@ -374,7 +374,7 @@ class CASCache():
                      for chunk in iter(lambda: tmp.read(4096), b""):
                          h.update(chunk)
                  else:
 -                    tmp = stack.enter_context(tempfile.NamedTemporaryFile(dir=self.tmpdir))
 +                    tmp = stack.enter_context(utils._tempnamedfile(dir=self.tmpdir))
                      # Set mode bits to 0644
                      os.chmod(tmp.name, stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
@@ -545,11 +545,7 @@ class CASCache():
      def remove(self, ref, *, defer_prune=False):
          # Remove cache ref
 -        refpath = self._refpath(ref)
 -        if not os.path.exists(refpath):
 -            raise CASCacheError("Could not find ref '{}'".format(ref))
+-
 -        os.unlink(refpath)
 +        self._remove_ref(ref)
          if not defer_prune:
              pruned = self.prune()
@@ -626,6 +622,55 @@ class CASCache():
      def _refpath(self, ref):
          return os.path.join(self.casdir, 'refs', 'heads', ref)
 +    # _remove_ref()
 +    #
 +    # Removes a ref.
 +    #
 +    # This also takes care of pruning away directories which can
 +    # be removed after having removed the given ref.
 +    #
 +    # Args:
 +    #    ref (str): The ref to remove
 +    #
 +    # Raises:
 +    #    (CASCacheError): If the ref didnt exist, or a system error
 +    #                     occurred while removing it
 +    #
 +    def _remove_ref(self, ref):
++
 +        # Remove the ref itself
 +        refpath = self._refpath(ref)
 +        try:
 +            os.unlink(refpath)
 +        except FileNotFoundError as e:
 +            raise CASCacheError("Could not find ref '{}'".format(ref)) from e
++
 +        # Now remove any leading directories
 +        basedir = os.path.join(self.casdir, 'refs', 'heads')
 +        components = list(os.path.split(ref))
 +        while components:
 +            components.pop()
 +            refdir = os.path.join(basedir, *components)
++
 +            # Break out once we reach the base
 +            if refdir == basedir:
 +                break
++
 +            try:
 +                os.rmdir(refdir)
 +            except FileNotFoundError:
 +                # The parent directory did not exist, but it's
 +                # parent directory might still be ready to prune
 +                pass
 +            except OSError as e:
 +                if e.errno == errno.ENOTEMPTY:
 +                    # The parent directory was not empty, so we
 +                    # cannot prune directories beyond this point
 +                    break
++
 +                # Something went wrong here
 +                raise CASCacheError("System error while removing ref '{}': {}".format(ref, e)) from e
++
      # _commit_directory():
+     #
      # Adds local directory to content addressable store.
@@ -797,7 +842,7 @@ class CASCache():
              # already in local repository
              return objpath
 -        with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
 +        with utils._tempnamedfile(dir=self.tmpdir) as f:
              remote._fetch_blob(digest, f)
              added_digest = self.add_object(path=f.name, link_directly=True)
@@ -807,7 +852,7 @@ class CASCache():
      def _batch_download_complete(self, batch):
          for digest, data in batch.send():
 -            with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
 +            with utils._tempnamedfile(dir=self.tmpdir) as f:
                  f.write(data)
                  f.flush()
@@ -904,7 +949,7 @@ class CASCache():
      def _fetch_tree(self, remote, digest):
          # download but do not store the Tree object
 -        with tempfile.NamedTemporaryFile(dir=self.tmpdir) as out:
 +        with utils._tempnamedfile(dir=self.tmpdir) as out:
              remote._fetch_blob(digest, out)
              tree = remote_execution_pb2.Tree()

buildstream/plugins/elements/import.py

@@ -28,17 +28,14 @@ some configuration data.
  The empty configuration is as such:
    .. literalinclude:: ../../../buildstream/plugins/elements/import.yaml
       :language: yaml
+-
 -See :ref:`built-in functionality documentation <core_buildelement_builtins>` for
 -details on common configuration options for build elements.
  """
  import os
 -from buildstream import Element, BuildElement, ElementError
 +from buildstream import Element, ElementError
  # Element implementation for the 'import' kind.
 -class ImportElement(BuildElement):
 +class ImportElement(Element):
      # pylint: disable=attribute-defined-outside-init
      # This plugin has been modified to avoid the use of Sandbox.get_directory
@@ -93,10 +90,6 @@ class ImportElement(BuildElement):
          # And we're done
          return '/output'
 -    def prepare(self, sandbox):
 -        # We inherit a non-default prepare from BuildElement.
 -        Element.prepare(self, sandbox)
+-
      def generate_script(self):
          build_root = self.get_variable('build-root')
          install_root = self.get_variable('install-root')

buildstream/utils.py

@@ -1032,6 +1032,36 @@ def _tempdir(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-bu
          cleanup_tempdir()
 +# _tempnamedfile()
 +#
 +# A context manager for doing work on an open temporary file
 +# which is guaranteed to be named and have an entry in the filesystem.
 +#
 +# Args:
 +#    dir (str): A path to a parent directory for the temporary file
 +#    suffix (str): A suffix for the temproary file name
 +#    prefix (str): A prefix for the temporary file name
 +#
 +# Yields:
 +#    (str): The temporary file handle
 +#
 +# Do not use tempfile.NamedTemporaryFile() directly, as this will
 +# leak files on the filesystem when BuildStream exits a process
 +# on SIGTERM.
 +#
 +@contextmanager
 +def _tempnamedfile(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-builtin
 +    temp = None
++
 +    def close_tempfile():
 +        if temp is not None:
 +            temp.close()
++
 +    with _signals.terminator(close_tempfile), \
 +        tempfile.NamedTemporaryFile(suffix=suffix, prefix=prefix, dir=dir) as temp:
 +        yield temp
++
++
  # _kill_process_tree()
+ #
  # Brutally murder a process and all of its children

tests/artifactcache/expiry.py

@@ -382,6 +382,7 @@ def test_extract_expiry(cli, datafiles, tmpdir):
      res = cli.run(project=project, args=['checkout', 'target.bst', os.path.join(str(tmpdir), 'checkout')])
      res.assert_success()
 +    # Get a snapshot of the extracts in advance
      extractdir = os.path.join(project, 'cache', 'artifacts', 'extract', 'test', 'target')
      extracts = os.listdir(extractdir)
      assert(len(extracts) == 1)
@@ -395,3 +396,16 @@ def test_extract_expiry(cli, datafiles, tmpdir):
      # Now the extract should be removed.
      assert not os.path.exists(extract)
++
 +    # As an added bonus, let's ensure that no directories have been left behind
 +    #
 +    # Now we should have a directory for the cached target2.bst, which
 +    # replaced target.bst in the cache, we should not have a directory
 +    # for the target.bst
 +    refsdir = os.path.join(project, 'cache', 'artifacts', 'cas', 'refs', 'heads')
 +    refsdirtest = os.path.join(refsdir, 'test')
 +    refsdirtarget = os.path.join(refsdirtest, 'target')
 +    refsdirtarget2 = os.path.join(refsdirtest, 'target2')
++
 +    assert os.path.isdir(refsdirtarget2)
 +    assert not os.path.exists(refsdirtarget)

[Notes] [Git][BuildStream/buildstream][chandan/import-is-not-buildelement] 6 commits: _cas/cascache.py: Cleanup directories when removing refs

Chandan Singh pushed to branch chandan/import-is-not-buildelement at BuildStream / buildstream

Commits:

4 changed files:

Changes: