PR#2080: backend: equivalent priority for concurrent sandboxes - copr/copr

copr / copr

#2080 backend: equivalent priority for concurrent sandboxes

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

Unknown source priority-for-concurrent-sandboxes into main

backend: equivalent priority for concurrent sandboxes

Pavel Raiskup • 2 years ago

20160d9

backend/copr_backend/daemons/build_dispatcher.py

file modified

+50 -15

		`@@ -19,22 +19,57 @@`

		`def get_priority(self, task):`
		`"""`
		`- Calculate the "dynamic" task priority, based on the queue size on`
		`- backend. We have a matrix of counters`
		`- '_counter["background"]["user"]["arch"]',`
		`- This is because we want to have separate counters for normal and`
		`- background builds, and each architecture (including source builds).`
		`- According to the counter - simply - the later tasks is submitted (higher`
		`- build ID) the less priority it has (higher priority number).`
		`+ Calculate the "dynamic" task priority, based on the actual queue size`
		`+ and queue structure. We have this matrix of counters:`
		`+`
		`+ _counter["background"]["user"]["arch"]["sandbox"] = value`
		`+`
		`+ Each dimension "re-sets" the priority and starts counting from zero.`
		`+ Simply, the later the task is submitted (higher build ID) the less`
		`+ priority it gets (== gets higher priority number).`
		`+`
		`+ We want to have separate counters for:`
		`+`
		`+ - normal and background builds (background jobs are always penalized by`
		`+ the BuildQueueTask.frontend_priority), those are two separate but`
		`+ equivalent queues here`
		`+`
		`+ - for each architecture (includes the source builds), that's because`
		`+ each architecture needs to have it's own "queue"/pool of builders`
		`+ (doesn't make sense to penalize e.g. ppc64le tasks and delay them`
		`+ because of a heavy x86_64 queue)`
		`+`
		`+ - For each sandbox. Each sandbox is equivalent to each other. This`
		`+ means that builds submitted to 'jdoe/foo--user1' should be taken with`
		`+ the same priority as 'jdoe/baz-user1' (note the same owner pattern!)`
		`+ no matter when they are submitted. This is useful when 'jdoe/foo'`
		`+ get's huge batch of builds, and _then_ after some time some _other_`
		`+ builds are submitted into the 'jdoe/baz'; those 'jdoe/baz builds could`
		`+ be blocked for a very long time. See below.`
		`+`
		`+ Note that for large queues, build_dispatcher isn't sometimes able to`
		`+ handle all the tasks in the priority queue in one cycle (we are able to`
		`+ process several hundreds, or at most thousands of tasks — depending on`
		`+ the 'sleeptime' value). Therefore, the priority really matters here, as`
		`+ we want to avoid all kinds of the weird builder/queue starving`
		`+ situations. In other words — if task isn't processed in one`
		`+ WorkerManager.run(timeout=???) cycle — it may stay "pending" for many`
		`+ other cycles too (depending on how quickly the overall queue get's`
		`+ processed).`
		`"""`
		`- self._counter.setdefault(task.background, {})`
		`- background = self._counter[task.background]`
		`- background.setdefault(task.owner, {})`
		`- owner = background[task.owner]`
		`- arch = task.requested_arch or 'srpm'`
		`- owner.setdefault(arch, 0)`
		`- owner[arch] += 1`
		`- return owner[arch]`
		`+`
		`+ def _get_subdict(source, arg):`
		`+ source.setdefault(arg, {})`
		`+ return source[arg]`
		`+`
		`+ background = _get_subdict(self._counter, task.background)`
		`+ owner = _get_subdict(background, task.owner)`
		`+ arch = _get_subdict(owner, task.requested_arch or "srpm")`
frostyx commented 2 years ago Is this basically this? background = self.counter.get(task.background, {}) owner = background.get(task.owner, {}) arch = owner.get(task.requested_arch or "srpm", {}) Or do I read the code incorrectly?
praiskup commented 2 years ago Not sure, the get() method is unlikely to setup the default value of the field?
frostyx commented 2 years ago You are correct, it cannot be done as easily as I suggested (I just tested it).
		`+`
		`+ # calculate from zero`
		`+ arch.setdefault(task.sandbox, 0)`
		`+ arch[task.sandbox] += 1`
		`+ return arch[task.sandbox]`


		`class BuildDispatcher(Dispatcher):`

backend/tests/test_build_dispatcher.py

file modified

+38

		`@@ -71,3 +71,41 @@`
		`"background": background,`
		`})`
		`assert task.frontend_priority == result + PRIORITY_SECTION_SIZE`
		`+`
		`+`
		`+ def test_sandbox_priority():`
		`+ prio = _PriorityCounter()`
		`+ assert prio.get_priority(BuildQueueTask({`
		`+ "build_id": "9",`
		`+ "task_id": "9",`
		`+ "project_owner": "cecil",`
		`+ "background": False,`
		`+ "sandbox": "cecil/foo--submitter",`
		`+ })) == 1`
		`+`
		`+ assert prio.get_priority(BuildQueueTask({`
		`+ "build_id": "9",`
		`+ "task_id": "9-fedora-rawhide-x86_64",`
		`+ "chroot": "fedora-rawhide-x86_64",`
		`+ "project_owner": "cecil",`
		`+ "background": True,`
		`+ "sandbox": "cecil/foo--submitter",`
		`+ })) == 1 # different arch`
		`+`
		`+ assert prio.get_priority(BuildQueueTask({`
		`+ "build_id": "10",`
		`+ "task_id": "10-fedora-rawhide-x86_64",`
		`+ "chroot": "fedora-rawhide-x86_64",`
		`+ "project_owner": "cecil",`
		`+ "background": True,`
		`+ "sandbox": "cecil/foo--submitter",`
		`+ })) == 2 # the same arch`
		`+`
		`+ assert prio.get_priority(BuildQueueTask({`
		`+ "build_id": "11",`
		`+ "task_id": "11-fedora-rawhide-x86_64",`
		`+ "chroot": "fedora-rawhide-x86_64",`
		`+ "project_owner": "cecil",`
		`+ "background": True,`
		`+ "sandbox": "cecil/baz--submitter",`
		`+ })) == 1 # the same arch, but different sandbox`

praiskup commented 2 years ago

With the recent @copr/PyPI tasks (tens of thousands builds) we observed
that other builds in @copr/ namespace aren't processed. That's because
the WorkerManager.run(timeout=) isn't able to go through that huge bunch
of tasks in the priority queue -- and because the other (submitted
later) had lower priority.

Resolves: #2079

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 3m 44s

praiskup commented 2 years ago

For the record; I'm really tempted to push this to production ASAP. Please speak-up
if you think this is a bad idea.

schlupov commented 2 years ago

I agree with having this in production, but would it be possible to have that long comment on the get_priority function as a small blog post? Or somewhere else than just directly in the code...

praiskup commented 2 years ago

What is the problem with long comments?

I'm OK to simplify it (proposals are welcome, feel free to rewrite it!),
but "VCS rules" ... everything will be lost somewhere in the future, but
not the distributed VCS content.

This info is exactly the thing to be stored in VCS, but I admit the format
might be worth fixing :-)

praiskup commented 2 years ago

I agree with having this in production

Thanks for looking! I will patch it, so we can see that even CI here gets processed.

Edited 2 years ago by praiskup

praiskup commented 2 years ago

Seems to work :-) on top of this, we had some temporary problems with subscriptions
so spawning builders was blocked (therefore the tasks spent a longer time period
in the "starting" state). This has automatically disappeared in the meantime...
I'll monitor this, but I suppose it is just fine now.

frostyx commented on line 65 of backend/copr_backend/daemons/build_dispatcher.py 2 years ago

Is this basically this?

background = self.counter.get(task.background, {})
owner = background.get(task.owner, {})
arch = owner.get(task.requested_arch or "srpm", {})

Or do I read the code incorrectly?

praiskup commented on line 65 of backend/copr_backend/daemons/build_dispatcher.py 2 years ago

Not sure, the get() method is unlikely to setup the default value of the field?

frostyx commented on line 65 of backend/copr_backend/daemons/build_dispatcher.py 2 years ago

You are correct, it cannot be done as easily as I suggested (I just tested it).

frostyx commented 2 years ago

praiskup commented 2 years ago

Commit 67769c9 fixes this pull-request

Pull-Request has been merged by praiskup

2 years ago

Metadata

Assignee

None

Tags

No Tags

Changes Summary 2

+50 -15

file changed

backend/copr_backend/daemons/build_dispatcher.py

+38 -0

file changed

backend/tests/test_build_dispatcher.py

copr / copr

Source Code

Documentation

#2080 backend: equivalent priority for concurrent sandboxes Merged 2 years ago by praiskup. Opened 2 years ago by praiskup. Unknown source priority-for-concurrent-sandboxes into main

Metadata

Changes Summary 2

#2080 backend: equivalent priority for concurrent sandboxes

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

Unknown source priority-for-concurrent-sandboxes into main