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

2 files changed, 91 insertions, 24 deletions

diff --git a/kolla/image/build.py b/kolla/image/build.py
index f7906dce5..e2382e1ba 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
-
-                    # 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
+                    image.status = STATUS_MATCHED
 
-                    # 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 5b3e981d6..7cdeab3eb 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