[live-images] Fix path processing

* Add option to keep filenames generated by Koji.
 * Put results of spin-appliance into image dir
 * On failure images are no longer deleted.

Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

doc/configuration.rst

@@ -689,6 +689,12 @@ Live Images Settings
         is ``appliance`` corresponding to ``koji spin-appliance``.
       * ``sign`` (*bool*) -- only RPM-wrapped images can be signed
 
+**live_images_no_rename**
+    (*bool*) -- When set to ``True``, filenames generated by Koji will be used.
+    When ``False``, filenames will be generated based on ``image_name_format``
+    configuration option.
+
+
 Live Media Settings
 ===================
 

pungi/phases/live_images.py

@@ -66,6 +66,11 @@ class LiveImagesPhase(PhaseBase):
             "expected_types": [str],
             "optional": True,
         },
+        {
+            "name": "live_images_no_rename",
+            "expected_types": [bool],
+            "optional": True,
+        }
     )
 
     def __init__(self, compose):
@@ -116,21 +121,35 @@ class LiveImagesPhase(PhaseBase):
         for variant in self.compose.variants.values():
             for arch in variant.arches + ["src"]:
                 for data in get_arch_variant_data(self.compose.conf, "live_images", arch, variant):
-                    iso_dir = self.compose.paths.compose.iso_dir(arch, variant, symlink_to=symlink_isos_to)
+                    type = data.get('type', 'live')
-                    if not iso_dir:
+
+                    if type == 'live':
+                        dest_dir = self.compose.paths.compose.iso_dir(arch, variant, symlink_to=symlink_isos_to)
+                    elif type == 'appliance':
+                        dest_dir = self.compose.paths.compose.image_dir(variant, symlink_to=symlink_isos_to)
+                        dest_dir = dest_dir % {'arch': arch}
+                    else:
+                        raise RuntimeError('Unknown live image type %s' % type)
+                    if not dest_dir:
                         continue
 
                     cmd = {
-                        "name": None,
+                        "name": data.get("name", None),
-                        "version": None,
+                        "version": data.get("version", None),
-                        "iso_path": None,
+                        "release": self._get_release(data),
-                        "wrapped_rpms_path": iso_dir,
+                        "dest_dir": dest_dir,
                         "build_arch": arch,
                         "ks_file": data['kickstart'],
                         "ksurl": None,
-                        "specfile": None,
+                        # Used for images wrapped in RPM
-                        "scratch": False,
+                        "specfile": data.get("specfile", None),
+                        # Scratch (only taken in consideration if specfile
+                        # specified) For images wrapped in rpm is scratch
+                        # disabled by default For other images is scratch
+                        # always on
+                        "scratch": data.get("scratch", False),
                         "sign": False,
+                        "type": type,
                         "label": "",  # currently not used
                     }
 
@@ -139,42 +158,11 @@ class LiveImagesPhase(PhaseBase):
 
                     cmd["repos"] = self._get_repos(arch, variant, data)
 
-                    # Explicit name and version
-                    cmd["name"] = data.get("name", None)
-                    cmd["version"] = data.get("version", None)
-
-                    cmd['type'] = data.get('type', 'live')
-                    cmd['release'] = self._get_release(data)
-
-                    # Specfile (for images wrapped in rpm)
-                    cmd["specfile"] = data.get("specfile", None)
-
-                    # Scratch (only taken in consideration if specfile specified)
-                    # For images wrapped in rpm is scratch disabled by default
-                    # For other images is scratch always on
-                    cmd["scratch"] = data.get("scratch", False)
-
                     # Signing of the rpm wrapped image
                     if not cmd["scratch"] and data.get("sign"):
                         cmd["sign"] = True
 
-                    format = "%(compose_id)s-%(variant)s-%(arch)s-%(disc_type)s%(disc_num)s%(suffix)s"
+                    cmd['filename'] = self._get_file_name(arch, variant, cmd['name'], cmd['version'])
-                    # Custom name (prefix)
-                    if cmd["name"]:
-                        custom_iso_name = cmd["name"]
-                        if cmd["version"]:
-                            custom_iso_name += "-%s" % cmd["version"]
-                        format = custom_iso_name + "-%(variant)s-%(arch)s-%(disc_type)s%(disc_num)s%(suffix)s"
-
-                    # XXX: hardcoded disc_type and disc_num
-                    filename = self.compose.get_image_name(arch, variant, disc_type="live",
-                                                           disc_num=None, format=format)
-                    iso_path = self.compose.paths.compose.iso_path(arch, variant, filename,
-                                                                   symlink_to=symlink_isos_to)
-                    if os.path.isfile(iso_path):
-                        self.compose.log_warning("Skipping creating live image, it already exists: %s" % iso_path)
-                        continue
-                    cmd["iso_path"] = iso_path
 
                     commands.append((cmd, variant, arch))
 
@@ -184,6 +172,22 @@ class LiveImagesPhase(PhaseBase):
 
         self.pool.start()
 
+    def _get_file_name(self, arch, variant, name=None, version=None):
+        if self.compose.conf.get('live_images_no_rename', False):
+            return None
+
+        format = "%(compose_id)s-%(variant)s-%(arch)s-%(disc_type)s%(disc_num)s%(suffix)s"
+        # Custom name (prefix)
+        if name:
+            custom_iso_name = name
+            if version:
+                custom_iso_name += "-%s" % version
+            format = custom_iso_name + "-%(variant)s-%(arch)s-%(disc_type)s%(disc_num)s%(suffix)s"
+
+        # XXX: hardcoded disc_type and disc_num
+        return self.compose.get_image_name(arch, variant, disc_type="live",
+                                           disc_num=None, format=format)
+
     def stop(self, *args, **kwargs):
         PhaseBase.stop(self, *args, **kwargs)
         if self.skip():
@@ -191,14 +195,6 @@ class LiveImagesPhase(PhaseBase):
 
 
 class CreateLiveImageThread(WorkerThread):
-    def fail(self, compose, cmd):
-        compose.log_error("LiveImage failed, removing ISO: %s" % cmd["iso_path"])
-        try:
-            # remove (possibly?) incomplete ISO
-            os.unlink(cmd["iso_path"])
-        except OSError:
-            pass
-
     def process(self, item, num):
         compose, cmd, variant, arch = item
         try:
@@ -212,9 +208,10 @@ class CreateLiveImageThread(WorkerThread):
                 self.pool.log_info(msg)
 
     def worker(self, compose, cmd, variant, arch, num):
-        log_file = compose.paths.log.log_file(arch, "createiso-%s" % os.path.basename(cmd["iso_path"]))
+        self.basename = '%(name)s-%(version)s-%(release)s' % cmd
+        log_file = compose.paths.log.log_file(arch, "createiso-%s" % self.basename)
 
-        msg = "Creating ISO (arch: %s, variant: %s): %s" % (arch, variant, os.path.basename(cmd["iso_path"]))
+        msg = "Creating ISO (arch: %s, variant: %s): %s" % (arch, variant, self.basename)
         self.pool.log_info("[BEGIN] %s" % msg)
 
         koji_wrapper = KojiWrapper(compose.conf["koji_profile"])
@@ -243,7 +240,6 @@ class CreateLiveImageThread(WorkerThread):
 
         output = koji_wrapper.run_blocking_cmd(koji_cmd, log_file=log_file)
         if output["retcode"] != 0:
-            self.fail(compose, cmd)
             raise RuntimeError("LiveImage task failed: %s. See %s for more details." % (output["task_id"], log_file))
 
         # copy finished image to isos/
@@ -253,7 +249,9 @@ class CreateLiveImageThread(WorkerThread):
             raise RuntimeError('Got %d images from task %d, expected 1.'
                                % (len(image_path), output['task_id']))
         image_path = image_path[0]
-        shutil.copy2(image_path, cmd["iso_path"])
+        filename = cmd.get('filename') or os.path.basename(image_path)
+        destination = os.path.join(cmd['dest_dir'], filename)
+        shutil.copy2(image_path, destination)
 
         # copy finished rpm to isos/ (if rpm wrapped ISO was built)
         if cmd["specfile"]:
@@ -261,15 +259,16 @@ class CreateLiveImageThread(WorkerThread):
 
             if cmd["sign"]:
                 # Sign the rpm wrapped images and get their paths
-                compose.log_info("Signing rpm wrapped images in task_id: %s (expected key ID: %s)" % (output["task_id"], compose.conf.get("signing_key_id")))
+                compose.log_info("Signing rpm wrapped images in task_id: %s (expected key ID: %s)"
+                                 % (output["task_id"], compose.conf.get("signing_key_id")))
                 signed_rpm_paths = self._sign_image(koji_wrapper, compose, cmd, output["task_id"])
                 if signed_rpm_paths:
                     rpm_paths = signed_rpm_paths
 
             for rpm_path in rpm_paths:
-                shutil.copy2(rpm_path, cmd["wrapped_rpms_path"])
+                shutil.copy2(rpm_path, cmd["dest_dir"])
 
-        self._write_manifest(cmd['iso_path'])
+        self._write_manifest(destination)
 
         self.pool.log_info("[DONE ] %s" % msg)
 
@@ -302,11 +301,14 @@ class CreateLiveImageThread(WorkerThread):
             return None
 
         # Prepare signing log file
-        signing_log_file = compose.paths.log.log_file(cmd["build_arch"], "live_images-signing-%s" % os.path.basename(cmd["iso_path"]))
+        signing_log_file = compose.paths.log.log_file(cmd["build_arch"],
+                                                      "live_images-signing-%s" % self.basename)
 
         # Sign the rpm wrapped images
         try:
-            sign_builds_in_task(koji_wrapper, koji_task_id, signing_command, log_file=signing_log_file, signing_key_password=compose.conf.get("signing_key_password"))
+            sign_builds_in_task(koji_wrapper, koji_task_id, signing_command,
+                                log_file=signing_log_file,
+                                signing_key_password=compose.conf.get("signing_key_password"))
         except RuntimeError:
             compose.log_error("Error while signing rpm wrapped images. See log: %s" % signing_log_file)
             raise
@@ -316,7 +318,7 @@ class CreateLiveImageThread(WorkerThread):
         rpm_paths = koji_wrapper.get_signed_wrapped_rpms_paths(koji_task_id, signing_key_id)
 
         # Wait untill files are available
-        if wait_paths(rpm_paths, 60*15):
+        if wait_paths(rpm_paths, 60 * 15):
             # Files are ready
             return rpm_paths
 

tests/test_liveimagesphase.py

@@ -30,6 +30,11 @@ class _DummyCompose(object):
                         '/iso_dir', arch, variant.uid
                     )
                 ),
+                image_dir=mock.Mock(
+                    side_effect=lambda variant, symlink_to: os.path.join(
+                        '/image_dir/%(arch)s', variant.uid
+                    )
+                ),
                 iso_path=mock.Mock(
                     side_effect=lambda arch, variant, filename, symlink_to: os.path.join(
                         '/iso_dir', arch, variant.uid, filename
@@ -88,14 +93,58 @@ class TestLiveImagesPhase(unittest.TestCase):
                               [mock.call((compose,
                                           {'ks_file': 'test.ks',
                                            'build_arch': 'amd64',
-                                           'wrapped_rpms_path': '/iso_dir/amd64/Client',
+                                           'dest_dir': '/iso_dir/amd64/Client',
                                            'scratch': False,
                                            'repos': ['/repo/amd64/Client',
                                                      'http://example.com/repo/',
                                                      '/repo/amd64/Everything'],
                                            'label': '',
                                            'name': None,
-                                           'iso_path': '/iso_dir/amd64/Client/image-name',
+                                           'filename': 'image-name',
+                                           'version': None,
+                                           'specfile': None,
+                                           'sign': False,
+                                           'type': 'live',
+                                           'release': '20151203.0',
+                                           'ksurl': None},
+                                          compose.variants['Client'],
+                                          'amd64'))])
+
+    @mock.patch('pungi.phases.live_images.ThreadPool')
+    def test_live_image_build_without_rename(self, ThreadPool):
+        compose = _DummyCompose({
+            'live_images_no_rename': True,
+            'live_images': [
+                ('^Client$', {
+                    'amd64': {
+                        'kickstart': 'test.ks',
+                        'additional_repos': ['http://example.com/repo/'],
+                        'repo_from': ['Everything'],
+                        'release': None,
+                    }
+                })
+            ],
+        })
+
+        phase = LiveImagesPhase(compose)
+
+        phase.run()
+
+        # assert at least one thread was started
+        self.assertTrue(phase.pool.add.called)
+        self.maxDiff = None
+        self.assertItemsEqual(phase.pool.queue_put.mock_calls,
+                              [mock.call((compose,
+                                          {'ks_file': 'test.ks',
+                                           'build_arch': 'amd64',
+                                           'dest_dir': '/iso_dir/amd64/Client',
+                                           'scratch': False,
+                                           'repos': ['/repo/amd64/Client',
+                                                     'http://example.com/repo/',
+                                                     '/repo/amd64/Everything'],
+                                           'label': '',
+                                           'name': None,
+                                           'filename': None,
                                            'version': None,
                                            'specfile': None,
                                            'sign': False,
@@ -134,14 +183,14 @@ class TestLiveImagesPhase(unittest.TestCase):
                               [mock.call((compose,
                                           {'ks_file': 'test.ks',
                                            'build_arch': 'amd64',
-                                           'wrapped_rpms_path': '/iso_dir/amd64/Client',
+                                           'dest_dir': '/iso_dir/amd64/Client',
                                            'scratch': False,
                                            'repos': ['/repo/amd64/Client',
                                                      'http://example.com/repo/',
                                                      '/repo/amd64/Everything'],
                                            'label': '',
                                            'name': None,
-                                           'iso_path': '/iso_dir/amd64/Client/image-name',
+                                           'filename': 'image-name',
                                            'version': None,
                                            'specfile': None,
                                            'sign': False,
@@ -153,14 +202,14 @@ class TestLiveImagesPhase(unittest.TestCase):
                                mock.call((compose,
                                           {'ks_file': 'another.ks',
                                            'build_arch': 'amd64',
-                                           'wrapped_rpms_path': '/iso_dir/amd64/Client',
+                                           'dest_dir': '/iso_dir/amd64/Client',
                                            'scratch': False,
                                            'repos': ['/repo/amd64/Client',
                                                      'http://example.com/repo/',
                                                      '/repo/amd64/Everything'],
                                            'label': '',
                                            'name': None,
-                                           'iso_path': '/iso_dir/amd64/Client/image-name',
+                                           'filename': 'image-name',
                                            'version': None,
                                            'specfile': None,
                                            'sign': False,
@@ -200,14 +249,14 @@ class TestLiveImagesPhase(unittest.TestCase):
                               [mock.call((compose,
                                           {'ks_file': 'test.ks',
                                            'build_arch': 'amd64',
-                                           'wrapped_rpms_path': '/iso_dir/amd64/Client',
+                                           'dest_dir': '/image_dir/amd64/Client',
                                            'scratch': False,
                                            'repos': ['/repo/amd64/Client',
                                                      'http://example.com/repo/',
                                                      '/repo/amd64/Everything'],
                                            'label': '',
                                            'name': None,
-                                           'iso_path': '/iso_dir/amd64/Client/image-name',
+                                           'filename': 'image-name',
                                            'version': None,
                                            'specfile': None,
                                            'sign': False,
@@ -231,14 +280,14 @@ class TestCreateLiveImageThread(unittest.TestCase):
         cmd = {
             'ks_file': '/path/to/ks_file',
             'build_arch': 'amd64',
-            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'dest_dir': '/iso_dir/amd64/Client',
             'scratch': False,
             'repos': ['/repo/amd64/Client',
                       'http://example.com/repo/',
                       '/repo/amd64/Everything'],
             'label': '',
             'name': None,
-            'iso_path': '/iso_dir/amd64/Client/image-name',
+            'filename': 'image-name',
             'version': None,
             'specfile': None,
             'type': 'live',
@@ -286,20 +335,81 @@ class TestCreateLiveImageThread(unittest.TestCase):
     @mock.patch('shutil.copy2')
     @mock.patch('pungi.phases.live_images.run')
     @mock.patch('pungi.phases.live_images.KojiWrapper')
-    def test_process_applicance(self, KojiWrapper, run, copy2):
+    def test_process_no_rename(self, KojiWrapper, run, copy2):
         compose = _DummyCompose({'koji_profile': 'koji'})
         pool = mock.Mock()
         cmd = {
             'ks_file': '/path/to/ks_file',
             'build_arch': 'amd64',
-            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'dest_dir': '/iso_dir/amd64/Client',
             'scratch': False,
             'repos': ['/repo/amd64/Client',
                       'http://example.com/repo/',
                       '/repo/amd64/Everything'],
             'label': '',
             'name': None,
-            'iso_path': '/iso_dir/amd64/Client/image-name',
+            'filename': None,
+            'version': None,
+            'specfile': None,
+            'type': 'live',
+            'ksurl': 'https://git.example.com/kickstarts.git?#CAFEBABE',
+            'release': None,
+        }
+
+        koji_wrapper = KojiWrapper.return_value
+        koji_wrapper.get_create_image_cmd.return_value = 'koji spin-livecd ...'
+        koji_wrapper.run_blocking_cmd.return_value = {
+            'retcode': 0,
+            'output': 'some output',
+            'task_id': 123
+        }
+        koji_wrapper.get_image_path.return_value = ['/path/to/image.iso']
+
+        t = CreateLiveImageThread(pool)
+        with mock.patch('time.sleep'):
+            t.process((compose, cmd, compose.variants['Client'], 'amd64'), 1)
+
+        self.assertEqual(koji_wrapper.run_blocking_cmd.mock_calls,
+                         [mock.call('koji spin-livecd ...', log_file='/a/b/log/log_file')])
+        self.assertEqual(koji_wrapper.get_image_path.mock_calls, [mock.call(123)])
+        self.assertEqual(copy2.mock_calls,
+                         [mock.call('/path/to/image.iso', '/iso_dir/amd64/Client/image.iso')])
+
+        write_manifest_cmd = ' && '.join([
+            'cd /iso_dir/amd64/Client',
+            'isoinfo -R -f -i image.iso | grep -v \'/TRANS.TBL$\' | sort >> image.iso.manifest'
+        ])
+        self.assertEqual(run.mock_calls, [mock.call(write_manifest_cmd)])
+        self.assertEqual(koji_wrapper.get_create_image_cmd.mock_calls,
+                         [mock.call('Test', '20151203.0.t', 'rhel-7.0-candidate',
+                                    'amd64', '/path/to/ks_file',
+                                    ['/repo/amd64/Client',
+                                     'http://example.com/repo/',
+                                     '/repo/amd64/Everything'],
+                                    image_type='live',
+                                    archive=False,
+                                    specfile=None,
+                                    wait=True,
+                                    release=None,
+                                    ksurl='https://git.example.com/kickstarts.git?#CAFEBABE')])
+
+    @mock.patch('shutil.copy2')
+    @mock.patch('pungi.phases.live_images.run')
+    @mock.patch('pungi.phases.live_images.KojiWrapper')
+    def test_process_applicance(self, KojiWrapper, run, copy2):
+        compose = _DummyCompose({'koji_profile': 'koji'})
+        pool = mock.Mock()
+        cmd = {
+            'ks_file': '/path/to/ks_file',
+            'build_arch': 'amd64',
+            'dest_dir': '/iso_dir/amd64/Client',
+            'scratch': False,
+            'repos': ['/repo/amd64/Client',
+                      'http://example.com/repo/',
+                      '/repo/amd64/Everything'],
+            'label': '',
+            'name': None,
+            'filename': 'image-name',
             'version': None,
             'specfile': None,
             'type': 'appliance',
@@ -356,14 +466,14 @@ class TestCreateLiveImageThread(unittest.TestCase):
         cmd = {
             'ks_file': '/path/to/ks_file',
             'build_arch': 'amd64',
-            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'dest_dir': '/iso_dir/amd64/Client',
             'scratch': False,
             'repos': ['/repo/amd64/Client',
                       'http://example.com/repo/',
                       '/repo/amd64/Everything'],
             'label': '',
             'name': None,
-            'iso_path': '/iso_dir/amd64/Client/image-name',
+            'filename': 'image-name',
             'version': None,
             'specfile': None,
             'ksurl': None,
@@ -393,7 +503,7 @@ class TestCreateLiveImageThread(unittest.TestCase):
         cmd = {
             'ks_file': '/path/to/ks_file',
             'build_arch': 'amd64',
-            'wrapped_rpms_path': '/iso_dir/amd64/Client',
+            'dest_dir': '/iso_dir/amd64/Client',
             'scratch': False,
             'repos': ['/repo/amd64/Client',
                       'http://example.com/repo/',