From b1b810a733aa8c9f43ed520a9911bfbd252e50bd Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Apr 27 2022 08:13:58 +0000 Subject: [PATCH 1/3] backend: leave the JSON conversion to SafeRequest See PR#2166 The PR introduced a bug, causing the `data` to by converted into JSON twice. When doing `json.loads` on keygen, the data is still string. The reason why didn't discover the issue earlier is because we had two tests with the same name (`test_create_user_keys`) and therefore one of them wasn't being executed. --- diff --git a/backend/copr_backend/sign.py b/backend/copr_backend/sign.py index 9c7e8e8..5c5b761 100644 --- a/backend/copr_backend/sign.py +++ b/backend/copr_backend/sign.py @@ -5,7 +5,6 @@ Wrapper for /bin/sign from obs-sign package """ from subprocess import Popen, PIPE, SubprocessError -import json import os from packaging import version @@ -189,10 +188,10 @@ def create_user_keys(username, projectname, opts): :return: None """ - data = json.dumps({ + data = { "name_real": "{}_{}".format(username, projectname), "name_email": create_gpg_email(username, projectname) - }) + } log = get_redis_logger(opts, "sign", "actions") keygen_url = "http://{}/gen_key".format(opts.keygen_host) diff --git a/backend/tests/test_sign.py b/backend/tests/test_sign.py index 9929b40..1cd27c5 100644 --- a/backend/tests/test_sign.py +++ b/backend/tests/test_sign.py @@ -153,7 +153,7 @@ class TestSign(object): assert mc_request.called expected_call = mock.call( url="http://example.com/gen_key", - data='{"name_real": "foo_bar", "name_email": "foo_bar@copr.fedorahosted.org"}', + data={"name_real": "foo_bar", "name_email": "foo#bar@copr.fedorahosted.org"}, method="post" ) assert mc_request.call_args == expected_call @@ -168,7 +168,7 @@ class TestSign(object): @mock.patch("copr_backend.sign.SafeRequest.send") - def test_create_user_keys(self, mc_request): + def test_create_user_keys_err(self, mc_request): for code in [400, 401, 404, 500, 599]: mc_request.return_value.status_code = code mc_request.return_value.content = "error: {}".format(code) From 4736a1be46f21cb1a586d328aeffe86113eb6376 Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Apr 27 2022 11:46:54 +0000 Subject: [PATCH 2/3] backend: attempt to sign multiple times Fix #2133 I don't really know what caused #2133, we assumed it's a glitch caused by overloaded backend/keygen, and that simply running the /bin/sign a second time would fix the issue. We added `time.sleep` between failed attempts, so we need to mock some unrelated tests to save some time. --- diff --git a/backend/copr_backend/sign.py b/backend/copr_backend/sign.py index 5c5b761..ec30db7 100644 --- a/backend/copr_backend/sign.py +++ b/backend/copr_backend/sign.py @@ -6,6 +6,7 @@ Wrapper for /bin/sign from obs-sign package from subprocess import Popen, PIPE, SubprocessError import os +import time from packaging import version @@ -42,8 +43,12 @@ def call_sign_bin(cmd, log): new_err = CoprSignError("Failed to invoke '{}'".format(cmd_pretty)) raise new_err from err - if handle.returncode != 0 and "Connection timed out" in stderr: - log.warning("Timeout on %s, re-trying", cmd_pretty) + if handle.returncode != 0: + log.warning("Command '%s' failed with: %s", + cmd_pretty, stderr.rstrip()) + sleeptime = 20 + log.warning("Going to sleep %ss and re-try.", sleeptime) + time.sleep(sleeptime) continue break return handle.returncode, stdout, stderr diff --git a/backend/tests/test_sign.py b/backend/tests/test_sign.py index 1cd27c5..1bf6cc5 100644 --- a/backend/tests/test_sign.py +++ b/backend/tests/test_sign.py @@ -12,6 +12,7 @@ from copr_backend.exceptions import CoprSignError, CoprSignNoKeyError, CoprKeyge from copr_backend.sign import ( get_pubkey, _sign_one, sign_rpms_in_dir, create_user_keys, gpg_hashtype_for_chroot, + call_sign_bin, ) STDOUT = "stdout" @@ -19,6 +20,7 @@ STDERR = "stderr" class TestSign(object): + # pylint: disable=too-many-public-methods def setup_method(self, method): self.username = "foo" @@ -70,8 +72,9 @@ class TestSign(object): get_pubkey(self.username, self.projectname, MagicMock()) + @mock.patch("copr_backend.sign.time.sleep") @mock.patch("copr_backend.sign.Popen") - def test_get_pubkey_unknown_key(self, mc_popen): + def test_get_pubkey_unknown_key(self, mc_popen, _sleep): mc_handle = MagicMock() mc_handle.communicate.return_value = (STDOUT, "unknown key: foobar") mc_handle.returncode = 1 @@ -82,8 +85,9 @@ class TestSign(object): assert "There are no gpg keys for user foo in keyring" in str(err) + @mock.patch("copr_backend.sign.time.sleep") @mock.patch("copr_backend.sign.Popen") - def test_get_pubkey_unknown_error(self, mc_popen): + def test_get_pubkey_unknown_error(self, mc_popen, _sleep): mc_handle = MagicMock() mc_handle.communicate.return_value = (STDOUT, STDERR) mc_handle.returncode = 1 @@ -134,8 +138,9 @@ class TestSign(object): with pytest.raises(CoprSignError): _sign_one(fake_path, self.usermail, "sha256", MagicMock()) + @mock.patch("copr_backend.sign.time.sleep") @mock.patch("copr_backend.sign.Popen") - def test_sign_one_cmd_erro(self, mc_popen): + def test_sign_one_cmd_erro(self, mc_popen, _sleep): mc_handle = MagicMock() mc_handle.communicate.return_value = (STDOUT, STDERR) mc_handle.returncode = 1 @@ -145,6 +150,21 @@ class TestSign(object): with pytest.raises(CoprSignError): _sign_one(fake_path, self.usermail, "sha256", MagicMock()) + @staticmethod + @mock.patch("copr_backend.sign.time.sleep") + @mock.patch("copr_backend.sign.Popen") + def test_call_sign_bin_repeatedly(mc_popen, _sleep): + """ + Test that we attempt to run /bin/sign multiple times if it returns + non-zero exit status + """ + mc_handle = MagicMock() + mc_handle.communicate.return_value = (STDOUT, STDERR) + mc_handle.returncode = 1 + mc_popen.return_value = mc_handle + call_sign_bin(cmd=[], log=MagicMock()) + assert mc_popen.call_count == 3 + @mock.patch("copr_backend.sign.SafeRequest.send") def test_create_user_keys(self, mc_request): mc_request.return_value.status_code = 200 From 4f38d32ea43663a64793a0c7e4aaf7d4a20ad309 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Apr 27 2022 11:48:26 +0000 Subject: [PATCH 3/3] pylint: ignore also test-only pylint errors for backend --- diff --git a/.pylintpath/pylint_copr_plugin.py b/.pylintpath/pylint_copr_plugin.py index b5c2e32..8bc6e41 100644 --- a/.pylintpath/pylint_copr_plugin.py +++ b/.pylintpath/pylint_copr_plugin.py @@ -27,6 +27,7 @@ class Cache: None: False, } test_paths = { + "backend/tests", "cli/tests", "common/tests", "dist-git/tests",