From dfddcce3a5bc98cbd177c984fd9d0a8bab51d40c Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Mon, 16 Mar 2020 12:54:11 +0000 Subject: [PATCH] Fix --skip-existing and --skip-parents There are currently a few corner cases with the --skip-existing and --skip-parents flags. Examples: 1. kolla-build --type source --skip-parents monasca-grafana This does not try to build the monasca-grafana image. This appears to be an issue when the image is a grandchild. 2. kolla-build --type source --skip-parents This will build all images. Expect it to skip parents. 3. kolla-build --type source --skip-existing This will build all images. Expect it to skip existing images. The filter_images method does quite a lot, including handling whether images are buildable, regex/profile matching, and which images to skip. The matching and skipping parts are done in a single pass, which can lead to some weird effects due to dependencies between the images and their statuses. Also, skipping is only currently applied when there is a regex/profile filter. This change splits out the matching and skipping into two separate passes. In the first pass, we mark all buildable images that match the filter as matched. In the second, we iterate over matched images, applying the skip existing and skip parents rules. Change-Id: I2f895aea0cc59d808129e9fc636af0890196af33 Closes-Bug: #1867614 Related-Bug: #1810979 --- kolla/image/build.py | 43 ++++++----- kolla/tests/test_build.py | 72 ++++++++++++++++++- .../notes/fix-skips-d5cb9546110300ee.yaml | 11 +++ 3 files changed, 102 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/fix-skips-d5cb9546110300ee.yaml diff --git a/kolla/image/build.py b/kolla/image/build.py index f7906dce55..e2382e1ba6 100755 --- a/kolla/image/build.py +++ b/kolla/image/build.py @@ -1102,48 +1102,45 @@ class KollaWorker(object): # When we want to build a subset of images then filter_ part kicks in. # Otherwise we just mark everything buildable as matched for build. + # First, determine which buildable images match. if filter_: patterns = re.compile(r"|".join(filter_).join('()')) for image in self.images: # as we now list not buildable/skipped images we need to # process them otherwise list will contain also not requested # entries - if image.status == STATUS_MATCHED: + if image.status in (STATUS_MATCHED, STATUS_UNBUILDABLE): continue if re.search(patterns, image.name): - if image.status not in [STATUS_SKIPPED, - STATUS_UNBUILDABLE]: - image.status = STATUS_MATCHED + image.status = STATUS_MATCHED - # skip image if --skip-existing was given and image - # was already built - if (self.conf.skip_existing and image.in_docker_cache()): - image.status = STATUS_SKIPPED - - # handle image ancestors ancestor_image = image while (ancestor_image.parent is not None and - ancestor_image.parent.status not in - (STATUS_MATCHED, STATUS_SKIPPED)): + ancestor_image.parent.status != STATUS_MATCHED): ancestor_image = ancestor_image.parent - if self.conf.skip_parents: - ancestor_image.status = STATUS_SKIPPED - elif (self.conf.skip_existing and - ancestor_image.in_docker_cache()): - ancestor_image.status = STATUS_SKIPPED - else: - if ancestor_image.status != STATUS_UNBUILDABLE: - ancestor_image.status = STATUS_MATCHED - LOG.debug('Image %s matched regex', image.name) + # Parents of a buildable image must also be buildable. + ancestor_image.status = STATUS_MATCHED + LOG.debug('Image %s matched regex', image.name) else: - # we do not care if it is skipped or not as we did not - # request it image.status = STATUS_UNMATCHED else: for image in self.images: if image.status != STATUS_UNBUILDABLE: image.status = STATUS_MATCHED + # Next, mark any skipped images. + for image in self.images: + if image.status != STATUS_MATCHED: + continue + # Skip image if --skip-existing was given and image exists. + if (self.conf.skip_existing and image.in_docker_cache()): + LOG.debug('Skipping existing image %s', image.name) + image.status = STATUS_SKIPPED + # Skip image if --skip-parents was given and image has children. + elif self.conf.skip_parents and image.children: + LOG.debug('Skipping parent image %s', image.name) + image.status = STATUS_SKIPPED + def summary(self): """Walk the dictionary of images statuses and print results.""" # For debug we print the logs again if the image error'd. This is to diff --git a/kolla/tests/test_build.py b/kolla/tests/test_build.py index 5b3e981d6f..7cdeab3eb6 100644 --- a/kolla/tests/test_build.py +++ b/kolla/tests/test_build.py @@ -43,6 +43,10 @@ FAKE_IMAGE_CHILD_BUILT = build.Image( 'image-child-built', 'image-child-built:latest', '/fake/path5', parent_name='image-base', parent=FAKE_IMAGE, status=build.STATUS_BUILT) +FAKE_IMAGE_GRANDCHILD = build.Image( + 'image-grandchild', 'image-grandchild:latest', + '/fake/path6', parent_name='image-child', + parent=FAKE_IMAGE_CHILD, status=build.STATUS_MATCHED) class TasksTest(base.TestCase): @@ -434,14 +438,80 @@ class KollaWorkerTest(base.TestCase): if image.status == build.STATUS_MATCHED] def test_skip_parents(self): + self.conf.set_override('skip_parents', True) + kolla = build.KollaWorker(self.conf) + kolla.images = self.images[:2] + for i in kolla.images: + i.status = build.STATUS_UNPROCESSED + if i.parent: + i.parent.children.append(i) + kolla.filter_images() + + self.assertEqual(build.STATUS_MATCHED, kolla.images[1].status) + self.assertEqual(build.STATUS_SKIPPED, kolla.images[1].parent.status) + + def test_skip_parents_regex(self): self.conf.set_override('regex', ['image-child']) self.conf.set_override('skip_parents', True) kolla = build.KollaWorker(self.conf) - kolla.images = self.images + kolla.images = self.images[:2] + for i in kolla.images: + i.status = build.STATUS_UNPROCESSED + if i.parent: + i.parent.children.append(i) kolla.filter_images() + self.assertEqual(build.STATUS_MATCHED, kolla.images[1].status) self.assertEqual(build.STATUS_SKIPPED, kolla.images[1].parent.status) + def test_skip_parents_match_grandchildren(self): + self.conf.set_override('skip_parents', True) + kolla = build.KollaWorker(self.conf) + image_grandchild = FAKE_IMAGE_GRANDCHILD.copy() + image_grandchild.parent = self.images[1] + self.images[1].children.append(image_grandchild) + kolla.images = self.images[:2] + [image_grandchild] + for i in kolla.images: + i.status = build.STATUS_UNPROCESSED + if i.parent: + i.parent.children.append(i) + kolla.filter_images() + + self.assertEqual(build.STATUS_MATCHED, kolla.images[2].status) + self.assertEqual(build.STATUS_SKIPPED, kolla.images[2].parent.status) + self.assertEqual(build.STATUS_SKIPPED, kolla.images[1].parent.status) + + def test_skip_parents_match_grandchildren_regex(self): + self.conf.set_override('regex', ['image-grandchild']) + self.conf.set_override('skip_parents', True) + kolla = build.KollaWorker(self.conf) + image_grandchild = FAKE_IMAGE_GRANDCHILD.copy() + image_grandchild.parent = self.images[1] + self.images[1].children.append(image_grandchild) + kolla.images = self.images[:2] + [image_grandchild] + for i in kolla.images: + i.status = build.STATUS_UNPROCESSED + if i.parent: + i.parent.children.append(i) + kolla.filter_images() + + self.assertEqual(build.STATUS_MATCHED, kolla.images[2].status) + self.assertEqual(build.STATUS_SKIPPED, kolla.images[2].parent.status) + self.assertEqual(build.STATUS_SKIPPED, kolla.images[1].parent.status) + + @mock.patch.object(build.Image, 'in_docker_cache') + def test_skip_existing(self, mock_in_cache): + mock_in_cache.side_effect = [True, False] + self.conf.set_override('skip_existing', True) + kolla = build.KollaWorker(self.conf) + kolla.images = self.images[:2] + for i in kolla.images: + i.status = build.STATUS_UNPROCESSED + kolla.filter_images() + + self.assertEqual(build.STATUS_SKIPPED, kolla.images[0].status) + self.assertEqual(build.STATUS_MATCHED, kolla.images[1].status) + def test_without_profile(self): kolla = build.KollaWorker(self.conf) kolla.images = self.images diff --git a/releasenotes/notes/fix-skips-d5cb9546110300ee.yaml b/releasenotes/notes/fix-skips-d5cb9546110300ee.yaml new file mode 100644 index 0000000000..c3f427a2af --- /dev/null +++ b/releasenotes/notes/fix-skips-d5cb9546110300ee.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Fixes an issue with the ``--skip-existing`` and ``--skip-parents`` flags + which could cause images to not build. `LP#1867614 + `__. +upgrade: + - | + Changes the behaviour of the ``--skip-existing`` and ``--skip-parents`` + flags. Previously these were not applied if no regular expression or + profile argument was provided to ``kolla-build``, but now they are.