From c6bb04041867c242629c9becd83d47f2722b2432 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= <lsedlar@redhat.com>
Date: Wed, 11 Apr 2018 09:18:59 +0200
Subject: [PATCH 1/3] Revert "Move ostree phase and pipelines for running
 phases"

This reverts commit 660c04368ba1abed310f121d01f0fa029eea5f11.
---
 bin/pungi-koji           |  54 +++++++++-------
 doc/_static/phases.svg   | 162 +++++++++++++++++++++++++++++++++++------------
 pungi/phases/__init__.py |  19 +++++-
 pungi/phases/weaver.py   |  72 ---------------------
 tests/test_phase_base.py | 129 ++++++++-----------------------------
 5 files changed, 198 insertions(+), 238 deletions(-)
 delete mode 100644 pungi/phases/weaver.py

diff --git a/bin/pungi-koji b/bin/pungi-koji
index b3b49de5..e797ce86 100755
--- a/bin/pungi-koji
+++ b/bin/pungi-koji
@@ -365,27 +365,41 @@ def run_compose(compose, create_latest_link=True, latest_link_status=None):
             # Store the password
             compose.conf["signing_key_password"] = signing_key_password
 
+    # INIT phase
     init_phase.start()
     init_phase.stop()
 
+    # PKGSET phase
     pkgset_phase.start()
     pkgset_phase.stop()
 
-    # WEAVER phase - launches other phases which can safely run in parallel
-    essentials_schema = (
-        buildinstall_phase,
-        (gather_phase, extrafiles_phase, createrepo_phase),
-        (ostree_phase, ostree_installer_phase),
-    )
-    essentials_phase = pungi.phases.WeaverPhase(compose, essentials_schema)
-    essentials_phase.start()
-    essentials_phase.stop()
+    # BUILDINSTALL phase - start, we can run gathering, extra files and
+    # createrepo while buildinstall is in progress.
+    buildinstall_phase.start()
+
+    # If any of the following three phases fail, we must ensure that
+    # buildinstall is stopped. Otherwise the whole process will hang.
+    try:
+        gather_phase.start()
+        gather_phase.stop()
+
+        extrafiles_phase.start()
+        extrafiles_phase.stop()
+
+        createrepo_phase.start()
+        createrepo_phase.stop()
+
+    finally:
+        buildinstall_phase.stop()
 
     if not buildinstall_phase.skip():
         buildinstall_phase.copy_files()
 
-    productimg_phase.start()
-    productimg_phase.stop()
+    ostree_phase.start()
+    ostree_phase.stop()
+
+    pungi.phases.run_all([productimg_phase,
+                          ostree_installer_phase])
 
     # write treeinfo before ISOs are created
     for variant in compose.get_variants():
@@ -401,17 +415,12 @@ def run_compose(compose, create_latest_link=True, latest_link_status=None):
             pungi.metadata.write_media_repo(compose, arch, variant, timestamp)
 
     # Start all phases for image artifacts
-    compose_images_schema = (
-        createiso_phase,
-        extra_isos_phase,
-        liveimages_phase,
-        image_build_phase,
-        livemedia_phase,
-        osbs_phase,
-    )
-    compose_images_phase = pungi.phases.WeaverPhase(compose, compose_images_schema)
-    compose_images_phase.start()
-    compose_images_phase.stop()
+    pungi.phases.run_all([createiso_phase,
+                          extra_isos_phase,
+                          liveimages_phase,
+                          image_build_phase,
+                          livemedia_phase,
+                          osbs_phase])
 
     image_checksum_phase.start()
     image_checksum_phase.stop()
@@ -420,6 +429,7 @@ def run_compose(compose, create_latest_link=True, latest_link_status=None):
     compose.im.dump(compose.paths.compose.metadata("images.json"))
     osbs_phase.dump_metadata()
 
+    # TEST phase
     test_phase.start()
     test_phase.stop()
 
diff --git a/doc/_static/phases.svg b/doc/_static/phases.svg
index 7bbc5e1f..611eb06b 100644
--- a/doc/_static/phases.svg
+++ b/doc/_static/phases.svg
@@ -104,7 +104,87 @@
            style="font-size:13.14787769px;line-height:1.25">Pkgset</tspan></text>
     </g>
     <g
-       transform="translate(3.704976,-80.47309)"
+       transform="translate(-4.582059,-80.47309)"
+       id="g3416">
+      <rect
+         style="fill:#fcaf3e;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         id="rect3342"
+         width="26.295755"
+         height="231.47725"
+         x="953.49097"
+         y="108.04571"
+         transform="matrix(0,1,1,0,0,0)" />
+      <text
+         xml:space="preserve"
+         style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;letter-spacing:0px;word-spacing:0px;fill:#000000;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         x="110.35005"
+         y="971.54041"
+         id="text3364"><tspan
+           sodipodi:role="line"
+           id="tspan3366"
+           x="110.35005"
+           y="971.54041"
+           style="font-size:13.14787769px;line-height:1.25">Buildinstall</tspan></text>
+      <rect
+         style="fill:#729fcf;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         id="rect3344"
+         width="26.295755"
+         height="54.197887"
+         x="989.65247"
+         y="112.96759"
+         transform="matrix(0,1,1,0,0,0)" />
+      <text
+         xml:space="preserve"
+         style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;letter-spacing:0px;word-spacing:0px;fill:#000000;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         x="115.82405"
+         y="1007.7019"
+         id="text3368"><tspan
+           sodipodi:role="line"
+           id="tspan3370"
+           x="115.82405"
+           y="1007.7019"
+           style="font-size:13.14787769px;line-height:1.25">Gather</tspan></text>
+      <rect
+         style="fill:#ad7fa8;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         id="rect3346"
+         width="26.295755"
+         height="72.729973"
+         x="989.65247"
+         y="172.61172"
+         transform="matrix(0,1,1,0,0,0)" />
+      <text
+         xml:space="preserve"
+         style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;letter-spacing:0px;word-spacing:0px;fill:#000000;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         x="174.91608"
+         y="1007.7019"
+         id="text3372"><tspan
+           sodipodi:role="line"
+           id="tspan3374"
+           x="174.91608"
+           y="1007.7019"
+           style="font-size:13.14787769px;line-height:1.25">ExtraFiles</tspan></text>
+      <rect
+         style="fill:#e9b96e;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         id="rect3348"
+         width="26.295755"
+         height="78.636055"
+         x="989.65247"
+         y="250.78795"
+         transform="matrix(0,1,1,0,0,0)" />
+      <text
+         xml:space="preserve"
+         style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;letter-spacing:0px;word-spacing:0px;fill:#000000;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         x="253.64439"
+         y="1006.312"
+         id="text3376"><tspan
+           sodipodi:role="line"
+           id="tspan3378"
+           x="253.64439"
+           y="1006.312"
+           style="font-size:13.14787769px;line-height:1.25">Createrepo</tspan></text>
+    </g>
+    <g
+       transform="translate(63.81395,-80.47309)"
        id="g3446">
       <rect
          y="554.10059"
@@ -127,7 +207,7 @@
            style="font-size:13.14749908px;line-height:1.25">ImageChecksum</tspan></text>
     </g>
     <g
-       transform="translate(125.75393,-80.47309)"
+       transform="translate(185.8629,-80.47309)"
        id="g3398">
       <rect
          y="553.98242"
@@ -150,14 +230,15 @@
            style="font-size:13.14787769px;line-height:1.25">Test</tspan></text>
     </g>
     <g
-       id="g3720">
+       transform="translate(2.318656,-80.47309)"
+       id="g3406">
       <rect
          style="fill:#fce94f;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
          id="rect3336"
          width="26.295755"
          height="39.669899"
-         x="873.01788"
-         y="2.3186533"
+         x="953.49097"
+         y="-2.7716319e-06"
          transform="matrix(0,1,1,0,0,0)" />
       <text
          xml:space="preserve"
@@ -167,8 +248,8 @@
          id="text3356"><tspan
            sodipodi:role="line"
            id="tspan3358"
-           x="6.2600794"
-           y="891.1604"
+           x="1.1060941"
+           y="971.63348"
            style="font-size:13.14787769px;line-height:1.25">Init</tspan></text>
     </g>
     <path
@@ -177,40 +258,41 @@
        d="M 100.90864,859.8891 H 654.22706"
        style="fill:none;fill-rule:evenodd;stroke:#000000;stroke-width:1.17466855px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1;marker-end:url(#Arrow1Lend)" />
     <g
-       id="g241"
-       transform="translate(-60.108974,42.1407)">
+       transform="translate(86.469501,49.471116)"
+       id="g3408">
       <rect
-         style="fill:#a40000;fill-rule:evenodd;stroke:none;stroke-width:1.10477591px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
-         id="rect3350"
+         style="fill:#729fcf;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         id="rect3350-3"
          width="26.295755"
-         height="101.73411"
-         x="830.8772"
-         y="400.8551"
+         height="53.653927"
+         x="823.54675"
+         y="254.60153"
          transform="matrix(0,1,1,0,0,0)" />
       <text
          xml:space="preserve"
-         style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;letter-spacing:0px;word-spacing:0px;fill:#ffffff;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
-         x="403.15945"
-         y="847.65234"
-         id="text3380"><tspan
+         style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;text-align:start;letter-spacing:0px;word-spacing:0px;writing-mode:lr-tb;text-anchor:start;fill:#ffffff;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
+         x="256.90588"
+         y="840.3219"
+         id="text3380-2"><tspan
+           y="840.3219"
+           x="256.90588"
            sodipodi:role="line"
-           id="tspan3382"
-           x="403.15945"
-           y="847.65234"
-           style="font-size:13.14787769px;line-height:1.25">Productimg</tspan></text>
+           id="tspan3406"
+           style="font-size:13.14787769px;line-height:1.25">OSTree</tspan></text>
     </g>
     <g
-       id="g3668">
+       id="g251"
+       transform="translate(0,42.1407)">
       <g
-         id="g3663">
+         id="g241">
         <rect
            transform="matrix(0,1,1,0,0,0)"
-           y="103.46365"
-           x="873.01788"
-           height="231.47725"
+           y="400.8551"
+           x="830.8772"
+           height="101.73411"
            width="26.295755"
-           id="rect3342"
-           style="fill:#fcaf3e;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1" />
+           id="rect3350"
+           style="fill:#a40000;fill-rule:evenodd;stroke:none;stroke-width:1.10477591px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1" />
         <text
            id="text3364"
            y="891.06732"
@@ -287,22 +369,22 @@
            style="font-style:normal;font-weight:normal;line-height:0%;font-family:sans-serif;letter-spacing:0px;word-spacing:0px;fill:#000000;fill-opacity:1;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1"
            xml:space="preserve"><tspan
              style="font-size:13.14787769px;line-height:1.25"
-             y="921.86945"
-             x="243.95874"
-             id="tspan3378"
-             sodipodi:role="line">Createrepo</tspan></text>
+             y="847.65234"
+             x="403.15945"
+             id="tspan3382"
+             sodipodi:role="line">Productimg</tspan></text>
       </g>
       <g
-         id="g3408"
-         transform="translate(-150.564,114.11662)">
+         transform="translate(-89.482556,-154.87768)"
+         id="g288">
         <rect
            transform="matrix(0,1,1,0,0,0)"
-           y="254.60153"
-           x="823.54675"
-           height="53.653927"
+           y="490.33765"
+           x="1022.637"
+           height="101.85102"
            width="26.295755"
-           id="rect3350-3"
-           style="fill:#729fcf;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1" />
+           id="rect3428"
+           style="fill:#fcaf3e;fill-rule:evenodd;stroke:none;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1" />
         <text
            id="text3380-2"
            y="840.3219"
diff --git a/pungi/phases/__init__.py b/pungi/phases/__init__.py
index 0ed3e564..77720f80 100644
--- a/pungi/phases/__init__.py
+++ b/pungi/phases/__init__.py
@@ -17,7 +17,6 @@ import sys
 
 # phases in runtime order
 from .init import InitPhase  # noqa
-from .weaver import WeaverPhase  # noqa
 from .pkgset import PkgsetPhase  # noqa
 from .gather import GatherPhase  # noqa
 from .createrepo import CreaterepoPhase  # noqa
@@ -39,3 +38,21 @@ from .phases_metadata import gather_phases_metadata  # noqa
 
 this_module = sys.modules[__name__]
 PHASES_NAMES = gather_phases_metadata(this_module)
+
+
+def run_all(phases):
+    """Start and stop all given phases and make them run in parallel.
+
+    This function makes sure that even if one of the phases fails and raises an
+    exception, all the phases will still be correctly stopped.
+
+    If multiple phases fail, a exception from one of them will be raised, but
+    there are no guarantees which one it will be.
+    """
+    if not phases:
+        return
+    phases[0].start()
+    try:
+        run_all(phases[1:])
+    finally:
+        phases[0].stop()
diff --git a/pungi/phases/weaver.py b/pungi/phases/weaver.py
deleted file mode 100644
index e5dc6ca2..00000000
--- a/pungi/phases/weaver.py
+++ /dev/null
@@ -1,72 +0,0 @@
-# -*- coding: utf-8 -*-
-
-from kobo import shortcuts
-from kobo.threads import ThreadPool, WorkerThread
-
-
-class WeaverPhase(object):
-    """
-    Special "phase" that manages other phases' run.
-    It needs input-running schema where particular phases are composed
-    sequentially and in parallel as well. A Sequential set of phases
-    is named "pipeline".
-    If any of the phases fail, we must ensure that others will stop correctly.
-    Otherwise the whole process will hang.
-
-    :param compose: it is needed for logging
-    :param phases_schema: two-dimensional array of phases. Top dimension
-        denotes particular pipelines. Second dimension contains phases.
-    """
-    name = "weaver"
-
-    def __init__(self, compose, phases_schema):
-        self.msg = "---------- PHASE: %s ----------" % self.name.upper()
-        self.compose = compose
-        self.finished = False
-        self.pool = ThreadPool(logger=self.compose._logger)
-        if not phases_schema:
-            msg = "No running schema was set for WeaverPhase"
-            self.pool.log_error(msg)
-            raise ValueError(msg)
-        self._phases_schema = phases_schema
-
-    def start(self):
-        if self.finished:
-            msg = "Phase '%s' has already finished and can not be started twice" % self.name
-            self.pool.log_error(msg)
-            raise RuntimeError(msg)
-
-        self.compose.log_info("[BEGIN] %s" % self.msg)
-        self.run()
-
-    def run(self):
-        for pipeline in shortcuts.force_list(self._phases_schema):
-            self.pool.add(PipelineThread(self.pool))
-            self.pool.queue_put(shortcuts.force_list(pipeline))
-
-        self.pool.start()
-
-    def stop(self):
-        if self.finished:
-            return
-        if hasattr(self, "pool"):
-            self.pool.stop()
-        self.finished = True
-        self.compose.log_info("[DONE ] %s" % self.msg)
-
-
-class PipelineThread(WorkerThread):
-    """
-    Launches phases in pipeline sequentially
-    """
-    def process(self, item, num):
-        pipeline = shortcuts.force_list(item)
-        phases_names = ", ".join(phase.name for phase in pipeline)
-        msg = "Running pipeline (%d/%d). Phases: %s" % (num, self.pool.queue_total, phases_names)
-        self.pool.log_info("[BEGIN] %s" % (msg,))
-
-        for phase in pipeline:
-            phase.start()
-            phase.stop()
-
-        self.pool.log_info("[DONE ] %s" % (msg,))
diff --git a/tests/test_phase_base.py b/tests/test_phase_base.py
index ba80afe3..a8b9469a 100644
--- a/tests/test_phase_base.py
+++ b/tests/test_phase_base.py
@@ -7,13 +7,11 @@ try:
 except ImportError:
     import unittest
 import os
-import random
 import sys
-import time
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
-from pungi.phases import base, weaver
+from pungi.phases import run_all, base
 from tests.helpers import DummyCompose, PungiTestCase, boom
 
 
@@ -74,124 +72,49 @@ class ImageConfigMixinTestCase(PungiTestCase):
         self.assertEqual(resolve_git_url.num, 3, 'Resolver was not called three times')
 
 
-class TestWeaver(unittest.TestCase):
-    def setUp(self):
-        # prepare 6 phase mock-objects.
-        for test_phase_number in range(1, 7):
-            # This is equvalent to:
-            # self.pX = mock.Mock()
-            # self.pX.name = "phase X"
-            # self.pX.start = default_method
-            test_phase_name = "p" + repr(test_phase_number)
-            tmp = mock.Mock()
-            tmp.name = "phase %d" % test_phase_number
-            tmp.start.side_effect = self.method_regular
-            setattr(self, test_phase_name, tmp)
-
-        self.compose = DummyCompose(None, {})
-
-    def method_regular(self):
-        """
-        It only have to cause some delay (tens of miliseconds).
-        Delay is needed for threads that has enough time to start.
-        """
-        multiplier = random.sample(range(1, 10), 1)
-        time.sleep(multiplier[0] * 0.01)
-
-    def method_with_exception(self):
-        self.method_regular()  # make some delay
-        boom()  # throw exception
-
+class TestRunAll(unittest.TestCase):
     def assertFinalized(self, p):
         self.assertEqual(p.mock_calls, [mock.call.start(), mock.call.stop()])
 
-    def assertInterrupted(self, p):
-        self.assertEqual(p.mock_calls, [mock.call.start()])
-
-    def assertMissed(self, p):
-        self.assertEqual(p.mock_calls, [])
-
-    def test_parallel(self):
-        phases_schema = (self.p1, self.p2)
-        weaver_phase = weaver.WeaverPhase(self.compose, phases_schema)
-        weaver_phase.start()
-        weaver_phase.stop()
-
-        self.assertFinalized(self.p1)
-        self.assertFinalized(self.p2)
-
-    def test_pipeline(self):
-        phases_schema = ((self.p1, self.p2),)
-        weaver_phase = weaver.WeaverPhase(self.compose, phases_schema)
-        weaver_phase.start()
-        weaver_phase.stop()
+    def test_calls_stop(self):
+        p1 = mock.Mock()
+        p2 = mock.Mock()
 
-        self.assertFinalized(self.p1)
-        self.assertFinalized(self.p2)
+        run_all([p1, p2])
 
-    def test_stop_on_failure(self):
-        self.p2.start.side_effect = self.method_with_exception
+        self.assertFinalized(p1)
+        self.assertFinalized(p2)
 
-        phases_schema = ((self.p1, self.p2, self.p3),)  # one pipeline
-        weaver_phase = weaver.WeaverPhase(self.compose, phases_schema)
-        with self.assertRaises(Exception) as ctx:
-            weaver_phase.start()
-            weaver_phase.stop()
-
-        self.assertEqual('BOOM', str(ctx.exception))
-        self.assertFinalized(self.p1)
-        self.assertInterrupted(self.p2)
-        self.assertMissed(self.p3)
+    def test_calls_stop_on_failure(self):
+        p1 = mock.Mock()
+        p2 = mock.Mock()
+        p3 = mock.Mock()
 
-    def test_parallel_stop_on_failure(self):
-        self.p2.start.side_effect = self.method_with_exception
+        p2.stop.side_effect = boom
 
-        phases_schema = (self.p1, self.p2, self.p3)  # one pipeline
-        weaver_phase = weaver.WeaverPhase(self.compose, phases_schema)
         with self.assertRaises(Exception) as ctx:
-            weaver_phase.start()
-            weaver_phase.stop()
+            run_all([p1, p2, p3])
 
         self.assertEqual('BOOM', str(ctx.exception))
-        self.assertFinalized(self.p1)
-        self.assertInterrupted(self.p2)
-        self.assertFinalized(self.p3)
+        self.assertFinalized(p1)
+        self.assertFinalized(p2)
+        self.assertFinalized(p3)
 
     def test_multiple_fail(self):
-        self.p2.start.side_effect = self.method_with_exception
-        self.p3.start.side_effect = self.method_with_exception
+        p1 = mock.Mock(name='p1')
+        p2 = mock.Mock(name='p2')
+        p3 = mock.Mock(name='p3')
 
-        phases_schema = ((self.p1, self.p2, self.p3),)  # one pipeline
-        weaver_phase = weaver.WeaverPhase(self.compose, phases_schema)
-        with self.assertRaises(Exception) as ctx:
-            weaver_phase.start()
-            weaver_phase.stop()
+        p2.stop.side_effect = boom
+        p3.stop.side_effect = boom
 
-        self.assertEqual('BOOM', str(ctx.exception))
-        self.assertFinalized(self.p1)
-        self.assertInterrupted(self.p2)
-        self.assertMissed(self.p3)
-
-    def test_multi_pipeline(self):
-        self.p2.start.side_effect = self.method_with_exception
-        phases_schema = (
-            self.p1,
-            (self.p2, self.p3, self.p4),
-            (self.p5, self.p6),
-        )
-
-        weaver_phase = weaver.WeaverPhase(self.compose, phases_schema)
         with self.assertRaises(Exception) as ctx:
-            weaver_phase.start()
-            weaver_phase.stop()
+            run_all([p1, p2, p3])
 
         self.assertEqual('BOOM', str(ctx.exception))
-        self.assertFinalized(self.p1)
-        self.assertInterrupted(self.p2)
-        self.assertMissed(self.p3)
-        self.assertMissed(self.p4)
-        self.assertFinalized(self.p5)
-        self.assertFinalized(self.p6)
+        self.assertFinalized(p1)
+        self.assertFinalized(p2)
+        self.assertFinalized(p3)
 
 
 if __name__ == "__main__":
-- 
2.14.3