Split Locks: Use dedicated lock files for builds which will use build hooks for building

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.

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`