From 3d0b5c8f630f6c98d540603ef108a89ee69a9f5f Mon Sep 17 00:00:00 2001 From: Daniel Milnes Date: Tue, 19 Sep 2023 00:44:12 +0100 Subject: [PATCH] Collect and compress fedora-review logs after run This commit also adds support for using environment variables with `run_cmd` --- .../copr_backend/background_worker_build.py | 17 +++++-- backend/copr_backend/job.py | 7 +++ .../copr_rpmbuild/automation/fedora_review.py | 51 +++++++++++++++---- rpmbuild/copr_rpmbuild/helpers.py | 5 +- 4 files changed, 64 insertions(+), 16 deletions(-) diff --git a/backend/copr_backend/background_worker_build.py b/backend/copr_backend/background_worker_build.py index 233549b4d..d1f9a37d6 100644 --- a/backend/copr_backend/background_worker_build.py +++ b/backend/copr_backend/background_worker_build.py @@ -513,14 +513,15 @@ def _transfer_log_file(self): except SSHConnectionError as exc: return "Stopped following builder for broken SSH: {}".format(exc) - def _compress_live_logs(self): + def _compress_logs(self): """ - Compress builder-live.log and backend.log by gzip. + Compress builder-live.log, backend.log, and fedora-review.log using gzip. Never raise any exception! """ logs = [ self.job.builder_log, self.job.backend_log, + self.job.review_log, ] # For automatic redirect from log to log.gz, consider configuring @@ -530,6 +531,8 @@ def _compress_live_logs(self): # url.rewrite-if-not-file = ("^/(.*)/builder-live.log$" => "/$1/redirect-builder-live.log") # url.redirect += ( "^/(.*)/redirect-backend.log$" => "/$1/backend.log.gz" ) # url.rewrite-if-not-file += ("^/(.*)/backend.log$" => "/$1/redirect-backend.log") + # url.redirect += ( "^/(.*)/redirect-backend.log$" => "/$1/fedora-review.log.gz" ) + # url.rewrite-if-not-file += ("^/(.*)/backend.log$" => "/$1/redirect-fedora-review.log") # # $HTTP["url"] =~ "\.log\.gz$" { # magnet.attract-physical-path-to = ( "/etc/lighttpd/content-encoding-gzip-if-exists.lua" ) @@ -543,7 +546,7 @@ def _compress_live_logs(self): # end # # Or Apache with: - # + # # RewriteEngine on # RewriteCond %{REQUEST_FILENAME} !-f # RewriteRule ^(.*)$ %{REQUEST_URI}.gz [R] @@ -558,6 +561,12 @@ def _compress_live_logs(self): self.log.error("Compressed log %s exists", dest) continue + if not os.path.exists(src): + # fedora-review.log has a good chance of not existing + # We should be ready for other similar files + self.log.warning("Not trying to compress %s as it does not exist", src) + continue + self.log.info("Compressing %s by gzip", src) res = run_cmd(["gzip", src], logger=self.log) if res.returncode not in [0, 2]: @@ -814,7 +823,7 @@ def handle_task(self): self._drop_host() if self.job: self._mark_finished() - self._compress_live_logs() + self._compress_logs() else: self.log.error("No job object from Frontend") self.redis_set_worker_flag("status", "done") diff --git a/backend/copr_backend/job.py b/backend/copr_backend/job.py index 47f8d8367..76046852a 100644 --- a/backend/copr_backend/job.py +++ b/backend/copr_backend/job.py @@ -144,6 +144,13 @@ def builder_log(self): """ return os.path.join(self.results_dir, "builder-live.log") + @property + def review_log(self): + """ + The log file from running fedora-review + """ + return os.path.join(self.results_dir, "fedora-review/fedora-review.log") + @property def rsync_log_name(self): return "build-{:08d}.rsync.log".format(self.build_id) diff --git a/rpmbuild/copr_rpmbuild/automation/fedora_review.py b/rpmbuild/copr_rpmbuild/automation/fedora_review.py index 006e75d70..2269d11e0 100644 --- a/rpmbuild/copr_rpmbuild/automation/fedora_review.py +++ b/rpmbuild/copr_rpmbuild/automation/fedora_review.py @@ -10,10 +10,29 @@ import os import shutil +from contextlib import contextmanager +from typing import Generator + from copr_rpmbuild.helpers import run_cmd from copr_rpmbuild.automation.base import AutomationTool +@contextmanager +def cache_directory(resultdir) -> Generator[str, None, None]: + """ + Create a directory for a job to use as the XDG cache. + + :param str resultdir: Parent directory for the run results + :return: Path to cache directory + """ + cachedir = os.path.join(resultdir, "cache") + try: + os.makedirs(cachedir, exist_ok=True) + yield cachedir + finally: + shutil.rmtree(cachedir) + + class FedoraReview(AutomationTool): """ Optionally run `fedora-review` tool after build @@ -44,17 +63,17 @@ def run(self): "--name", self.package_name, "--mock-config", self.mock_config_file, ] + with cache_directory(self.resultdir) as cachedir: + try: + result = run_cmd(cmd, cwd=self.resultdir, env={"XDG_CACHE_HOME": cachedir}) + self.log.info(result.stdout) + except RuntimeError as ex: + self.log.warning("Fedora review failed\nerr:\n%s", ex) + self.log.warning("The build itself will not be marked " + "as failed because of this") + self._filter_results_directory(cachedir) - try: - result = run_cmd(cmd, cwd=self.resultdir) - self.log.info(result.stdout) - except RuntimeError as ex: - self.log.warning("Fedora review failed\nerr:\n%s", ex) - self.log.warning("The build itself will not be marked " - "as failed because of this") - self._filter_results_directory() - - def _filter_results_directory(self): + def _filter_results_directory(self, cachedir: str): """ Currently, fedora-review tool doesn't have an option to specify a destdir, and produces output to a directory called after the package @@ -67,6 +86,18 @@ def _filter_results_directory(self): dstdir = os.path.join(self.resultdir, "fedora-review") os.makedirs(dstdir, exist_ok=True) + logfile = os.path.join(cachedir, "fedora-review.log") + + # The fedora-review command failed to start so badly that it didn't even create a log file. + # Given how early that should happen, this merits investigation. + if not os.path.exists(logfile): + self.log.error("Can't find fedora-review log: %s", logfile) + self.log.error("Please raise a bug on https://github.com/fedora-copr/copr including a link to this build") + # If the log file doesn't exist, there's zero chance the results exist + return + + os.rename(logfile, os.path.join(dstdir, "fedora-review.log")) + # The fedora-review command failed so early that it didn't even create # the resultdir. Nothing to do here. if not os.path.exists(srcdir): diff --git a/rpmbuild/copr_rpmbuild/helpers.py b/rpmbuild/copr_rpmbuild/helpers.py index eddc19544..9ae852580 100644 --- a/rpmbuild/copr_rpmbuild/helpers.py +++ b/rpmbuild/copr_rpmbuild/helpers.py @@ -38,13 +38,14 @@ def cmd_readable(cmd): return ' '.join([shlex.quote(part) for part in cmd]) -def run_cmd(cmd, cwd=".", preexec_fn=None): +def run_cmd(cmd, cwd=".", preexec_fn=None, env=None): """ Runs given command in a subprocess. :param list(str) cmd: command to be executed and its arguments :param str cwd: In which directory to execute the command :param func preexec_fn: a callback invoked before exec in subprocess + :param dict env: environment variables to set for process :raises RuntimeError :returns munch.Munch(cmd, stdout, stderr, returncode) @@ -53,7 +54,7 @@ def run_cmd(cmd, cwd=".", preexec_fn=None): try: process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, preexec_fn=preexec_fn) + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, preexec_fn=preexec_fn, env=env) (stdout, stderr) = process.communicate() except OSError as e: if e.errno == errno.ENOENT: