From 79ba31a3c3d80103437013b8a163b5ee046f7376 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Sep 17 2019 11:46:51 +0000 Subject: [PATCH 1/3] backend: avoid excception-hell in check_build_success() Previously, we expected that Builder.check_build_success() raised BuilderError in case of error, but this never happened - we instead raised RemoteCmdError which was never catched. Instead of cathing RemoteCmdError, execute `conn.run()` directly; it is both faster (we ignore stdout/stderr) and both we can avoid the exception handling mesh in every call. The only exception which could be raised there is SSHConnectionError, which is carefully catched in worker.py. Relates: #987 --- diff --git a/backend/backend/mockremote/__init__.py b/backend/backend/mockremote/__init__.py index 4d8ab5f..995a90c 100644 --- a/backend/backend/mockremote/__init__.py +++ b/backend/backend/mockremote/__init__.py @@ -306,9 +306,10 @@ class MockRemote(object): raise MockRemoteError("Builder error during job {}.".format(self.job)) def check_build_success(self): - try: - self.builder.check_build_success() - except BuilderError as error: + """ + Raise MockRemoteError if builder claims that the build failed. + """ + if not self.builder.check_build_success(): raise MockRemoteError("Build {} failed".format(self.job)) def download_results(self): diff --git a/backend/backend/mockremote/builder.py b/backend/backend/mockremote/builder.py index 10e0aea..735a897 100644 --- a/backend/backend/mockremote/builder.py +++ b/backend/backend/mockremote/builder.py @@ -56,8 +56,11 @@ class Builder(object): return out, err def check_build_success(self): + """ + Check if the build succeeded. If yes, return True. + """ successfile = os.path.join(self.resultdir, "success") - self._run_ssh_cmd("/usr/bin/test -f {0}".format(successfile)) + return self.conn.run("/usr/bin/test -f {0}".format(successfile)) == 0 def run_async_build(self): cmd = self._copr_builder_cmd() From da68783dc36c5df60b4735292f4adf8cbfe36804 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Sep 17 2019 11:46:51 +0000 Subject: [PATCH 2/3] backend: don't leave tracebacks in exception log entries alone I reformated the log entry dump mechanism (commit 05b161e7659908c830d) so the msg/args parameters are expanded before they go to redis. But exceptions can have msg/args as well - and it made the logs less readable: [2019-09-11 13:52:25,973][ ERROR][backend.worker-5-AARCH64][worker.py:do_job:218] Traceback (most recent call last): File "/usr/share/copr/backend/daemons/worker.py", line 211, in do_job mr.check_build_success() # raises if build didn't succeed File "/usr/share/copr/backend/mockremote/__init__.py", line 309, in check_build_success raise MockRemoteError("Build {} failed".format(self.job.task_id)) backend.exceptions.MockRemoteError: Build 846265-fedora-rawhide-aarch64 failed vs.: [2019-09-11 13:52:25,973][ ERROR][backend.worker-5-AARCH64][worker.py:do_job:218] Error during the build, host=38.145.48.106, build_id=846265, chroot=fedora-rawhide-aarch64 Traceback (most recent call last): File "/usr/share/copr/backend/daemons/worker.py", line 211, in do_job mr.check_build_success() # raises if build didn't succeed File "/usr/share/copr/backend/mockremote/__init__.py", line 309, in check_build_success raise MockRemoteError("Build {} failed".format(self.job.task_id)) backend.exceptions.MockRemoteError: Build 846265-fedora-rawhide-aarch64 failed This commit fixes it, so it is easy to get oriented in the exception log entries again. --- diff --git a/backend/backend/helpers.py b/backend/backend/helpers.py index 66ed2da..bf17ae0 100644 --- a/backend/backend/helpers.py +++ b/backend/backend/helpers.py @@ -396,14 +396,13 @@ class RedisPublishHandler(logging.Handler): # copr specific semantics record.who = self.who + # For the message arguments, it is better to expand them right now + # instead of relying on method in json.dumps(..., default=default) + # and even worse rely on it's reverse action in RedisLogHandler. + record.msg = record.msg % record.args if record.exc_info: _, error, tb = record.exc_info - record.msg = format_tb(error, tb) - else: - # For the message arguments, it is better to expand them right now - # instead of relying on method in json.dumps(..., default=default) - # and even worse rely on it's reverse action in RedisLogHandler. - record.msg = record.msg % record.args + record.msg += "\n" + format_tb(error, tb) # cleanup the hard to json.dumps() stuff record.exc_info = None From c0dc91fea93071e5cec62e6139a27cc8c82bd839 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Sep 17 2019 11:46:51 +0000 Subject: [PATCH 3/3] backend: lower the MockRemoteError exception to error It doesn't make sense to dump tracebacks to worker.log for general build failures. Fixes: #998 --- diff --git a/backend/backend/daemons/worker.py b/backend/backend/daemons/worker.py index 711443f..80a0b17 100644 --- a/backend/backend/daemons/worker.py +++ b/backend/backend/daemons/worker.py @@ -213,9 +213,10 @@ class Worker(multiprocessing.Process): except MockRemoteError as e: # record and break - self.log.exception( - "Error during the build, host=%s, build_id=%s, chroot=%s", - self.vm.vm_ip, job.build_id, job.chroot) + self.log.error( + "Error during the build, host=%s, build_id=%s, " + "chroot=%s, error='%s'", + self.vm.vm_ip, job.build_id, job.chroot, str(e)) failed = True mr.download_results()