Fix/publish concurrency #1271
Merged
Fix/publish concurrency #1271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neolynx
force-pushed
the
fix/publish-concurrency
branch
from
8464cad
to
346e782
Compare
|
the test t12_api:TaskAPITestParallelTasks failes with this implementation... does it need to be updated? Create Mirror in parallel seems now to return 202, but 409 was expected. |
neolynx
force-pushed
the
fix/publish-concurrency
branch
from
fa7b78b
to
37d521f
Compare
|
Sorry for the tardy response, I'll look over what happened after the rebase :) |
neolynx
force-pushed
the
fix/publish-concurrency
branch
2 times, most recently
from
fb753df
to
348ea3e
Compare
neolynx
force-pushed
the
fix/publish-concurrency
branch
2 times, most recently
from
2f20f12
to
2eedb67
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1271 +/- ##
==========================================
- Coverage 74.86% 74.63% -0.24%
==========================================
Files 144 144
Lines 16261 16299 +38
==========================================
- Hits 12174 12164 -10
- Misses 3149 3188 +39
- Partials 938 947 +9 ☔ View full report in Codecov by Sentry. |
|
Hi ! I implemented a queue, async tasks are now executed squentially in order, as soon as the required resources are avaiilable. could you give this a try ? |
neolynx
force-pushed
the
fix/publish-concurrency
branch
2 times, most recently
from
471872d
to
629957a
Compare
neolynx
requested review from
randombenj,
mfolusiak,
szakalboss,
dario-gallucci and
a team
neolynx
added
needs review
|
Aptly is now also using the queue for synchronous tasks. this should fix the concurrency problem completely. Could you test and confirm ? Thanks ! |
neolynx
force-pushed
the
fix/publish-concurrency
branch
2 times, most recently
from
dbd357c
to
7e2ce5a
Compare
neolynx
force-pushed
the
fix/publish-concurrency
branch
from
40906aa
to
b3f76c1
Compare
6 tasks
neolynx
force-pushed
the
fix/publish-concurrency
branch
from
b3f76c1
to
3794c84
Compare
This commit introduces a test which runs concurrent publishes (which could be parallell with multiproccessing, python is fun). The test purposly fails (at the point in history that this patch is written) in order to make it as easy as possible to verify later patches, which hopefully addresses concurrency problems. The same behaviour can easily be tested outside of the system tests with the following (or similar) shell $ aptly serve -listen=:8080 -no-lock $ aptly repo create create -distributions=testing local-repo $ atply publish repo -architectures=amd64 local-repo $ apt download aptly $ aptly repo add local-repo ./aptly*.deb $ for _ in $(seq 10); do curl -X PUT 127.0.0.1:8080/api/publish//testing In the local testing perfomed (on a dual core vm) the first 1-4 jobs would typically succeed and the rest would error out.
This commit blocks concurrent calls to RunTaskInBackground which is intended to fix the quirky behaviour where concurrent PUT calls to api/publish/<prefix>/<distribution> would immedietly reuturn an error. The solution proposed in this commit is not elegant and probaly has unintended side-effects. The intention of this commit is to highlight the area that actually needs to be addressed. Ideally this patch is amended or dropped entierly in favor of a better fixup.
neolynx
force-pushed
the
fix/publish-concurrency
branch
from
3794c84
to
fdce655
Compare
mfolusiak
approved these changes
Jun 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces #1261
Fixes #1276, #1125
Description of the Change
PR Updated: synchronous and asynchronous requests are queued until the resoures become available.
This MR addresses a concurrency issue with the
api/publishendpoint, where concurrent PUTs typically fail. The MR in it self is not pretty, so consider this initial state of the MR a starting point of discussion.The commits are intentionally separated in order to make it as easy as possible to observe the failing test (and test it against other likely better code changes).