Merge branch 'tristan/cache-quota-max-only' into 'master'

_artifactcache.py: Don't require the quota to be available on disk.

Closes #869 and #733

See merge request BuildStream/buildstream!1106

4 files changed, 63 insertions, 24 deletions

diff --git a/buildstream/_artifactcache.py b/buildstream/_artifactcache.py
index aa40f571a..48ab278c4 100644
--- a/buildstream/_artifactcache.py
+++ b/buildstream/_artifactcache.py
@@ -98,6 +98,7 @@ class ArtifactCache():
         self._cache_size = None               # The current cache size, sometimes it's an estimate
         self._cache_quota = None              # The cache quota
         self._cache_quota_original = None     # The cache quota as specified by the user, in bytes
+        self._cache_quota_headroom = None     # The headroom in bytes before reaching the quota or full disk
         self._cache_lower_threshold = None    # The target cache size for a cleanup
         self._remotes_setup = False           # Check to prevent double-setup of remotes
 
@@ -314,7 +315,7 @@ class ArtifactCache():
                                   len(self._required_elements),
                                   (context.config_origin or default_conf)))
 
-                if self.has_quota_exceeded():
+                if self.full():
                     raise ArtifactError("Cache too full. Aborting.",
                                         detail=detail,
                                         reason="cache-too-full")
@@ -431,15 +432,25 @@ class ArtifactCache():
         self._cache_size = cache_size
         self._write_cache_size(self._cache_size)
 
-    # has_quota_exceeded()
+    # full()
     #
-    # Checks if the current artifact cache size exceeds the quota.
+    # Checks if the artifact cache is full, either
+    # because the user configured quota has been exceeded
+    # or because the underlying disk is almost full.
     #
     # Returns:
-    #    (bool): True of the quota is exceeded
+    #    (bool): True if the artifact cache is full
     #
-    def has_quota_exceeded(self):
-        return self.get_cache_size() > self._cache_quota
+    def full(self):
+
+        if self.get_cache_size() > self._cache_quota:
+            return True
+
+        _, volume_avail = self._get_cache_volume_size()
+        if volume_avail < self._cache_quota_headroom:
+            return True
+
+        return False
 
     # preflight():
     #
@@ -936,9 +947,9 @@ class ArtifactCache():
         # is taken from the user requested cache_quota.
         #
         if 'BST_TEST_SUITE' in os.environ:
-            headroom = 0
+            self._cache_quota_headroom = 0
         else:
-            headroom = 2e9
+            self._cache_quota_headroom = 2e9
 
         try:
             cache_quota = utils._parse_size(self.context.config_cache_quota,
@@ -960,27 +971,39 @@ class ArtifactCache():
         #
         if cache_quota is None:  # Infinity, set to max system storage
             cache_quota = cache_size + available_space
-        if cache_quota < headroom:  # Check minimum
+        if cache_quota < self._cache_quota_headroom:  # Check minimum
             raise LoadError(LoadErrorReason.INVALID_DATA,
                             "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 / total_size) * 100
-                available = '{}% of total disk space'.format(round(available, 1))
-            else:
-                available = utils._pretty_size(available_space)
-
+        elif cache_quota > total_size:
+            # A quota greater than the total disk size is certianly an error
             raise ArtifactError("Your system does not have enough available " +
                                 "space to support the cache quota specified.",
                                 detail=("You have specified a quota of {quota} total disk space.\n" +
                                         "The filesystem containing {local_cache_path} only " +
-                                        "has {available_size} available.")
+                                        "has {total_size} total disk space.")
                                 .format(
                                     quota=self.context.config_cache_quota,
                                     local_cache_path=self.context.artifactdir,
-                                    available_size=available),
+                                    total_size=utils._pretty_size(total_size)),
                                 reason='insufficient-storage-for-quota')
+        elif cache_quota > cache_size + available_space:
+            # The quota does not fit in the available space, this is a warning
+            if '%' in self.context.config_cache_quota:
+                available = (available_space / total_size) * 100
+                available = '{}% of total disk space'.format(round(available, 1))
+            else:
+                available = utils._pretty_size(available_space)
+
+            self._message(MessageType.WARN,
+                          "Your system does not have enough available " +
+                          "space to support the cache quota specified.",
+                          detail=("You 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.
@@ -990,7 +1013,7 @@ class ArtifactCache():
         # already really fuzzy.
         #
         self._cache_quota_original = cache_quota
-        self._cache_quota = cache_quota - headroom
+        self._cache_quota = cache_quota - self._cache_quota_headroom
         self._cache_lower_threshold = self._cache_quota / 2
 
     # _get_cache_volume_size()
diff --git a/buildstream/_scheduler/queues/buildqueue.py b/buildstream/_scheduler/queues/buildqueue.py
index 66ec4c69b..60ec19ff4 100644
--- a/buildstream/_scheduler/queues/buildqueue.py
+++ b/buildstream/_scheduler/queues/buildqueue.py
@@ -100,7 +100,7 @@ class BuildQueue(Queue):
         # If the estimated size outgrows the quota, ask the scheduler
         # to queue a job to actually check the real cache size.
         #
-        if artifacts.has_quota_exceeded():
+        if artifacts.full():
             self._scheduler.check_cache_size()
 
     def done(self, job, element, result, status):
diff --git a/buildstream/_scheduler/scheduler.py b/buildstream/_scheduler/scheduler.py
index f9d627912..f35a85b23 100644
--- a/buildstream/_scheduler/scheduler.py
+++ b/buildstream/_scheduler/scheduler.py
@@ -303,7 +303,7 @@ class Scheduler():
         # starts while we are checking the cache.
         #
         artifacts = self.context.artifactcache
-        if artifacts.has_quota_exceeded():
+        if artifacts.full():
             self._sched_cache_size_job(exclusive=True)
 
     # _spawn_job()
@@ -338,7 +338,7 @@ class Scheduler():
         context = self.context
         artifacts = context.artifactcache
 
-        if artifacts.has_quota_exceeded():
+        if artifacts.full():
             self._cleanup_scheduled = True
 
     # Callback for the cleanup job
diff --git a/tests/artifactcache/expiry.py b/tests/artifactcache/expiry.py
index 2230b70bd..2cc59e03c 100644
--- a/tests/artifactcache/expiry.py
+++ b/tests/artifactcache/expiry.py
@@ -317,6 +317,16 @@ def test_never_delete_required_track(cli, datafiles, tmpdir):
 # has 10K total disk space, and 6K of it is already in use (not
 # including any space used by the artifact cache).
 #
+# Parameters:
+#    quota (str): A quota size configuration for the config file
+#    err_domain (str): An ErrorDomain, or 'success' or 'warning'
+#    err_reason (str): A reson to compare with an error domain
+#
+# If err_domain is 'success', then err_reason is unused.
+#
+# If err_domain is 'warning', then err_reason is asserted to
+# be in the stderr.
+#
 @pytest.mark.parametrize("quota,err_domain,err_reason", [
     # Valid configurations
     ("1", 'success', None),
@@ -328,9 +338,13 @@ def test_never_delete_required_track(cli, datafiles, tmpdir):
     ("-1", ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA),
     ("pony", ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA),
     ("200%", ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA),
+
+    # Not enough space on disk even if you cleaned up
+    ("11K", ErrorDomain.ARTIFACT, 'insufficient-storage-for-quota'),
+
     # Not enough space for these caches
-    ("7K", ErrorDomain.ARTIFACT, 'insufficient-storage-for-quota'),
-    ("70%", ErrorDomain.ARTIFACT, 'insufficient-storage-for-quota')
+    ("7K", 'warning', 'Your system does not have enough available'),
+    ("70%", 'warning', 'Your system does not have enough available')
 ])
 @pytest.mark.datafiles(DATA_DIR)
 def test_invalid_cache_quota(cli, datafiles, tmpdir, quota, err_domain, err_reason):
@@ -374,6 +388,8 @@ def test_invalid_cache_quota(cli, datafiles, tmpdir, quota, err_domain, err_reas
 
     if err_domain == 'success':
         res.assert_success()
+    elif err_domain == 'warning':
+        assert err_reason in res.stderr
     else:
         res.assert_main_error(err_domain, err_reason)