[Notes] [Git][BuildStream/buildstream][bschubert/fix-atomic-move-git-rep

Benjamin Schubert pushed to branch bschubert/fix-atomic-move-git-repo at BuildStream / buildstream

Commits:

fe33e328

by James Ennis at 2018-11-08T17:54:18Z

using_config.rst: Add documentation to showing how to impose quotas on the local cache

This patch partially resolves #700

09faf002

by James Ennis at 2018-11-08T17:54:18Z

artifactcache.py: Fix misleading error message when using % cache quota

Due to the changed Exception message, this patch also changes
the test_parse_size_over_1024T test in misc.py

d153453c

by Javier Jardón at 2018-11-08T18:22:54Z

Merge branch 'jennis/quota_declaration_fix' into 'master'

Add local cache expiry documentation and fix misleading error message when specifying a percentage cache quota

Closes #700

See merge request BuildStream/buildstream!939

1557b491

by Benjamin Schubert at 2018-11-09T11:21:18Z

Fix os.rename in git source element to correctly handle error codes

According to the documentation
(https://www.unix.com/man-page/POSIX/3posix/rename/), when the directory
already is there, either EEXIST or ENOTEMPTY could be thrown.

Previously only ENOTEMPTY was checked.

Done:
  - Separated the move into its own function
  - Check for both errors
  - Create unit tests for it, covering most test cases

942a4329

by Benjamin Schubert at 2018-11-09T11:21:18Z

Extract atomic move function to utils.py

Moving atomically a file/directory can be tricky since different
errors might be raised for the same underlying problem.

Having a utility function to reduce this discrepancies will help
in ensuring we have correct behavior

7 changed files:

buildstream/_artifactcache/artifactcache.py
buildstream/plugins/sources/git.py
buildstream/utils.py
doc/source/using_config.rst
tests/frontend/buildtrack.py
tests/utils/misc.py
+ tests/utils/movedirectory.py

Changes:

buildstream/_artifactcache/artifactcache.py

@@ -937,15 +937,22 @@ class ArtifactCache():
                              "Invalid cache quota ({}): ".format(utils._pretty_size(cache_quota)) +
                              "BuildStream requires a minimum cache quota of 2G.")
          elif cache_quota > cache_size + available_space:  # Check maximum
 +            if '%' in self.context.config_cache_quota:
 +                available = (available_space / (stat.f_blocks * stat.f_bsize)) * 100
 +                available = '{}% of total disk space'.format(round(available, 1))
 +            else:
 +                available = utils._pretty_size(available_space)
++
              raise LoadError(LoadErrorReason.INVALID_DATA,
                              ("Your system does not have enough available " +
                               "space to support the cache quota specified.\n" +
 -                             "You currently have:\n" +
 -                             "- {used} of cache in use at {local_cache_path}\n" +
 -                             "- {available} of available system storage").format(
 -                                 used=utils._pretty_size(cache_size),
 -                                 local_cache_path=self.context.artifactdir,
 -                                 available=utils._pretty_size(available_space)))
 +                             "\nYou have specified a quota of {quota} total disk space.\n" +
 +                             "- The filesystem containing {local_cache_path} only " +
 +                             "has: {available_size} available.")
 +                            .format(
 +                                quota=self.context.config_cache_quota,
 +                                local_cache_path=self.context.artifactdir,
 +                                available_size=available))
          # Place a slight headroom (2e9 (2GB) on the cache_quota) into
          # cache_quota to try and avoid exceptions.

buildstream/plugins/sources/git.py

@@ -97,6 +97,7 @@ from configparser import RawConfigParser
  from buildstream import Source, SourceError, Consistency, SourceFetcher
  from buildstream import utils
  from buildstream.plugin import CoreWarnings
 +from buildstream.utils import move_atomic, DirectoryExistsError
  GIT_MODULES = '.gitmodules'
@@ -141,21 +142,16 @@ class GitMirror(SourceFetcher):
                                   fail="Failed to clone git repository {}".format(url),
                                   fail_temporarily=True)
 -                # Attempt atomic rename into destination, this will fail if
 -                # another process beat us to the punch
                  try:
 -                    os.rename(tmpdir, self.mirror)
 -                except OSError as e:
+-
 -                    # When renaming and the destination repo already exists, os.rename()
 -                    # will fail with ENOTEMPTY, since an empty directory will be silently
 -                    # replaced
 -                    if e.errno == errno.ENOTEMPTY:
 -                        self.source.status("{}: Discarding duplicate clone of {}"
 -                                           .format(self.source, url))
 -                    else:
 -                        raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
 -                                          .format(self.source, url, tmpdir, self.mirror, e)) from e
 +                    move_atomic(tmpdir, self.mirror)
 +                except DirectoryExistsError:
 +                    # Another process was quicker to download this repository.
 +                    # Let's discard our own
 +                    self.source.status("{}: Discarding duplicate clone of {}"
 +                                    .format(self.source, url))
 +                except OSError:
 +                    raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
 +                                      .format(self.source, url, tmpdir, self.mirror, e)) from e
      def _fetch(self, alias_override=None):
          url = self.source.translate_url(self.url,

buildstream/utils.py

@@ -72,6 +72,11 @@ class ProgramNotFoundError(BstError):
          super().__init__(message, domain=ErrorDomain.PROG_NOT_FOUND, reason=reason)
 +class DirectoryExistsError(OSError):
 +    """Raised when a `os.rename` is attempted but the destination is an existing directory.
 +    """
++
++
  class FileListResult():
      """An object which stores the result of one of the operations
      which run on a list of files.
@@ -500,6 +505,32 @@ def get_bst_version():
                          .format(__version__))
 +def move_atomic(source, destination, ensure_parents=True):
 +    """Move the source to the destination using atomic primitives.
++
 +    This uses `os.rename` to move a file or directory to a new destination.
 +    It wraps some `OSError` thrown errors to ensure their handling is correct.
++
 +    The main reason for this to exist is that rename can throw different errors
 +    for the same symptom (https://www.unix.com/man-page/POSIX/3posix/rename/).
++
 +    We are especially interested here in the case when the destination already
 +    exists. In this case, either EEXIST or ENOTEMPTY are thrown.
++
 +    In order to ensure consistent handling of these exceptions, this function
 +    should be used instead of `os.rename`
 +    """
 +    if ensure_parents:
 +        os.makedirs(os.path.dirname(destination), exist_ok=True)
++
 +    try:
 +        os.rename(source, destination)
 +    except OSError as exc:
 +        if exc.errno in (errno.EEXIST, errno.ENOTEMPTY):
 +            raise DirectoryExistsError(*exc.args) from exc
 +        raise
++
++
  @contextmanager
  def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
                       errors=None, newline=None, closefd=True, opener=None, tempdir=None):

doc/source/using_config.rst

@@ -147,6 +147,44 @@ The default mirror is defined by its name, e.g.
     ``--default-mirror`` command-line option.
 +Local cache expiry
 +~~~~~~~~~~~~~~~~~~
 +BuildStream locally caches artifacts, build trees, log files and sources within a
 +cache located at ``~/.cache/buildstream`` (unless a $XDG_CACHE_HOME environment
 +variable exists). When building large projects, this cache can get very large,
 +thus BuildStream will attempt to clean up the cache automatically by expiring the least
 +recently *used* artifacts.
++
 +By default, cache expiry will begin once the file system which contains the cache
 +approaches maximum usage. However, it is also possible to impose a quota on the local
 +cache in the user configuration. This can be done in two ways:
++
 +1. By restricting the maximum size of the cache directory itself.
++
 +For example, to ensure that BuildStream's cache does not grow beyond 100 GB,
 +simply declare the following in your user configuration (``~/.config/buildstream.conf``):
++
 +.. code:: yaml
++
 +  cache:
 +    quota: 100G
++
 +This quota defines the maximum size of the artifact cache in bytes.
 +Other accepted values are: K, M, G or T (or you can simply declare the value in bytes, without the suffix).
 +This uses the same format as systemd's
 +`resource-control <https://www.freedesktop.org/software/systemd/man/systemd.resource-control.html>`_.
++
 +2. By expiring artifacts once the file system which contains the cache exceeds a specified usage.
++
 +To ensure that we start cleaning the cache once we've used 80% of local disk space (on the file system
 +which mounts the cache):
++
 +.. code:: yaml
++
 +  cache:
 +    quota: 80%
++
++
  Default configuration
  ---------------------
  The default BuildStream configuration is specified here for reference:

tests/frontend/buildtrack.py

@@ -115,6 +115,7 @@ def test_build_track(cli, datafiles, tmpdir, ref_storage,
      args += ['0.bst']
      result = cli.run(project=project, silent=True, args=args)
 +    result.assert_success()
      tracked_elements = result.get_tracked_elements()
      assert set(tracked_elements) == set(tracked)

tests/utils/misc.py

@@ -27,4 +27,5 @@ def test_parse_size_over_1024T(cli, tmpdir):
      patched_statvfs = mock_os.mock_statvfs(f_bavail=bavail, f_bsize=BLOCK_SIZE)
      with mock_os.monkey_patch("statvfs", patched_statvfs):
          result = cli.run(project, args=["build", "file.bst"])
 -        assert "1025T of available system storage" in result.stderr
 +        failure_msg = 'Your system does not have enough available space to support the cache quota specified.'
 +        assert failure_msg in result.stderr

tests/utils/movedirectory.py

 +import pytest
++
 +from buildstream.utils import move_atomic, DirectoryExistsError
++
++
 +@pytest.fixture
 +def src(tmp_path):
 +    src = tmp_path.joinpath("src")
 +    src.mkdir()
++
 +    with src.joinpath("test").open("w") as fp:
 +        fp.write("test")
++
 +    return src
++
++
 +def test_move_to_empty_dir(src, tmp_path):
 +    dst = tmp_path.joinpath("dst")
++
 +    move_atomic(src, dst)
++
 +    assert dst.joinpath("test").exists()
++
++
 +def test_move_to_empty_dir_create_parents(src, tmp_path):
 +    dst = tmp_path.joinpath("nested/dst")
++
 +    move_atomic(src, dst)
 +    assert dst.joinpath("test").exists()
++
++
 +def test_move_to_empty_dir_no_create_parents(src, tmp_path):
 +    dst = tmp_path.joinpath("nested/dst")
++
 +    with pytest.raises(FileNotFoundError):
 +        move_atomic(src, dst, ensure_parents=False)
++
++
 +def test_move_non_existing_dir(tmp_path):
 +    dst = tmp_path.joinpath("dst")
 +    src = tmp_path.joinpath("src")
++
 +    with pytest.raises(FileNotFoundError):
 +        move_atomic(src, dst)
++
++
 +def test_move_to_existing_empty_dir(src, tmp_path):
 +    dst = tmp_path.joinpath("dst")
 +    dst.mkdir()
++
 +    move_atomic(src, dst)
 +    assert dst.joinpath("test").exists()
++
++
 +def test_move_to_existing_file(src, tmp_path):
 +    dst = tmp_path.joinpath("dst")
++
 +    with dst.open("w") as fp:
 +        fp.write("error")
++
 +    with pytest.raises(NotADirectoryError):
 +        move_atomic(src, dst)
++
++
 +def test_move_file_to_existing_file(tmp_path):
 +    dst = tmp_path.joinpath("dst")
 +    src = tmp_path.joinpath("src")
++
 +    with src.open("w") as fp:
 +        fp.write("src")
++
 +    with dst.open("w") as fp:
 +        fp.write("dst")
++
 +    move_atomic(src, dst)
 +    with dst.open() as fp:
 +        assert fp.read() == "src"
++
++
 +def test_move_to_existing_non_empty_dir(src, tmp_path):
 +    dst = tmp_path.joinpath("dst")
 +    dst.mkdir()
++
 +    with dst.joinpath("existing").open("w") as fp:
 +        fp.write("already there")
++
 +    with pytest.raises(DirectoryExistsError):
 +        move_atomic(src, dst)

[Notes] [Git][BuildStream/buildstream][bschubert/fix-atomic-move-git-repo] 5 commits: using_config.rst: Add documentation to showing how to impose quotas on the local cache

Benjamin Schubert pushed to branch bschubert/fix-atomic-move-git-repo at BuildStream / buildstream

Commits:

7 changed files:

Changes: