[copr] master: [backend] [rhbz:#1171796] copr sometimes doesn't delete build from repository (d20dc04)

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master commit d20dc04e32784d938d471705ab65b71d2127864e Author: Valentin Gologuzov <vgologuz(a)redhat.com&gt; Date: Fri Jan 30 14:00:07 2015 +0100 [backend] [rhbz:#1171796] copr sometimes doesn't delete build from repository backend/backend/actions.py | 61 ++++--- backend/tests/_resources/1171796.tar.gz | Bin 0 -> 587365 bytes backend/tests/_resources/1171796_doubled.tar.gz | Bin 0 -> 1172212 bytes backend/tests/test_action.py | 217 ++++++++++++++++++----- frontend/run_tests.sh | 4 +- 5 files changed, 207 insertions(+), 75 deletions(-) diff --git a/backend/backend/actions.py b/backend/backend/actions.py index 4ae9c7c..f1d4557 100644 --- a/backend/backend/actions.py +++ b/backend/backend/actions.py @@ -39,6 +39,9 @@ class Action(object): self.front_url = front_url self.results_root_url = results_root_url + def __str__(self): + return "<Action: {}>".format(self.data) + def add_event(self, what): self.events.put({"when": time.time(), "who": "action", "what": what}) @@ -101,6 +104,7 @@ class Action(object): ext_data = json.loads(self.data["data"]) username = ext_data["username"] projectname = ext_data["projectname"] + chroots_requested = set(ext_data["chroots"]) packages = [os.path.basename(x).replace(".src.rpm", "") for x in ext_data["pkgs"].split()] @@ -111,34 +115,31 @@ class Action(object): self.add_event("Copr path {0}".format(path)) try: - chroot_list = os.listdir(path) + chroot_list = set(os.listdir(path)) except OSError: # already deleted - chroot_list = [] + chroot_list = set() + + chroots_to_do = chroot_list.intersection(chroots_requested) + if not chroots_to_do: + self.add_event("Nothing to delete for delete action: packages {}, {}" + .format(packages, ext_data)) + return - for chroot in chroot_list: + for chroot in chroots_to_do: self.add_event("In chroot {0}".format(chroot)) altered = False - # We need to delete the files only if they belong - # to the build. For example if my build fails and I send - # fixed pkg with the same version again, it succeeds and - # than I delete the failed, it would delete the succeeded - # files as well - that would be wrong. for pkg in packages: - if self.data["object_type"] == "build-succeeded" or ( - self.data["object_type"] == "build-failed" and - os.path.exists(os.path.join(path, chroot, pkg, "fail"))): - - pkg_path = os.path.join(path, chroot, pkg) - if os.path.isdir(pkg_path): - self.add_event("Removing build {0}".format(pkg_path)) - shutil.rmtree(pkg_path) - altered = True - else: - self.add_event( - "Package {0} dir not found in chroot {1}" - .format(pkg, chroot)) + pkg_path = os.path.join(path, chroot, pkg) + if os.path.isdir(pkg_path): + self.add_event("Removing build {0}".format(pkg_path)) + shutil.rmtree(pkg_path) + altered = True + else: + self.add_event( + "Package {0} dir not found in chroot {1}" + .format(pkg, chroot)) if altered: self.add_event("Running createrepo") @@ -154,13 +155,14 @@ class Action(object): self.add_event( "Error making local repo: {0}".format(err)) - log_path = os.path.join( - path, chroot, - 'build-{0}.log'.format(self.data['object_id'])) - - if os.path.isfile(log_path): - self.add_event("Removing log {0}".format(log_path)) - os.unlink(log_path) + logs_to_remove = [ + os.path.join(path, chroot, template.format(self.data['object_id'])) + for template in ['build-{}.log', 'build-{}.rsync.log'] + ] + for log_path in logs_to_remove: + if os.path.isfile(log_path): + self.add_event("Removing log {0}".format(log_path)) + os.remove(log_path) def run(self): """ Handle action (other then builds) - like rename or delete of project """ @@ -172,8 +174,7 @@ class Action(object): if action_type == ActionType.DELETE: if self.data["object_type"] == "copr": self.handle_delete_copr_project() - elif self.data["object_type"] in \ - ["build-succeeded", "build-skipped", "build-failed"]: + elif self.data["object_type"] == "build": self.handle_delete_build() result.result = ActionResult.SUCCESS diff --git a/backend/tests/_resources/1171796.tar.gz b/backend/tests/_resources/1171796.tar.gz new file mode 100644 index 0000000..6d3b522 Binary files /dev/null and b/backend/tests/_resources/1171796.tar.gz differ diff --git a/backend/tests/_resources/1171796_doubled.tar.gz b/backend/tests/_resources/1171796_doubled.tar.gz new file mode 100644 index 0000000..6cd04f3 Binary files /dev/null and b/backend/tests/_resources/1171796_doubled.tar.gz differ diff --git a/backend/tests/test_action.py b/backend/tests/test_action.py index 24e1367..3e7ce91 100644 --- a/backend/tests/test_action.py +++ b/backend/tests/test_action.py @@ -3,6 +3,7 @@ import json import tempfile import shutil import time +import tarfile import pytest import six @@ -60,7 +61,8 @@ class TestAction(object): self.ext_data_for_delete_build = json.dumps({ "pkgs": " ".join(self.pkgs), "username": "foo", - "projectname": "bar" + "projectname": "bar", + "chroots": ["fedora20", "epel7"] }) def teardown_method(self, method): @@ -83,6 +85,16 @@ class TestAction(object): return self.tmp_dir_name + def unpack_resource(self, resource_name): + if self.tmp_dir_name is None: + self.make_temp_dir() + + src_path = os.path.join(os.path.dirname(__file__), + "_resources", resource_name) + + with tarfile.open(src_path, "r:gz") as tfile: + tfile.extractall(os.path.join(self.tmp_dir_name, "old_dir")) + def test_action_event(self, mc_time): test_action = Action( events=self.test_q, @@ -308,40 +320,23 @@ class TestAction(object): tmp_dir = self.make_temp_dir() - for obj_type in ["build-succeeded", "build-skipped", "build-failed"]: - test_action = Action( - action={ - "action_type": ActionType.DELETE, - "object_type": obj_type, - "id": 7, - "old_value": "not-existing-project", - "data": self.ext_data_for_delete_build, - }, - events=self.test_q, lock=None, - frontend_callback=mc_front_cb, - destdir=tmp_dir, - front_url=None, - results_root_url=RESULTS_ROOT_URL - ) - with mock.patch("backend.actions.shutil") as mc_shutil: - test_action.run() - assert not mc_shutil.rmtree.called - - ev_1 = self.test_q.get_nowait() - assert "Action delete build" == ev_1["what"] - assert ev_1["who"] == "action" - - ev_2 = self.test_q.get_nowait() - assert "Packages to delete" in ev_2["what"] - assert " ".join(self.pkgs_stripped) in ev_2["what"] - assert ev_2["who"] == "action" - - ev_3 = self.test_q.get_nowait() - assert "Copr path" in ev_3["what"] - assert ev_3["who"] == "action" - - with pytest.raises(EmptyQueue): - self.test_q.get_nowait() + test_action = Action( + action={ + "action_type": ActionType.DELETE, + "object_type": "build", + "id": 7, + "old_value": "not-existing-project", + "data": self.ext_data_for_delete_build, + }, + events=self.test_q, lock=None, + frontend_callback=mc_front_cb, + destdir=tmp_dir, + front_url=None, + results_root_url=RESULTS_ROOT_URL + ) + with mock.patch("backend.actions.shutil") as mc_shutil: + test_action.run() + assert not mc_shutil.rmtree.called @mock.patch("backend.actions.createrepo") def test_delete_build_succeeded(self, mc_createrepo, mc_time): @@ -370,7 +365,7 @@ class TestAction(object): test_action = Action( action={ "action_type": ActionType.DELETE, - "object_type": "build-succeeded", + "object_type": "build", "id": 7, "old_value": "old_dir", "data": self.ext_data_for_delete_build, @@ -458,11 +453,11 @@ class TestAction(object): test_action = Action( action={ "action_type": ActionType.DELETE, - "object_type": "build-succeeded", + "object_type": "build", "id": 7, "old_value": "old_dir", "data": self.ext_data_for_delete_build, - "object_id": 42 + "object_id": 42, }, events=self.test_q, lock=None, frontend_callback=mc_front_cb, @@ -481,11 +476,147 @@ class TestAction(object): assert error_event_recorded + @mock.patch("backend.actions.createrepo") + def test_delete_two_chroots(self, mc_createrepo_unsafe, mc_time): + """ + Regression test, https://bugzilla.redhat.com/show_bug.cgi?id=1171796 + + """ + mc_createrepo_unsafe.return_value = 0, STDOUT, "" + + resource_name = "1171796.tar.gz" + self.unpack_resource(resource_name) + + chroot_20_path = os.path.join(self.tmp_dir_name, "old_dir", "fedora-20-x86_64") + chroot_21_path = os.path.join(self.tmp_dir_name, "old_dir", "fedora-21-x86_64") + + assert os.path.exists(os.path.join(chroot_20_path, "build-15.log")) + assert os.path.exists(os.path.join(chroot_21_path, "build-15.log")) + + assert os.path.exists(os.path.join(chroot_20_path, "build-15.rsync.log")) + assert os.path.exists(os.path.join(chroot_21_path, "build-15.rsync.log")) + + assert os.path.isdir(os.path.join(chroot_20_path, "rubygem-log4r-1.1.10-2.fc21")) + assert os.path.isdir(os.path.join(chroot_21_path, "rubygem-log4r-1.1.10-2.fc21")) + + mc_time.time.return_value = self.test_time + mc_front_cb = MagicMock() + + test_action = Action( + action={ + "action_type": ActionType.DELETE, + "object_type": "build", + "object_id": 15, + "id": 15, + "old_value": "old_dir", + "data": json.dumps({ + "pkgs": "rubygem-log4r-1.1.10-2.fc21.src.rpm", + "username": "foo", + "projectname": "bar", + "chroots": ["fedora-20-x86_64", "fedora-21-x86_64"] + }), + }, + events=self.test_q, lock=None, + frontend_callback=mc_front_cb, + destdir=self.tmp_dir_name, + front_url=None, + results_root_url=RESULTS_ROOT_URL + ) + test_action.run() + + assert not os.path.exists(os.path.join(chroot_20_path, "build-15.log")) + assert not os.path.exists(os.path.join(chroot_21_path, "build-15.log")) + + assert not os.path.exists(os.path.join(chroot_20_path, "build-15.rsync.log")) + assert not os.path.exists(os.path.join(chroot_21_path, "build-15.rsync.log")) + + assert not os.path.isdir(os.path.join(chroot_20_path, "rubygem-log4r-1.1.10-2.fc21")) + assert not os.path.isdir(os.path.join(chroot_21_path, "rubygem-log4r-1.1.10-2.fc21")) + + @mock.patch("backend.actions.createrepo") + def test_delete_two_chroots_two_remains(self, mc_createrepo_unsafe, mc_time): + """ + Regression test, https://bugzilla.redhat.com/show_bug.cgi?id=1171796 + extended: we also put two more chroots, which should be unaffected + """ + mc_createrepo_unsafe.return_value = 0, STDOUT, "" + + resource_name = "1171796_doubled.tar.gz" + self.unpack_resource(resource_name) + + chroot_20_path = os.path.join(self.tmp_dir_name, "old_dir", "fedora-20-x86_64") + chroot_21_path = os.path.join(self.tmp_dir_name, "old_dir", "fedora-21-x86_64") + chroot_20_i386_path = os.path.join(self.tmp_dir_name, "old_dir", "fedora-20-i386") + chroot_21_i386_path = os.path.join(self.tmp_dir_name, "old_dir", "fedora-21-i386") + + assert os.path.exists(os.path.join(chroot_20_path, "build-15.log")) + assert os.path.exists(os.path.join(chroot_21_path, "build-15.log")) + assert os.path.exists(os.path.join(chroot_20_i386_path, "build-15.log")) + assert os.path.exists(os.path.join(chroot_21_i386_path, "build-15.log")) + + assert os.path.exists(os.path.join(chroot_20_path, "build-15.rsync.log")) + assert os.path.exists(os.path.join(chroot_21_path, "build-15.rsync.log")) + assert os.path.exists(os.path.join(chroot_20_i386_path, "build-15.rsync.log")) + assert os.path.exists(os.path.join(chroot_21_i386_path, "build-15.rsync.log")) + + assert os.path.isdir(os.path.join(chroot_20_path, "rubygem-log4r-1.1.10-2.fc21")) + assert os.path.isdir(os.path.join(chroot_21_path, "rubygem-log4r-1.1.10-2.fc21")) + assert os.path.isdir(os.path.join(chroot_20_i386_path, "rubygem-log4r-1.1.10-2.fc21")) + assert os.path.isdir(os.path.join(chroot_21_i386_path, "rubygem-log4r-1.1.10-2.fc21")) + + mc_time.time.return_value = self.test_time + mc_front_cb = MagicMock() + + test_action = Action( + action={ + "action_type": ActionType.DELETE, + "object_type": "build", + "object_id": 15, + "id": 15, + "old_value": "old_dir", + "data": json.dumps({ + "pkgs": "rubygem-log4r-1.1.10-2.fc21.src.rpm", + "username": "foo", + "projectname": "bar", + "chroots": ["fedora-20-x86_64", "fedora-21-x86_64"] + }), + }, + events=self.test_q, lock=None, + frontend_callback=mc_front_cb, + destdir=self.tmp_dir_name, + front_url=None, + results_root_url=RESULTS_ROOT_URL + ) + test_action.run() + + assert not os.path.exists(os.path.join(chroot_20_path, "build-15.log")) + assert not os.path.exists(os.path.join(chroot_21_path, "build-15.log")) + assert os.path.exists(os.path.join(chroot_20_i386_path, "build-15.log")) + assert os.path.exists(os.path.join(chroot_21_i386_path, "build-15.log")) + + assert not os.path.exists(os.path.join(chroot_20_path, "build-15.rsync.log")) + assert not os.path.exists(os.path.join(chroot_21_path, "build-15.rsync.log")) + assert os.path.exists(os.path.join(chroot_20_i386_path, "build-15.rsync.log")) + assert os.path.exists(os.path.join(chroot_21_i386_path, "build-15.rsync.log")) + + assert not os.path.isdir(os.path.join(chroot_20_path, "rubygem-log4r-1.1.10-2.fc21")) + assert not os.path.isdir(os.path.join(chroot_21_path, "rubygem-log4r-1.1.10-2.fc21")) + assert os.path.isdir(os.path.join(chroot_20_i386_path, "rubygem-log4r-1.1.10-2.fc21")) + assert os.path.isdir(os.path.join(chroot_21_i386_path, "rubygem-log4r-1.1.10-2.fc21")) + + @mock.patch("backend.actions.createrepo_unsafe") + def test_delete_two_chroots_two_builds_stay_untouched(self, mc_createrepo_unsafe, mc_time): + # TODO: prepare archive + """ + Before: 2 builds of the same package-version, all using different chroot + """ + pass + @mock.patch("backend.actions.createrepo_unsafe") - def test_handle_createrepo_ok(self, mc_createrepo_unsfe, mc_time): + def test_handle_createrepo_ok(self, mc_createrepo_unsafe, mc_time): mc_front_cb = MagicMock() tmp_dir = self.make_temp_dir() - mc_createrepo_unsfe.return_value = 0, STDOUT, "" + mc_createrepo_unsafe.return_value = 0, STDOUT, "" action_data = json.dumps({ "chroots": ["epel-6-i386", "fedora-20-x86_64"], @@ -513,9 +644,9 @@ class TestAction(object): exp_call_1 = mock.call(lock=None, path=tmp_dir + u'/foo/bar/epel-6-i386') exp_call_2 = mock.call(lock=None, path=tmp_dir + u'/foo/bar/fedora-20-x86_64') - assert exp_call_1 in mc_createrepo_unsfe.call_args_list - assert exp_call_2 in mc_createrepo_unsfe.call_args_list - assert len(mc_createrepo_unsfe.call_args_list) == 2 + assert exp_call_1 in mc_createrepo_unsafe.call_args_list + 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") diff --git a/frontend/run_tests.sh b/frontend/run_tests.sh index 41e390e..2ecd9b9 100755 --- a/frontend/run_tests.sh +++ b/frontend/run_tests.sh @@ -1,6 +1,6 @@ #!/bin/sh cd coprs_frontend -COPR_CONFIG="$(pwd)/config/copr_unit_test.conf" python -m pytest tests \ - --cov-report term-missing --cov coprs $@ +COPR_CONFIG="$(pwd)/config/copr_unit_test.conf" python -m pytest tests -s \ + --cov-report term-missing --cov coprs $@