PR#788: [backend] compress backend-live.log and make redirect to compressed file, Closes #86 - copr/copr

copr / copr

#788 [backend] compress backend-live.log and make redirect to compressed file, Closes #86

Merged 4 years ago by praiskup. Opened 4 years ago by hojang.

copr/ hojang/copr log_compression into master

[backend] compress backend-live.log by calling gzip, Closes #86

Jan Buchmaier • 4 years ago

78553d3

file modified

		`@@ -207,6 +207,7 @@`
		`mr.check()`
		`mr.build_pkg()`

		`+ mr.compress_live_log(job)`
		`mr.check_build_success() # raises if build didn't succeed`
		`mr.download_results()`

backend/backend/mockremote/__init__.py

file modified

+16

		`@@ -17,6 +17,7 @@`
		`import fcntl`
		`import logging`
		`import os`
		`+ import subprocess`

		`from munch import Munch`
		`import time`
		`@@ -273,6 +274,21 @@`
		`self.log.error(str(error))`
		`raise MockRemoteError("Builder error during job {}.".format(self.job))`

		`+ def compress_live_log(self, job):`
		`+ log_basename = "builder-live.log"`
		`+ src = os.path.join(job.results_dir, log_basename)`
		`+`
		`+ self.log.info("Compressing {} by gzip".format(src))`
		`+ if subprocess.call(["gzip", src]) not in [0, 2]:`
		`+ self.log.error("Unable to compress file {}".format(src))`
		`+ return`
		`+`
		`+ try:`
		`+ with open(src, "w") as f_src:`
		`+ f_src.write("{} was moved to {}.gz\n".format(log_basename, log_basename))`
		`+ except Exception as e:`
		`+ self.log.exception(e)`
		`+`
		`def reattach_to_pkg_build(self):`
		`"""`
		`:raises VmError, BuilderError`

hojang commented 4 years ago

Because we want to use less space, we can compress backend-live.log after a build.

praiskup commented on line 14 of backend/backend/daemons/worker.py 4 years ago

please wrap this into separate method, probably in MockRemote. I think that we should compress even failed logs, so perhaps move this before check_build_success()?

praiskup commented on line 14 of backend/backend/daemons/worker.py 4 years ago

Also make sure that it never throws exceptions; we can afford failures here.

praiskup commented on line 6 of docker/backend/files/etc/lighttpd/lighttpd.conf 4 years ago

The files look like:

..snip../00921247-copr-backend/builder-live.log
..snip../srpm-builds/00921248/builder-live.log

msuchy commented on line 6 of docker/backend/files/etc/lighttpd/lighttpd.conf 4 years ago

This will redirect always. We want to redirect only conditionally when the .log file does not exist.

rebased onto 16112a1c3a96fb92a02c577e33b5b5ce82f68a24

4 years ago

praiskup commented on line 6 of docker/backend/files/etc/lighttpd/lighttpd.conf 4 years ago

Typo, but I believe it is meant correctly: we want to redirect only if the .log.gz file exists.

Metadata Update from @praiskup:
- Pull-request tagged with: needs-work

4 years ago

rebased onto 2a3a5bc368b60322a99eba9aaba81d233fe936c2

4 years ago

praiskup commented on line 18 of backend/backend/mockremote/__init__.py 4 years ago

I'd probably prefer simply calling subprocess.call(['gzip', 'file']). Or alternatively we should have catch-all exception handling here, because we don't want to fail the process for this low-prio thing.

praiskup commented on line 5 of docker/backend/files/etc/lighttpd/lighttpd.conf 4 years ago

Please avoid taking care of the configuration in this PR; it is pitty to keep it open when it is almost ready :-)

rebased onto 92e1e8a5f3d62e991a9cdd76bd2be684921c1aa7

4 years ago

praiskup commented on line 18 of backend/backend/mockremote/__init__.py 4 years ago

Please call the gzip binary directly, by subprocess.call(['gzip', src]).

praiskup commented on line 21 of backend/backend/mockremote/__init__.py 4 years ago

Nice, please add newline here. ("\n" missing)

Edited 4 years ago by praiskup

praiskup commented on line 5 of docker/backend/files/etc/lighttpd/lighttpd.conf 4 years ago

Please avoid this empty-line removal.

rebased onto df66d6a415aa006252ac1459c0b4eaa1be44e722

4 years ago

praiskup commented on line 18 of backend/backend/mockremote/__init__.py 4 years ago

There's no exception here, so please use log.error.

praiskup commented on line 17 of backend/backend/mockremote/__init__.py 4 years ago

the != 0 isn't needed here.

praiskup commented 4 years ago

two nits, but otherwise looks good to me ... I'll try to deploy this to staging

praiskup commented on line 22 of backend/backend/mockremote/__init__.py 4 years ago

The dst variable is not defined, what about to just write:
"build finished, build-live.log was compressed and moved to build-live.log.gz"

hojang commented 4 years ago

the != 0 isn't needed here.
Well, subprocess.call returns int, not boolean.

rebased onto 6f007bc465e3a6d2ee5c7a58c501046996f4280f

4 years ago

rebased onto bca5a6f1b9899b2a008ffd52dd9814299f3a4165

4 years ago

praiskup commented on line 17 of backend/backend/mockremote/__init__.py 4 years ago

looks good, gzip returns 2 when there's some warning

praiskup commented on line 23 of backend/backend/mockremote/__init__.py 4 years ago

This produces:
/var/lib/copr/public_html/results/praiskup/ping/srpm-builds/00842157/builder-live.log was moved to /var/lib/copr/public_html/results/praiskup/ping/srpm-builds/00842157/builder-live.log.gz

I'd prefer just builder-live.log was moved to builder-live.log.gz variant.

rebased onto c0d3cbd669ea1bf85c997a851631abbd0dd1eb34

4 years ago

praiskup commented 4 years ago

awesome! temporarily on staging: https://copr-fe-dev.cloud.fedoraproject.org/coprs/praiskup/ping/build/842178/

Please change the commit message so it doesn't mention the "redirect" part, it's not truth anymore; otherwise good.

rebased onto 1dab92b47091f967fe2add2d740c6e8ecbf13764

4 years ago

praiskup commented 4 years ago

+1, thank you!

Metadata Update from @praiskup:
- Pull-request untagged with: needs-work

4 years ago