backend: provide per-arch & per-owner worker limit #2909
Conversation
praiskup
force-pushed
the
praiskup-owner-arch-limits
branch
from
ff48c0d to
5f3f1a3
Compare
| lclass = ArchitectureWorkerLimit if tag_type == "arch" else \ | ||
| BuildTagLimit | ||
| for tag_type in ["arch", "tag", "arch_per_owner"]: | ||
| match tag_type: |
There was a problem hiding this comment.
Whaaat, we have pattern matching in python?!
TIL ... :-)
There was a problem hiding this comment.
Yeah, but when someone says "pattern" matching, I expect it to work with fnmatch patterns (my shell deformation..) :-) TIL too, I actually thought this was just a dummy C switch-case alternative.
There was a problem hiding this comment.
I am not a huge fan of switch cases in python, I'd personally use a tuple in for loop to get rid of match but it's just syntax preference
backend/copr_backend/rpm_builds.py
Outdated
| Copr owner (user or group). | ||
| """ | ||
| def __init__(self, architecture, limit): | ||
| def hasher(x): |
There was a problem hiding this comment.
This probably doesn't have to be nested if we make it @staticmethod
There was a problem hiding this comment.
that can be lambda, going to change...
backend/copr_backend/rpm_builds.py
Outdated
| @@ -133,6 +137,18 @@ def predicate(x): | |||
| super().__init__(predicate, limit, name="arch_{}".format(architecture)) | |||
|
|
|||
|
|
|||
| class ArchitectureUserWorkerLimit(GroupWorkerLimit): | |||
There was a problem hiding this comment.
There is probably nothing we can do but the inheritance doesn't look right (UserWorkerLimit from GroupWorkerLimit)
There was a problem hiding this comment.
Yeah, I think we could rename GroupWorkerLimit into HashedWorkerLimit :-)?
praiskup
force-pushed
the
praiskup-owner-arch-limits
branch
2 times, most recently
from
b0b8462 to
b25f271
Compare
The default "max_workers" limit might be too large (e.g. in Fedora Copr it is 45) compared to the number of available workers of some architecture type (e.g. for s390x there's now max 18 workers, and ppc64le is often limited too due to fedora-copr#2869). This patch then brings another FUP mitigation.
The old name caused confusion, see PR#2909. The new name better describes what the class actually does. Even though the dispatcher/worker/limit/etc. logic is supposed to be a library-like code, at this point we don't know about any other users so rename like this seems OK.
praiskup
force-pushed
the
praiskup-owner-arch-limits
branch
from
b25f271 to
3f0536b
Compare
| @@ -6,7 +6,7 @@ | |||
| %global tests_version 4 | |||
| %global tests_tar test-data-copr-backend | |||
|
|
|||
| %global copr_common_version 0.16.4.dev | |||
| %global copr_common_version 0.20.1.dev1 | |||
There was a problem hiding this comment.
why do we need to jump to version 20 instead of 17?
There was a problem hiding this comment.
We ar jumping from 0.20.1 => 0.20.1.dev1; this is just a requirement that we haven't had to bump for quite some time.
| lclass = ArchitectureWorkerLimit if tag_type == "arch" else \ | ||
| BuildTagLimit | ||
| for tag_type in ["arch", "tag", "arch_per_owner"]: | ||
| match tag_type: |
There was a problem hiding this comment.
I am not a huge fan of switch cases in python, I'd personally use a tuple in for loop to get rid of match but it's just syntax preference
The default "max_workers" limit might be too large (e.g. in Fedora Copr it is 45) compared to the number of available workers of some architecture type (e.g. for s390x there's now max 18 workers, and ppc64le is often limited too due to #2869). This patch then brings another FUP mitigation.