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

Prepare for CA derivation support with lower impact changes #1316

Merged
merged 14 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 19 additions & 6 deletions src/hydra-eval-jobs/hydra-eval-jobs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,11 @@ static void worker(

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

DrvInfo::Outputs outputs = drv->queryOutputs();
// CA derivations do not have static output paths, so we
// have to defensively not query output paths in case we
// encounter one.
DrvInfo::Outputs outputs = drv->queryOutputs(
!experimentalFeatureSettings.isEnabled(Xp::CaDerivations));

if (drv->querySystem() == "unknown")
throw EvalError("derivation must have a 'system' attribute");
Expand Down Expand Up @@ -239,12 +243,21 @@ 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);
for (auto & [outputName, optOutputPath] : outputs) {
if (optOutputPath) {
out[outputName] = state.store->printStorePath(*optOutputPath);
} else {
// See the `queryOutputs` call above; we should
// not encounter missing output paths otherwise.
assert(experimentalFeatureSettings.isEnabled(Xp::CaDerivations));
// TODO it would be better to set `null` than an
// empty string here, to force the consumer of
// this JSON to more explicitly handle this
// case.
out[outputName] = "";
Copy link
Member Author

@Ericson2314 Ericson2314 Dec 4, 2023

Choose a reason for hiding this comment

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

This new case is dead code so long as ca-derivations is not an enabled experimental feature.

Copy link
Member

Choose a reason for hiding this comment

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

Should we warn or errror for this atm (at least if ca-derivations isn't enabled), just to make sure that all these are indeed dead code and we're not accidentally triggering something unexpected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, good call!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

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

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

Expand Down
76 changes: 59 additions & 17 deletions src/hydra-queue-runner/build-remote.cc
Original file line number Diff line number Diff line change
Expand Up @@ -228,17 +228,22 @@ static BasicDerivation sendInputs(
counter & nrStepsCopyingTo
)
{
BasicDerivation basicDrv(*step.drv);

for (const auto & [drvPath, node] : step.drv->inputDrvs.map) {
auto drv2 = localStore.readDerivation(drvPath);
for (auto & name : node.value) {
if (auto i = get(drv2.outputs, name)) {
auto outPath = i->path(localStore, drv2.name, name);
basicDrv.inputSrcs.insert(*outPath);
}
}
}
/* Replace the input derivations by their output paths to send a
minimal closure to the builder.

`tryResolve` currently does *not* rewrite input addresses, so it
is safe to do this in all cases. (It should probably have a mode
to do that, however, but we would not use it here.)
*/
BasicDerivation basicDrv = ({
auto maybeBasicDrv = step.drv->tryResolve(destStore, &localStore);
if (!maybeBasicDrv)
throw Error(
"the derivation '%s' can’t be resolved. It’s probably "
"missing some outputs",
localStore.printStorePath(step.drvPath));
*maybeBasicDrv;
});

/* Ensure that the inputs exist in the destination store. This is
a no-op for regular stores, but for the binary cache store,
Expand Down Expand Up @@ -319,6 +324,30 @@ static BuildResult performBuild(
result.stopTime = stopTime;
}

// If the protocol was too old to give us `builtOutputs`, initialize
// it manually by introspecting the derivation.
if (GET_PROTOCOL_MINOR(conn.remoteVersion) < 6)
{
// 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(localStore);
// Since this a `BasicDerivation`, `staticOutputHashes` will not
// do any real work.
auto outputHashes = staticOutputHashes(localStore, drv);
Copy link
Member Author

Choose a reason for hiding this comment

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

These output hashes are not really used in the input-addressing case, but they may be expensive to calculate because the underlying hashDerivationModulo loads the derivation closure (but cached).

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 };
result.builtOutputs.insert_or_assign(
std::move(outputName),
Realisation { drvOutput, *outputPath });
}
}

return result;
}

Expand Down Expand Up @@ -584,6 +613,10 @@ void State::buildRemote(ref<Store> destStore,
result.logFile = "";
}

StorePathSet outputs;
for (auto & [_, realisation] : buildResult.builtOutputs)
outputs.insert(realisation.outPath);

/* Copy the output paths. */
if (!machine->isLocalhost() || localStore != std::shared_ptr<Store>(destStore)) {
updateStep(ssReceivingOutputs);
Expand All @@ -592,12 +625,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);
}

size_t totalNarSize = 0;
auto infos = build_remote::queryPathInfos(conn, *localStore, outputs, totalNarSize);

Expand All @@ -616,6 +643,21 @@ 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 (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Skipped in non-CA case

auto outputHashes = staticOutputHashes(*localStore, *step->drv);
for (auto & [outputName, realisation] : buildResult.builtOutputs) {
// Register the resolved drv output
destStore->registerDrvOutput(realisation);

// Also register the unresolved one
auto unresolvedRealisation = realisation;
unresolvedRealisation.signatures.clear();
unresolvedRealisation.id.drvHash = outputHashes.at(outputName);
destStore->registerDrvOutput(unresolvedRealisation);
}
}

/* 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();
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 whole file is the same logic as before. In the CA case more work has to be done to get the paths so that was moved to the caller. In the input-addressed case it is easy and moving the tiny bit of work back and forth should not be noticeable.


auto file = narMembers.find(product.path);
assert(file != narMembers.end());
Expand Down
11 changes: 7 additions & 4 deletions src/hydra-queue-runner/builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,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, destStore->queryDerivationOutputMap(step->drvPath, &*localStore));
}
}

Expand Down Expand Up @@ -277,9 +277,12 @@ 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 & [outputName, optOutputPath] : destStore->queryPartialDerivationOutputMap(step->drvPath, &*localStore)) {
if (!optOutputPath)
throw Error(
"Missing output %s for derivation %d which was supposed to have succeeded",
outputName, localStore->printStorePath(step->drvPath));
addRoot(*optOutputPath);
}

/* 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);
27 changes: 24 additions & 3 deletions src/hydra-queue-runner/hydra-queue-runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -312,10 +312,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)
for (auto & [name, output] : getDestStore()->queryPartialDerivationOutputMap(step->drvPath, &*localStore))
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)));
buildId, stepNr, name, output ? localStore->printStorePath(*output) : "");

if (status == bsBusy)
txn.exec(fmt("notify step_started, '%d\t%d'", buildId, stepNr));
Expand Down Expand Up @@ -352,11 +352,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] : getDestStore()->queryDerivationOutputMap(drvPath, &*localStore))
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 Down Expand Up @@ -457,6 +469,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)
);
}

Comment on lines +472 to +480
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 is an idempotent / no changed output extra step in the input-addressed case.

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

unsigned int productNr = 1;
Expand Down
Loading
Loading