VM: check remote image Last-Modified header

lib/rift/VM.py

@@ -62,7 +62,7 @@
 from rift.Config import _DEFAULT_VIRTIOFSD
 from rift.Repository import ProjectArchRepositories
 from rift.TempDir import TempDir
-from rift.utils import download_file, setup_dl_opener, message
+from rift.utils import last_modified, download_file, setup_dl_opener, message
 from rift.run import run_command
 
 __all__ = ['VM']
@@ -389,6 +389,9 @@ def _download(self, force: bool):
         if not self.image_is_remote():
             return
 
+        # Setup proxy if defined
+        setup_dl_opener(self.proxy, self.no_proxy)
+
         # Check presence of the local copy. If present and force is True, remove it
         # to force re-download. Otherwise skip download.
         if os.path.exists(self.image_local):
@@ -399,14 +402,32 @@ def _download(self, force: bool):
                 )
                 os.unlink(self.image_local)
             else:
-                logging.debug(
-                    "Local copy of VM image is present, skipping download of "
-                    "remote image"
+                try:
+                    last_remote_modification = last_modified(self._image_src.geturl())
+                except RiftError as err:
+                    logging.debug(
+                        "Local copy of VM image is present, unable to get remote image "
+                        "modification date because of error (%s), skipping download of "
+                        "remote image",
+                        err
+                    )
+                    return
+                # Compare local copy mtime with remote image Last-Modified header.
+                local_copy_mtime = int(os.path.getmtime(self.image_local))
+                if local_copy_mtime > last_remote_modification:
+                    logging.debug(
+                        "Local copy of VM image is already updated (%d > %d), "
+                        "skipping download of remote image",
+                        int(local_copy_mtime),
+                        int(last_remote_modification)
+                    )
+                    return
+                logging.info(
+                    "Remote VM image has been updated, removing local copy"
                 )
-                return
+                os.unlink(self.image_local)
+
         message(f"Download remote VM image {self._image_src.geturl()}")
-        # Setup proxy if defined
-        setup_dl_opener(self.proxy, self.no_proxy)
         # Download VM image
         download_file(self._image_src.geturl(), self.image_local)
 

lib/rift/utils.py

@@ -35,6 +35,7 @@
 
 import os
 import urllib
+from datetime import datetime, timezone
 
 from rift import RiftError
 
@@ -66,6 +67,32 @@ def download_file(url, output):
             f"URL error while downloading {url}: {str(error)}"
         ) from error
 
+def last_modified(url):
+    """
+    Return the mtime of the URL using the Last-Modified header. By convention,
+    Last-Modified is always in GMT/UTC timezone. Raises RiftError when unable to
+    get or convert Last-Modified header to timestamp.
+    """
+    req = urllib.request.Request(url, method='HEAD')
+
+    try:
+        with urllib.request.urlopen(req) as response:
+            return int(datetime.strptime(
+                response.getheader('Last-Modified'), '%a, %d %b %Y %H:%M:%S %Z'
+            ).replace(tzinfo=timezone.utc).timestamp())
+    except urllib.error.URLError as err:
+        raise RiftError(
+            f"Unable to send HTTP HEAD request for URL {url}: {err}"
+        ) from err
+    except TypeError as err:
+        raise RiftError(
+            f"Unable to get Last-Modified header for URL {url}"
+        ) from err
+    except ValueError as err:
+        raise RiftError(
+            f"Unable to convert Last-Modified header to datetime for URL {url}"
+        ) from err
+
 def setup_dl_opener(proxy, no_proxy, fake_user_agent=True):
     """
     Setup urllib handler/opener with proxy, no_proxy settings. Also set fake

tests/VM.py

@@ -438,7 +438,38 @@ def test_download(self, mock_message, mock_download_file):
 
     @patch('rift.VM.download_file')
     @patch('rift.VM.message')
-    def test_download_skip_exists(self, mock_message, mock_download_file):
+    def test_download_force(self, mock_message, mock_download_file):
+        """Test VM download force remove local image when present"""
+        url = 'http://localhost/path/to/my_image.qcow2'
+        self.config.set(
+            'vm',
+            {
+                'image': url,
+            }
+        )
+        with patch(
+            'rift.VM.VM.image_local', new_callable=PropertyMock
+        ) as mock_image_local:
+            vm = VM(self.config, platform.machine())
+            tmpfile = make_temp_file("")
+            mock_image_local.return_value = tmpfile.name
+            self.assertTrue(os.path.exists(vm.image_local))
+            with self.assertLogs(level='DEBUG') as cm:
+                vm._download(True)
+            mock_message.assert_called_once_with(f"Download remote VM image {url}")
+            mock_download_file.assert_called_once_with(url, vm.image_local)
+        self.assertIn(
+            'INFO:root:Remove VM image local copy and force re-download for remote '
+            'image',
+            cm.output
+        )
+
+    @patch('rift.VM.last_modified')
+    @patch('rift.VM.download_file')
+    @patch('rift.VM.message')
+    def test_download_exists_last_modified_older(
+        self, mock_message, mock_download_file, mock_last_modified
+    ):
         """Test VM download skipped when local copy is present"""
         url = 'http://localhost/path/to/my_image.qcow2'
         self.config.set(
@@ -447,6 +478,7 @@ def test_download_skip_exists(self, mock_message, mock_download_file):
                 'image': url,
             }
         )
+        mock_last_modified.return_value = 0.0
         with patch(
             'rift.VM.VM.image_local', new_callable=PropertyMock
         ) as mock_image_local:
@@ -457,24 +489,61 @@ def test_download_skip_exists(self, mock_message, mock_download_file):
             with self.assertLogs(level='DEBUG') as cm:
                 vm._download(False)
             mock_message.assert_not_called()
+            # Check download_file() has not been called
             mock_download_file.assert_not_called()
+            self.assertIn(
+                "DEBUG:root:Local copy of VM image is already updated "
+                f"({int(os.path.getmtime(tmpfile.name))} > 0), skipping download of "
+                "remote image",
+                cm.output
+            )
+
+    @patch('rift.VM.last_modified')
+    @patch('rift.VM.download_file')
+    @patch('rift.VM.message')
+    def test_download_exists_last_modified_newer(
+        self, mock_message, mock_download_file, mock_last_modified
+    ):
+        """Test VM download skipped when local copy is present"""
+        url = 'http://localhost/path/to/my_image.qcow2'
+        self.config.set(
+            'vm',
+            {
+                'image': url,
+            }
+        )
+        mock_last_modified.return_value = float(2**32)
+        with patch(
+            'rift.VM.VM.image_local', new_callable=PropertyMock
+        ) as mock_image_local:
+            vm = VM(self.config, platform.machine())
+            tmpfile = make_temp_file("")
+            mock_image_local.return_value = tmpfile.name
+            self.assertTrue(os.path.exists(vm.image_local))
+            with self.assertLogs(level='DEBUG') as cm:
+                vm._download(False)
+            mock_message.assert_called_once_with(f"Download remote VM image {url}")
+            mock_download_file.assert_called_once_with(url, vm.image_local)
         self.assertIn(
-            'DEBUG:root:Local copy of VM image is present, skipping download of remote '
-            'image',
+            'INFO:root:Remote VM image has been updated, removing local copy',
             cm.output
         )
 
+    @patch('rift.VM.last_modified')
     @patch('rift.VM.download_file')
     @patch('rift.VM.message')
-    def test_download_force(self, mock_message, mock_download_file):
-        """Test VM download force remove local image when present"""
+    def test_download_exists_last_modified_error(
+        self, mock_message, mock_download_file, mock_last_modified
+    ):
+        """Test VM download skipped when local copy is present"""
         url = 'http://localhost/path/to/my_image.qcow2'
         self.config.set(
             'vm',
             {
                 'image': url,
             }
         )
+        mock_last_modified.side_effect = RiftError("last-modified failure")
         with patch(
             'rift.VM.VM.image_local', new_callable=PropertyMock
         ) as mock_image_local:
@@ -483,12 +552,13 @@ def test_download_force(self, mock_message, mock_download_file):
             mock_image_local.return_value = tmpfile.name
             self.assertTrue(os.path.exists(vm.image_local))
             with self.assertLogs(level='DEBUG') as cm:
-                vm._download(True)
-            mock_message.assert_called_once_with(f"Download remote VM image {url}")
-            mock_download_file.assert_called_once_with(url, vm.image_local)
+                vm._download(False)
+            mock_message.assert_not_called()
+            mock_download_file.assert_not_called()
         self.assertIn(
-            'INFO:root:Remove VM image local copy and force re-download for remote '
-            'image',
+            "DEBUG:root:Local copy of VM image is present, unable to get remote image "
+            "modification date because of error (last-modified failure), skipping "
+            "download of remote image",
             cm.output
         )
 

tests/utils.py

@@ -3,10 +3,11 @@
 #
 
 from io import StringIO
-from unittest.mock import patch
+from unittest.mock import patch, Mock
 
 from TestUtils import RiftTestCase
-from rift.utils import message, banner
+from rift import RiftError
+from rift.utils import message, banner, last_modified
 
 class UtilsTest(RiftTestCase):
 
@@ -19,3 +20,38 @@ def test_message(self, mock_stdout):
     def test_banner(self, mock_stdout):
         banner("bar")
         self.assertEqual(mock_stdout.getvalue(), "** bar **\n")
+
+    @patch('urllib.request.urlopen')
+    def test_last_modified(self, mock_urlopen):
+        mock_response = Mock()
+        mock_response.getheader.return_value = "Sat, 1 Jan 2000 00:00:00 GMT"
+        mock_urlopen.return_value.__enter__.return_value = mock_response
+        self.assertEqual(last_modified("http://test"), 946684800)
+
+    @patch('urllib.request.urlopen')
+    def test_last_modified_header_not_found(self, mock_urlopen):
+        mock_response = Mock()
+        mock_response.getheader.return_value = None
+        mock_urlopen.return_value.__enter__.return_value = mock_response
+        with self.assertRaisesRegex(
+            RiftError, "^Unable to get Last-Modified header for URL http://test$"
+        ):
+            last_modified("http://test")
+
+    @patch('urllib.request.urlopen')
+    def test_last_modified_header_conversion_error(self, mock_urlopen):
+        mock_response = Mock()
+        mock_response.getheader.return_value = "Sat, 1 Jan 2000 00:00:00"
+        mock_urlopen.return_value.__enter__.return_value = mock_response
+        with self.assertRaisesRegex(
+            RiftError,
+            "^Unable to convert Last-Modified header to datetime for URL http://test$"
+        ):
+            last_modified("http://test")
+
+    def test_last_modified_url_error(self):
+        with self.assertRaisesRegex(
+            RiftError,
+            "^Unable to send HTTP HEAD request for URL http://localhost: .*$"
+        ):
+            last_modified("http://localhost")