Merge "Fix --skip-existing and --skip-parents"

3 files changed, 102 insertions, 24 deletions

diff --git a/kolla/image/build.py b/kolla/image/build.py
index f66a7c076..b13ff5cab 100755
--- a/kolla/image/build.py
+++ b/kolla/image/build.py
@@ -1099,48 +1099,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 b7f5c30c0..1b633de4c 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 000000000..c3f427a2a
--- /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
+    <https://launchpad.net/bugs/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.