Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master commit 22baf5b82ae959f303688d1c4336346b2ad202db Author: Valentin Gologuzov <vgologuz(a)redhat.com&gt; Date: Thu Jun 4 21:18:41 2015 +0200 [backend] createrepo_unsafe now returns only STDOUT and raise exception on errors backend/backend/actions.py | 36 +++++++++++---------- backend/backend/createrepo.py | 25 +++++++++----- backend/backend/exceptions.py | 23 ++++++++++++++ backend/backend/mockremote/__init__.py | 24 +++++++------- backend/run/copr_prune_results.py | 15 +++------ backend/tests/run/test_copr_prune_results.py | 18 +--------- backend/tests/test_action.py | 43 +++----------------------- backend/tests/test_createrepo.py | 4 ++ 8 files changed, 86 insertions(+), 102 deletions(-) diff --git a/backend/backend/actions.py b/backend/backend/actions.py index 96e8cc2..2af0f41 100644 --- a/backend/backend/actions.py +++ b/backend/backend/actions.py @@ -6,6 +6,7 @@ import time from bunch import Bunch from .createrepo import createrepo, createrepo_unsafe +from exceptions import CreateRepoError from .helpers import get_redis_logger @@ -54,22 +55,22 @@ class Action(object): projectname = data["projectname"] chroots = data["chroots"] - failure = False + done_count = 0 for chroot in chroots: self.log.info("Creating repo for: {}/{}/{}".format(username, projectname, chroot)) path = os.path.join(self.destdir, username, projectname, chroot) - errcode, _, err = createrepo_unsafe(path=path, lock=self.lock) - if errcode != 0 or err.strip(): - # TODO: createrepo should raise error on errors, handle it - self.log.error("Error making local repo: {0}".format(err)) - failure = True + try: + createrepo_unsafe(path=path, lock=self.lock) + done_count += 1 + except CreateRepoError: + self.log.exception("Error making local repo") - if failure: - result.result = ActionResult.FAILURE - else: + if done_count == len(chroots): result.result = ActionResult.SUCCESS + else: + result.result = ActionResult.FAILURE def handle_rename(self, result): self.log.debug("Action rename") @@ -146,14 +147,15 @@ class Action(object): result_base_url = "/".join( [self.results_root_url, username, projectname, chroot]) - _, _, err = createrepo( - path=os.path.join(path, chroot), lock=self.lock, - front_url=self.front_url, base_url=result_base_url, - username=username, projectname=projectname - ) - if err.strip(): - # TODO: createrepo should raise exception, handle it - self.log.error("Error making local repo: {0}".format(err)) + createrepo_target = os.path.join(path, chroot) + try: + createrepo( + path=createrepo_target, lock=self.lock, + front_url=self.front_url, base_url=result_base_url, + username=username, projectname=projectname + ) + except CreateRepoError: + self.log.exception("Error making local repo: {}".format(createrepo_target)) logs_to_remove = [ os.path.join(path, chroot, template.format(self.data['object_id'])) diff --git a/backend/backend/createrepo.py b/backend/backend/createrepo.py index c20656f..b786847 100644 --- a/backend/backend/createrepo.py +++ b/backend/backend/createrepo.py @@ -1,6 +1,7 @@ import os import subprocess from subprocess import Popen +from exceptions import CreateRepoError from .helpers import get_auto_createrepo_status @@ -39,17 +40,23 @@ def createrepo_unsafe(path, lock=None, dest_dir=None, base_url=None): comm.append(path) - if lock: - with lock: + try: + # todo: replace with file lock on target dir + if lock: + with lock: + cmd = Popen(comm, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out, err = cmd.communicate() + else: cmd = Popen(comm, stdout=subprocess.PIPE, stderr=subprocess.PIPE) out, err = cmd.communicate() - else: - cmd = Popen(comm, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - out, err = cmd.communicate() - - # TODO: rewrite with raise CreateRepoException on returncode != 0 - # and return only stdout on success - return cmd.returncode, out, err + except Exception as err: + raise CreateRepoError(msg="Failed to execute: {}".format(err), cmd=comm) + + if cmd.returncode != 0: + raise CreateRepoError(msg="createrepo exit code != 0", + cmd=comm, exit_code=cmd.returncode, + stdout=out, stderr=err) + return out def createrepo(path, front_url, username, projectname, base_url=None, lock=None): diff --git a/backend/backend/exceptions.py b/backend/backend/exceptions.py index c9e83d8..b50f6d8 100644 --- a/backend/backend/exceptions.py +++ b/backend/backend/exceptions.py @@ -138,3 +138,26 @@ class VmDescriptorNotFound(VmError): class NoVmAvailable(VmError): pass + +class CmdError(CoprBackendError): + def __init__(self, msg, cmd, exit_code=None, stdout=None, stderr=None): + super(CmdError, self).__init__(msg) + + self.cmd = cmd + self.exit_code = exit_code + self.stdout = stdout + self.stderr = stderr + + def __str__(self): + out = super(CmdError, self).__str__() + if self.cmd: + out += ("\n" + "return code {} after invocation of: {} \n" + "stderr: {}\n" + "stdout: {}\n").format( + self.exit_code, self.cmd, self.stdout, self.stderr) + return out + + +class CreateRepoError(CmdError): + pass diff --git a/backend/backend/mockremote/__init__.py b/backend/backend/mockremote/__init__.py index 6e7a336..cd2faa8 100755 --- a/backend/backend/mockremote/__init__.py +++ b/backend/backend/mockremote/__init__.py @@ -41,7 +41,7 @@ import time from ..constants import DEF_REMOTE_BASEDIR, DEF_BUILD_TIMEOUT, DEF_REPOS, \ DEF_BUILD_USER, DEF_MACROS -from ..exceptions import MockRemoteError, BuilderError +from ..exceptions import MockRemoteError, BuilderError, CreateRepoError # TODO: replace sign & createrepo with dependency injection @@ -252,17 +252,17 @@ class MockRemote(object): .format(self.job.project_owner, self.job.project_name, self.opts.frontend_base_url, self.chroot_dir, base_url)) - _, _, err = createrepo( - path=self.chroot_dir, - front_url=self.opts.frontend_base_url, - base_url=base_url, - username=self.job.project_owner, - projectname=self.job.project_name, - lock=self.lock, - ) - if err.strip(): - self.log.error( - "Error making local repo: {}: {}".format(self.chroot_dir, err)) + try: + createrepo( + path=self.chroot_dir, + front_url=self.opts.frontend_base_url, + base_url=base_url, + username=self.job.project_owner, + projectname=self.job.project_name, + lock=self.lock, + ) + except CreateRepoError: + self.log.exception("Error making local repo: {}: {}".format(self.chroot_dir)) # FIXME - maybe clean up .repodata and .olddata # here? diff --git a/backend/run/copr_prune_results.py b/backend/run/copr_prune_results.py index 8401f2a..cfed3c0 100755 --- a/backend/run/copr_prune_results.py +++ b/backend/run/copr_prune_results.py @@ -23,6 +23,7 @@ sys.path.append("/usr/share/copr/") from backend.helpers import BackendConfigReader, get_auto_createrepo_status from backend.createrepo import createrepo_unsafe +from backend.exceptions import CreateRepoError DEF_DAYS = 14 @@ -133,18 +134,12 @@ class Pruner(object): log.error("Error during prune copr {}/{}:{}".format(username, projectname, sub_dir_name)) log.debug("Prune done for {}/{}:{}".format(username, projectname, sub_dir_name)) - # run createrepo try: - retcode, stdout, stderr = createrepo_unsafe(chroot_path) - if retcode != 0: - log.error( - "Failed to createrepo for copr {}/{}:{}\n STDOUT: \n{}\n STDERR: \n{}\n" - .format(username, projectname, sub_dir_name, stdout.decode(), stderr.decode())) - else: - log.info("Createrepo done for copr {}/{}:{}" - .format(username, projectname, sub_dir_name)) - except Exception as exception: + createrepo_unsafe(chroot_path) + log.info("Createrepo done for copr {}/{}:{}" + .format(username, projectname, sub_dir_name)) + except CreateRepoError as exception: log.exception("Createrepo for copr {}/{}:{} failed with error: {}" .format(username, projectname, sub_dir_name, exception)) diff --git a/backend/tests/run/test_copr_prune_results.py b/backend/tests/run/test_copr_prune_results.py index 1a4be8c..f599656 100644 --- a/backend/tests/run/test_copr_prune_results.py +++ b/backend/tests/run/test_copr_prune_results.py @@ -23,6 +23,7 @@ else: sys.path.append("../../run") +from backend.exceptions import CreateRepoError MODULE_REF = "copr_prune_results" @@ -232,7 +233,6 @@ class TestPruneResults(object): mc_cru.call_args_list ]) == expected_path_set - def test_prune_project_handle_gacs_error(self, test_pruner, mc_cru, mc_gacs): self.pruner.prune_failed_builds = MagicMock() self.pruner.prune_obsolete_success_builds = MagicMock() @@ -250,22 +250,8 @@ class TestPruneResults(object): self.pruner.prune_obsolete_success_builds = MagicMock() mc_gacs.return_value = True - # 0. createrepo_unsafe failure - mc_cru.return_value = (1, "stdout", "stderr") - - self.pruner.prune_project(os.path.join(self.tmp_dir_name, self.prj), - self.username, self.coprname) - - assert self.pruner.prune_failed_builds.called - assert self.pruner.prune_obsolete_success_builds.called - - self.pruner.prune_failed_builds.reset_mock() - self.pruner.prune_obsolete_success_builds.reset_mock() - - mc_cru.return_value = (0, "", "") - # 1. createrepo_unsafe raises error - mc_cru.side_effect = IOError() + mc_cru.side_effect = CreateRepoError("test exception", ["foo", "bar"], 1) self.pruner.prune_project(os.path.join(self.tmp_dir_name, self.prj), self.username, self.coprname) diff --git a/backend/tests/test_action.py b/backend/tests/test_action.py index 2344197..d1e1ac9 100644 --- a/backend/tests/test_action.py +++ b/backend/tests/test_action.py @@ -19,7 +19,7 @@ else: from backend.actions import Action, ActionType, ActionResult - +from backend.exceptions import CreateRepoError RESULTS_ROOT_URL = "http://example.com/results" @@ -220,7 +220,6 @@ class TestAction(object): assert result_dict["result"] == ActionResult.SUCCESS assert result_dict["job_ended_on"] == self.test_time - assert os.path.exists(os.path.join(tmp_dir, "old_dir")) def test_action_run_delete_copr_remove_folders(self, mc_time): @@ -364,7 +363,6 @@ class TestAction(object): test_action.run() - @mock.patch("backend.actions.createrepo") def test_delete_two_chroots(self, mc_createrepo_unsafe, mc_time): """ @@ -424,7 +422,6 @@ class TestAction(object): assert os.path.exists(chroot_20_path) assert os.path.exists(chroot_21_path) - @mock.patch("backend.actions.createrepo") def test_delete_two_chroots_two_remains(self, mc_createrepo_unsafe, mc_time): """ @@ -582,12 +579,12 @@ class TestAction(object): assert exp_call_2 in mc_createrepo_unsafe.call_args_list assert len(mc_createrepo_unsafe.call_args_list) == 2 - @mock.patch("backend.actions.createrepo_unsafe") def test_handle_createrepo_failure_1(self, mc_createrepo_unsfe, mc_time): mc_front_cb = MagicMock() tmp_dir = self.make_temp_dir() - mc_createrepo_unsfe.return_value = 1, STDOUT, "" + mc_createrepo_unsfe.side_effect = CreateRepoError("test exception", ["foo", "bar"], 1) + # return_value = 1, STDOUT, "" action_data = json.dumps({ "chroots": ["epel-6-i386", "fedora-20-x86_64"], @@ -613,44 +610,14 @@ class TestAction(object): assert result_dict["result"] == ActionResult.FAILURE @mock.patch("backend.actions.createrepo_unsafe") - def test_handle_createrepo_failure_2(self, mc_createrepo_unsfe, mc_time): - mc_front_cb = MagicMock() - tmp_dir = self.make_temp_dir() - mc_createrepo_unsfe.return_value = 0, STDOUT, STDERR - - action_data = json.dumps({ - "chroots": ["epel-6-i386", "fedora-20-x86_64"], - "username": "foo", - "projectname": "bar" - }) - self.opts.destdir = tmp_dir - test_action = Action( - opts=self.opts, - action={ - "action_type": ActionType.CREATEREPO, - "data": action_data, - "id": 10 - }, - lock=None, - frontend_client=mc_front_cb, - ) - test_action.run() - - result_dict = mc_front_cb.update.call_args[0][0]["actions"][0] - - assert result_dict["id"] == 10 - assert result_dict["result"] == ActionResult.FAILURE - - @mock.patch("backend.actions.createrepo_unsafe") def test_handle_createrepo_failure_3(self, mc_createrepo_unsfe, mc_time): mc_front_cb = MagicMock() tmp_dir = self.make_temp_dir() mc_createrepo_unsfe.side_effect = [ - (0, STDOUT, ""), - (1, STDOUT, STDERR), + STDOUT, + CreateRepoError("test exception", ["foo", "bar"], 1), ] - action_data = json.dumps({ "chroots": ["epel-6-i386", "fedora-20-x86_64"], "username": "foo", diff --git a/backend/tests/test_createrepo.py b/backend/tests/test_createrepo.py index 3e4ff60..cf425c3 100644 --- a/backend/tests/test_createrepo.py +++ b/backend/tests/test_createrepo.py @@ -90,6 +90,7 @@ class TestCreaterepoUnsafe(object): mc_popen.side_effect = popen_side_effect mc_popen.return_value.communicate.return_value = ("", "") + mc_popen.return_value.returncode = 0 createrepo_unsafe(self.tmp_dir_name, lock=mocked_lock) assert self.shared_state["lock_status"] @@ -100,6 +101,7 @@ class TestCreaterepoUnsafe(object): def test_createrepo_generated_commands_existing_repodata(self, mc_popen): mc_popen.return_value.communicate.return_value = ("", "") + mc_popen.return_value.returncode = 0 path_epel_5 = os.path.join(self.tmp_dir_name, "epel-5") expected_epel_5 = ['/usr/bin/createrepo_c', '--database', '--ignore-lock', '--update', '-s', 'sha', '--checksum', 'md5', path_epel_5] @@ -120,6 +122,7 @@ class TestCreaterepoUnsafe(object): def test_createrepo_devel_generated_commands_existing_repodata(self, mc_popen): mc_popen.return_value.communicate.return_value = ("", "") + mc_popen.return_value.returncode = 0 path_epel_5 = os.path.join(self.tmp_dir_name, "epel-5") expected_epel_5 = ['/usr/bin/createrepo_c', '--database', '--ignore-lock', '-s', 'sha', '--checksum', 'md5', @@ -143,6 +146,7 @@ class TestCreaterepoUnsafe(object): def test_createrepo_devel_generated_commands(self, mc_popen): mc_popen.return_value.communicate.return_value = ("", "") + mc_popen.return_value.returncode = 0 path_epel_5 = os.path.join(self.tmp_dir_name, "epel-5") expected_epel_5 = ['/usr/bin/createrepo_c', '--database', '--ignore-lock', '-s', 'sha', '--checksum', 'md5',