PR#997: backend: avoid excception-hell in check_build_success() - copr/copr

copr / copr

#997 backend: avoid excception-hell in check_build_success()

Merged 5 years ago by msuchy. Opened 5 years ago by praiskup.

Unknown source exception-for-success-file into master

backend: lower the MockRemoteError exception to error

Pavel Raiskup • 5 years ago

c0dc91f

backend: don't leave tracebacks in exception log entries alone

Pavel Raiskup • 5 years ago

da68783

backend: avoid excception-hell in check_build_success()

Pavel Raiskup • 5 years ago

79ba31a

backend/backend/daemons/worker.py

file modified

+4 -3

		`@@ -213,9 +213,10 @@`

		`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()`

backend/backend/helpers.py

file modified

+5 -6

		`@@ -396,14 +396,13 @@`
		`# 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`

backend/backend/mockremote/__init__.py

file modified

+4 -3

		`@@ -306,9 +306,10 @@`
		`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):`

backend/backend/mockremote/builder.py

file modified

+4 -1

		`@@ -56,8 +56,11 @@`
		`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()`

praiskup commented 5 years ago

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

1 new commit added