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

nixos/hercules-ci-agent: init #94804

Merged
merged 1 commit into from Aug 29, 2020

Conversation

@roberth
Copy link
Contributor

roberth commented Aug 6, 2020

Motivation for this change

Make Hercules CI Agent easier to install and configure.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved

format = pkgs.formats.toml {};

checkNix =

This comment has been minimized.

@flokli

flokli Aug 6, 2020 Contributor

I'm not a fan of something failing at build time here.

What about checking the nix version and failing if it's too low at evaluation time, and prompting users to use nixUnstable in that case?

This comment has been minimized.

@roberth

roberth Aug 6, 2020 Author Contributor

I would like to avoid depending on unreleased versions, which is why I've created the option to apply a minimal patch on the released version.
Deriving this information from options only seems a bit risky. For example if the user has their own patched Nix, how do we check that it's ok?

It's only a temporary solution. Once 2.4 is released, I'm happy to reduce this to a simple eval-only check.

This comment has been minimized.

@flokli

flokli Aug 6, 2020 Contributor

Hmmh, this code also depends on unreleased versions. Contrary to nixUnstable, you build your own version on-the-fly here, which is even less risky for sudden build failure.

I'd assume Nix 2.4 will be released before 19.10 - mind just recommending nixUnstable in an evaluation failure/warning (depending on how required it is) here if !(lib.versionAtLeast (lib.getVersion config.nix.package))?

This comment has been minimized.

@roberth

roberth Aug 7, 2020 Author Contributor

The patch has been tested since 2.3.4 and has not needed any changes for the total of four releases. It's a patch on the stable branch after all.
I'd be happy to remove the patching when 2.4 is released. But I wouldn't just assume it's released before NixOS.

baseDirectory = mkOption {
type = types.path;
default = "/var/lib/hercules-ci-agent";
description = "State directory (secrets, work directory, etc) for agent";
};
Comment on lines 102 to 106

This comment has been minimized.

@flokli

flokli Aug 6, 2020 Contributor

Can you use StateDirectory= in the unit?

This comment has been minimized.

@roberth

roberth Aug 6, 2020 Author Contributor

This maps to an entry in the config file. I suppose I could derive StateDirectory from it, but I'm not sure what would be the benefit of doing so.

This comment has been minimized.

@flokli

flokli Aug 6, 2020 Contributor

Hmm, I wasn't specific here - if this were using DynamicUser (which it can't, due to suid support), We could just have defaulted the state directory to this location.

It's probably still beneficial to distinguish between the home directory of the user running the service (which could be somewhere else than /var/lib/buildkite-agent), and state files (which should always live in /var/lib/$serviceName.

This comment has been minimized.

@roberth

roberth Aug 7, 2020 Author Contributor

It currently depends on dotfiles to configure automatic S3 push for example, so the default home /var/empty wouldn't work. Disallowed or transient dotfiles are normally the benefit I suppose. Is there another benefit that could offset the migration that will be required for current installations?

This comment has been minimized.

@roberth

roberth Aug 17, 2020 Author Contributor

Relevant issue: hercules-ci/hercules-ci-agent#239
For now I don't see a reason to use StateDirectory.

@roberth roberth force-pushed the hercules-ci:init-nixos-hercules-ci-agent branch 2 times, most recently from 255d25a to 5bf1199 Aug 6, 2020
Copy link
Contributor

aanderse left a comment

This seems to break a number of conventions, deviating from accepted NixOS module standards. @flokli mentioned a number of these and I wonder if this module is more appropriate for NUR instead. If there is motivation to get this upstream (which I think is a good idea, this module provides high value) I think the comments @flokli left are important.

@roberth roberth force-pushed the hercules-ci:init-nixos-hercules-ci-agent branch 3 times, most recently from 0dc9774 to 2c65ab0 Aug 17, 2020
@roberth roberth requested review from flokli and aanderse Aug 17, 2020
@roberth
Copy link
Contributor Author

roberth commented Aug 17, 2020

I've made the changes to turn this into a conventional module.
This now also uses RFC 42-style settings with freeform modules (thanks @Infinisil !)

@flokli
Copy link
Contributor

flokli commented Aug 17, 2020

Thanks, this now looks way more appropriate :-)

I still think the "patching nix if required" part doesn't belong into the module - raise an erorr if the version of Nix isn't recent enough (or grep the header files if the version string isn't enough).

Copy link
Member

Infinisil left a comment

Lovely to see some nice use of the recent module additions!

# even shortly after the previous lookup. This *also* applies to the daemon.
narinfo-cache-negative-ttl = 0
'';
nix.package = mkIf cfg.patchNix patchedNix;

This comment has been minimized.

@Infinisil

Infinisil Aug 17, 2020 Member

It might be better to use an overlay for this instead, because it will allow users to still specify e.g. nix.package = pkgs.nixFlakes, and the overlay can patch all of pkgs.{nix,nixUnstable,nixFlakes} (if necessary for each of them).

This comment has been minimized.

@roberth

roberth Aug 23, 2020 Author Contributor

It's only necessary for the daemon and only when its 2.3. They can set it to nixFlakes just fine because that version does not need patching.
It's a temporary situation until 2.4 is released. At that point, I'll drop the whole patching business, so there's little point in improving this.

This comment has been minimized.

@Infinisil

Infinisil Aug 23, 2020 Member

Ah so the intended flow is:

  • User enables module
  • Because checkNix = true by default and patchNix = false, user gets error that the Nix check failed and they need to use a version with a patch
  • User sets patchNix = true, therefore overriding the default Nix version (or I guess it would error if the user also assigned nix.package in their config)

I guess that's fine

@roberth roberth requested a review from Infinisil Aug 23, 2020
@roberth
Copy link
Contributor Author

roberth commented Aug 23, 2020

@flokli @aanderse @Infinisil

I have addressed all comments.

The patching options are useful until 2.4 is released, so I suggest they stick around but only until then.
Any final comments?

@roberth roberth force-pushed the hercules-ci:init-nixos-hercules-ci-agent branch from 2c65ab0 to 346a1b0 Aug 23, 2020
Copy link
Member

Infinisil left a comment

Looks good to me! The checking and patching is indeed a bit gnarly, but I think that's fine for a temporary convenience improvement

@roberth roberth merged commit 4841b30 into NixOS:master Aug 29, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="346a1b0"; rev="346a1b0ec608b1c926b2e8d5a959c60b04656358"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.