From f602ed0d86a661d6f668c8b0f99c6ce7ca417e97 Mon Sep 17 00:00:00 2001 From: regnat Date: Tue, 23 Feb 2021 09:50:15 +0100 Subject: [PATCH] Remove the `sendDerivation` logic from the builder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The queue runner used to special-case `localhost` as a remote builder: Rather than using the normal remote-build (using the `cmdBuildDerivation` command), it was using the (generally less efficient, except when running against localhost) `cmdBuildPaths` command because the latter didn't require a privileged Nix user (so made testing easier − allowing to run hydra in a container in particular). However: 1. this means that the build loop can follow two discint code paths depending on the setup, the irony being that the most commonly used one in production (the “non-localhost” case) isn't the one used in the testsuite (because all the tests run against a local store); 2. It turns out that the “localhost” version is buggy in relatively obvious ways − in particular a failure in a fixed-output derivation or a hash mismatch isn't reported properly; 3. If the “run in a container” use-case is indeed that important, it can be (partially) restored using a chroot store (which wouldn't behave excactly the same way of course, but would be more than good-enough for testing) --- src/hydra-queue-runner/build-remote.cc | 154 ++++++++++--------------- 1 file changed, 60 insertions(+), 94 deletions(-) diff --git a/src/hydra-queue-runner/build-remote.cc b/src/hydra-queue-runner/build-remote.cc index 2f9df16f..a68575c7 100644 --- a/src/hydra-queue-runner/build-remote.cc +++ b/src/hydra-queue-runner/build-remote.cc @@ -210,7 +210,6 @@ void State::buildRemote(ref destStore, }); /* Handshake. */ - bool sendDerivation = true; unsigned int remoteVersion; try { @@ -223,12 +222,6 @@ void State::buildRemote(ref destStore, remoteVersion = readInt(from); if (GET_PROTOCOL_MAJOR(remoteVersion) != 0x200) throw Error("unsupported ‘nix-store --serve’ protocol version on ‘%1%’", machine->sshName); - // Always send the derivation to localhost, since it's a - // no-op anyway but we might not be privileged to use - // cmdBuildDerivation (e.g. if we're running in a NixOS - // container). - if (GET_PROTOCOL_MINOR(remoteVersion) >= 1 && !machine->isLocalhost()) - sendDerivation = false; if (GET_PROTOCOL_MINOR(remoteVersion) < 3 && repeats > 0) throw Error("machine ‘%1%’ does not support repeating a build; please upgrade it to Nix 1.12", machine->sshName); @@ -253,11 +246,8 @@ void State::buildRemote(ref destStore, StorePathSet inputs; BasicDerivation basicDrv(*step->drv); - if (sendDerivation) - inputs.insert(step->drvPath); - else - for (auto & p : step->drv->inputSrcs) - inputs.insert(p); + for (auto & p : step->drv->inputSrcs) + inputs.insert(p); for (auto & input : step->drv->inputDrvs) { auto drv2 = localStore->readDerivation(input.first); @@ -313,13 +303,8 @@ void State::buildRemote(ref destStore, updateStep(ssBuilding); - if (sendDerivation) { - to << cmdBuildPaths; - worker_proto::write(*localStore, to, StorePathSet{step->drvPath}); - } else { - to << cmdBuildDerivation << localStore->printStorePath(step->drvPath); - writeDerivation(to, *localStore, basicDrv); - } + to << cmdBuildDerivation << localStore->printStorePath(step->drvPath); + writeDerivation(to, *localStore, basicDrv); to << maxSilentTime << buildTimeout; if (GET_PROTOCOL_MINOR(remoteVersion) >= 2) to << maxLogSize; @@ -337,83 +322,64 @@ void State::buildRemote(ref destStore, } result.stopTime = time(0); - if (sendDerivation) { - if (res) { - result.errorMsg = fmt("%s on ‘%s’", readString(from), machine->sshName); - if (res == 100) { - result.stepStatus = bsFailed; - result.canCache = true; - } - else if (res == 101) { - result.stepStatus = bsTimedOut; - } - else { - result.stepStatus = bsAborted; - result.canRetry = true; - } - return; + result.errorMsg = readString(from); + if (GET_PROTOCOL_MINOR(remoteVersion) >= 3) { + result.timesBuilt = readInt(from); + result.isNonDeterministic = readInt(from); + auto start = readInt(from); + auto stop = readInt(from); + if (start && start) { + /* Note: this represents the duration of a single + round, rather than all rounds. */ + result.startTime = start; + result.stopTime = stop; } - result.stepStatus = bsSuccess; - } else { - result.errorMsg = readString(from); - if (GET_PROTOCOL_MINOR(remoteVersion) >= 3) { - result.timesBuilt = readInt(from); - result.isNonDeterministic = readInt(from); - auto start = readInt(from); - auto stop = readInt(from); - if (start && start) { - /* Note: this represents the duration of a single - round, rather than all rounds. */ - result.startTime = start; - result.stopTime = stop; - } - } - switch ((BuildResult::Status) res) { - case BuildResult::Built: - result.stepStatus = bsSuccess; - break; - case BuildResult::Substituted: - case BuildResult::AlreadyValid: - result.stepStatus = bsSuccess; - result.isCached = true; - break; - case BuildResult::PermanentFailure: - result.stepStatus = bsFailed; - result.canCache = true; - result.errorMsg = ""; - break; - case BuildResult::InputRejected: - case BuildResult::OutputRejected: - result.stepStatus = bsFailed; - result.canCache = true; - break; - case BuildResult::TransientFailure: - result.stepStatus = bsFailed; - result.canRetry = true; - result.errorMsg = ""; - break; - case BuildResult::TimedOut: - result.stepStatus = bsTimedOut; - result.errorMsg = ""; - break; - case BuildResult::MiscFailure: - result.stepStatus = bsAborted; - result.canRetry = true; - break; - case BuildResult::LogLimitExceeded: - result.stepStatus = bsLogLimitExceeded; - break; - case BuildResult::NotDeterministic: - result.stepStatus = bsNotDeterministic; - result.canRetry = false; - result.canCache = true; - break; - default: - result.stepStatus = bsAborted; - break; - } - if (result.stepStatus != bsSuccess) return; } + switch ((BuildResult::Status) res) { + case BuildResult::Built: + result.stepStatus = bsSuccess; + break; + case BuildResult::Substituted: + case BuildResult::AlreadyValid: + result.stepStatus = bsSuccess; + result.isCached = true; + break; + case BuildResult::PermanentFailure: + result.stepStatus = bsFailed; + result.canCache = true; + result.errorMsg = ""; + break; + case BuildResult::InputRejected: + case BuildResult::OutputRejected: + result.stepStatus = bsFailed; + result.canCache = true; + break; + case BuildResult::TransientFailure: + result.stepStatus = bsFailed; + result.canRetry = true; + result.errorMsg = ""; + break; + case BuildResult::TimedOut: + result.stepStatus = bsTimedOut; + result.errorMsg = ""; + break; + case BuildResult::MiscFailure: + result.stepStatus = bsAborted; + result.canRetry = true; + break; + case BuildResult::LogLimitExceeded: + result.stepStatus = bsLogLimitExceeded; + break; + case BuildResult::NotDeterministic: + result.stepStatus = bsNotDeterministic; + result.canRetry = false; + result.canCache = true; + break; + default: + result.stepStatus = bsAborted; + break; + } + if (result.stepStatus != bsSuccess) return; result.errorMsg = "";