From f550edec01ca991efd5e1d4c7c30785ddc99a4f0 Mon Sep 17 00:00:00 2001 From: MaxHearnden Date: Thu, 14 May 2026 20:54:21 +0100 Subject: [PATCH] Use dedicated lock files for builds which will use build hooks for building This allows the build hook to use the local store. In order for the split locks to be used, derivations must either only use the build hook (avoid-local) or only be built locally (forceLocal/bmLocal). --- doc/manual/rl-next/split-locks.md | 12 +++++++ .../build/derivation-building-goal.cc | 35 ++++++++++++------- src/libstore/build/derivation-goal.cc | 5 +-- src/libstore/include/nix/store/machines.hh | 4 ++- src/libstore/include/nix/store/store-api.hh | 2 +- .../include/nix/store/worker-settings.hh | 12 +++++++ src/libstore/machines.cc | 8 +++-- .../unix/build/chroot-derivation-builder.cc | 2 +- src/libstore/worker-protocol.cc | 5 +++ src/nix/build-remote/build-remote.cc | 27 +++++++++----- 10 files changed, 85 insertions(+), 27 deletions(-) create mode 100644 doc/manual/rl-next/split-locks.md diff --git a/doc/manual/rl-next/split-locks.md b/doc/manual/rl-next/split-locks.md new file mode 100644 index 000000000000..bc9261812e13 --- /dev/null +++ b/doc/manual/rl-next/split-locks.md @@ -0,0 +1,12 @@ +--- +synopsis: Split locks +issues: 10740 2589 2029 +prs: 15856 +--- + +New remote only locks are used when [`avoid-local`]{#conf-avoid-local} is true. +To further aid recursive setups, a new builder parameter is added which when +true, disable build hooks (remote builds) on the builder. + +Build hooks will now need to hold the local lock when copying outputs from a +remote builder if [`avoid-local`]{#conf-avoid-local} is true. diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 248ef7507c4e..ccef1d715ed4 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -430,8 +430,14 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) return LocalBuildCapability{*localStoreP, ext}; }(); - auto acquireResources = [&](bool & done, PathLocks & outputLocks) -> Goal::Co { - trace("trying to build"); + auto acquireResources = [&](bool & done, bool remote, PathLocks & outputLocks) -> Goal::Co { + // Enable split locks if build hooks are used for all applicable builds + if (!worker.settings.avoidLocal) + remote = false; + if (remote) + trace("trying to build remotely"); + else + trace("trying to build locally"); /** * Output paths to acquire locks on, if known a priori. @@ -447,17 +453,20 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) if (auto * localStore = dynamic_cast(&worker.store)) { /* If we aren't a local store, we might need to use the local store as a build remote, but that would cause a deadlock. */ - /* FIXME: Make it so we can use ourselves as a build remote even if we - are the local store (separate locking for building vs scheduling? */ /* FIXME: find some way to lock for scheduling for the other stores so a forking daemon with --store still won't farm out redundant builds. */ for (auto & i : drv->outputsAndOptPaths(worker.store)) { - if (i.second.second) - lockFiles.insert(localStore->toRealPath(*i.second.second)); - else { + if (i.second.second) { + auto lockPath = localStore->toRealPath(*i.second.second); + if (remote) + lockPath += "-remote"; + lockFiles.insert(std::move(lockPath)); + } else { auto lockPath = localStore->toRealPath(drvPath); lockPath += "." + i.first; + if (remote) + lockPath += "-remote"; lockFiles.insert(std::move(lockPath)); } } @@ -513,7 +522,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) auto tryHookLoop = [&](bool & valid) -> Goal::Co { { PathLocks outputLocks; - co_await acquireResources(valid, outputLocks); + co_await acquireResources(valid, true, outputLocks); if (valid) co_return doneSuccess(BuildResult::Success::AlreadyValid, checkPathValidity(initialOutputs).second); @@ -546,7 +555,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) while (true) { co_await waitForAWhile(); - co_await acquireResources(valid, outputLocks); + co_await acquireResources(valid, true, outputLocks); if (valid) break; @@ -580,7 +589,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild(StorePathSet inputPaths) auto tryBuildLocally = [&](bool & valid) -> Goal::Co { if (auto * cap = std::get_if(&localBuildResult)) { PathLocks outputLocks; - co_await acquireResources(valid, outputLocks); + co_await acquireResources(valid, false, outputLocks); if (valid) co_return doneSuccess(BuildResult::Success::AlreadyValid, checkPathValidity(initialOutputs).second); @@ -1198,9 +1207,11 @@ HookReply DerivationBuildingGoal::tryBuildHook(const DerivationOptionssink << "try" << (worker.getNrLocalBuilds() < worker.settings.maxBuildJobs ? 1 : 0) - << drv->platform << worker.store.printStorePath(drvPath) + worker.hook->sink << "try" << (canBuildLocally ? 1 : 0) << drv->platform << worker.store.printStorePath(drvPath) << drvOptions.getRequiredSystemFeatures(*drv); worker.hook->sink.flush(); diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index fc4c8a5bb507..f2f43fafc0c8 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -75,7 +75,8 @@ Goal::Co DerivationGoal::haveDerivation(bool storeDerivation) auto checkResult = checkPathValidity(); /* If they are all valid, then we're done. */ - if (checkResult && checkResult->second == PathStatus::Valid && buildMode == bmNormal) { + if (checkResult && checkResult->second == PathStatus::Valid + && (buildMode == bmNormal || buildMode == bmLocal)) { co_return doneSuccess(BuildResult::Success::AlreadyValid, checkResult->first); } @@ -134,7 +135,7 @@ Goal::Co DerivationGoal::haveDerivation(bool storeDerivation) bool allValid = checkResult && checkResult->second == PathStatus::Valid; - if (buildMode == bmNormal && allValid) { + if ((buildMode == bmNormal || buildMode == bmLocal) && allValid) { co_return doneSuccess(BuildResult::Success::Substituted, checkResult->first); } if (buildMode == bmRepair && allValid) { diff --git a/src/libstore/include/nix/store/machines.hh b/src/libstore/include/nix/store/machines.hh index a3e9353c80fa..9f446e826c21 100644 --- a/src/libstore/include/nix/store/machines.hh +++ b/src/libstore/include/nix/store/machines.hh @@ -23,6 +23,7 @@ struct Machine const StringSet supportedFeatures; const StringSet mandatoryFeatures; const std::string sshPublicHostKey; + const bool forceLocal; bool enabled = true; /** @@ -50,7 +51,8 @@ struct Machine decltype(speedFactor) speedFactor, decltype(supportedFeatures) supportedFeatures, decltype(mandatoryFeatures) mandatoryFeatures, - decltype(sshPublicHostKey) sshPublicHostKey); + decltype(sshPublicHostKey) sshPublicHostKey, + decltype(forceLocal) forceLocal); /** * Elaborate `storeUri` into a complete store reference, diff --git a/src/libstore/include/nix/store/store-api.hh b/src/libstore/include/nix/store/store-api.hh index b6114cce65e7..9ed166840af1 100644 --- a/src/libstore/include/nix/store/store-api.hh +++ b/src/libstore/include/nix/store/store-api.hh @@ -50,7 +50,7 @@ enum CheckSigsFlag : bool { NoCheckSigs = false, CheckSigs = true }; enum SubstituteFlag : bool { NoSubstitute = false, Substitute = true }; -enum BuildMode : uint8_t { bmNormal, bmRepair, bmCheck }; +enum BuildMode : uint8_t { bmNormal, bmRepair, bmCheck, bmLocal }; enum TrustedFlag : bool { NotTrusted = false, Trusted = true }; diff --git a/src/libstore/include/nix/store/worker-settings.hh b/src/libstore/include/nix/store/worker-settings.hh index 13fa0261d8d0..997b56e176e3 100644 --- a/src/libstore/include/nix/store/worker-settings.hh +++ b/src/libstore/include/nix/store/worker-settings.hh @@ -198,6 +198,8 @@ public: The value for this field can be obtained via `base64 -w0`. + 9. A boolean which if true, forces the build to not use builders on the remote store + > **Example** > > Multiple builders specified on the command line: @@ -270,6 +272,16 @@ public: This can drastically reduce build times if the network connection between the local machine and the remote build host is slow. )"}; + Setting avoidLocal{ + this, + false, + "avoid-local", + R"( + If set to `true`, Nix will not build derivations locally which can be built by the [`builders`]{#conf-builders}. This does not include derivations with preferLocalBuild set for which [`max-jobs`]{#conf-max-jobs} still applies. + + This enables remote only locks which are not used by local builds + )"}; + Setting useSubstitutes{ this, true, diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index 17864ca5ace6..e3ee41d0738c 100644 --- a/src/libstore/machines.cc +++ b/src/libstore/machines.cc @@ -14,7 +14,8 @@ Machine::Machine( decltype(speedFactor) speedFactor, decltype(supportedFeatures) supportedFeatures, decltype(mandatoryFeatures) mandatoryFeatures, - decltype(sshPublicHostKey) sshPublicHostKey) + decltype(sshPublicHostKey) sshPublicHostKey, + decltype(forceLocal) forceLocal) : storeUri( StoreReference::parse( // Backwards compatibility: if the URI is schemeless, is not a path, @@ -32,6 +33,7 @@ Machine::Machine( , supportedFeatures(supportedFeatures) , mandatoryFeatures(mandatoryFeatures) , sshPublicHostKey(sshPublicHostKey) + , forceLocal(forceLocal) { if (speedFactor < 0.0) throw UsageError("speed factor must be >= 0"); @@ -188,7 +190,9 @@ static Machine parseBuilderLine(const StringSet & defaultSystems, const std::str // `mandatoryFeatures` isSet(6) ? tokenizeString(tokens[6], ",") : StringSet{}, // `sshPublicHostKey` - isSet(7) ? ensureBase64(7) : ""}; + isSet(7) ? ensureBase64(7) : "", + // `forceLocal` + isSet(8) ? (tokens[8] == "true") : false}; } static Machines parseBuilderLines(const StringSet & defaultSystems, const std::vector & builders) diff --git a/src/libstore/unix/build/chroot-derivation-builder.cc b/src/libstore/unix/build/chroot-derivation-builder.cc index 157d9e319173..a384fc81a6d3 100644 --- a/src/libstore/unix/build/chroot-derivation-builder.cc +++ b/src/libstore/unix/build/chroot-derivation-builder.cc @@ -114,7 +114,7 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl /* Move paths out of the chroot for easier debugging of build failures. */ - if (!force && buildMode == bmNormal) + if (!force && (buildMode == bmNormal || buildMode == bmLocal)) for (auto & [_, status] : initialOutputs) { if (!status.known) continue; diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc index 37aa4e31bec2..32aaeb1bb0a7 100644 --- a/src/libstore/worker-protocol.cc +++ b/src/libstore/worker-protocol.cc @@ -67,6 +67,8 @@ BuildMode WorkerProto::Serialise::read(const StoreDirConfig & store, return bmRepair; case 2: return bmCheck; + case 3: + return bmLocal; default: throw Error("Invalid build mode"); } @@ -85,6 +87,9 @@ void WorkerProto::Serialise::write( case bmCheck: conn.to << uint8_t{2}; break; + case bmLocal: + conn.to << uint8_t{3}; + break; default: assert(false); }; diff --git a/src/nix/build-remote/build-remote.cc b/src/nix/build-remote/build-remote.cc index e5ebf948d7e2..d5589aa1c18e 100644 --- a/src/nix/build-remote/build-remote.cc +++ b/src/nix/build-remote/build-remote.cc @@ -110,6 +110,10 @@ static int main_build_remote(int argc, char ** argv) std::shared_ptr sshStore; AutoCloseFD bestSlotLock; + // Whether to force local building on the target store bypassing build + // hooks + bool forceLocal = false; + auto machines = Machine::parseConfig({settings.thisSystem}, settings.getWorkerSettings().builders); debug("got %d remote builders", machines.size()); @@ -252,6 +256,8 @@ static int main_build_remote(int argc, char ** argv) sshStore = bestMachine->openStore(); sshStore->connect(); + + forceLocal = bestMachine->forceLocal; } catch (std::exception & e) { auto msg = chomp(drainFD(5, {.block = false})); printError("cannot build on '%s': %s%s", storeUri, e.what(), msg.empty() ? "" : ": " + msg); @@ -321,6 +327,8 @@ static int main_build_remote(int argc, char ** argv) !trusted || *trusted; }); + auto buildMode = forceLocal ? bmLocal : bmNormal; + // See the very large comment in `case WorkerProto::Op::BuildDerivation:` in // `src/libstore/daemon.cc` that explains the trust model here. // @@ -338,7 +346,7 @@ static int main_build_remote(int argc, char ** argv) // output ids, which break CA derivations if (!drv.inputDrvs.map.empty()) drv.inputSrcs = store->parseStorePathSet(inputs); - optResult = sshStore->buildDerivation(*drvPath, static_cast(drv)); + optResult = sshStore->buildDerivation(*drvPath, static_cast(drv), buildMode); auto & result = *optResult; if (auto * failureP = result.tryGetFailure()) { if (settings.keepFailed) { @@ -353,10 +361,12 @@ static int main_build_remote(int argc, char ** argv) } } else { copyClosure(*store, *sshStore, StorePathSet{*drvPath}, NoRepair, NoCheckSigs, substitute); - auto res = sshStore->buildPathsWithResults({DerivedPath::Built{ - .drvPath = makeConstantStorePathRef(*drvPath), - .outputs = OutputsSpec::All{}, - }}); + auto res = sshStore->buildPathsWithResults( + {DerivedPath::Built{ + .drvPath = makeConstantStorePathRef(*drvPath), + .outputs = OutputsSpec::All{}, + }}, + buildMode); // One path to build should produce exactly one build result assert(res.size() == 1); optResult = std::move(res[0]); @@ -392,9 +402,10 @@ static int main_build_remote(int argc, char ** argv) if (!missingPaths.empty()) { Activity act(*logger, lvlTalkative, actUnknown, fmt("copying outputs from '%s'", storeUri)); - if (auto localStore = store.dynamic_pointer_cast()) - for (auto & path : missingPaths) - localStore->locksHeld.insert(store->printStorePath(path)); /* FIXME: ugly */ + if (!settings.getWorkerSettings().avoidLocal) + if (auto localStore = store.dynamic_pointer_cast()) + for (auto & path : missingPaths) + localStore->locksHeld.insert(store->printStorePath(path)); /* FIXME: ugly */ copyPaths(*sshStore, *store, missingPaths, NoRepair, NoCheckSigs, NoSubstitute); } // XXX: Should be done as part of `copyPaths`