-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement RemoteStore.addToStoreFromDump #4009
Conversation
Not sure we should add this. |
This is the only method in the This is more efficient than
Whereas with
That seems quite important. I'll check if there's more commonality that can be factored out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Baring potentially using FramedSink
/FramedSource
, looks good to me. Agreed we should not get rid of addToStoreFromDump
because it's the only one that calculates the content address for you.
For similar reasons I made a |
Also, https://github.com/NixOS/nix/pull/3829/files too helps with chaining stores. |
fddb7c4
to
9fff3ec
Compare
I've added the new
|
If we are making a new version of The other end is s bit cumbersome because the protocol always dumps a NAR, but the |
@Ericson2314 I've spent a long time reasoning the implications of using a plain dump and trying to implement it, but it doesn't seem to pay off. We'll introduce a lot of complexity in the form of compatibility logic around the daemon protocol and all it might ever save is a single
I have to conclude that the refactoring isn't worth it. I suggest someone could do it if they really want to, and they can do it even after this PR. I don't think it's worth their time and effort though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ready for review now.
I've added some comments to clarify the changes.
Just in case, I've done additional testing by building a fresh package including a filterSource
source and running nix-store --verify-path
on the derivation closure and output closure, using a chained nix-daemon where the final daemon is Nix 2.4 before this change, the intermediate daemon is from this PR and the client is either from this PR or from before.
src/libstore/remote-store.cc
Outdated
} | ||
} | ||
conn.withFramedSink([&](Sink & sink) { | ||
copyNAR(source, sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored, behavior unchanged.
@@ -982,6 +931,83 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * | |||
return nullptr; | |||
} | |||
|
|||
|
|||
struct FramedSink : nix::BufferedSink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved for reuse, unchanged.
}; | ||
|
||
void | ||
ConnectionHandle::withFramedSink(std::function<void(Sink &sink)> fun) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted from void RemoteStore::addToStore(const ValidPathInfo & info, Source & source, .....
, unchanged.
StorePath addToStoreFromDump(Source & dump, const string & name, | ||
FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, | ||
PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair) override; | ||
RepairFlag repair = NoRepair) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub may render this as a "change" but it's really a deletion and an addition.
Addition:
- Providing a real implementation instead of throwing an exception realizes the goal of this PR, allowing daemons to be chained
Deletion:
addToStore(... Path ...)
has a default implementation that delegates toaddToStoreFromDump
which is all we need
@@ -398,4 +400,51 @@ struct StreamToSourceAdapter : Source | |||
}; | |||
|
|||
|
|||
/* Like SizedSource, but guarantees that the whole frame is consumed after | |||
destruction. This ensures that the original stream is in a known state. */ | |||
struct FramedSource : Source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved, unchanged.
src/libstore/daemon.cc
Outdated
} | ||
logger->stopWork(); | ||
|
||
if (path /* always */) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can be removed or replaced by assert(path)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the scope to a lambda to allow returning path without the optional
contraption.
@@ -50,6 +50,7 @@ typedef enum { | |||
wopAddToStoreNar = 39, | |||
wopQueryMissing = 40, | |||
wopQueryDerivationOutputMap = 41, | |||
wopAddToStoreCANar = 42, // since Nix 3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be called wopAddToStoreFromDump
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it's a sibling of wopAddToStoreNar
, so I've added CA
to distinguish it, being only capable of producing that kind of store path. Dump
sounds a bit vague to me, whereas Nar
is specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah wopAddToStoreFromDump
sounds like it isn't always expected nar-formatted objects, which would be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually I wonder if we shouldn't just reuse wopAddToStore
(by checking the daemon protocol) since this fully replaces it.
That's fine, we've had trouble with it too. But remember the second alternative I proposed is fixing up Your current deamon code is wrong because it doesn't yet jump through the hoops of using Could you either change the interface of |
If you go with the |
I don't think that's the case (I don't think I've made a mistake with my testing..) because the protocol now uses the exact same format as
The only downside of having NAR as the format for It's quite nice having to deal with only one dump format. Only I'd like to keep a reasonable scope for this PR and not overhaul |
529b115
to
bef138c
Compare
bef138c
to
32a2883
Compare
Ok, rebased and formatted, so should be mergeable. @Ericson2314 Thanks for the example. I think I'm going to leave it as is because the new daemon implementation is already constant memory with |
This is not true. |
Yes, that is still the case. See Maybe let's just get the preperatory change pulling out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ericson2314 Thanks for pointing out a problem I wasn't going to find with testing.
I'll rename it to wopAddToStoreCA
because it's not always a NAR.
I didn't assume depending on the server/client version was an option because it's not how compatibility was handled earlier, but it seems quite sensible to just replace wopAddToStore
.
@edolstra I propose that we simplify the handling of these dumps by not parsing in "intermediate" methods unless strictly necessary. The only reason we had to do so is to find the end of the NAR, which can now be done with FramedSource
, which separates the end of the NAR from the end of the stream that transports the frames. The legacy store protocol code and nix-store --import
can still perform the NAR parsing that's required for their correctness, but it doesn't have to be part of the core.
EDIT: I'll update the PR later today.
FramedSource narSource(from); | ||
return store->addToStoreFromDump(narSource, baseName, method, hashAlgo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't NAR-specific so it already works for either method.
|
||
{ | ||
FramedSink sink(conn, ex); | ||
conn.withFramedSink([&](Sink & sink) { | ||
copyNAR(source, sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated but refactored.
<< printHashType(hashAlgo); | ||
|
||
conn.withFramedSink([&](Sink & sink) { | ||
copyNAR(narSource, sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fail on Flat
but didn't in my testing because addToStoreFromDump
invocations are rare and most if not all current use cases for adding Flat
store paths build a ValidPathInfo
first and use wopAddToStoreNar
.
@roberth Yeah it's very annoying it's not tested, but I think |
@Ericson2314 I've used |
This implements
RemoteStore.addToStoreFromDump
, allowingnix-daemon
to serve as a proxy for anothernix-daemon
. A possible use case for this is distrusting a container that runs as an otherwise trusted uid. The implementation was fairly straightforward.