Skip unnecessary image uploads
This modifies the container image upload to avoid doing an upload when the destination image already exists and is identical to the source image. This is done by doing a skopeo inspect on the source and destination images and compares their digest hash. In RDO Cloud, a full upload takes around 8 minutes. With this change and an already populated local registry it takes less than 10 seconds. The extra 2 skopeo inspect calls per upload doesn't measurably impact the full upload time. This makes the upload fast enough to run within the main deploy workflow on every run (which will take more work because the upload yaml file needs to be stored in the plan). Change-Id: Ic9422b0ca168b4405cb1973f4e33c91b0f69f02c Closes-Bug: #1738668
This commit is contained in:
Steve Baker
parent
32b48af416
commit
18a1349a77
@ -149,7 +149,8 @@ class DockerImageUploader(ImageUploader):
|
||||
self.insecure_registries = set()
|
||||
|
||||
@staticmethod
|
||||
def upload_image(image_name, pull_source, push_destination):
|
||||
def upload_image(image_name, pull_source, push_destination,
|
||||
insecure_registries):
|
||||
LOG.info('imagename: %s' % image_name)
|
||||
dockerc = Client(base_url='unix://var/run/docker.sock', version='auto')
|
||||
if ':' in image_name:
|
||||
@ -163,13 +164,17 @@ class DockerImageUploader(ImageUploader):
|
||||
else:
|
||||
repo = image
|
||||
|
||||
DockerImageUploader._pull_retry(dockerc, repo, tag=tag)
|
||||
|
||||
full_image = repo + ':' + tag
|
||||
|
||||
new_repo = push_destination + '/' + repo.partition('/')[2]
|
||||
full_new_repo = new_repo + ':' + tag
|
||||
|
||||
if DockerImageUploader._images_match(full_image, full_new_repo,
|
||||
insecure_registries):
|
||||
LOG.info('Skipping upload for image %s' % image_name)
|
||||
return []
|
||||
|
||||
DockerImageUploader._pull_retry(dockerc, repo, tag=tag)
|
||||
|
||||
response = dockerc.tag(image=full_image, repository=new_repo,
|
||||
tag=tag, force=True)
|
||||
LOG.debug(response)
|
||||
@ -178,7 +183,7 @@ class DockerImageUploader(ImageUploader):
|
||||
tag=tag, stream=True)]
|
||||
LOG.debug(response)
|
||||
|
||||
LOG.info('Completed upload for docker image %s' % image_name)
|
||||
LOG.info('Completed upload for image %s' % image_name)
|
||||
return full_image, full_new_repo
|
||||
|
||||
@staticmethod
|
||||
@ -206,6 +211,31 @@ class DockerImageUploader(ImageUploader):
|
||||
time.sleep(3)
|
||||
LOG.warning('retrying pulling image: %s' % image)
|
||||
|
||||
@staticmethod
|
||||
def _images_match(image1, image2, insecure_registries):
|
||||
try:
|
||||
image1_digest = DockerImageUploader._image_digest(
|
||||
image1, insecure_registries)
|
||||
except Exception:
|
||||
return False
|
||||
try:
|
||||
image2_digest = DockerImageUploader._image_digest(
|
||||
image2, insecure_registries)
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
# missing digest, no way to know if they match
|
||||
if not image1_digest or not image2_digest:
|
||||
return False
|
||||
return image1_digest == image2_digest
|
||||
|
||||
@staticmethod
|
||||
def _image_digest(image, insecure_registries):
|
||||
image_url = DockerImageUploader._image_to_url(image)
|
||||
insecure = image_url.netloc in insecure_registries
|
||||
i = DockerImageUploader._inspect(image_url.geturl(), insecure)
|
||||
return i.get('Digest')
|
||||
|
||||
@staticmethod
|
||||
def _inspect(image, insecure=False):
|
||||
|
||||
@ -217,7 +247,8 @@ class DockerImageUploader(ImageUploader):
|
||||
|
||||
LOG.info('Running %s' % ' '.join(cmd))
|
||||
env = os.environ.copy()
|
||||
process = subprocess.Popen(cmd, env=env, stdout=subprocess.PIPE)
|
||||
process = subprocess.Popen(cmd, env=env, stdout=subprocess.PIPE,
|
||||
stderr=subprocess.PIPE)
|
||||
|
||||
out, err = process.communicate()
|
||||
if process.returncode != 0:
|
||||
@ -298,7 +329,14 @@ class DockerImageUploader(ImageUploader):
|
||||
LOG.warning(e)
|
||||
|
||||
def add_upload_task(self, image_name, pull_source, push_destination):
|
||||
self.upload_tasks.append((image_name, pull_source, push_destination))
|
||||
# prime self.insecure_registries
|
||||
if pull_source:
|
||||
self.is_insecure_registry(self._image_to_url(pull_source).netloc)
|
||||
else:
|
||||
self.is_insecure_registry(self._image_to_url(image_name).netloc)
|
||||
self.is_insecure_registry(self._image_to_url(push_destination).netloc)
|
||||
self.upload_tasks.append((image_name, pull_source, push_destination,
|
||||
self.insecure_registries))
|
||||
|
||||
def run_tasks(self):
|
||||
if not self.upload_tasks:
|
||||
|
||||
@ -46,11 +46,17 @@ class TestImageUploadManager(base.TestCase):
|
||||
|
||||
@mock.patch('tripleo_common.image.base.open',
|
||||
mock.mock_open(read_data=filedata), create=True)
|
||||
@mock.patch('tripleo_common.image.image_uploader.'
|
||||
'DockerImageUploader.is_insecure_registry',
|
||||
return_value=True)
|
||||
@mock.patch('tripleo_common.image.image_uploader.'
|
||||
'DockerImageUploader._images_match',
|
||||
return_value=False)
|
||||
@mock.patch('os.path.isfile', return_value=True)
|
||||
@mock.patch('fcntl.ioctl', side_effect=Exception)
|
||||
@mock.patch('tripleo_common.image.image_uploader.Client')
|
||||
def test_file_parsing(self, mockdocker, mockioctl, mockpath):
|
||||
print(filedata)
|
||||
def test_file_parsing(self, mockdocker, mockioctl, mockpath,
|
||||
mock_images_match, mock_is_insecure):
|
||||
manager = image_uploader.ImageUploadManager(self.filelist, debug=True)
|
||||
parsed_data = manager.upload()
|
||||
mockpath(self.filelist[0])
|
||||
@ -111,7 +117,23 @@ class TestDockerImageUploader(base.TestCase):
|
||||
super(TestDockerImageUploader, self).tearDown()
|
||||
self.patcher.stop()
|
||||
|
||||
def test_upload_image(self):
|
||||
@mock.patch('subprocess.Popen')
|
||||
def test_upload_image(self, mock_popen):
|
||||
result1 = {
|
||||
'Digest': 'a'
|
||||
}
|
||||
result2 = {
|
||||
'Digest': 'b'
|
||||
}
|
||||
mock_process = mock.Mock()
|
||||
mock_process.communicate.side_effect = [
|
||||
(json.dumps(result1), ''),
|
||||
(json.dumps(result2), ''),
|
||||
]
|
||||
|
||||
mock_process.returncode = 0
|
||||
mock_popen.return_value = mock_process
|
||||
|
||||
image = 'docker.io/tripleoupstream/heat-docker-agents-centos'
|
||||
tag = 'latest'
|
||||
push_destination = 'localhost:8787'
|
||||
@ -119,7 +141,8 @@ class TestDockerImageUploader(base.TestCase):
|
||||
|
||||
self.uploader.upload_image(image + ':' + tag,
|
||||
None,
|
||||
push_destination)
|
||||
push_destination,
|
||||
set())
|
||||
|
||||
self.dockermock.assert_called_once_with(
|
||||
base_url='unix://var/run/docker.sock', version='auto')
|
||||
@ -130,11 +153,12 @@ class TestDockerImageUploader(base.TestCase):
|
||||
image=image + ':' + tag,
|
||||
repository=push_image,
|
||||
tag=tag, force=True)
|
||||
self.dockermock.return_value.push(
|
||||
self.dockermock.return_value.push.assert_called_once_with(
|
||||
push_image,
|
||||
tag=tag, stream=True)
|
||||
|
||||
def test_upload_image_missing_tag(self):
|
||||
@mock.patch('subprocess.Popen')
|
||||
def test_upload_image_missing_tag(self, mock_popen):
|
||||
image = 'docker.io/tripleoupstream/heat-docker-agents-centos'
|
||||
expected_tag = 'latest'
|
||||
push_destination = 'localhost:8787'
|
||||
@ -142,7 +166,8 @@ class TestDockerImageUploader(base.TestCase):
|
||||
|
||||
self.uploader.upload_image(image,
|
||||
None,
|
||||
push_destination)
|
||||
push_destination,
|
||||
set())
|
||||
|
||||
self.dockermock.assert_called_once_with(
|
||||
base_url='unix://var/run/docker.sock', version='auto')
|
||||
@ -153,10 +178,36 @@ class TestDockerImageUploader(base.TestCase):
|
||||
image=image + ':' + expected_tag,
|
||||
repository=push_image,
|
||||
tag=expected_tag, force=True)
|
||||
self.dockermock.return_value.push(
|
||||
self.dockermock.return_value.push.assert_called_once_with(
|
||||
push_image,
|
||||
tag=expected_tag, stream=True)
|
||||
|
||||
@mock.patch('subprocess.Popen')
|
||||
def test_upload_image_existing(self, mock_popen):
|
||||
result = {
|
||||
'Digest': 'a'
|
||||
}
|
||||
mock_process = mock.Mock()
|
||||
mock_process.communicate.return_value = (json.dumps(result), '')
|
||||
mock_process.returncode = 0
|
||||
mock_popen.return_value = mock_process
|
||||
image = 'docker.io/tripleoupstream/heat-docker-agents-centos'
|
||||
tag = 'latest'
|
||||
push_destination = 'localhost:8787'
|
||||
|
||||
self.uploader.upload_image(image + ':' + tag,
|
||||
None,
|
||||
push_destination,
|
||||
set())
|
||||
|
||||
self.dockermock.assert_called_once_with(
|
||||
base_url='unix://var/run/docker.sock', version='auto')
|
||||
|
||||
# both digests are the same, no pull/push
|
||||
self.dockermock.return_value.pull.assert_not_called()
|
||||
self.dockermock.return_value.tag.assert_not_called()
|
||||
self.dockermock.return_value.push.assert_not_called()
|
||||
|
||||
@mock.patch('requests.get')
|
||||
def test_is_insecure_registry_known(self, mock_get):
|
||||
self.assertFalse(
|
||||
@ -351,3 +402,24 @@ class TestDockerImageUploader(base.TestCase):
|
||||
dockerc.pull.assert_has_calls([
|
||||
mock.call(image, tag=None, stream=True)
|
||||
])
|
||||
|
||||
@mock.patch('tripleo_common.image.image_uploader.'
|
||||
'DockerImageUploader._inspect')
|
||||
def test_images_match(self, mock_inspect):
|
||||
mock_inspect.side_effect = [{'Digest': 'a'}, {'Digest': 'b'}]
|
||||
self.assertFalse(self.uploader._images_match('foo', 'bar', set()))
|
||||
|
||||
mock_inspect.side_effect = [{'Digest': 'a'}, {'Digest': 'a'}]
|
||||
self.assertTrue(self.uploader._images_match('foo', 'bar', set()))
|
||||
|
||||
mock_inspect.side_effect = [{}, {'Digest': 'b'}]
|
||||
self.assertFalse(self.uploader._images_match('foo', 'bar', set()))
|
||||
|
||||
mock_inspect.side_effect = [{'Digest': 'a'}, {}]
|
||||
self.assertFalse(self.uploader._images_match('foo', 'bar', set()))
|
||||
|
||||
mock_inspect.side_effect = [None, None]
|
||||
self.assertFalse(self.uploader._images_match('foo', 'bar', set()))
|
||||
|
||||
mock_inspect.side_effect = ImageUploaderException()
|
||||
self.assertFalse(self.uploader._images_match('foo', 'bar', set()))
|
||||
|
||||
Loading…
Reference in New Issue
Block a user