Update cached images based on update time

Currently, if non-glance image is used in ironic, and contents of this
image are updated on HTTP(S) server or on file system available
locally (except for the case when it is the file system where image
cache is located), ironic won't be able to determine that the image
was updated and will not update the cache.

In this change it is proposed to use last modification date to
determine if an image is up to date. For file system it can be
accomplished with standard operating system means, and for HTTP(S)
images 'Last-Modified' HTTP header can be used.

Closes-bug: #1459328
Change-Id: Ia3e6b6072fdf7a4bfffdc3fbaf31103605c7ff5b

doc/source/deploy/install-guide.rst

@@ -1762,10 +1762,12 @@ For iLO drivers, fields that should be provided are:
 * ``ilo_boot_iso``, ``image_source``, ``root_gb`` under ``instance_info``.
 
 .. note::
-   There is one limitation in this method - Ironic is not tracking changes of
-   content under hrefs that are specified. I.e., if the content under
-   "http://my.server.net/images/deploy.ramdisk" changes, Ironic does not know
-   about that and does not redownload the content.
+   Before Liberty release Ironic was not able to track non-Glance images'
+   content changes. Starting with Liberty, it is possible to do so using image
+   modification date. For example, for HTTP image, if 'Last-Modified' header
+   value of HEAD request to "http://my.server.net/images/deploy.ramdisk" is
+   greater than cached image modification time, Ironic will re-download the
+   content. For "file://" images, the file system modification time is used.
 
 
 Other references

ironic/common/glance_service/service_utils.py

@@ -77,10 +77,15 @@ def _extract_attributes(image):
 
 
 def _convert_timestamps_to_datetimes(image_meta):
-    """Returns image with timestamp fields converted to datetime objects."""
+    """Convert timestamps to datetime objects
+
+    Returns image metadata with timestamp fields converted to naive UTC
+    datetime objects.
+    """
     for attr in ['created_at', 'updated_at', 'deleted_at']:
         if image_meta.get(attr):
-            image_meta[attr] = timeutils.parse_isotime(image_meta[attr])
+            image_meta[attr] = timeutils.normalize_time(
+                timeutils.parse_isotime(image_meta[attr]))
     return image_meta
 
 _CONVERT_PROPS = ('block_device_mapping', 'mappings')

ironic/common/image_service.py

@@ -16,6 +16,7 @@
 
 
 import abc
+import datetime
 import os
 import shutil
 
@@ -30,6 +31,7 @@ import six.moves.urllib.parse as urlparse
 from ironic.common import exception
 from ironic.common.i18n import _
 from ironic.common import keystone
+from ironic.common import utils
 
 LOG = logging.getLogger(__name__)
 
@@ -119,7 +121,9 @@ class BaseImageService(object):
 
         :param image_href: Image reference.
         :raises: exception.ImageRefValidationFailed.
-        :returns: dictionary of image properties.
+        :returns: dictionary of image properties. It has three of them: 'size',
+            'updated_at' and 'properties'. 'updated_at' attribute is a naive
+            UTC datetime object.
         """
 
 
@@ -178,7 +182,9 @@ class HttpImageService(BaseImageService):
             * HEAD request failed;
             * HEAD request returned response code not equal to 200;
             * Content-Length header not found in response to HEAD request.
-        :returns: dictionary of image properties.
+        :returns: dictionary of image properties. It has three of them: 'size',
+            'updated_at' and 'properties'. 'updated_at' attribute is a naive
+            UTC datetime object.
         """
         response = self.validate_href(image_href)
         image_size = response.headers.get('Content-Length')
@@ -188,8 +194,26 @@ class HttpImageService(BaseImageService):
                 reason=_("Cannot determine image size as there is no "
                          "Content-Length header specified in response "
                          "to HEAD request."))
+
+        # Parse last-modified header to return naive datetime object
+        str_date = response.headers.get('Last-Modified')
+        date = None
+        if str_date:
+            http_date_format_strings = [
+                '%a, %d %b %Y %H:%M:%S GMT',  # RFC 822
+                '%A, %d-%b-%y %H:%M:%S GMT',  # RFC 850
+                '%a %b %d %H:%M:%S %Y'        # ANSI C
+            ]
+            for fmt in http_date_format_strings:
+                try:
+                    date = datetime.datetime.strptime(str_date, fmt)
+                    break
+                except ValueError:
+                    continue
+
         return {
             'size': int(image_size),
+            'updated_at': date,
             'properties': {}
         }
 
@@ -248,11 +272,15 @@ class FileImageService(BaseImageService):
         :param image_href: Image reference.
         :raises: exception.ImageRefValidationFailed if image file specified
             doesn't exist.
-        :returns: dictionary of image properties.
+        :returns: dictionary of image properties. It has three of them: 'size',
+            'updated_at' and 'properties'. 'updated_at' attribute is a naive
+            UTC datetime object.
         """
         source_image_path = self.validate_href(image_href)
         return {
             'size': os.path.getsize(source_image_path),
+            'updated_at': utils.unix_file_modification_datetime(
+                source_image_path),
             'properties': {}
         }
 

ironic/common/utils.py

@@ -19,6 +19,7 @@
 """Utilities and helper functions."""
 
 import contextlib
+import datetime
 import errno
 import hashlib
 import os
@@ -32,7 +33,9 @@ from oslo_concurrency import processutils
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import excutils
+from oslo_utils import timeutils
 import paramiko
+import pytz
 import six
 
 from ironic.common import exception
@@ -658,3 +661,13 @@ def get_updated_capabilities(current_capabilities, new_capabilities):
 def is_regex_string_in_file(path, string):
     with open(path, 'r') as inf:
         return any(re.search(string, line) for line in inf.readlines())
+
+
+def unix_file_modification_datetime(file_name):
+    return timeutils.normalize_time(
+        # normalize time to be UTC without timezone
+        datetime.datetime.fromtimestamp(
+            # fromtimestamp will return local time by default, make it UTC
+            os.path.getmtime(file_name), tz=pytz.utc
+        )
+    )

ironic/drivers/modules/image_cache.py

@@ -34,6 +34,7 @@ from ironic.common.glance_service import service_utils
 from ironic.common.i18n import _
 from ironic.common.i18n import _LI
 from ironic.common.i18n import _LW
+from ironic.common import image_service
 from ironic.common import images
 from ironic.common import utils
 
@@ -75,10 +76,12 @@ class ImageCache(object):
     def fetch_image(self, href, dest_path, ctx=None, force_raw=True):
         """Fetch image by given href to the destination path.
 
-        Does nothing if destination path exists and corresponds to a file that
-        exists.
-        Only creates a link if master image for this UUID is already in cache.
-        Otherwise downloads an image and also stores it in cache.
+        Does nothing if destination path exists and is up to date with cache
+        and href contents.
+        Only creates a hard link (dest_path) to cached image if requested
+        image is already in cache and up to date with href contents.
+        Otherwise downloads an image, stores it in cache and creates a hard
+        link (dest_path) to it.
 
         :param href: image UUID or href to fetch
         :param dest_path: destination file path
@@ -115,33 +118,31 @@ class ImageCache(object):
 
         # TODO(dtantsur): lock expiration time
         with lockutils.lock(img_download_lock_name, 'ironic-'):
-            if os.path.exists(dest_path):
-                # NOTE(vdrok): After rebuild requested image can change, so we
-                # should ensure that dest_path and master_path (if exists) are
-                # pointing to the same file
-                if (os.path.exists(master_path) and
-                        (os.stat(dest_path).st_ino ==
-                         os.stat(master_path).st_ino)):
-                    LOG.debug("Destination %(dest)s already exists for "
-                              "image %(uuid)s" %
-                              {'uuid': href,
-                               'dest': dest_path})
-                    return
-                os.unlink(dest_path)
+            # NOTE(vdrok): After rebuild requested image can change, so we
+            # should ensure that dest_path and master_path (if exists) are
+            # pointing to the same file and their content is up to date
+            cache_up_to_date = _delete_master_path_if_stale(master_path, href,
+                                                            ctx)
+            dest_up_to_date = _delete_dest_path_if_stale(master_path,
+                                                         dest_path)
 
-            try:
+            if cache_up_to_date and dest_up_to_date:
+                LOG.debug("Destination %(dest)s already exists "
+                          "for image %(href)s",
+                          {'href': href, 'dest': dest_path})
+                return
+
+            if cache_up_to_date:
                 # NOTE(dtantsur): ensure we're not in the middle of clean up
                 with lockutils.lock('master_image', 'ironic-'):
                     os.link(master_path, dest_path)
-            except OSError:
-                LOG.info(_LI("Master cache miss for image %(uuid)s, "
-                             "starting download"),
-                         {'uuid': href})
-            else:
-                LOG.debug("Master cache hit for image %(uuid)s",
-                          {'uuid': href})
+                LOG.debug("Master cache hit for image %(href)s",
+                          {'href': href})
                 return
 
+            LOG.info(_LI("Master cache miss for image %(href)s, "
+                         "starting download"),
+                     {'href': href})
             self._download_image(
                 href, master_path, dest_path, ctx=ctx, force_raw=force_raw)
 
@@ -381,3 +382,59 @@ def cleanup(priority):
         return cls
 
     return _add_property_to_class_func
+
+
+def _delete_master_path_if_stale(master_path, href, ctx):
+    """Delete image from cache if it is not up to date with href contents.
+
+    :param master_path: path to an image in master cache
+    :param href: image href
+    :param ctx: context to use
+    :returns: True if master_path is up to date with href contents,
+        False if master_path was stale and was deleted or it didn't exist
+    """
+    if service_utils.is_glance_image(href):
+        # Glance image contents cannot be updated without its UUID change
+        return os.path.exists(master_path)
+    if os.path.exists(master_path):
+        img_service = image_service.get_image_service(href, context=ctx)
+        img_mtime = img_service.show(href).get('updated_at')
+        if not img_mtime:
+            # This means that href is not a glance image and doesn't have an
+            # updated_at attribute
+            LOG.warn(_LW("Image service couldn't determine last "
+                         "modification time of %(href)s, considering "
+                         "cached image up to date."), {'href': href})
+            return True
+        master_mtime = utils.unix_file_modification_datetime(master_path)
+        if img_mtime < master_mtime:
+            return True
+        # Delete image from cache as it is outdated
+        LOG.info(_LI('Image %(href)s was last modified at %(remote_time)s. '
+                     'Deleting the cached copy since it was cached at '
+                     '%(local_time)s and may be outdated.'),
+                 {'href': href, 'remote_time': img_mtime,
+                  'local_time': master_mtime})
+        os.unlink(master_path)
+    return False
+
+
+def _delete_dest_path_if_stale(master_path, dest_path):
+    """Delete dest_path if it does not point to cached image.
+
+    :param master_path: path to an image in master cache
+    :param dest_path: hard link to an image
+    :returns: True if dest_path points to master_path, False if dest_path was
+        stale and was deleted or it didn't exist
+    """
+    dest_path_exists = os.path.exists(dest_path)
+    if not dest_path_exists:
+        # Image not cached, re-download
+        return False
+    master_path_exists = os.path.exists(master_path)
+    if (not master_path_exists or
+            os.stat(master_path).st_ino != os.stat(dest_path).st_ino):
+        # Image exists in cache, but dest_path out of date
+        os.unlink(dest_path)
+        return False
+    return True

ironic/tests/drivers/test_image_cache.py

@@ -16,6 +16,7 @@
 
 """Tests for ImageCache class and helper functions."""
 
+import datetime
 import os
 import tempfile
 import time
@@ -37,7 +38,6 @@ def touch(filename):
     open(filename, 'w').close()
 
 
-@mock.patch.object(image_cache, '_fetch', autospec=True)
 class TestImageCacheFetch(base.TestCase):
 
     def setUp(self):
@@ -49,6 +49,7 @@ class TestImageCacheFetch(base.TestCase):
         self.uuid = uuidutils.generate_uuid()
         self.master_path = os.path.join(self.master_dir, self.uuid)
 
+    @mock.patch.object(image_cache, '_fetch', autospec=True)
     @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
     @mock.patch.object(image_cache.ImageCache, '_download_image',
                        autospec=True)
@@ -61,94 +62,101 @@ class TestImageCacheFetch(base.TestCase):
             None, self.uuid, self.dest_path, True)
         self.assertFalse(mock_clean_up.called)
 
-    @mock.patch.object(os, 'unlink', autospec=True)
     @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
     @mock.patch.object(image_cache.ImageCache, '_download_image',
                        autospec=True)
-    def test_fetch_image_dest_and_master_exist_uptodate(
-            self, mock_download, mock_clean_up, mock_unlink, mock_fetch):
-        touch(self.master_path)
-        os.link(self.master_path, self.dest_path)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
+                       return_value=True, autospec=True)
+    @mock.patch.object(image_cache, '_delete_master_path_if_stale',
+                       return_value=True, autospec=True)
+    def test_fetch_image_dest_and_master_uptodate(
+            self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
+            mock_clean_up):
         self.cache.fetch_image(self.uuid, self.dest_path)
-        self.assertFalse(mock_unlink.called)
+        mock_cache_upd.assert_called_once_with(self.master_path, self.uuid,
+                                               None)
+        mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
+        self.assertFalse(mock_link.called)
         self.assertFalse(mock_download.called)
-        self.assertFalse(mock_fetch.called)
         self.assertFalse(mock_clean_up.called)
 
     @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
     @mock.patch.object(image_cache.ImageCache, '_download_image',
                        autospec=True)
-    def test_fetch_image_dest_and_master_exist_outdated(
-            self, mock_download, mock_clean_up, mock_fetch):
-        touch(self.master_path)
-        touch(self.dest_path)
-        self.assertNotEqual(os.stat(self.dest_path).st_ino,
-                            os.stat(self.master_path).st_ino)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
+                       return_value=False, autospec=True)
+    @mock.patch.object(image_cache, '_delete_master_path_if_stale',
+                       return_value=True, autospec=True)
+    def test_fetch_image_dest_out_of_date(
+            self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
+            mock_clean_up):
         self.cache.fetch_image(self.uuid, self.dest_path)
+        mock_cache_upd.assert_called_once_with(self.master_path, self.uuid,
+                                               None)
+        mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
+        mock_link.assert_called_once_with(self.master_path, self.dest_path)
         self.assertFalse(mock_download.called)
-        self.assertFalse(mock_fetch.called)
-        self.assertTrue(os.path.isfile(self.dest_path))
-        self.assertEqual(os.stat(self.dest_path).st_ino,
-                         os.stat(self.master_path).st_ino)
         self.assertFalse(mock_clean_up.called)
 
-    @mock.patch.object(os, 'unlink', autospec=True)
     @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
     @mock.patch.object(image_cache.ImageCache, '_download_image',
                        autospec=True)
-    def test_fetch_image_only_dest_exists(
-            self, mock_download, mock_clean_up, mock_unlink, mock_fetch):
-        touch(self.dest_path)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
+                       return_value=True, autospec=True)
+    @mock.patch.object(image_cache, '_delete_master_path_if_stale',
+                       return_value=False, autospec=True)
+    def test_fetch_image_master_out_of_date(
+            self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
+            mock_clean_up):
         self.cache.fetch_image(self.uuid, self.dest_path)
-        mock_unlink.assert_called_once_with(self.dest_path)
-        self.assertFalse(mock_fetch.called)
+        mock_cache_upd.assert_called_once_with(self.master_path, self.uuid,
+                                               None)
+        mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
+        self.assertFalse(mock_link.called)
         mock_download.assert_called_once_with(
             self.cache, self.uuid, self.master_path, self.dest_path,
             ctx=None, force_raw=True)
-        self.assertTrue(mock_clean_up.called)
+        mock_clean_up.assert_called_once_with(self.cache)
 
     @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
     @mock.patch.object(image_cache.ImageCache, '_download_image',
                        autospec=True)
-    def test_fetch_image_master_exists(self, mock_download, mock_clean_up,
-                                       mock_fetch):
-        touch(self.master_path)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(image_cache, '_delete_dest_path_if_stale',
+                       return_value=True, autospec=True)
+    @mock.patch.object(image_cache, '_delete_master_path_if_stale',
+                       return_value=False, autospec=True)
+    def test_fetch_image_both_master_and_dest_out_of_date(
+            self, mock_cache_upd, mock_dest_upd, mock_link, mock_download,
+            mock_clean_up):
         self.cache.fetch_image(self.uuid, self.dest_path)
-        self.assertFalse(mock_download.called)
-        self.assertFalse(mock_fetch.called)
-        self.assertTrue(os.path.isfile(self.dest_path))
-        self.assertEqual(os.stat(self.dest_path).st_ino,
-                         os.stat(self.master_path).st_ino)
-        self.assertFalse(mock_clean_up.called)
-
-    @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
-    @mock.patch.object(image_cache.ImageCache, '_download_image',
-                       autospec=True)
-    def test_fetch_image(self, mock_download, mock_clean_up,
-                         mock_fetch):
-        self.cache.fetch_image(self.uuid, self.dest_path)
-        self.assertFalse(mock_fetch.called)
+        mock_cache_upd.assert_called_once_with(self.master_path, self.uuid,
+                                               None)
+        mock_dest_upd.assert_called_once_with(self.master_path, self.dest_path)
+        self.assertFalse(mock_link.called)
         mock_download.assert_called_once_with(
             self.cache, self.uuid, self.master_path, self.dest_path,
             ctx=None, force_raw=True)
-        self.assertTrue(mock_clean_up.called)
+        mock_clean_up.assert_called_once_with(self.cache)
 
     @mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True)
     @mock.patch.object(image_cache.ImageCache, '_download_image',
                        autospec=True)
-    def test_fetch_image_not_uuid(self, mock_download, mock_clean_up,
-                                  mock_fetch):
+    def test_fetch_image_not_uuid(self, mock_download, mock_clean_up):
         href = u'http://abc.com/ubuntu.qcow2'
         href_encoded = href.encode('utf-8') if six.PY2 else href
         href_converted = str(uuid.uuid5(uuid.NAMESPACE_URL, href_encoded))
         master_path = os.path.join(self.master_dir, href_converted)
         self.cache.fetch_image(href, self.dest_path)
-        self.assertFalse(mock_fetch.called)
         mock_download.assert_called_once_with(
             self.cache, href, master_path, self.dest_path,
             ctx=None, force_raw=True)
         self.assertTrue(mock_clean_up.called)
 
+    @mock.patch.object(image_cache, '_fetch', autospec=True)
     def test__download_image(self, mock_fetch):
         def _fake_fetch(ctx, uuid, tmp_path, *args):
             self.assertEqual(self.uuid, uuid)
@@ -167,6 +175,119 @@ class TestImageCacheFetch(base.TestCase):
             self.assertEqual("TEST", fp.read())
 
 
+@mock.patch.object(os, 'unlink', autospec=True)
+class TestUpdateImages(base.TestCase):
+
+    def setUp(self):
+        super(TestUpdateImages, self).setUp()
+        self.master_dir = tempfile.mkdtemp()
+        self.dest_dir = tempfile.mkdtemp()
+        self.dest_path = os.path.join(self.dest_dir, 'dest')
+        self.uuid = uuidutils.generate_uuid()
+        self.master_path = os.path.join(self.master_dir, self.uuid)
+
+    @mock.patch.object(os.path, 'exists', return_value=False, autospec=True)
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    def test__delete_master_path_if_stale_glance_img_not_cached(
+            self, mock_gis, mock_path_exists, mock_unlink):
+        res = image_cache._delete_master_path_if_stale(self.master_path,
+                                                       self.uuid, None)
+        self.assertFalse(mock_gis.called)
+        self.assertFalse(mock_unlink.called)
+        mock_path_exists.assert_called_once_with(self.master_path)
+        self.assertFalse(res)
+
+    @mock.patch.object(os.path, 'exists', return_value=True, autospec=True)
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    def test__delete_master_path_if_stale_glance_img(
+            self, mock_gis, mock_path_exists, mock_unlink):
+        res = image_cache._delete_master_path_if_stale(self.master_path,
+                                                       self.uuid, None)
+        self.assertFalse(mock_gis.called)
+        self.assertFalse(mock_unlink.called)
+        mock_path_exists.assert_called_once_with(self.master_path)
+        self.assertTrue(res)
+
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    def test__delete_master_path_if_stale_no_master(self, mock_gis,
+                                                    mock_unlink):
+        res = image_cache._delete_master_path_if_stale(self.master_path,
+                                                       'http://11', None)
+        self.assertFalse(mock_gis.called)
+        self.assertFalse(mock_unlink.called)
+        self.assertFalse(res)
+
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    def test__delete_master_path_if_stale_no_updated_at(self, mock_gis,
+                                                        mock_unlink):
+        touch(self.master_path)
+        href = 'http://awesomefreeimages.al/img111'
+        mock_gis.return_value.show.return_value = {}
+        res = image_cache._delete_master_path_if_stale(self.master_path, href,
+                                                       None)
+        mock_gis.assert_called_once_with(href, context=None)
+        self.assertFalse(mock_unlink.called)
+        self.assertTrue(res)
+
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    def test__delete_master_path_if_stale_master_up_to_date(self, mock_gis,
+                                                            mock_unlink):
+        touch(self.master_path)
+        href = 'http://awesomefreeimages.al/img999'
+        mock_gis.return_value.show.return_value = {
+            'updated_at': datetime.datetime(1999, 11, 15, 8, 12, 31)
+        }
+        res = image_cache._delete_master_path_if_stale(self.master_path, href,
+                                                       None)
+        mock_gis.assert_called_once_with(href, context=None)
+        self.assertFalse(mock_unlink.called)
+        self.assertTrue(res)
+
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    def test__delete_master_path_if_stale_out_of_date(self, mock_gis,
+                                                      mock_unlink):
+        touch(self.master_path)
+        href = 'http://awesomefreeimages.al/img999'
+        mock_gis.return_value.show.return_value = {
+            'updated_at': datetime.datetime((datetime.datetime.utcnow().year
+                                             + 1), 11, 15, 8, 12, 31)
+        }
+        res = image_cache._delete_master_path_if_stale(self.master_path, href,
+                                                       None)
+        mock_gis.assert_called_once_with(href, context=None)
+        mock_unlink.assert_called_once_with(self.master_path)
+        self.assertFalse(res)
+
+    def test__delete_dest_path_if_stale_no_dest(self, mock_unlink):
+        res = image_cache._delete_dest_path_if_stale(self.master_path,
+                                                     self.dest_path)
+        self.assertFalse(mock_unlink.called)
+        self.assertFalse(res)
+
+    def test__delete_dest_path_if_stale_no_master(self, mock_unlink):
+        touch(self.dest_path)
+        res = image_cache._delete_dest_path_if_stale(self.master_path,
+                                                     self.dest_path)
+        mock_unlink.assert_called_once_with(self.dest_path)
+        self.assertFalse(res)
+
+    def test__delete_dest_path_if_stale_out_of_date(self, mock_unlink):
+        touch(self.master_path)
+        touch(self.dest_path)
+        res = image_cache._delete_dest_path_if_stale(self.master_path,
+                                                     self.dest_path)
+        mock_unlink.assert_called_once_with(self.dest_path)
+        self.assertFalse(res)
+
+    def test__delete_dest_path_if_stale_up_to_date(self, mock_unlink):
+        touch(self.master_path)
+        os.link(self.master_path, self.dest_path)
+        res = image_cache._delete_dest_path_if_stale(self.master_path,
+                                                     self.dest_path)
+        self.assertFalse(mock_unlink.called)
+        self.assertTrue(res)
+
+
 class TestImageCacheCleanUp(base.TestCase):
 
     def setUp(self):

ironic/tests/test_glance_service.py

@@ -94,12 +94,7 @@ class TestGlanceImageService(base.TestCase):
     NOW_GLANCE_OLD_FORMAT = "2010-10-11T10:30:22"
     NOW_GLANCE_FORMAT = "2010-10-11T10:30:22.000000"
 
-    class tzinfo(datetime.tzinfo):
-        @staticmethod
-        def utcoffset(*args, **kwargs):
-            return datetime.timedelta()
-
-    NOW_DATETIME = datetime.datetime(2010, 10, 11, 10, 30, 22, tzinfo=tzinfo())
+    NOW_DATETIME = datetime.datetime(2010, 10, 11, 10, 30, 22)
 
     def setUp(self):
         super(TestGlanceImageService, self).setUp()

ironic/tests/test_image_service.py

@@ -10,6 +10,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import datetime
 import os
 import shutil
 
@@ -66,11 +67,28 @@ class HttpImageServiceTestCase(base.TestCase):
         head_mock.assert_called_once_with(self.href)
 
     @mock.patch.object(requests, 'head', autospec=True)
-    def test_show(self, head_mock):
+    def _test_show(self, head_mock, mtime, mtime_date):
         head_mock.return_value.status_code = 200
+        head_mock.return_value.headers = {
+            'Content-Length': 100,
+            'Last-Modified': mtime
+        }
         result = self.service.show(self.href)
-        head_mock.assert_called_with(self.href)
-        self.assertEqual({'size': 1, 'properties': {}}, result)
+        head_mock.assert_called_once_with(self.href)
+        self.assertEqual({'size': 100, 'updated_at': mtime_date,
+                          'properties': {}}, result)
+
+    def test_show_rfc_822(self):
+        self._test_show(mtime='Tue, 15 Nov 2014 08:12:31 GMT',
+                        mtime_date=datetime.datetime(2014, 11, 15, 8, 12, 31))
+
+    def test_show_rfc_850(self):
+        self._test_show(mtime='Tuesday, 15-Nov-14 08:12:31 GMT',
+                        mtime_date=datetime.datetime(2014, 11, 15, 8, 12, 31))
+
+    def test_show_ansi_c(self):
+        self._test_show(mtime='Tue Nov 15 08:12:31 2014',
+                        mtime_date=datetime.datetime(2014, 11, 15, 8, 12, 31))
 
     @mock.patch.object(requests, 'head', autospec=True)
     def test_show_no_content_length(self, head_mock):
@@ -132,15 +150,21 @@ class FileImageServiceTestCase(base.TestCase):
                           self.service.validate_href, self.href)
         path_exists_mock.assert_called_once_with(self.href_path)
 
+    @mock.patch.object(os.path, 'getmtime', return_value=1431087909.1641912,
+                       autospec=True)
     @mock.patch.object(os.path, 'getsize', return_value=42, autospec=True)
     @mock.patch.object(image_service.FileImageService, 'validate_href',
                        autospec=True)
-    def test_show(self, _validate_mock, getsize_mock):
+    def test_show(self, _validate_mock, getsize_mock, getmtime_mock):
         _validate_mock.return_value = self.href_path
         result = self.service.show(self.href)
         getsize_mock.assert_called_once_with(self.href_path)
+        getmtime_mock.assert_called_once_with(self.href_path)
         _validate_mock.assert_called_once_with(mock.ANY, self.href)
-        self.assertEqual({'size': 42, 'properties': {}}, result)
+        self.assertEqual({'size': 42,
+                          'updated_at': datetime.datetime(2015, 5, 8,
+                                                          12, 25, 9, 164191),
+                          'properties': {}}, result)
 
     @mock.patch.object(os, 'link', autospec=True)
     @mock.patch.object(os, 'remove', autospec=True)

ironic/tests/test_utils.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import datetime
 import errno
 import hashlib
 import os
@@ -434,6 +435,14 @@ class GenericUtilsTestCase(base.TestCase):
         # original value.
         self.assertEqual(value, utils.safe_rstrip(value))
 
+    @mock.patch.object(os.path, 'getmtime', return_value=1439465889.4964755,
+                       autospec=True)
+    def test_unix_file_modification_datetime(self, mtime_mock):
+        expected = datetime.datetime(2015, 8, 13, 11, 38, 9, 496475)
+        self.assertEqual(expected,
+                         utils.unix_file_modification_datetime('foo'))
+        mtime_mock.assert_called_once_with('foo')
+
 
 class MkfsTestCase(base.TestCase):
 

requirements.txt

@@ -16,6 +16,7 @@ python-neutronclient<3,>=2.3.11
 python-glanceclient>=0.18.0
 python-keystoneclient>=1.6.0
 python-swiftclient>=2.2.0
+pytz>=2013.6
 stevedore>=1.5.0 # Apache-2.0
 pysendfile>=2.0.0
 websockify>=0.6.0