Skip to content
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

Using a casual RPC protocol in nix-daemon? #4655

Open
kvtb opened this issue Mar 20, 2021 · 15 comments
Open

Using a casual RPC protocol in nix-daemon? #4655

kvtb opened this issue Mar 20, 2021 · 15 comments
Labels

Comments

@kvtb
Copy link
Contributor

kvtb commented Mar 20, 2021

What about using a casual RPC protocol (GRPC, Thrift, or something) to communicate with nix-daemon?
It would improve readability of the code and simplify development of alternative clients.

@kvtb
Copy link
Contributor Author

kvtb commented Mar 20, 2021

Same about communicating with remote storage.

@kvtb
Copy link
Contributor Author

kvtb commented Mar 20, 2021

And remote builders

@fogti
Copy link
Contributor

fogti commented Mar 23, 2021

what protocol is currently used?

@Ericson2314
Copy link
Member

Ad-hoc one, mainly length encoded things.

@kvtb
Copy link
Contributor Author

kvtb commented Mar 23, 2021

server example:

nix/src/libstore/daemon.cc

Lines 777 to 787 in 61bb1e2

case wopVerifyStore: {
bool checkContents, repair;
from >> checkContents >> repair;
logger->startWork();
if (repair && !trusted)
throw Error("you are not privileged to repair paths");
bool errors = store->verifyStore(checkContents, (RepairFlag) repair);
logger->stopWork();
to << errors;
break;
}

client example:

bool RemoteStore::verifyStore(bool checkContents, RepairFlag repair)
{
auto conn(getConnection());
conn->to << wopVerifyStore << checkContents << repair;
conn.processStderr();
return readInt(conn->from);
}

I'd also say it is a hard job to have all those code pieces in sync, especially for authors of 3rd-party client software.

@kvtb
Copy link
Contributor Author

kvtb commented Mar 23, 2021

I have one of the commands extended with an additional parameter, and it broke compatibility with mainline Nix. And I have to be careful not to run mainline Nix daemon with modified Nix client (or vice versa), because it could crash the daemon (which runs as root)

@fogti
Copy link
Contributor

fogti commented Mar 23, 2021

an easier way might be just using a proper serialization format, e.g. CBOR.

@kvtb
Copy link
Contributor Author

kvtb commented Mar 23, 2021

an easier way might be just using a proper serialization format, e.g. CBOR.

Exactly.
Also many serialization format libraries are accompanied with fellow socket/RPC library, so it might be useful to pick it too at the same moment.

@Ericson2314
Copy link
Member

See #4665, I think that should happen first. Better structuring the code with the legacy protocol, and having just one legacy protocol (!), will free up our developer time and complexity budgets to do a big overhaul like this.

@tomberek
Copy link
Contributor

Would this be appropriate for 3.0 or 4.0 in the roadmap?

@kvtb
Copy link
Contributor Author

kvtb commented Sep 20, 2021

See #4665, I think that should happen first. Better structuring the code with the legacy protocol, and having just one legacy protocol (!)

nix-store --serve may be the best choice for "one legacy protocol" to keep.
It is needed to upgrade old machines anyway.
We will brick them if cancel nix-store --serve and keep only ssh-ng + coming grpc/whatever.

Not fully adopted ssh-ng is much safer to cancel.

@fogti
Copy link
Contributor

fogti commented Sep 20, 2021

I'm against potentially cancelling ssh-ng, as it currently seems necessary to transfer content-addressed derivation realization mappings... (although it still has a few bugs, e.g. it often cancels transfers with "unexpected EOF" errors, which aren't handled properly)

@Ericson2314
Copy link
Member

ssh-ng is easier to keep because it is one we basically get "for free" from the daemon protocol. If we did a fancier thing, we would want to keep that where we can reuse the same protocol on a number of different transports.

@stale
Copy link

stale bot commented Apr 16, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Apr 16, 2022
@QuantumGhost
Copy link

It's still relevant.

@stale stale bot removed the stale label Apr 18, 2022
@stale stale bot added the stale label Oct 30, 2022
@stale stale bot removed the stale label Dec 5, 2022
@stale stale bot added the stale label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants