Always fall back from hard linking to copying files
The current check is insufficient: it passes for Kubernetes shared volumes, although hard-linking between them is not possible. This patch changes the approach to trying a hard link and falling back to copyfile instead. The patch relies on optimizations in Python 3.8 and thus should not be backported beyond the Zed series to avoid performance regression. Change-Id: I929944685b3ac61b2f63d2549198a2d8a1c8fe35
This commit is contained in:
Dmitry Tantsur
parent
abbd859b76
commit
59c6ad96ce
@ -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))
|
||||
|
||||
@ -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(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(self, _validate_mock, stat_mock, access_mock,
|
||||
open_mock, size_mock, copy_mock):
|
||||
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
|
||||
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)
|
||||
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', 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_fail(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.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'
|
||||
link_mock.side_effect = PermissionError
|
||||
copy_mock.side_effect = PermissionError
|
||||
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)
|
||||
|
||||
@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(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):
|
||||
_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.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):
|
||||
|
||||
5
releasenotes/notes/cross-link-1ffd1a4958f14fd7.yaml
Normal file
5
releasenotes/notes/cross-link-1ffd1a4958f14fd7.yaml
Normal file
@ -0,0 +1,5 @@
|
||||
---
|
||||
fixes:
|
||||
- |
|
||||
Fixes ``Invalid cross-device link`` in some cases when using ``file://``
|
||||
image URLs.
|
||||
Loading…
Reference in New Issue
Block a user