[backend] reduce race condition in debian repo update #4517
Conversation
suihkulokki
force-pushed
the
bspublish_race
branch
from
2c90717
to
9cd0231
Compare
|
Fixed two mistakes from original PR
|
|
There is apparently another place where Release.gpg is deleted. Looking at obspublisher logs: Adding patch to fix this as well |
|
Please take a look. We've deployed a backport of this, and it would be good to know if it's broken ;) |
|
@DavidKang @mlschroe ping |
|
The travis failure appears a network infrastructure error. |
|
@mlschroe Please review:-) |
|
I don't see the point of the tmpdir. Wouldn't it be enough to just move the .new files at the end of the function instead of right after the compression? |
|
@mlschroe operating in temporary directory with correct filename to begin with just seemed cleaner. We can't just move the files in the end of the function, since they are used as input for generating the md5sums of the Release file. I'll update the PR with an (untested) version that uses .new files. If you still like way better in principle, test and finish that version, else I'll revert to the tmpdir approach. |
suihkulokki
force-pushed
the
bspublish_race
branch
from
59729f0
to
8281aeb
Compare
|
@mlschroe ping! |
|
any update here @mlschroe ? do you want to close it or accept it? @suihkulokki thank you for your contribution.. could you maybe rebase it with master to fix the conflict? |
Regenerating, compressing and signing the debian repository index files takes a while, at least upto an minute observed. Any "apt-get update" during this period will fail. Move temporary files copy to the end, so compress_and_rename is now just copy_compress. Adapt the createrelease_debian funtion to use .new files while printing the names of the final files into the release files. Finally, treat Release.gpg like all the other generated repo files, skip deleting it unless the whole repo is deleted. There still remains a race condition, but now only during milliseconds while the rename loop runs, rather than over minute.
suihkulokki
force-pushed
the
bspublish_race
branch
from
8281aeb
to
2161b0f
Compare
Codecov Report
@@ Coverage Diff @@
## master #4517 +/- ##
==========================================
+ Coverage 90.73% 90.76% +0.02%
==========================================
Files 499 499
Lines 21330 21330
==========================================
+ Hits 19354 19360 +6
+ Misses 1976 1970 -6 |
|
I've updated the review against the current master - this is the version using .new files as suggested by @mlschroe . In production we are still using the temporary directory based approach which I find cleaner. I'm ready to proceed with either version, assuming there is some hope of getting it merged. |
|
It looks like @mlschroe isn't particularly interested in merging this. Sorry @suihkulokki |
|
That's a shame, as this would be definitely be nice to have, as we occasionally see the race on our deployment. |
|
Do not close submissions which are not for your code ... |
|
What is the status of this PR? Is there any prospect of it being merged to fix this race condition? |
Regenerating, compressing and signing the debian repository index
files takes a while, updates taking over a minute have been observed.
Any "apt-get update" during this period can fail. To fix this, update
the indexes in a temporary directory and then replace all index files at
once.
There still remains a race condition, but now only during
milliseconds rather than over minute.
Use of temporary directory means the functions can now be cleaned from
temporary filenames, and this compress_and_rename becomes copy_compress.