From f92235e1d2ebe7f5fa8048543c4f59661d52f787 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 15 Apr 2025 19:14:30 +0200 Subject: [PATCH] libstore: stop using ssh connection sharing our implementation is mostly unused, completely untested, and simply breaks when users configure connection sharing independently. we can safely delete this "feature" and inherit user configuration instead. also have the remote build test check that connection sharing works. fixes #304, fixes #644 Change-Id: Iea44cc0f8e51a1d231ad186a95c7e310bbfeb303 --- .../rl-next/ssh-connection-sharing-removed.md | 14 ++++ lix/libstore/legacy-ssh-store.cc | 2 - lix/libstore/ssh-store.cc | 2 - lix/libstore/ssh.cc | 76 +------------------ lix/libstore/ssh.hh | 8 +- tests/nixos/remote-builds.nix | 24 +++++- 6 files changed, 42 insertions(+), 84 deletions(-) create mode 100644 doc/manual/rl-next/ssh-connection-sharing-removed.md diff --git a/doc/manual/rl-next/ssh-connection-sharing-removed.md b/doc/manual/rl-next/ssh-connection-sharing-removed.md new file mode 100644 index 000000000..e9023497d --- /dev/null +++ b/doc/manual/rl-next/ssh-connection-sharing-removed.md @@ -0,0 +1,14 @@ +--- +synopsis: "Remove lix-initiated ssh connection sharing" +issues: [fj#304, fj#644] +cls: [3005] +category: Fixes +credits: [horrors] +--- + +Lix no longer explicitly requests ssh connection sharing when connecting to remote stores. +This may impact command latency when `NIX_REMOTE` is set to a `ssh://` or `ssh-ng://` url, +or if `--store` is specified. Remote build connections did not use ssh connection sharing. + +Connection sharing configuration is now inherited from user configuration at all times. It +is now advisable to configure connection sharing for remote builders for improved latency. diff --git a/lix/libstore/legacy-ssh-store.cc b/lix/libstore/legacy-ssh-store.cc index d5de5f19b..5a939f2a6 100644 --- a/lix/libstore/legacy-ssh-store.cc +++ b/lix/libstore/legacy-ssh-store.cc @@ -118,8 +118,6 @@ struct LegacySSHStore final : public Store config_.port, config_.sshKey, config_.sshPublicHostKey, - // Use SSH master only if using more than 1 connection. - connections->capacity() > 1, config_.compress, config_.logFD) { diff --git a/lix/libstore/ssh-store.cc b/lix/libstore/ssh-store.cc index 757230d86..0a3fa0706 100644 --- a/lix/libstore/ssh-store.cc +++ b/lix/libstore/ssh-store.cc @@ -42,8 +42,6 @@ public: config_.port, config_.sshKey, config_.sshPublicHostKey, - // Use SSH master only if using more than 1 connection. - connections->capacity() > 1, config_.compress) { } diff --git a/lix/libstore/ssh.cc b/lix/libstore/ssh.cc index 8ad7f7e53..8311b2c8a 100644 --- a/lix/libstore/ssh.cc +++ b/lix/libstore/ssh.cc @@ -8,13 +8,12 @@ namespace nix { -SSHMaster::SSHMaster(const std::string & host, const std::optional port, const std::string & keyFile, const std::string & sshPublicHostKey, bool useMaster, bool compress, int logFD) +SSHMaster::SSHMaster(const std::string & host, const std::optional port, const std::string & keyFile, const std::string & sshPublicHostKey, bool compress, int logFD) : host(host) , port(port) , fakeSSH(host == "localhost") , keyFile(keyFile) , sshPublicHostKey(sshPublicHostKey) - , useMaster(useMaster && !fakeSSH) , compress(compress) , logFD(logFD) { @@ -44,23 +43,10 @@ void SSHMaster::addCommonSSHOpts(Strings & args) } if (compress) args.push_back("-C"); - - args.push_back("-oPermitLocalCommand=yes"); - args.push_back("-oLocalCommand=echo started"); -} - -bool SSHMaster::isMasterRunning() { - Strings args = {"-O", "check", host}; - addCommonSSHOpts(args); - - auto res = runProgram(RunOptions {.program = "ssh", .args = args, .mergeStderrToStdout = true}); - return res.first == 0; } std::unique_ptr SSHMaster::startCommand(const std::string & command) { - Path socketPath = startMaster(); - Pipe in, out; in.create(); out.create(); @@ -70,7 +56,7 @@ std::unique_ptr SSHMaster::startCommand(const std::string options.dieWithParent = false; std::optional>> resumeLoggerDefer; - if (!fakeSSH && !useMaster) { + if (!fakeSSH) { logger->pause(); resumeLoggerDefer.emplace([&]() { logger->resume(); }); } @@ -95,11 +81,9 @@ std::unique_ptr SSHMaster::startCommand(const std::string } else { args = { "ssh", host.c_str(), "-x" }; addCommonSSHOpts(args); - if (socketPath != "") - args.insert(args.end(), {"-S", socketPath}); } - args.push_back(command); + args.push_back(fmt("echo started; %s", command)); execvp(args.begin()->c_str(), stringsToCharPtrs(args).data()); // could not exec ssh/bash @@ -112,7 +96,7 @@ std::unique_ptr SSHMaster::startCommand(const std::string // Wait for the SSH connection to be established, // So that we don't overwrite the password prompt with our progress bar. - if (!fakeSSH && !useMaster && !isMasterRunning()) { + { std::string reply; try { reply = readLine(out.readSide.get()); @@ -130,56 +114,4 @@ std::unique_ptr SSHMaster::startCommand(const std::string return conn; } -Path SSHMaster::startMaster() -{ - if (!useMaster) return ""; - - auto state(state_.lock()); - - if (state->sshMaster) return state->socketPath; - - state->socketPath = (Path) *state->tmpDir + "/ssh.sock"; - - Pipe out; - out.create(); - - ProcessOptions options; - options.dieWithParent = false; - - logger->pause(); - Finally cleanup = [&]() { logger->resume(); }; - - if (isMasterRunning()) - return state->socketPath; - - state->sshMaster = startProcess([&]() { - restoreProcessContext(); - - close(out.readSide.get()); - - if (dup2(out.writeSide.get(), STDOUT_FILENO) == -1) - throw SysError("duping over stdout"); - - Strings args = { "ssh", host.c_str(), "-M", "-N", "-S", state->socketPath }; - addCommonSSHOpts(args); - execvp(args.begin()->c_str(), stringsToCharPtrs(args).data()); - - throw SysError("unable to execute '%s'", args.front()); - }, options); - - out.writeSide.reset(); - - std::string reply; - try { - reply = readLine(out.readSide.get()); - } catch (EndOfFile & e) { } - - if (reply != "started") { - printTalkative("SSH master stdout first line: %s", reply); - throw Error("failed to start SSH master connection to '%s'", host); - } - - return state->socketPath; -} - } diff --git a/lix/libstore/ssh.hh b/lix/libstore/ssh.hh index 94609e74e..31d6dd576 100644 --- a/lix/libstore/ssh.hh +++ b/lix/libstore/ssh.hh @@ -17,25 +17,21 @@ private: bool fakeSSH; const std::string keyFile; const std::string sshPublicHostKey; - const bool useMaster; const bool compress; const int logFD; struct State { - Pid sshMaster; std::unique_ptr tmpDir; - Path socketPath; }; Sync state_; void addCommonSSHOpts(Strings & args); - bool isMasterRunning(); public: - SSHMaster(const std::string & host, const std::optional port, const std::string & keyFile, const std::string & sshPublicHostKey, bool useMaster, bool compress, int logFD = -1); + SSHMaster(const std::string & host, const std::optional port, const std::string & keyFile, const std::string & sshPublicHostKey, bool compress, int logFD = -1); struct Connection { @@ -44,8 +40,6 @@ public: }; std::unique_ptr startCommand(const std::string & command); - - Path startMaster(); }; } diff --git a/tests/nixos/remote-builds.nix b/tests/nixos/remote-builds.nix index d2ed7853a..cfaa17c08 100644 --- a/tests/nixos/remote-builds.nix +++ b/tests/nixos/remote-builds.nix @@ -75,7 +75,23 @@ in virtualisation.writableStore = true; virtualisation.additionalPaths = [ config.system.build.extraUtils ]; nix.settings.substituters = lib.mkForce [ ]; - programs.ssh.extraConfig = "ConnectTimeout 30"; + programs.ssh.extraConfig = '' + ConnectTimeout 30 + Host builder2-cs + HostName builder2 + ControlMaster auto + ControlPersist yes + ControlPath ~/.ssh/builder2-cs.sock + ''; + specialisation.with-sharing.configuration.nix.buildMachines = lib.mkForce [ + { + hostName = "builder2-cs"; + sshUser = "root"; + sshKey = "/root/.ssh/id_ed25519"; + system = "i686-linux"; + maxJobs = 1; + } + ]; }; }; @@ -126,6 +142,12 @@ in # Test whether the build hook automatically skips unavailable builders. builder1.block() client.succeed("nix-build ${expr nodes.client 4}") + + # test that connection sharing doesn't break anything + client.succeed("/run/current-system/specialisation/with-sharing/bin/switch-to-configuration test") + client.succeed("ssh builder2-cs true") + client.succeed("ssh -O check builder2-cs") + client.succeed("nix-build ${expr nodes.client 6}") ''; }; }