Add temproots in copyPaths, plus all the associated stuff#15719
Add temproots in copyPaths, plus all the associated stuff#15719dramforever wants to merge 8 commits into
Conversation
github-actions
Bot
added
new-cli
dramforever
force-pushed
the
batch-addtemproots
branch
from
f236067 to
67d061f
Compare
Move the writing of the temproots file to before checking the GC lock. This closes a race, however unlikely, where yet another GC may start again after we sent the temproots to the old running GC, but before writing the temproots file.
Introduce LocalStore::addTempRoots, which adds all temproots in a batch for LocalStore. Make this available from the base class Store by making a virtual method Store::addTempRoots, overriding it in LocalStore and RemoteStore (it is just a loop for each path because the benefit of having this be batched is unclear), and making the single-path Store::addTempRoot a wrapper around that.
This avoids some system calls in case of duplicate temproots.
Use the new batched addTempRoots for both convenience and better performance for LocalStore.
Use the new batched addTempRoots for both convenience and better performance for LocalStore.
Add AddTempRootsFlag for optionally adding all queried paths as temproots. This will be used to close a GC race in copyPaths. The existing LegacySSHStore::queryValidPaths subclass overload with the lock parameter is absorbed. LegacySSHStore::Connection::queryValidPaths (i.e. ServeProto::BasicClientConnection::queryValidPaths) remains unchanged.
This closes a race where paths in the result of queryValidPaths can become invalid if a GC happens later that deletes the path, leaving paths invalid after return from copyPaths.
This asks the QueryValidPaths operation to add temproots on the remote, saving roundtrips in RemoteStore::queryValidPaths.
dramforever
force-pushed
the
batch-addtemproots
branch
from
67d061f to
3a2bd29
Compare
| worker.store.addTempRoot(*i.second.second); | ||
| outputPaths.insert(*i.second.second); | ||
|
|
||
| worker.store.addTempRoots(outputPaths); |
There was a problem hiding this comment.
Calling addTempRoots() here is too late. The temp root should be created before creating the derivation, otherwise there is a time window between creation and this point where it can be GC'ed.
I wrote some docs about that here: https://github.com/NixOS/nix/pull/15798/changes
Edit: however this behavior was already present before this PR so it's fine
| if (!tempRootsCache->get(path)) { | ||
| tempRootsCache->upsert(path, {}); |
There was a problem hiding this comment.
We should probably make LRUCache::upsert() return a bool indicating whether it's a new key (similar to std::map::insert_or_assign(), then we can write:
| if (!tempRootsCache->get(path)) { | |
| tempRootsCache->upsert(path, {}); | |
| if (tempRootsCache->upsert(path, {})) { |
edolstra
left a comment
edolstra
left a comment
There was a problem hiding this comment.
Not sure whether we need a addTempRoots() (since it's only used in copyPaths(), where the overhead of calling addTempRoot() N times is probably not a big deal), but it seems useful to have anyway.
| SubstituteFlag substitute) | ||
| { | ||
| auto valid = dstStore.queryValidPaths(storePaths, substitute); | ||
| auto valid = dstStore.queryValidPaths(storePaths, substitute, AddTempRoots); |
There was a problem hiding this comment.
Adding a AddTempRoots flag to queryValidPaths() seems unnecessary (since there is only one use site) and non-orthogonal API design. We can also just do:
| auto valid = dstStore.queryValidPaths(storePaths, substitute, AddTempRoots); | |
| dstStore.addTempRoots(storePaths); | |
| auto valid = dstStore.queryValidPaths(storePaths, substitute); |
There was a problem hiding this comment.
I think it seems unorthogonal, but that's because queryValidPaths already is - if you don't add the temproots, the info returned is already out of date when queryValidPaths returns.
There only one use site of AddTempRoots in this PR because there aren't many use sites to begin with. Most of mentions of queryValidPaths are implementations. There's here, then there's one in overlay store where the lower store is supposed to be read-only, and then there's stuff for querySubstitutablePaths which is used for nix-env -q which can be racy.
Note: this only prevents the paths from being GCed during the copy. You still need to use `nix copy --out-link ...` to prevent GC afterwards. Less ambitious fix than NixOS#15719.
2 participants
Motivation
GC "race" whack-a-mole.
Context
Fixes, hopefully, #1970. Supersedes #15613. Originally discussed in #15616 but split out.
Make
copyPathsadd the copied paths as temproots, but hopefully in a better manner than individually rooting every single path:Store::addTempRootvirtual method to a batchedStore::addTempRootsmethod, with the original one remaining as a non-virtual helper. ImplementLocalStore::addTempRootsas writing out temproots in a batch. Also add an LRU cache forLocalStore::addTempRoots.RemoteStore::addTempRootsis just a loop.AddTempRootsFlagtoStore::queryValidPathscopyPaths, setAddTempRootsFlagwhile callingqueryValidPathsAddTempRootsFlagflag worker protocol operationQueryValidPaths, and use that inRemoteStore::queryValidPaths. This requires a new daemon on the remote side, in exchange for (hopefully) better performance.Adding a flag to worker protocol
QueryValidPathsis somewhat unorthogonal, but it was how it's done in the legacy SSH store, and I do think it still makes sense to do inQueryValidPathsin the worker protocol since it closes a TOCTOU for the result of list of valid paths, if needed by the caller. I haven't added a batchAddTempRootsworker protocol operation since it doesn't seem to be needed.Also note that
LocalStore::addTempRootscode. Including it makes the later commit make more sense.Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.