Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow building content-addressed derivations with hydra - reprise #1228

Closed
wants to merge 40 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
00c2e2c
Fix with ca derivations
thufschmitt Dec 21, 2020
030d72e
Add some tests for the content-addressed derivations
thufschmitt Feb 18, 2021
111abeb
Properly register the unresolved derivation
thufschmitt Apr 12, 2021
22a638e
Fix a localStore/destStore confusion
thufschmitt Apr 20, 2021
ddfece5
Check for previous builds using the drvPath rather than the output paths
thufschmitt Apr 20, 2021
fad8839
Don't call queryPartialDerivationOutputMap on the dest store
thufschmitt Apr 20, 2021
410d7a0
getBuildOutput: Don't query the dest store for the outputs
thufschmitt Apr 22, 2021
68d18b2
Correctly handle the non-ca case when building
thufschmitt Apr 22, 2021
af8f070
Allow gc-ing the destination store
thufschmitt Apr 22, 2021
70402e5
Actually use the `content-addressed` test
thufschmitt Apr 28, 2021
d1e41cf
Update the ca test
thufschmitt Apr 28, 2021
c5a092e
Clarify what happens with the out paths in the db
thufschmitt Apr 28, 2021
d243424
Move mkDerivation-for-ca to config.nix.in
thufschmitt Apr 28, 2021
4a6a342
eval-jobs: Don't strip the output path for ia derivations
thufschmitt Apr 28, 2021
89a55b9
Add the output paths of CA derivations to the database
thufschmitt Apr 28, 2021
34aaa0b
Add more ca tests
thufschmitt Apr 28, 2021
2a2aa72
Test evaluating a ca drv without the experimental feature
thufschmitt Apr 28, 2021
a07b1db
Move the ca tests to a subdirectory
thufschmitt Apr 28, 2021
3d64cfb
Fix a potential queue-runner crash
thufschmitt Apr 28, 2021
9f2b576
Prevent enabling ca-derivations from breaking substitution
thufschmitt Apr 28, 2021
a79afe0
Document the state of ca derivations
thufschmitt Apr 28, 2021
f676dc8
Also register the ca outputs on the dest store
thufschmitt May 3, 2021
79e5e73
Fix the resolving of derivations
thufschmitt May 31, 2021
529c28f
Make non-ca-depending-on-ca derivations work
thufschmitt Jul 28, 2021
c6005f4
Correctly handle “real” input-addressed derivations
thufschmitt Jul 29, 2021
1036075
Make perlcritic happy
thufschmitt Oct 18, 2021
a084efc
Extract some parts of `State::buildRemote` into auxiliary functions
thufschmitt Oct 20, 2021
46ae574
Fix build with Nix 2.6pre
thufschmitt Jan 19, 2022
cdff806
Flake: Update
thufschmitt Jan 20, 2022
77df6b4
Merge remote-tracking branch 'thufschmitt/nix-ca' into nix-ca4
aciceri Jun 25, 2022
8b34b2f
Don't query outputs in case of CA derivations
aciceri Jun 25, 2022
d8639ac
Using new libstore method and fixes due to CA derivations
aciceri Jun 25, 2022
de2fc58
`string` -> `std::string`
aciceri Jun 25, 2022
0ef435e
Forgot to remove old comment in the merge
aciceri Jun 27, 2022
db89bbe
Fixed regression to #1126
aciceri Jun 27, 2022
8d52640
DB model changed
aciceri Jun 30, 2022
954e41f
Keep track if step build outputs are CA or not
aciceri Jun 30, 2022
3fdc9e4
Improved UI to correctly handle CA derivations
aciceri Jun 30, 2022
391ada9
"Content addressed" field in builds details
aciceri Jul 2, 2022
8311b49
Merge branch 'NixOS:master' into aciceri/ca-derivations
aciceri Aug 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/manual/src/projects.md
Original file line number Diff line number Diff line change
Expand Up @@ -399,3 +399,10 @@ analogous:
| `String value` | `gitea_status_repo` | *Name of the `Git checkout` input* |
| `String value` | `gitea_http_url` | *Public URL of `gitea`*, optional |

Content-addressed derivations
-----------------------------

Hydra can to a certain extent use the [`ca-derivations` experimental Nix feature](https://github.com/NixOS/rfcs/pull/62).
To use it, make sure that the Nix version you use is at least as recent as the one used in hydra's flake.

Be warned that this support is still highly experimental, and anything beyond the basic functionality might be broken at that point.
13 changes: 7 additions & 6 deletions src/hydra-eval-jobs/hydra-eval-jobs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ static void worker(

if (auto drv = getDerivation(state, *v, false)) {

DrvInfo::Outputs outputs = drv->queryOutputs();
DrvInfo::Outputs outputs = drv->queryOutputs(!settings.isExperimentalFeatureEnabled(Xp::CaDerivations));

if (drv->querySystem() == "unknown")
throw EvalError("derivation must have a 'system' attribute");
Expand Down Expand Up @@ -231,12 +231,13 @@ static void worker(
}

nlohmann::json out;
for (auto & j : outputs)
// FIXME: handle CA/impure builds.
if (j.second)
out[j.first] = state.store->printStorePath(*j.second);
if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations))
for (auto & j : outputs)
out[j.first] = "";
else
for (auto & j : outputs)
out[j.first] = state.store->printStorePath(*j.second);
job["outputs"] = std::move(out);

reply["job"] = std::move(job);
}

Expand Down
119 changes: 91 additions & 28 deletions src/hydra-queue-runner/build-remote.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,71 @@ StorePaths reverseTopoSortPaths(const std::map<StorePath, ValidPathInfo> & paths
return sorted;
}

/**
* Replace the input derivations by their output paths to send a minimal closure
* to the builder.
*
* If we can afford it, resolve it, so that the newly generated derivation still
* has some sensible output paths.
*/
BasicDerivation inlineInputDerivations(Store & store, Derivation & drv, const StorePath & drvPath)
{
BasicDerivation ret;
auto outputHashes = staticOutputHashes(store, drv);
if (!drv.type().hasKnownOutputPaths()) {
auto maybeBasicDrv = drv.tryResolve(store);
if (!maybeBasicDrv)
throw Error(
"the derivation '%s' can’t be resolved. It’s probably "
"missing some outputs",
store.printStorePath(drvPath));
ret = *maybeBasicDrv;
} else {
// If the derivation is a real `InputAddressed` derivation, we must
// resolve it manually to keep the original output paths
ret = BasicDerivation(drv);
for (auto & input : drv.inputDrvs) {
auto drv2 = store.readDerivation(input.first);
auto drv2Outputs = drv2.outputsAndOptPaths(store);
for (auto & name : input.second) {
auto inputPath = drv2Outputs.at(name);
ret.inputSrcs.insert(*inputPath.second);
}
}
}
return ret;
}

/**
* Get the newly built outputs, either from the remote if it supports it, or by
* introspecting the derivation if the remote is too old
*/
DrvOutputs getBuiltOutputs(Store & store, const int remoteVersion, FdSource & from, Derivation & drv)
{
DrvOutputs builtOutputs;
if (GET_PROTOCOL_MINOR(remoteVersion) >= 6) {
builtOutputs
= worker_proto::read(store, from, Phantom<DrvOutputs> {});
} else {
// If the remote is too old to handle CA derivations, we can’t get this
// far anyways
assert(drv.type().hasKnownOutputPaths());
DerivationOutputsAndOptPaths drvOutputs
= drv.outputsAndOptPaths(store);
auto outputHashes = staticOutputHashes(store, drv);
for (auto & [outputName, output] : drvOutputs) {
auto outputPath = output.second;
// We’ve just asserted that the output paths of the derivation
// were known
assert(outputPath);
auto outputHash = outputHashes.at(outputName);
auto drvOutput = DrvOutput { outputHash, outputName };
builtOutputs.insert(
{ drvOutput, Realisation { drvOutput, *outputPath } });
}
}
return builtOutputs;
}

void State::buildRemote(ref<Store> destStore,
Machine::ptr machine, Step::ptr step,
Expand Down Expand Up @@ -234,7 +299,7 @@ void State::buildRemote(ref<Store> destStore,
unsigned int remoteVersion;

try {
to << SERVE_MAGIC_1 << 0x204;
to << SERVE_MAGIC_1 << 0x205;
to.flush();

unsigned int magic = readInt(from);
Expand Down Expand Up @@ -264,22 +329,7 @@ void State::buildRemote(ref<Store> destStore,
outputs of the input derivations. */
updateStep(ssSendingInputs);

StorePathSet inputs;
BasicDerivation basicDrv(*step->drv);

for (auto & p : step->drv->inputSrcs)
inputs.insert(p);

for (auto & input : step->drv->inputDrvs) {
auto drv2 = localStore->readDerivation(input.first);
for (auto & name : input.second) {
if (auto i = get(drv2.outputs, name)) {
auto outPath = i->path(*localStore, drv2.name, name);
inputs.insert(*outPath);
basicDrv.inputSrcs.insert(*outPath);
}
}
}
BasicDerivation basicDrv = inlineInputDerivations(*localStore, *step->drv, step->drvPath);

/* Ensure that the inputs exist in the destination store. This is
a no-op for regular stores, but for the binary cache store,
Expand All @@ -304,10 +354,10 @@ void State::buildRemote(ref<Store> destStore,
/* Copy the input closure. */
if (machine->isLocalhost()) {
StorePathSet closure;
destStore->computeFSClosure(inputs, closure);
destStore->computeFSClosure(basicDrv.inputSrcs, closure);
copyPaths(*destStore, *localStore, closure, NoRepair, NoCheckSigs, NoSubstitute);
} else {
copyClosureTo(machine->state->sendLock, *destStore, from, to, inputs, true);
copyClosureTo(machine->state->sendLock, *destStore, from, to, step->drv->inputSrcs, true);
}

auto now2 = std::chrono::steady_clock::now();
Expand Down Expand Up @@ -366,9 +416,6 @@ void State::buildRemote(ref<Store> destStore,
result.stopTime = stop;
}
}
if (GET_PROTOCOL_MINOR(remoteVersion) >= 6) {
worker_proto::read(*localStore, from, Phantom<DrvOutputs> {});
}
switch ((BuildResult::Status) res) {
case BuildResult::Built:
result.stepStatus = bsSuccess;
Expand Down Expand Up @@ -426,6 +473,11 @@ void State::buildRemote(ref<Store> destStore,
result.logFile = "";
}

auto builtOutputs = getBuiltOutputs(*localStore, remoteVersion, from, *step->drv);
StorePathSet outputs;
for (auto & [_, realisation] : builtOutputs)
outputs.insert(realisation.outPath);

/* Copy the output paths. */
if (!machine->isLocalhost() || localStore != std::shared_ptr<Store>(destStore)) {
updateStep(ssReceivingOutputs);
Expand All @@ -434,12 +486,6 @@ void State::buildRemote(ref<Store> destStore,

auto now1 = std::chrono::steady_clock::now();

StorePathSet outputs;
for (auto & i : step->drv->outputsAndOptPaths(*localStore)) {
if (i.second.second)
outputs.insert(*i.second.second);
}

/* Get info about each output path. */
std::map<StorePath, ValidPathInfo> infos;
size_t totalNarSize = 0;
Expand Down Expand Up @@ -509,6 +555,23 @@ void State::buildRemote(ref<Store> destStore,
result.overhead += std::chrono::duration_cast<std::chrono::milliseconds>(now2 - now1).count();
}

/* Register the outputs of the newly built drv */
if (settings.isExperimentalFeatureEnabled(Xp::CaDerivations)) {
auto outputHashes = staticOutputHashes(*localStore, *step->drv);
for (auto & [outputId, realisation] : builtOutputs) {
// Register the resolved drv output
localStore->registerDrvOutput(realisation);
destStore->registerDrvOutput(realisation);

// Also register the unresolved one
auto unresolvedRealisation = realisation;
unresolvedRealisation.signatures.clear();
unresolvedRealisation.id.drvHash = outputHashes.at(outputId.outputName);
localStore->registerDrvOutput(unresolvedRealisation);
destStore->registerDrvOutput(unresolvedRealisation);
Comment on lines +563 to +571
Copy link
Contributor

@DarkKirb DarkKirb Jul 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remote-building CA Derivations seems to fail here because the destStore already has the realisation registered. incorrect, see below. My hydra has this patch applied although you can probably remove the try {} catch for the localStore and pattern match the error to make sure it is about a constraint failure when inserting into Realisations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, also removing the try/catch for the local store and pattern matching the other errors.
Do you already know how can we check that the error is caused by a constraint failure?

However I would prefer leaving the final word to @thufschmitt about this part.

Copy link
Contributor

@DarkKirb DarkKirb Jul 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t know enough about nix’s c++ code to know how to match a specific error Will try changing the catch type to SQLiteError

also it seems i was wrong about the “remove the localStore try catch blocks” because the error only seems to occur on the local parts my bad

Jul 02 10:02:54 nas hydra-queue-runner[1352608]: Failed registering realisation locally: error: executing SQLite statement '
Jul 02 10:02:54 nas hydra-queue-runner[1352608]:                        insert into Realisations (drvPath, outputName, outputPath, signatures)
Jul 02 10:02:54 nas hydra-queue-runner[1352608]:                        values ('sha256:4768517dfe0afebd04abc714af41ddd08785e3067abd98471ad2b608e8448a1f', 'out', (select id from ValidPaths where path = '/nix/store/cna07xr4yfh8ck0q00ibbpgqgmcmxrf4-system-units'), '')
Jul 02 10:02:54 nas hydra-queue-runner[1352608]:                        ;': constraint failed (in '/nix/var/nix/db/db.sqlite') (already exists?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying this out made me discover that the issue is not that the realisation already exists but that the out output is not copied over for some reason

}
}

/* Shut down the connection. */
child.to = -1;
child.pid.wait();
Expand Down
19 changes: 9 additions & 10 deletions src/hydra-queue-runner/build-result.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,18 @@ using namespace nix;
BuildOutput getBuildOutput(
nix::ref<Store> store,
NarMemberDatas & narMembers,
const Derivation & drv)
const OutputPathMap derivationOutputs)
{
BuildOutput res;

/* Compute the closure size. */
StorePathSet outputs;
StorePathSet closure;
for (auto & i : drv.outputsAndOptPaths(*store))
if (i.second.second) {
store->computeFSClosure(*i.second.second, closure);
outputs.insert(*i.second.second);
}
for (auto& [outputName, outputPath] : derivationOutputs) {
store->computeFSClosure(outputPath, closure);
outputs.insert(outputPath);
res.outputs.insert({outputName, outputPath});
}
for (auto & path : closure) {
auto info = store->queryPathInfo(path);
res.closureSize += info->narSize;
Expand Down Expand Up @@ -107,13 +107,12 @@ BuildOutput getBuildOutput(
/* If no build products were explicitly declared, then add all
outputs as a product of type "nix-build". */
if (!explicitProducts) {
for (auto & [name, output] : drv.outputs) {
for (auto& [name, output] : derivationOutputs) {
BuildProduct product;
auto outPath = output.path(*store, drv.name, name);
product.path = store->printStorePath(*outPath);
product.path = store->printStorePath(output);
product.type = "nix-build";
product.subtype = name == "out" ? "" : name;
product.name = outPath->name();
product.name = output.name();

auto file = narMembers.find(product.path);
assert(file != narMembers.end());
Expand Down
8 changes: 4 additions & 4 deletions src/hydra-queue-runner/builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ State::StepResult State::doBuildStep(nix::ref<Store> destStore,

if (result.stepStatus == bsSuccess) {
updateStep(ssPostProcessing);
res = getBuildOutput(destStore, narMembers, *step->drv);
res = getBuildOutput(destStore, narMembers, localStore->queryDerivationOutputMap(step->drvPath));
}
}

Expand Down Expand Up @@ -275,9 +275,9 @@ State::StepResult State::doBuildStep(nix::ref<Store> destStore,

assert(stepNr);

for (auto & i : step->drv->outputsAndOptPaths(*localStore)) {
if (i.second.second)
addRoot(*i.second.second);
for (auto & i : localStore->queryPartialDerivationOutputMap(step->drvPath)) {
if (i.second)
addRoot(*i.second);
}

/* Register success in the database for all Build objects that
Expand Down
4 changes: 3 additions & 1 deletion src/hydra-queue-runner/hydra-build-result.hh
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ struct BuildOutput

std::list<BuildProduct> products;

std::map<std::string, nix::StorePath> outputs;

std::map<std::string, BuildMetric> metrics;
};

BuildOutput getBuildOutput(
nix::ref<nix::Store> store,
NarMemberDatas & narMembers,
const nix::Derivation & drv);
const nix::OutputPathMap derivationOutputs);
36 changes: 29 additions & 7 deletions src/hydra-queue-runner/hydra-queue-runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,10 @@ unsigned int State::createBuildStep(pqxx::work & txn, time_t startTime, BuildID

if (r.affected_rows() == 0) goto restart;

for (auto & [name, output] : step->drv->outputs)
txn.exec_params0
("insert into BuildStepOutputs (build, stepnr, name, path) values ($1, $2, $3, $4)",
buildId, stepNr, name, localStore->printStorePath(*output.path(*localStore, step->drv->name, name)));
for (auto& [name, output] : localStore->queryPartialDerivationOutputMap(step->drvPath))
txn.exec_params0
("insert into BuildStepOutputs (build, stepnr, name, path, contentAddressed) values ($1, $2, $3, $4, $5)",
buildId, stepNr, name, output ? localStore->printStorePath(*output) : "", step->drv->type().isCA());

if (status == bsBusy)
txn.exec(fmt("notify step_started, '%d\t%d'", buildId, stepNr));
Expand Down Expand Up @@ -358,11 +358,23 @@ void State::finishBuildStep(pqxx::work & txn, const RemoteResult & result,
assert(result.logFile.find('\t') == std::string::npos);
txn.exec(fmt("notify step_finished, '%d\t%d\t%s'",
buildId, stepNr, result.logFile));

if (result.stepStatus == bsSuccess) {
// Update the corresponding `BuildStepOutputs` row to add the output path
auto res = txn.exec_params1("select drvPath from BuildSteps where build = $1 and stepnr = $2", buildId, stepNr);
assert(res.size());
StorePath drvPath = localStore->parseStorePath(res[0].as<std::string>());
// If we've finished building, all the paths should be known
for (auto& [name, output] : localStore->queryDerivationOutputMap(drvPath))
txn.exec_params0
("update BuildStepOutputs set path = $4 where build = $1 and stepnr = $2 and name = $3",
buildId, stepNr, name, localStore->printStorePath(output));
}
}


int State::createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t stopTime,
Build::ptr build, const StorePath & drvPath, const std::string & outputName, const StorePath & storePath)
Build::ptr build, const StorePath & drvPath, const nix::Derivation drv, const std::string & outputName, const StorePath & storePath)
{
restart:
auto stepNr = allocBuildStep(txn, build->id);
Expand All @@ -381,9 +393,10 @@ int State::createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t sto
if (r.affected_rows() == 0) goto restart;

txn.exec_params0
("insert into BuildStepOutputs (build, stepnr, name, path) values ($1, $2, $3, $4)",
("insert into BuildStepOutputs (build, stepnr, name, path, contentAddressed) values ($1, $2, $3, $4, $5)",
build->id, stepNr, outputName,
localStore->printStorePath(storePath));
localStore->printStorePath(storePath),
drv.type().isCA());

return stepNr;
}
Expand Down Expand Up @@ -463,6 +476,15 @@ void State::markSucceededBuild(pqxx::work & txn, Build::ptr build,
res.releaseName != "" ? std::make_optional(res.releaseName) : std::nullopt,
isCachedBuild ? 1 : 0);

for (auto & [outputName, outputPath] : res.outputs) {
txn.exec_params0
("update BuildOutputs set path = $3 where build = $1 and name = $2",
build->id,
outputName,
localStore->printStorePath(outputPath)
);
}

txn.exec_params0("delete from BuildProducts where build = $1", build->id);

unsigned int productNr = 1;
Expand Down
Loading