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}") ''; }; }