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).

Author: MaxHearnden

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.

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<LocalStore *>(&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<LocalBuildCapability>(&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 DerivationOptions<StorePath
 
     try {
 
+        auto canBuildLocally =
+            (worker.getNrLocalBuilds() < worker.settings.maxBuildJobs) && !worker.settings.avoidLocal;
+
         /* Send the request to the hook. */
-        worker.hook->sink << "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();
 

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) {

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,

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 };
 

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<bool> 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<bool> useSubstitutes{
         this,
         true,

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<StringSet>(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<std::string> & builders)

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;

src/libstore/worker-protocol.cc

@@ -67,6 +67,8 @@ BuildMode WorkerProto::Serialise<BuildMode>::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<BuildMode>::write(
     case bmCheck:
         conn.to << uint8_t{2};
         break;
+    case bmLocal:
+        conn.to << uint8_t{3};
+        break;
     default:
         assert(false);
     };

src/nix/build-remote/build-remote.cc

@@ -110,6 +110,10 @@ static int main_build_remote(int argc, char ** argv)
         std::shared_ptr<Store> 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<const BasicDerivation &>(drv));
+            optResult = sshStore->buildDerivation(*drvPath, static_cast<const BasicDerivation &>(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<LocalStore>())
-                for (auto & path : missingPaths)
-                    localStore->locksHeld.insert(store->printStorePath(path)); /* FIXME: ugly */
+            if (!settings.getWorkerSettings().avoidLocal)
+                if (auto localStore = store.dynamic_pointer_cast<LocalStore>())
+                    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`