[Notes] [Git][BuildStream/buildstream][tristan/cas-cleanup-improve] 6 co

Tristan Van Berkom pushed to branch tristan/cas-cleanup-improve at BuildStream / buildstream

Commits:

951a8df1
by Angelos Evripiotis at 2019-01-18T18:02:22Z
```
cli.py: add a hint about '--' to 'bst shell' help
```

9911023f

by Angelos Evripiotis at 2019-01-18T18:56:21Z

Merge branch 'aevri/shell_separator_hint' into 'master'

cli.py: add a hint about '--' to 'bst shell' help

See merge request BuildStream/buildstream!1078

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.

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/_frontend/cli.py

@@ -554,6 +554,12 @@ def shell(app, element, sysroot, mount, isolate, build_, cli_buildtree, command)
      element, assuming it has already been built and all required
      artifacts are in the local cache.
 +    Use '--' to separate a command from the options to bst,
 +    otherwise bst may respond to them instead. e.g.
++
 +    \b
 +        bst shell example.bst -- df -h
++
      Use the --build option to create a temporary sysroot for
      building the element instead.

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][tristan/cas-cleanup-improve] 6 commits: cli.py: add a hint about '--' to 'bst shell' help

Tristan Van Berkom pushed to branch tristan/cas-cleanup-improve at BuildStream / buildstream

Commits:

4 changed files:

Changes: