PXE to pass hints to ImageCache on how much space to reclaim

After previous patch PXE triggers cache clean up, if there's not enough
disk space for deployment. However, standard clean up may reclaim too
little space, due to configuration. This patch implements hints to
ImageCache, so that it tries to reclaim exactly required amount
of disk space.

Note that this amount is calculated as total size of images multiplied
by two as an attempt to account for images converting to raw format.

Change-Id: I8f95b7e6334a37e9e6e98278a5b61a9ace7221b5
Closes-Bug: #1316168

ironic/drivers/modules/image_cache.py

@@ -146,44 +146,80 @@ def _download_image(self, uuid, master_path, dest_path, ctx=None):
             utils.rmtree_without_raise(tmp_dir)
 
     @lockutils.synchronized('master_image', 'ironic-')
-    def clean_up(self):
+    def clean_up(self, amount=None):
         """Clean up directory with images, keeping cache of the latest images.
 
         Files with link count >1 are never deleted.
         Protected by global lock, so that no one messes with master images
         after we get listing and before we actually delete files.
+
+        :param amount: if present, amount of space to reclaim in bytes,
+                       cleaning will stop, if this goal was reached,
+                       even if it is possible to clean up more files
         """
         if self.master_dir is None:
             return
 
         LOG.debug("Starting clean up for master image cache %(dir)s" %
                   {'dir': self.master_dir})
 
+        amount_copy = amount
         listing = _find_candidates_for_deletion(self.master_dir)
-        survived = self._clean_up_too_old(listing)
-        self._clean_up_ensure_cache_size(survived)
-
-    def _clean_up_too_old(self, listing):
+        survived, amount = self._clean_up_too_old(listing, amount)
+        if amount is not None and amount <= 0:
+            return
+        amount = self._clean_up_ensure_cache_size(survived, amount)
+        if amount is not None and amount > 0:
+            LOG.warn(_("Cache clean up was unable to reclaim %(required)d MiB "
+                       "of disk space, still %(left)d MiB required"),
+                     {'required': amount_copy / 1024 / 1024,
+                      'left': amount / 1024 / 1024})
+
+    def _clean_up_too_old(self, listing, amount):
         """Clean up stage 1: drop images that are older than TTL.
 
+        This method removes files all files older than TTL seconds
+        unless 'amount' is non-None. If 'amount' is non-None,
+        it starts removing files older than TTL seconds,
+        oldest first, until the required 'amount' of space is reclaimed.
+
         :param listing: list of tuples (file name, last used time)
-        :returns: list of files left after clean up
+        :param amount: if not None, amount of space to reclaim in bytes,
+                       cleaning will stop, if this goal was reached,
+                       even if it is possible to clean up more files
+        :returns: tuple (list of files left after clean up,
+                         amount still to reclaim)
         """
         threshold = time.time() - self._cache_ttl
         survived = []
         for file_name, last_used, stat in listing:
             if last_used < threshold:
-                utils.unlink_without_raise(file_name)
+                try:
+                    os.unlink(file_name)
+                except EnvironmentError as exc:
+                    LOG.warn(_("Unable to delete file %(name)s from "
+                               "master image cache: %(exc)s") %
+                             {'name': file_name, 'exc': exc})
+                else:
+                    if amount is not None:
+                        amount -= stat.st_size
+                        if amount <= 0:
+                            amount = 0
+                            break
             else:
                 survived.append((file_name, last_used, stat))
-        return survived
+        return survived, amount
 
-    def _clean_up_ensure_cache_size(self, listing):
+    def _clean_up_ensure_cache_size(self, listing, amount):
         """Clean up stage 2: try to ensure cache size < threshold.
         Try to delete the oldest files until conditions is satisfied
         or no more files are eligable for delition.
 
         :param listing: list of tuples (file name, last used time)
+        :param amount: amount of space to reclaim, if possible.
+                       if amount is not None, it has higher priority than
+                       cache size in settings
+        :returns: amount of space still required after clean up
         """
         # NOTE(dtantsur): Sort listing to delete the oldest files first
         listing = sorted(listing,
@@ -193,7 +229,8 @@ def _clean_up_ensure_cache_size(self, listing):
                          for f in os.listdir(self.master_dir))
         total_size = sum(os.path.getsize(f)
                          for f in total_listing)
-        while total_size > self._cache_size and listing:
+        while listing and (total_size > self._cache_size or
+               (amount is not None and amount > 0)):
             file_name, last_used, stat = listing.pop()
             try:
                 os.unlink(file_name)
@@ -203,13 +240,16 @@ def _clean_up_ensure_cache_size(self, listing):
                          {'name': file_name, 'exc': exc})
             else:
                 total_size -= stat.st_size
+                if amount is not None:
+                    amount -= stat.st_size
 
         if total_size > self._cache_size:
             LOG.info(_("After cleaning up cache dir %(dir)s "
                        "cache size %(actual)d is still larger than "
                        "threshold %(expected)d") %
                      {'dir': self.master_dir, 'actual': total_size,
                       'expected': self._cache_size})
+        return max(amount, 0)
 
 
 def _find_candidates_for_deletion(master_dir):

ironic/drivers/modules/pxe.py

@@ -277,7 +277,9 @@ def _cleanup_caches_if_required(ctx, cache, images_info):
         caches = [c for c in (InstanceImageCache(), TFTPImageCache())
                   if os.stat(c.master_dir).st_dev == st_dev]
         for cache_to_clean in caches:
-            cache_to_clean.clean_up()
+            # NOTE(dtantsur): multiplying by 2 is an attempt to account for
+            # images converting to raw format
+            cache_to_clean.clean_up(amount=(2 * total_size - free))
             free = _free_disk_space_for(cache.master_dir)
             if total_size < free:
                 break

ironic/tests/drivers/test_image_cache.py

@@ -117,6 +117,7 @@ def setUp(self):
 
     @mock.patch.object(image_cache.ImageCache, '_clean_up_ensure_cache_size')
     def test_clean_up_old_deleted(self, mock_clean_size):
+        mock_clean_size.return_value = None
         files = [os.path.join(self.master_dir, str(i))
                  for i in range(2)]
         for filename in files:
@@ -127,6 +128,7 @@ def test_clean_up_old_deleted(self, mock_clean_size):
         with mock.patch.object(time, 'time', lambda: new_current_time):
             self.cache.clean_up()
 
+        mock_clean_size.assert_called_once_with(mock.ANY, None)
         survived = mock_clean_size.call_args[0][0]
         self.assertEqual(1, len(survived))
         self.assertEqual(files[0], survived[0][0])
@@ -135,8 +137,24 @@ def test_clean_up_old_deleted(self, mock_clean_size):
         self.assertEqual(int(new_current_time - 100),
                          int(survived[0][2].st_mtime))
 
+    @mock.patch.object(image_cache.ImageCache, '_clean_up_ensure_cache_size')
+    def test_clean_up_old_with_amount(self, mock_clean_size):
+        files = [os.path.join(self.master_dir, str(i))
+                 for i in range(2)]
+        for filename in files:
+            open(filename, 'wb').write('X')
+        new_current_time = time.time() + 900
+        with mock.patch.object(time, 'time', lambda: new_current_time):
+            self.cache.clean_up(amount=1)
+
+        self.assertFalse(mock_clean_size.called)
+        # Exactly one file is expected to be deleted
+        self.assertTrue(any(os.path.exists(f) for f in files))
+        self.assertFalse(all(os.path.exists(f) for f in files))
+
     @mock.patch.object(image_cache.ImageCache, '_clean_up_ensure_cache_size')
     def test_clean_up_files_with_links_untouched(self, mock_clean_size):
+        mock_clean_size.return_value = None
         files = [os.path.join(self.master_dir, str(i))
                  for i in range(2)]
         for filename in files:
@@ -149,11 +167,11 @@ def test_clean_up_files_with_links_untouched(self, mock_clean_size):
 
         for filename in files:
             self.assertTrue(os.path.exists(filename))
-        mock_clean_size.assert_called_once_with([])
+        mock_clean_size.assert_called_once_with([], None)
 
     @mock.patch.object(image_cache.ImageCache, '_clean_up_too_old')
     def test_clean_up_ensure_cache_size(self, mock_clean_ttl):
-        mock_clean_ttl.side_effect = lambda listing: listing
+        mock_clean_ttl.side_effect = lambda *xx: xx
         # NOTE(dtantsur): Cache size in test is 10 bytes, we create 6 files
         # with 3 bytes each and expect 3 to be deleted
         files = [os.path.join(self.master_dir, str(i))
@@ -175,10 +193,36 @@ def test_clean_up_ensure_cache_size(self, mock_clean_ttl):
         for filename in files[3:]:
             self.assertFalse(os.path.exists(filename))
 
+        mock_clean_ttl.assert_called_once_with(mock.ANY, None)
+
+    @mock.patch.object(image_cache.ImageCache, '_clean_up_too_old')
+    def test_clean_up_ensure_cache_size_with_amount(self, mock_clean_ttl):
+        mock_clean_ttl.side_effect = lambda *xx: xx
+        # NOTE(dtantsur): Cache size in test is 10 bytes, we create 6 files
+        # with 3 bytes each and set amount to be 15, 5 files are to be deleted
+        files = [os.path.join(self.master_dir, str(i))
+                 for i in range(6)]
+        for filename in files:
+            with open(filename, 'w') as fp:
+                fp.write('123')
+        # NOTE(dtantsur): Make 1 file 'newer' to check that
+        # old ones are deleted first
+        new_current_time = time.time() + 100
+        os.utime(files[0], (new_current_time, new_current_time))
+
+        with mock.patch.object(time, 'time', lambda: new_current_time):
+            self.cache.clean_up(amount=15)
+
+        self.assertTrue(os.path.exists(files[0]))
+        for filename in files[5:]:
+            self.assertFalse(os.path.exists(filename))
+
+        mock_clean_ttl.assert_called_once_with(mock.ANY, 15)
+
     @mock.patch.object(image_cache.LOG, 'info')
     @mock.patch.object(image_cache.ImageCache, '_clean_up_too_old')
     def test_clean_up_cache_still_large(self, mock_clean_ttl, mock_log):
-        mock_clean_ttl.side_effect = lambda listing: listing
+        mock_clean_ttl.side_effect = lambda *xx: xx
         # NOTE(dtantsur): Cache size in test is 10 bytes, we create 2 files
         # than cannot be deleted and expected this to be logged
         files = [os.path.join(self.master_dir, str(i))
@@ -193,6 +237,7 @@ def test_clean_up_cache_still_large(self, mock_clean_ttl, mock_log):
         for filename in files:
             self.assertTrue(os.path.exists(filename))
         self.assertTrue(mock_log.called)
+        mock_clean_ttl.assert_called_once_with(mock.ANY, None)
 
     @mock.patch.object(utils, 'rmtree_without_raise')
     @mock.patch.object(images, 'fetch_to_raw')
@@ -219,3 +264,13 @@ def test_temp_dir_exception(self, mock_fetch_to_raw, mock_rmtree):
                           self.cache._download_image,
                           'uuid', 'fake', 'fake')
         self.assertTrue(mock_rmtree.called)
+
+    @mock.patch.object(image_cache.LOG, 'warn')
+    @mock.patch.object(image_cache.ImageCache, '_clean_up_too_old')
+    @mock.patch.object(image_cache.ImageCache, '_clean_up_ensure_cache_size')
+    def test_clean_up_amount_not_satisfied(self, mock_clean_size,
+                                           mock_clean_ttl, mock_log):
+        mock_clean_ttl.side_effect = lambda *xx: xx
+        mock_clean_size.side_effect = lambda listing, amount: amount
+        self.cache.clean_up(amount=15)
+        self.assertTrue(mock_log.called)

ironic/tests/drivers/test_pxe.py

@@ -410,7 +410,8 @@ def test_one_clean_up(self, mock_stat, mock_image_service, mock_statvfs,
         mock_statvfs.assert_called_with('master_dir')
         self.assertEqual(2, mock_statvfs.call_count)
         cache.fetch_image.assert_called_once_with('uuid', 'path', ctx=None)
-        mock_instance_cache.return_value.clean_up.assert_called_once_with()
+        mock_instance_cache.return_value.clean_up.assert_called_once_with(
+            amount=(42 * 2 - 1))
         self.assertFalse(mock_tftp_cache.return_value.clean_up.called)
         self.assertEqual(3, mock_stat.call_count)
 
@@ -436,7 +437,8 @@ def test_clean_up_another_fs(self, mock_stat, mock_image_service,
         mock_statvfs.assert_called_with('master_dir')
         self.assertEqual(2, mock_statvfs.call_count)
         cache.fetch_image.assert_called_once_with('uuid', 'path', ctx=None)
-        mock_tftp_cache.return_value.clean_up.assert_called_once_with()
+        mock_tftp_cache.return_value.clean_up.assert_called_once_with(
+            amount=(42 * 2 - 1))
         self.assertFalse(mock_instance_cache.return_value.clean_up.called)
         self.assertEqual(3, mock_stat.call_count)
 
@@ -449,7 +451,7 @@ def test_both_clean_up(self, mock_stat, mock_image_service, mock_statvfs,
         mock_show.return_value = dict(size=42)
         mock_statvfs.side_effect = [
             mock.Mock(f_frsize=1, f_bavail=1),
-            mock.Mock(f_frsize=1, f_bavail=1),
+            mock.Mock(f_frsize=1, f_bavail=2),
             mock.Mock(f_frsize=1, f_bavail=1024)
         ]
 
@@ -460,8 +462,10 @@ def test_both_clean_up(self, mock_stat, mock_image_service, mock_statvfs,
         mock_statvfs.assert_called_with('master_dir')
         self.assertEqual(3, mock_statvfs.call_count)
         cache.fetch_image.assert_called_once_with('uuid', 'path', ctx=None)
-        mock_instance_cache.return_value.clean_up.assert_called_once_with()
-        mock_tftp_cache.return_value.clean_up.assert_called_once_with()
+        mock_instance_cache.return_value.clean_up.assert_called_once_with(
+            amount=(42 * 2 - 1))
+        mock_tftp_cache.return_value.clean_up.assert_called_once_with(
+            amount=(42 * 2 - 2))
         self.assertEqual(3, mock_stat.call_count)
 
     @mock.patch.object(os, 'stat')
@@ -481,8 +485,10 @@ def test_clean_up_fail(self, mock_stat, mock_image_service, mock_statvfs,
         mock_statvfs.assert_called_with('master_dir')
         self.assertEqual(3, mock_statvfs.call_count)
         self.assertFalse(cache.return_value.fetch_image.called)
-        mock_instance_cache.return_value.clean_up.assert_called_once_with()
-        mock_tftp_cache.return_value.clean_up.assert_called_once_with()
+        mock_instance_cache.return_value.clean_up.assert_called_once_with(
+            amount=(42 * 2 - 1))
+        mock_tftp_cache.return_value.clean_up.assert_called_once_with(
+            amount=(42 * 2 - 1))
         self.assertEqual(3, mock_stat.call_count)