Resolver parallelization #225

msimacek · 2018-02-13T12:24:12Z

Use postgres advisory locking for mutual exclusion between multiple resolver processes:

Repo resolver locks are per-collection - i.e. one process can resolve rawhide, another one can resolve f26 at the same time
Build resolver locks are per-repo_id - i.e. it processes a batch of builds with the same repo_id. The repo setup (sack) is slower than build resolution. So I want to avoid duplicating the repo setup step across multiple processes. In the general case, where there are usually 5-20 builds per repo, this works well. The disadvantage is that after mass tagging, the batch can be quite big, but I'm not sure if it's worth optimizing for something that happens so infrequently.

codecov · 2018-02-13T12:28:48Z

Codecov Report

Merging #225 into master will increase coverage by 0.14%.
The diff coverage is 84.33%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage    82.1%   82.25%   +0.14%     
==========================================
  Files          34       35       +1     
  Lines        3130     3200      +70     
==========================================
+ Hits         2570     2632      +62     
- Misses        560      568       +8

Impacted Files	Coverage Δ
koschei/backend/service.py	`83.33% <ø> (-0.28%)`	⬇️
koschei/backend/services/polling.py	`54.28% <ø> (+2.93%)`	⬆️
koschei/session.py	`100% <100%> (ø)`	⬆️
koschei/backend/__init__.py	`80.74% <100%> (+0.74%)`	⬆️
koschei/backend/services/build_resolver.py	`81.17% <60%> (-1.33%)`	⬇️
koschei/backend/services/repo_resolver.py	`90.38% <71.42%> (-0.95%)`	⬇️
koschei/db.py	`91.13% <77.77%> (-0.53%)`	⬇️
koschei/backend/services/resolver.py	`96.77% <77.77%> (-1.52%)`	⬇️
koschei/locks.py	`91.89% <91.89%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bebca3...1ea3e5d. Read the comment docs.

mizdebsk

Repo resolution locking doesn't seem correct.

mizdebsk · 2018-02-13T13:03:42Z

koschei/locks.py

+    advisory lock.
+
+    :param: db Database session
+    :param: namespace Integer namespace key. Should use one of the constants


Formatting seems odd - this line describes two params, namespace and key, right?

No, the key should have it's own line, but I forgot that. Will fix

mizdebsk · 2018-02-13T13:14:01Z

koschei/backend/services/repo_resolver.py

+                    self.db, LOCK_REPO_RESOLVER, collection.id,
+                    block=False, transaction=True,
+                )
+                self.process_repo(collection)


You are using trancaction-bound lock, but process_repo() commits multiple transactions.

Good point. Even though other resolver processes won't pick the collection for processing of the same repo (the last repo_id is stored in the first transaction), it could pick a new one generated in the meantime and stomp on each other. Will fix.

msimacek · 2018-02-13T14:30:23Z

Changes done

mizdebsk

Looks good now. Thanks.

SQLA returns connections to the connection pool after a commit/rollback. That makes usage of session-bound locks tricky, as the following statements may not get the same connection (and thus the same session in postgres sense). This keeps a connection in KoscheiBackendSession across commits.

Ensure that only one collection is being processed at a time.

One resolver process locks single repo ID. Different processes can resolve different repo IDs. More granular parallelism would likely be counterproductive as the repo setup is expensive.

msimacek requested a review from mizdebsk February 13, 2018 12:24

mizdebsk requested changes Feb 13, 2018

View reviewed changes

mizdebsk approved these changes Feb 13, 2018

View reviewed changes

msimacek added 6 commits February 13, 2018 15:33

Add locks module for postgres advisory locking

bff095b

[repo_resolver] Use advisory locks

95d756f

Ensure that only one collection is being processed at a time.

[test] Add simple test for locking

a4fc659

[build_resolver] Allow parallel execution

32d19c1

One resolver process locks single repo ID. Different processes can resolve different repo IDs. More granular parallelism would likely be counterproductive as the repo setup is expensive.

[resolver] Make dependency_cache.get_or_create_nevra concurrency-safe

1ea3e5d

msimacek force-pushed the parallel-resolver branch from 88ce898 to 1ea3e5d Compare February 13, 2018 14:35

msimacek merged commit 079eba6 into master Feb 13, 2018

msimacek deleted the parallel-resolver branch February 13, 2018 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolver parallelization #225

Resolver parallelization #225

msimacek commented Feb 13, 2018

codecov bot commented Feb 13, 2018 •

edited

Loading

mizdebsk left a comment

mizdebsk Feb 13, 2018

msimacek Feb 13, 2018

mizdebsk Feb 13, 2018

msimacek Feb 13, 2018

msimacek commented Feb 13, 2018

mizdebsk left a comment

Resolver parallelization #225

Resolver parallelization #225

Conversation

msimacek commented Feb 13, 2018

codecov bot commented Feb 13, 2018 • edited Loading

Codecov Report

mizdebsk left a comment

Choose a reason for hiding this comment

mizdebsk Feb 13, 2018

Choose a reason for hiding this comment

msimacek Feb 13, 2018

Choose a reason for hiding this comment

mizdebsk Feb 13, 2018

Choose a reason for hiding this comment

msimacek Feb 13, 2018

Choose a reason for hiding this comment

msimacek commented Feb 13, 2018

mizdebsk left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2018 •

edited

Loading