aptly-dev / aptly

aptly - Debian repository management tool
https://www.aptly.info/
MIT License
2.54k stars 369 forks source link

[RFC] Split reflists to share their contents across snapshots #1235

Closed refi64 closed 1 month ago

refi64 commented 7 months ago

This builds on top of #1222, #1227, and #1233, and is thus in draft state until those are merged in. The only commit that's actually new is the very last one, whose commit message I copied below. (Even that single commit is admittedly quite big, but a sizable chunk of the changes are just plumbing a new RefListCollection around across the code.)

Description of the Change

In current aptly, each repository and snapshot has its own reflist in the database. This brings a few problems with it:

At the core, there are two problems here:

Split reflists aim at solving this by separating reflists into 64 buckets. Package refs are sorted into individual buckets according to the following system:

Once refs are placed in buckets, a sha256 digest of all the refs in the bucket is taken. These buckets are then stored in the database, split into roughly block-sized segments, and all the repositories and snapshots simply store an array of bucket digests.

This approach means that repositories and snapshots can share their reflist buckets. If a snapshot is taken of a repository, it will have the same contents, so its split reflist will point to the same buckets as the base repository, and only one copy of each bucket is stored in the database. When some packages in the repository change, only the buckets containing those packages will be modified; all the other buckets will remain unchanged, and thus their contents will still be shared. Later on, when these reflists are loaded, each bucket is only loaded once, short-cutting loaded many megabytes of data. In effect, split reflists are essentially copy-on-write, with only the changed buckets stored individually.

Changing the disk format means that a migration needs to take place, so that task is moved into the database cleanup step, which will migrate reflists over to split reflists, as well as delete any unused reflist buckets.

All the reflist tests are also changed to additionally test out split reflists; although the internal logic is all shared (since buckets are, themselves, just normal reflists), some special additions are needed to have native versions of the various reflist helper methods.

In our tests, we've observed the following improvements:

In my local tests, publish times had also decreased down to mere seconds but the same effect wasn't observed on the server, with the times staying around the same. My suspicions are that this is due to I/O performance: my local system is an M1 MBP, which almost certainly has much faster disk speeds than our DigitalOcean block volumes. Split reflists include a side effect of requiring more random accesses from reading all the buckets by their keys, so if your random I/O performance is slower, it might cancel out the benefits. That being said, even in that case, the memory usage and database size advantages still persist.

Checklist


It would be awesome if anyone could also test this out and report how it affects their performance & memory usage.

neolynx commented 5 months ago

Thanks for the analysis and solution ! I will give it a try on my side...

neolynx commented 5 months ago

Could you fix the golang-ci-lint errors ?

You can see them by running: docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint golangci-lint run -v

Then the pipeline passes further :)

refi64 commented 5 months ago

@neolynx whoops didn't notice those, fixed now!

neolynx commented 5 months ago

Now we have some tests failing:

https://github.com/aptly-dev/aptly/actions/runs/7558329700/job/20579601126?pr=1235#step:10:5192

not sure this is related... could you maybe check what this is ?

refi64 commented 5 months ago

ah I believe those are because the source apt repos themselves changed, so now the mirrored packages also changed. I think I've seen that test fixed before via manual updates...not sure if you'd like something like that in this PR or elsewhere.

refi64 commented 5 months ago

@neolynx I fixed the mirror tests separately in #1244. I did also discover a small bug buried in the other mirror tests (!) and fixed it here, but I left the commits in #1244 separate to avoid merge hell.

neolynx commented 2 months ago

rebasing dependent PRs on master:

neolynx commented 1 month ago

replaced by #1282