Prepare for CA derivation support with lower impact changes #1316
Conversation
This is just C++ changes without any Perl / Frontend / SQL Schema changes. The idea is that it should be possible to redeploy Hydra with these chnages with (a) no schema migration and also (b) no regressions. We should be able to much more safely deploy these to a staging server and then production `hydra.nixos.org`. Extracted from #875 Co-Authored-By: Théophane Hufschmitt <theophane.hufschmitt@tweag.io> Co-Authored-By: Alexander Sosedkin <monk@unboiled.info> Co-Authored-By: Andrea Ciceri <andrea.ciceri@autistici.org> Co-Authored-By: Charlotte 🦝 Delenk Mlotte@chir.rs> Co-Authored-By: Sandro Jäckel <sandro.jaeckel@gmail.com>
| if (optOutputPath) | ||
| out[outputName] = state.store->printStorePath(*optOutputPath); | ||
| else | ||
| out[outputName] = ""; |
There was a problem hiding this comment.
This new case is dead code so long as ca-derivations is not an enabled experimental feature.
There was a problem hiding this comment.
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?
This looks like a stray copy paste.
| // far anyways | ||
| assert(drv.type().hasKnownOutputPaths()); | ||
| DerivationOutputsAndOptPaths drvOutputs = drv.outputsAndOptPaths(localStore); | ||
| auto outputHashes = staticOutputHashes(localStore, drv); |
There was a problem hiding this comment.
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).
| @@ -624,6 +673,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 (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { | |||
There was a problem hiding this comment.
Skipped in non-CA case
| product.type = "nix-build"; | ||
| product.subtype = name == "out" ? "" : name; | ||
| product.name = outPath->name(); | ||
| product.name = output.name(); |
There was a problem hiding this comment.
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.
| 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)); | ||
| } |
There was a problem hiding this comment.
This is an idempotent / no changed output extra step in the input-addressed case.
| 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) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is an idempotent / no changed output extra step in the input-addressed case.
| if (!destStore->isValidPath(*maybeOutputPath.second)) { | ||
| valid = false; | ||
| missing.insert({{outputHash, outputName}, maybeOutputPath.second}); | ||
| } |
There was a problem hiding this comment.
This is essentially the same
| experimentalFeatureSettings.require(Xp::CaDerivations); | ||
| if (!destStore->queryRealisation(DrvOutput{outputHash, outputName})) { | ||
| valid = false; | ||
| missing.insert({{outputHash, outputName}, std::nullopt}); | ||
| } |
There was a problem hiding this comment.
This is dead code with ca-derivations disabled
| if ((maybePath && localStore->isValidPath(*maybePath))) | ||
| avail++; |
| else if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations) && localStore->queryRealisation(i)) { | ||
| maybePath = localStore->queryRealisation(i)->outPath; | ||
| avail++; |
There was a problem hiding this comment.
Dead code without CA derivations
| try { | ||
| time_t startTime = time(0); | ||
| for (auto & [i, path] : missing) { | ||
| if (path) { |
There was a problem hiding this comment.
Next chunk of diff goes away with whitepaper enabled in the non-CA case we always have a path so extra if doesn't make a difference
It looks like we accidentally got the old code back, probably after a merge conflict resolution.
|
OK I tried to do a pretty through review of the code, originally by @thufschmitt above. By @thufschmitt's own admission, a lot of this stuff duplicates code in Nix for CA derivations, but Hydra already duplicates Nix's scheduling logic quite badly. I don't think there is an easy way to "just not make that problem worse" --- I think the only way to not add more duplication would be to deduplicate the stuff that is already there. That, however, is a much larger project. I therefore think the right thing to do is merge what we've got. It has been on a low simmer tested after all these years, after all, and with our new staging workflow, will be tested some more. As the PR description says, I pulled out this change to avoid the scheme change, and misc web-app changes that went with it. While there is no real problem doing a schema change with the staging instance, it is a much larger headache with the prod instance, and I wouldn't want to race ahead with staging just to hit a brick wall with prod. Without the scheme changes, and with the If we can do that, then the rest of #875 is a lot smaller, and we can do (a) performing the migration, (b) finally enabling check boxes for this plan added to #838 |
|
@Ericson2314 : Thanks a lot for that. That looks like a good plan. I'm a bit stretched out, but I'll try to block enough time to review this and follow-ups (I'm the one who committed this in the first place after all :p ) |
| if (optOutputPath) | ||
| out[outputName] = state.store->printStorePath(*optOutputPath); | ||
| else | ||
| out[outputName] = ""; |
There was a problem hiding this comment.
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?
|
@thufschmitt Thank you! Yeah I'd like to think this plan has enough fail-safes that is is OK if you don't review it, but you definitely have the most context having figured out what needs to happen. I think I'll add some review comments where I'd like to write a code comment explaining why something, and if you could just provide the missing tidbit (if you remember it after all these years!) I think that would be plenty. |
…bled Brought up by @thufschmitt in #1316 (comment) . This makes this closer to what was originally there --- which just dispatched off the experimental feature rather than the presence/absense of the output, too.
An empty string is a sneaky way to avoid hard failures --- things that expect strings still get strings, but it does conversely open the door up to soft failures (spooky-action-at-a-distance ones because the string did not have the expected invariants). "Fail fast" with null will ultimately make the system more robust, but force us to fix more things up front, and I don't want to change this without also fixing those things up front, especially as this commit is for now just part of the the preparatory PR for which this is dead code.
That is NixOS/hydra#1316, preparation for broader CA derivations support in a way that does not require scheme changes.
|
After discussing with @tomberek https://github.com/NixOS/nixos-org-configurations/pull/316/files this PR is now deployed to https://hydra.ngi0.nixos.org/. I'll edit a CA derivations tracking issue for these things. Done: #838 |
That is NixOS/hydra#1316, preparation for broader CA derivations support in a way that does not require scheme changes.
It has a performance cost, and as the comment says we should be doing the better solution. We want to land this preparatory change on prod while the rest is still on staging, so we should just skip it for now. Skipping it will not affect regular fixed-output and input-addressed derivations, which are the only ones prod would deal with upon getting this code. The main CA derivations support branch will revert this commit so it still works.
As the revert commit's description says, we only want to remove this in the preperatory branch, for NixOS#1316. In the main branch, for NixOS#875, the change shoudl be put back. This revert puts it back. This reverts commit 2ee0068.
|
I think I want to fix the realisations issue first, now that I think I know how to. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/content-addressed-nix-call-for-testers/12881/221 |
|
As discussed with @delroth, now that |
That is NixOS/hydra#1316, preparation for broader CA derivations support in a way that does not require scheme changes.
That is NixOS/hydra#1316, preparation for broader CA derivations support in a way that does not require scheme changes.
This is just C++ changes without any Perl / Frontend / SQL Schema changes.
The idea is that it should be possible to redeploy Hydra with these chnages with (a) no schema migration and also (b) no regressions. We should be able to much more safely deploy these to a staging server and then production
hydra.nixos.org.Extracted from #875