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

Split the buildRemote function #1180

Merged
merged 21 commits into from
Dec 4, 2023
Merged

Conversation

thufschmitt
Copy link
Member

In an attempt to understand everything that’s going on here − and to make it easier to eventually simplify it − extract a handful of auxiliary functions from the State::buildRemote method.

I (hopefully) didn’t substantially change any logic yet, just moved things out of this method, so the behavior should remain exactly the same.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 21, 2022

Should we merge #1162 first?

Need to read not on my phone, but I like this incrementalism. Good first steps before other cleanups!

Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

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

Looks good, though I am reminded of this post by John Carmack on splitting big functions: http://number-none.com/blow/john_carmack_on_inlined_code.html. I.e. having a lot of small functions obfuscates the control flow.

src/hydra-queue-runner/build-remote.cc Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member

@edolstra I think it's instructive per the top of that linked page that John Carmack eventually changed his mind :). These functions are are basically the ones to dedup with Nix per the very cautious incremental plan in #1164 ensuring we have no regressions, before finally getting Hydra to use the Store abstraction properly as the last step. I therefore like this a lot.

@thufschmitt
Copy link
Member Author

I am reminded of this post by John Carmack on splitting big functions: http://number-none.com/blow/john_carmack_on_inlined_code.html. I.e. having a lot of small functions obfuscates the control flow.

Thanks for the reference, that’s indeed a very valid point (and a very interesting read :) ) I think we’re fine(ish) here, because most of these functions are conceptually pure (for a very broad definition of pure), or at least idempotent.

@thufschmitt
Copy link
Member Author

Mh, just noticed an issue with this, we’re sending the original derivation to the remote builders and not the basic one. Will fix asap

@thufschmitt
Copy link
Member Author

Mh, just noticed an issue with this, we’re sending the original derivation to the remote builders and not the basic one. Will fix asap

OK, fixed in c592293

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-27/18433/1

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 6, 2022

I would still like to do this. @edolstra did start a more aggressive rewrite to go directly to using the store abstraction (#1200). But that PR is still draft, and to me that adds evidence to my theory that this approach (there is no disagreement on the end goal!) is better:

  1. Smaller steps
  2. No behavior changes to start
  3. Easier to review those smaller steps
  4. Easier to revert/bisect if someone goes wrong.

We are all pressed for time, and no one wants to break Hydra / hydra.nixos,org, so I think this is the prudent way forward.

Please, let's do this.

Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

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

The change looks good itself apart from the naming issue and the conflict that arose

return {std::move(logFile), std::move(logFD)};
}

void handshake(Machine::Connection & conn, unsigned int repeats)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would either be better to rename the function (nixServeV1Handshake or something like that) or move it into a namespace/class to make its use more obvious

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup' fair point. I've moved all that new logic to its own namespace to make things clearer

throw Error("machine ‘%1%’ does not support repeating a build; please upgrade it to Nix 1.12", conn.machine->sshName);
}

BasicDerivation sendInputs(
Copy link
Member

Choose a reason for hiding this comment

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

Same with this function

Just don't pollute the global one
@edolstra
Copy link
Member

I'm not super in favor of this PR, since it is just a refactoring but it will make #1200 harder to merge.

@Ericson2314
Copy link
Member

@edolstra I am happy to fix all #1200 conflicts if we merge this. I am very much a fan of this PR!

@Ericson2314
Copy link
Member

master...obsidiansystems:hydra:split-buildRemote I fixed merge conflicts in this one here, but I could not yet due to limited internet.

@Ericson2314
Copy link
Member

@dasJ Is the new namespacing sufficient to you to clarify the identifiers?

@dasJ
Copy link
Member

dasJ commented Nov 1, 2022

Yeah I'm happy with it. Could you resolve the conflict? I will pick this PR into my downstream Hydra then and run it for a couple of days to make sure nothing odd happens (although I don't expect a lot of impact)

@Ericson2314 Ericson2314 mentioned this pull request Aug 4, 2023
@nixos-discourse
Copy link

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/220

auto narSize = readLongLong(conn.from);
auto narHash = Hash::parseAny(readString(conn.from), htSHA256);
auto ca = parseContentAddressOpt(readString(conn.from));
readStrings<StringSet>(conn.from); // sigs
Copy link

Choose a reason for hiding this comment

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

is the result of sigs discarded intentionally?

Copy link
Member

Choose a reason for hiding this comment

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

It was already done that way. After we upgrade to Nix 2.19 we can deduplicate this code better.

@Ericson2314 Ericson2314 merged commit a5d44b6 into NixOS:master Dec 4, 2023
2 checks passed
@nixos-discourse
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants