Merge "Always fall back from hard linking to copying files"
This commit is contained in:
Zuul
Gerrit Code Review
commit
8ef9db1570
@ -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…
x
Reference in New Issue
Block a user