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/buildkite-agents: support multiple buildkite agents #78373

Merged
merged 2 commits into from Feb 10, 2020

Conversation

@yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Jan 23, 2020

Motivation for this change

Continued from #68378, which was continued from #59358.

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
cc

@flokli @mkaito @balsoft @Infinisil

@mkaito
Copy link
Contributor

@mkaito mkaito commented Jan 23, 2020

@GrahamcOfBorg test buildkite-agents

@mkaito
Copy link
Contributor

@mkaito mkaito commented Jan 23, 2020

Oh you didn't rename the test attr

@GrahamcOfBorg test buildkite-agent

@yorickvP yorickvP force-pushed the serokell:yorickvp/buildkites branch from 2cc5149 to ea954b3 Jan 23, 2020
@yorickvP
Copy link
Contributor Author

@yorickvP yorickvP commented Jan 23, 2020

(test attr renamed now)

enable = mkEnableOption "buildkite-agent";
buildkiteOptions = { name ? "", config, ... }: {
options = {
enable = mkOption {

This comment has been minimized.

@mkaito

mkaito Jan 23, 2020
Contributor

Should the fact that it is declared assume that you want it enabled?

This comment has been minimized.

@yorickvP

yorickvP Jan 24, 2020
Author Contributor

Yes. Would be weird if you declared an agent and then you have to explicitly enable it.

This comment has been minimized.

@flokli

flokli Feb 10, 2020
Contributor

Do we still want to have the option to then explicitly disable it? Seems weird, as we filter by enabled agents…

@balsoft
Copy link
Member

@balsoft balsoft commented Jan 24, 2020

Do we not want to keep compatibility? Provide services.buildkite-agent, and if it's used then create a single agent in buildkite-agents (and perhaps show a warning that the option is soon to be removed)?

dataDir = mkOption {
default = "/var/lib/buildkite-agent";
default = "/var/lib/buildkite-agent-${name}";
description = "The workdir for the agent";
type = types.str;
};
Comment on lines 47 to 51

This comment has been minimized.

@flokli

flokli Jan 24, 2020
Contributor

do you know usecases of where this should be somewhere else than at this location?

If we restrict it to /var/lib/buildkite-agent-${name}, we could just set StateDirectory=buildkite-agent=${name}, and DynamicUser=true, and don't need to setup a system-wide user at all.

This comment has been minimized.

@yorickvP

yorickvP Jan 24, 2020
Author Contributor

Yes, we use the buildkite directory to store some gc roots that are used for Continuous Delivery. We could change to a ReadWriteDirectory, but I don't think DynamicUser is really set up for communication with the outside world.

This comment has been minimized.

@mkaito

mkaito Jan 24, 2020
Contributor

Our approach to CD is arguably horrible by abusing this.

This comment has been minimized.

@flokli

flokli Jan 24, 2020
Contributor

Well, I'd recommend using some post-build-hook on the global nix-daemon to sign and upload to a binary cache, but hmm… I can understand you can't change this that quick.

Still, what about defaulting to use DynamicUser=true, and StateDirectory=buidlkite-agent-${name}, and only use a system-wide user if both a user attribute and that dataDir attribute is set?

This comment has been minimized.

@kirelagin

kirelagin Feb 3, 2020
Member

using some post-build-hook on the global nix-daemon to sign and upload to a binary cache

Been there, tried that :(. Builds become horribly slow and the host becomes almost unusable as it is uploading everything, even your user environment after nix-env -i (and that’s slow too).

This comment has been minimized.

@flokli

flokli Feb 4, 2020
Contributor

Well, the naive approach blocks the build loop, as documented in https://nixos.org/nix/manual/#chap-post-build-hook-caveats .

In your case, you might want to have the executed hook to fork away as fast as possible.

However, I see that discussion as not really related to this PR - it doesn't yet introduce DynamicUser.

nixos/tests/buildkite-agents.nix Outdated Show resolved Hide resolved
nixos/tests/buildkite-agents.nix Outdated Show resolved Hide resolved
@flokli
Copy link
Contributor

@flokli flokli commented Jan 24, 2020

Do we not want to keep compatibility? Provide services.buildkite-agent, and if it's used then create a single agent in buildkite-agents (and perhaps show a warning that the option is soon to be removed)?

I'm fine with some breaking changes, if they're documented in the release notes and remove some cruft. We did some other updates when moving to buildkite 3, so I'd expect people to take a look at the release notes anyway and update their configuration.

@yorickvP yorickvP force-pushed the serokell:yorickvp/buildkites branch 2 times, most recently from 4e26f22 to 520361f Jan 24, 2020
(mkRemovedOptionModule [ "services" "buildkite-agent" "openssh" "publicKey" ] "SSH public keys aren't necessary to clone private repos.")
(mkRemovedOptionModule [ "services" "buildkite-agent" "openssh" "publicKeyPath" ] "SSH public keys aren't necessary to clone private repos.")
(mkRenamedOptionModule [ "services" "buildkite-agent" "meta-data"] [ "services" "buildkite-agent" "tags" ])
(mkRemovedOptionModule [ "services" "buildkite-agent"] "services.buildkite-agent has been moved to an attribute set at services.buildkite-agents")

This comment has been minimized.

@Infinisil

Infinisil Jan 27, 2020
Member

It should be relatively easy to make this backwards compatible with mkChangedOptionModule, which will also issue a warning when the old option is used.

This comment has been minimized.

@yorickvP

yorickvP Jan 29, 2020
Author Contributor

We could do that, but what should we make the default agent name? The state paths for it would still change.

This comment has been minimized.

@Infinisil

Infinisil Jan 30, 2020
Member

Since people are expected to have used dataDir to refer to the path, I don't think it should be a problem. Also from this PR I can see that the service previously didn't even use dataDir to get its config, seemingly indicating that it was never fully supported anyways.

Also I correct myself: A simple

mkRenamedOptionModule [ "services" "buildkite-agent" ]
  [ "services" "buildkite-agents" "legacy" ]

should work (using the name legacy, or maybe single or nixos-single or so)

This comment has been minimized.

@yorickvP

yorickvP Jan 30, 2020
Author Contributor

the dataDir actually contains long-term state. People would have to move the contents of /var/lib/buildkite-agent to /var/lib/buildkite-agent-legacy, or get unexpected behavior. I think it's safer to have it break during the build, instead of during runtime.

This comment has been minimized.

@Infinisil

Infinisil Jan 30, 2020
Member

Ohh good point, didn't think of that. Okay so a mkRenamedOptionModule won't work, but a mkChangedOptionModule should still be possible with a legacy attribute (or similar) and by defaulting its dataDir to /var/lib/buildkite-agent

This comment has been minimized.

@flokli

flokli Jan 31, 2020
Contributor

I still think breaking old config and making sure the user did read the release notes is a better way forward here.

There have been quite some changes, and just silently assuming everything will still work as it did before is probably a bad idea anyways.

I also proposed to default to use DynamicUsers, and make the usage of a non-dynamic user, and custom StateDirectory the non-default case, which might cause nontrivial migrations aswell.

I think batching all these with the option syntax change is a reasonable path forward.

This comment has been minimized.

@Infinisil

Infinisil Jan 31, 2020
Member

What are the "quite some changes"? From what I can see all this PR does is to add support for multiple agents as it says, which imo really doesn't warrant breaking people's configs, especially if it only takes a couple lines to make it automatic.

This comment has been minimized.

@flokli

flokli Jan 31, 2020
Contributor

@Infinisil see the earlier PRs and the corresponding release notes:

https://github.com/NixOS/nixpkgs/blob/master/nixos/doc/manual/release-notes/rl-2003.xml#L417

As for the "defaulting to DynamicUser": #78373 (comment)

This comment has been minimized.

@Infinisil

Infinisil Feb 4, 2020
Member

Were any of those not backwards compatible? If so they should've added a warning/error themselves. This PR itself seems 100% backwards compatible with a mkChangedOptionModule. Unless backwards compat has to be broken I'd rather not.

This comment has been minimized.

@flokli

flokli Feb 8, 2020
Contributor

Well, the module options themselves not, but the upgrade from buildkite 2 to 3 might have introduced some behavorial changes in how the bootstrap/hooks were configured: https://buildkite.com/docs/agent/v3/upgrading

yorickvP added 2 commits Jan 23, 2020
@yorickvP yorickvP force-pushed the serokell:yorickvp/buildkites branch from 04b97b7 to e242ecc Feb 10, 2020
@flokli
Copy link
Contributor

@flokli flokli commented Feb 10, 2020

@GrahamcOfBorg test buildkite-agent

@yorickvP
Copy link
Contributor Author

@yorickvP yorickvP commented Feb 10, 2020

@GrahamcOfBorg test buildkite-agents

@flokli
flokli approved these changes Feb 10, 2020
Copy link
Contributor

@flokli flokli left a comment

LGTM.

@yorickvP yorickvP force-pushed the serokell:yorickvp/buildkites branch from a16d757 to e242ecc Feb 10, 2020
Copy link
Member

@Infinisil Infinisil left a comment

We discussed this on IRC: https://logs.nix.samueldr.com/nixos-dev/2020-02-10#1581336556-1581341910;

Looks good to me now :)

@Infinisil Infinisil merged commit e3c5d29 into NixOS:master Feb 10, 2020
15 checks passed
15 checks passed
tests.buildkite-agent on aarch64-linux No attempt
Details
tests.buildkite-agent on x86_64-linux No attempt
Details
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="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
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
@grahamc
Copy link
Member

@grahamc grahamc commented Feb 18, 2020

After this PR I get:

Options `services.buildkite-agents.r13y.hooksPath' and
`services.buildkite-agents.r13y.hooks.<name>' are mutually exclusive

but I am not specifying hooksPath, and am only specifying hooks.environment.


assertions = [
config.assertions = mapAgents (name: cfg: [
{ assertion = cfg.hooksPath == hooksDir || all (v: v == null) (attrValues cfg.hooks);

This comment has been minimized.

@grahamc

grahamc Feb 19, 2020
Member

Suggested change
{ assertion = cfg.hooksPath == hooksDir || all (v: v == null) (attrValues cfg.hooks);
{ assertion = cfg.hooksPath == (hooksDir cfg) || all (v: v == null) (attrValues cfg.hooks);
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

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