Merge "Always fall back from hard linking to copying files"

3 files changed, 40 insertions, 107 deletions

diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py
index 5280ee6bc..4b95b5152 100644
--- a/ironic/common/image_service.py
+++ b/ironic/common/image_service.py
@@ -34,10 +34,6 @@ from ironic.common import utils
 from ironic.conf import CONF
 
 IMAGE_CHUNK_SIZE = 1024 * 1024  # 1mb
-# NOTE(kaifeng) Image will be truncated to 2GiB by sendfile,
-# we use a large chunk size here for a better performance
-# while keep the chunk size less than the size limit.
-SENDFILE_CHUNK_SIZE = 1024 * 1024 * 1024  # 1Gb
 LOG = log.getLogger(__name__)
 
 
@@ -264,26 +260,23 @@ class FileImageService(BaseImageService):
         """
         source_image_path = self.validate_href(image_href)
         dest_image_path = image_file.name
-        local_device = os.stat(dest_image_path).st_dev
         try:
-            # We should have read and write access to source file to create
-            # hard link to it.
-            if (local_device == os.stat(source_image_path).st_dev
-                    and os.access(source_image_path, os.R_OK | os.W_OK)):
-                image_file.close()
-                os.remove(dest_image_path)
+            image_file.close()
+            os.remove(dest_image_path)
+
+            try:
                 os.link(source_image_path, dest_image_path)
+            except OSError as exc:
+                LOG.debug('Could not create a link from %(src)s to %(dest)s, '
+                          'will copy the content instead. Error: %(exc)s.',
+                          {'src': source_image_path, 'dest': dest_image_path,
+                           'exc': exc})
             else:
-                filesize = os.path.getsize(source_image_path)
-                offset = 0
-                with open(source_image_path, 'rb') as input_img:
-                    while offset < filesize:
-                        count = min(SENDFILE_CHUNK_SIZE, filesize - offset)
-                        nbytes_out = os.sendfile(image_file.fileno(),
-                                                 input_img.fileno(),
-                                                 offset,
-                                                 count)
-                        offset += nbytes_out
+                return
+
+            # NOTE(dtantsur): starting with Python 3.8, copyfile() uses
+            # efficient copying (i.e. sendfile) under the hood.
+            shutil.copyfile(source_image_path, dest_image_path)
         except Exception as e:
             raise exception.ImageDownloadFailed(image_href=image_href,
                                                 reason=str(e))
diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py
index 197518e1a..297a1b4b9 100644
--- a/ironic/tests/unit/common/test_image_service.py
+++ b/ironic/tests/unit/common/test_image_service.py
@@ -10,7 +10,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import builtins
 import datetime
 from http import client as http_client
 import io
@@ -483,119 +482,55 @@ class FileImageServiceTestCase(base.TestCase):
                           'properties': {},
                           'no_cache': True}, result)
 
+    @mock.patch.object(shutil, 'copyfile', autospec=True)
     @mock.patch.object(os, 'link', autospec=True)
     @mock.patch.object(os, 'remove', autospec=True)
-    @mock.patch.object(os, 'access', return_value=True, autospec=True)
-    @mock.patch.object(os, 'stat', autospec=True)
     @mock.patch.object(image_service.FileImageService, 'validate_href',
                        autospec=True)
-    def test_download_hard_link(self, _validate_mock, stat_mock, access_mock,
-                                remove_mock, link_mock):
+    def test_download_hard_link(self, _validate_mock, remove_mock, link_mock,
+                                copy_mock):
         _validate_mock.return_value = self.href_path
-        stat_mock.return_value.st_dev = 'dev1'
         file_mock = mock.Mock(spec=io.BytesIO)
         file_mock.name = 'file'
         self.service.download(self.href, file_mock)
         _validate_mock.assert_called_once_with(mock.ANY, self.href)
-        self.assertEqual(2, stat_mock.call_count)
-        access_mock.assert_called_once_with(self.href_path, os.R_OK | os.W_OK)
         remove_mock.assert_called_once_with('file')
         link_mock.assert_called_once_with(self.href_path, 'file')
+        copy_mock.assert_not_called()
 
-    @mock.patch.object(os, 'sendfile', return_value=42, autospec=True)
-    @mock.patch.object(os.path, 'getsize', return_value=42, autospec=True)
-    @mock.patch.object(builtins, 'open', autospec=True)
-    @mock.patch.object(os, 'access', return_value=False, autospec=True)
-    @mock.patch.object(os, 'stat', autospec=True)
-    @mock.patch.object(image_service.FileImageService, 'validate_href',
-                       autospec=True)
-    def test_download_copy(self, _validate_mock, stat_mock, access_mock,
-                           open_mock, size_mock, copy_mock):
-        _validate_mock.return_value = self.href_path
-        stat_mock.return_value.st_dev = 'dev1'
-        file_mock = mock.MagicMock(spec=io.BytesIO)
-        file_mock.name = 'file'
-        input_mock = mock.MagicMock(spec=io.BytesIO)
-        open_mock.return_value = input_mock
-        self.service.download(self.href, file_mock)
-        _validate_mock.assert_called_once_with(mock.ANY, self.href)
-        self.assertEqual(2, stat_mock.call_count)
-        access_mock.assert_called_once_with(self.href_path, os.R_OK | os.W_OK)
-        copy_mock.assert_called_once_with(file_mock.fileno(),
-                                          input_mock.__enter__().fileno(),
-                                          0, 42)
-
-    @mock.patch.object(os, 'sendfile', autospec=True)
-    @mock.patch.object(os.path, 'getsize', return_value=42, autospec=True)
-    @mock.patch.object(builtins, 'open', autospec=True)
-    @mock.patch.object(os, 'access', return_value=False, autospec=True)
-    @mock.patch.object(os, 'stat', autospec=True)
+    @mock.patch.object(shutil, 'copyfile', autospec=True)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(os, 'remove', autospec=True)
     @mock.patch.object(image_service.FileImageService, 'validate_href',
                        autospec=True)
-    def test_download_copy_segmented(self, _validate_mock, stat_mock,
-                                     access_mock, open_mock, size_mock,
-                                     copy_mock):
-        # Fake a 3G + 1k image
-        chunk_size = image_service.SENDFILE_CHUNK_SIZE
-        fake_image_size = chunk_size * 3 + 1024
-        fake_chunk_seq = [chunk_size, chunk_size, chunk_size, 1024]
+    def test_download_copy(self, _validate_mock, remove_mock, link_mock,
+                           copy_mock):
         _validate_mock.return_value = self.href_path
-        stat_mock.return_value.st_dev = 'dev1'
+        link_mock.side_effect = PermissionError
         file_mock = mock.MagicMock(spec=io.BytesIO)
         file_mock.name = 'file'
-        input_mock = mock.MagicMock(spec=io.BytesIO)
-        open_mock.return_value = input_mock
-        size_mock.return_value = fake_image_size
-        copy_mock.side_effect = fake_chunk_seq
         self.service.download(self.href, file_mock)
         _validate_mock.assert_called_once_with(mock.ANY, self.href)
-        self.assertEqual(2, stat_mock.call_count)
-        access_mock.assert_called_once_with(self.href_path, os.R_OK | os.W_OK)
-        copy_calls = [mock.call(file_mock.fileno(),
-                                input_mock.__enter__().fileno(),
-                                chunk_size * i,
-                                fake_chunk_seq[i]) for i in range(4)]
-        copy_mock.assert_has_calls(copy_calls)
-        size_mock.assert_called_once_with(self.href_path)
-
-    @mock.patch.object(os, 'remove', side_effect=OSError, autospec=True)
-    @mock.patch.object(os, 'access', return_value=True, autospec=True)
-    @mock.patch.object(os, 'stat', autospec=True)
-    @mock.patch.object(image_service.FileImageService, 'validate_href',
-                       autospec=True)
-    def test_download_hard_link_fail(self, _validate_mock, stat_mock,
-                                     access_mock, remove_mock):
-        _validate_mock.return_value = self.href_path
-        stat_mock.return_value.st_dev = 'dev1'
-        file_mock = mock.MagicMock(spec=io.BytesIO)
-        file_mock.name = 'file'
-        self.assertRaises(exception.ImageDownloadFailed,
-                          self.service.download, self.href, file_mock)
-        _validate_mock.assert_called_once_with(mock.ANY, self.href)
-        self.assertEqual(2, stat_mock.call_count)
-        access_mock.assert_called_once_with(self.href_path, os.R_OK | os.W_OK)
+        link_mock.assert_called_once_with(self.href_path, 'file')
+        copy_mock.assert_called_once_with(self.href_path, 'file')
 
-    @mock.patch.object(os, 'sendfile', side_effect=OSError, autospec=True)
-    @mock.patch.object(os.path, 'getsize', return_value=42, autospec=True)
-    @mock.patch.object(builtins, 'open', autospec=True)
-    @mock.patch.object(os, 'access', return_value=False, autospec=True)
-    @mock.patch.object(os, 'stat', autospec=True)
+    @mock.patch.object(shutil, 'copyfile', autospec=True)
+    @mock.patch.object(os, 'link', autospec=True)
+    @mock.patch.object(os, 'remove', autospec=True)
     @mock.patch.object(image_service.FileImageService, 'validate_href',
                        autospec=True)
-    def test_download_copy_fail(self, _validate_mock, stat_mock, access_mock,
-                                open_mock, size_mock, copy_mock):
+    def test_download_copy_fail(self, _validate_mock, remove_mock, link_mock,
+                                copy_mock):
         _validate_mock.return_value = self.href_path
-        stat_mock.return_value.st_dev = 'dev1'
+        link_mock.side_effect = PermissionError
+        copy_mock.side_effect = PermissionError
         file_mock = mock.MagicMock(spec=io.BytesIO)
         file_mock.name = 'file'
-        input_mock = mock.MagicMock(spec=io.BytesIO)
-        open_mock.return_value = input_mock
         self.assertRaises(exception.ImageDownloadFailed,
                           self.service.download, self.href, file_mock)
         _validate_mock.assert_called_once_with(mock.ANY, self.href)
-        self.assertEqual(2, stat_mock.call_count)
-        access_mock.assert_called_once_with(self.href_path, os.R_OK | os.W_OK)
-        size_mock.assert_called_once_with(self.href_path)
+        link_mock.assert_called_once_with(self.href_path, 'file')
+        copy_mock.assert_called_once_with(self.href_path, 'file')
 
 
 class ServiceGetterTestCase(base.TestCase):
diff --git a/releasenotes/notes/cross-link-1ffd1a4958f14fd7.yaml b/releasenotes/notes/cross-link-1ffd1a4958f14fd7.yaml
new file mode 100644
index 000000000..caac13dd4
--- /dev/null
+++ b/releasenotes/notes/cross-link-1ffd1a4958f14fd7.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes ``Invalid cross-device link`` in some cases when using ``file://``
+    image URLs.