PR#1968: frontend: large project modification timeout fix - copr/copr

copr / copr

#1968 frontend: large project modification timeout fix

Merged 2 years ago by frostyx. Opened 2 years ago by praiskup.

Unknown source speedup-project-modify into main

frontend: fix pluralized statements

Pavel Raiskup • 2 years ago

bc0f982

frontend: large project modification timeout fix

Pavel Raiskup • 2 years ago

0c4c82c

frontend/coprs_frontend/coprs/helpers.py

file modified

+11 -8

		`@@ -22,7 +22,7 @@`

		`from werkzeug.urls import url_encode`

		`- from copr_common.enums import EnumType`
		`+ from copr_common.enums import EnumType, StatusEnum`
		`# TODO: don't import BuildSourceEnum from helpers, use copr_common.enum instead`
		`from copr_common.enums import BuildSourceEnum # pylint: disable=unused-import`
		`from copr_common.rpm import splitFilename`
		`@@ -46,7 +46,8 @@`
		`PROJECT_RPMS_DL_STAT_FMT = "project_rpms_dl_stat:hset::{copr_user}@{copr_project_name}"`


		`- FINISHED_STATUSES = ["succeeded", "forked", "canceled", "skipped", "failed"]`
		`+ FINISHED_STATES = ["succeeded", "forked", "canceled", "skipped", "failed"]`
		`+ FINISHED_STATUSES = [StatusEnum(s) for s in FINISHED_STATES]`


		`class WorkList:`
		`@@ -646,16 +647,18 @@`
		`"""`
		By giving ``what`` string (e.g. "build") and ``items`` array, return string
		`in either of those formats:`
		`- - "builds 1, 2, 3 and 4[ are]"`
		`+ - "builds 1, 2, 3, and 4[ are]"`
		`+ - "builds 1, 2, and others[ are]"`
		`- "builds 1 and 2[ are]"`
		`- "build 31[ is]"`
		`"""`
		`if len(items) > 1:`
		`- return "{}s {} and {}{}".format(`
		`- what,`
		`- ', '.join(str(item) for item in items[:-1]),`
		`- str(items[-1]),`
		`- " are" if be_suffix else ""`
		`+ return "{what}s {list}{comma} and {last}{be_suffix}".format(`
		`+ what=what,`
		`+ list=', '.join(str(item) for item in items[:-1]),`
		`+ comma=',' if len(items) > 2 else "",`
		`+ last=str(items[-1]),`
		`+ be_suffix=" are" if be_suffix else "",`
		`)`
		`return "{} {}{}".format(`
		`what,`

frontend/coprs_frontend/coprs/logic/coprs_logic.py

file modified

+29 -6

		`@@ -1020,21 +1020,32 @@`
		`# iteration`
		`to_remove.append(mock_chroot)`

		`+ shortened = False`
		`running_builds = set()`
		`for mc in to_remove:`
		`- for bch in chroot_map[mc].build_chroots:`
		`- if not bch.finished:`
		`- running_builds.add(bch.build_id)`
		`- continue`
		`+ # We don't overwhelm the user with too many build IDs in`
		`+ # the error message.`
		`+ max_items = 5`
		`+ copr_chroot = chroot_map[mc]`
		`+ query = CoprChrootsLogic.unfinished_buildchroot(copr_chroot).limit(`
		`+ max_items + 1)`
		`+ for bch in query:`
		`+ if len(running_builds) >= max_items:`
		`+ shortened = True`
		`+ break`
		`+ running_builds.add(bch.build_id)`
		`cls.remove_copr_chroot(flask.g.user, chroot_map[mc])`

		`+`
		`# reject the request when some build_chroots are not yet finished`
		`if running_builds:`
		`+ builds = list(running_builds)`
		`+ if shortened:`
		`+ builds.append("others")`
		`raise exceptions.ConflictingRequest(`
		`"Can't drop chroot from project, related "`
		`"{} still in progress".format(`
		`- helpers.pluralize("build", list(running_builds),`
		`- be_suffix=True)))`
		`+ helpers.pluralize("build", builds, be_suffix=True)))`


		`@classmethod`
		`@@ -1119,6 +1130,18 @@`
		`return query.filter(models.CoprChroot.delete_after`
		`< datetime.datetime.now())`

		`+ @classmethod`
		`+ def unfinished_buildchroot(cls, copr_chroot):`
		`+ """`
		`+ Query returning list of unfinished BuildChroots assigned to`
		`+ the given CoprChroot.`
		`+ """`
		`+ statuses = helpers.FINISHED_STATUSES`
		`+ return (`
		`+ models.BuildChroot.query`
		`+ .filter(models.BuildChroot.copr_chroot==copr_chroot)`
		`+ .filter(not_(models.BuildChroot.status.in_(statuses)))`
		`+ )`

		`class CoprScoreLogic:`
		`"""`

frontend/coprs_frontend/coprs/models.py

file modified

+2 -2

		`@@ -1327,7 +1327,7 @@`
		`if self.finished_early:`
		`return True`
		`if not self.build_chroots:`
		`- return StatusEnum(self.source_status) in helpers.FINISHED_STATUSES`
		`+ return StatusEnum(self.source_status) in helpers.FINISHED_STATES`
		`return all([chroot.finished for chroot in self.build_chroots])`

		`@property`
		`@@ -1879,7 +1879,7 @@`
		`def finished(self):`
		`if self.build.finished_early:`
		`return True`
		`- return self.state in helpers.FINISHED_STATUSES`
		`+ return self.state in helpers.FINISHED_STATES`

		`@property`
		`def task_id(self):`

frontend/coprs_frontend/tests/test_helpers.py

file modified

+2 -1

		`@@ -295,7 +295,8 @@`
		`def test_pluralize():`
		`""" test generic I/O for helpers.pluralize """`
		`# we don't explicitly re-order`
		`- assert pluralize("build", [2, 1, 3], be_suffix=True) == "builds 2, 1 and 3 are"`
		`+ assert pluralize("build", [2, 1, 3], be_suffix=True) == "builds 2, 1, and 3 are"`
		`+ assert pluralize("build", [2, 1, "others"], be_suffix=True) == "builds 2, 1, and others are"`
		`assert pluralize("action", [1], be_suffix=True) == "action 1 is"`
		`assert pluralize("sth", [1, 2], be_suffix=False) == "sths 1 and 2"`
		`assert pluralize("a", [2], be_suffix=False) == "a 2"`

praiskup commented 2 years ago

On @rubygems/rubygems in Fedora Copr, we faced timeouts when we tried to
disable mock chroots with too many BuildChroots. Turns out ORM lazy
loaded all the BuildChroot instances for disabled CoprChroot (which in
our case was more than 100k items).

Instead of this, prepare a custom query for related BuildChroots that
can be used with limit() safely.

For the purpose of the query, we need to use FINISHED_STATUSES variable
which was improperly using states, not statuses. So I'm fixing this
(status is the number, state is the string human-readable alternative).

Fixes: #1925

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 56s

rebased onto 7adf903be7a10aeb58f3f874fe0b736942b60bd7

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 06s

praiskup commented 2 years ago

Please take a look at this one.

frostyx commented 2 years ago

Just for the record the message for more then max_items looks like this

Can't drop chroot from project, related builds 2679387, 2679389, 2679390 and others are still in progress

I like the shortening.

Nitpick - there should be a comma before "and others". Also, I would reduce the max_items from 20 to 5 or so. But those are really minor things, it is +1 from me even as is.

rebased onto a36779c80e7ae8535fb02c880c3c4e34b2b210e4

2 years ago

praiskup commented 2 years ago

Thank you for the review. Updated, PTAL.

zuul commented 2 years ago

Build succeeded.