nixos/buildkite-agents: support multiple buildkite agents #78373

yorickvP · 2020-01-23T16:27:01Z

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 · 2020-01-23T16:39:41Z

@GrahamcOfBorg test buildkite-agents

mkaito · 2020-01-23T16:41:15Z

Oh you didn't rename the test attr

@GrahamcOfBorg test buildkite-agent

yorickvP · 2020-01-23T17:21:50Z

(test attr renamed now)

mkaito · 2020-01-23T17:12:29Z

nixos/modules/services/continuous-integration/buildkite-agents.nix

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


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

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

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

nixos/modules/services/continuous-integration/buildkite-agents.nix

balsoft · 2020-01-24T11:16:16Z

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)?

flokli · 2020-01-24T13:38:11Z

nixos/modules/services/continuous-integration/buildkite-agents.nix

      dataDir = mkOption {
-        default = "/var/lib/buildkite-agent";
+        default = "/var/lib/buildkite-agent-${name}";
        description = "The workdir for the agent";
        type = types.str;
      };


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.

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.

Our approach to CD is arguably horrible by abusing this.

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?

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).

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/modules/services/continuous-integration/buildkite-agents.nix

nixos/tests/buildkite-agents.nix

flokli · 2020-01-24T13:44:31Z

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.

infinisil · 2020-01-27T16:32:19Z

nixos/modules/services/continuous-integration/buildkite-agents.nix

-    (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")


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

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

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)

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.

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

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.

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.

@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)

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.

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

flokli · 2020-02-10T12:40:26Z

@GrahamcOfBorg test buildkite-agent

yorickvP · 2020-02-10T12:51:46Z

@GrahamcOfBorg test buildkite-agents

flokli

LGTM.

infinisil

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

Looks good to me now :)

grahamc · 2020-02-18T23:04:34Z

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.

grahamc · 2020-02-19T03:44:14Z

nixos/modules/services/continuous-integration/buildkite-agents.nix


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


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);

yorickvP requested a review from infinisil as a code owner January 23, 2020 16:27

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jan 23, 2020

yorickvP force-pushed the yorickvp/buildkites branch from 2cc5149 to ea954b3 Compare January 23, 2020 17:04

mkaito requested changes Jan 23, 2020

View reviewed changes

flokli reviewed Jan 24, 2020

View reviewed changes

yorickvP force-pushed the yorickvp/buildkites branch 2 times, most recently from 4e26f22 to 520361f Compare January 24, 2020 16:22

ofborg bot added 8.has: changelog 8.has: documentation labels Jan 24, 2020

infinisil requested changes Jan 27, 2020

View reviewed changes

yorickvP added 2 commits February 10, 2020 13:35

nixos/buildkite-agents: support multiple buildkite agents

f003810

nixos/buildkite-agents: update release notes

e242ecc

yorickvP force-pushed the yorickvp/buildkites branch from 04b97b7 to e242ecc Compare February 10, 2020 12:36

flokli approved these changes Feb 10, 2020

View reviewed changes

yorickvP force-pushed the yorickvp/buildkites branch from a16d757 to e242ecc Compare February 10, 2020 13:36

infinisil approved these changes Feb 10, 2020

View reviewed changes

infinisil merged commit e3c5d29 into NixOS:master Feb 10, 2020

grahamc reviewed Feb 19, 2020

View reviewed changes

yorickvP mentioned this pull request Feb 19, 2020

nixos/buildkite-agents: fix hooksDir assertion #80526

Merged

10 tasks

This was referenced Jun 2, 2020

Multiple buildkite agent support #59358

Closed

Multiple buildkite agent support #68378

Closed

nh2 mentioned this pull request Nov 25, 2020

How do I actually use this? nh2/static-haskell-nix#88

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/buildkite-agents: support multiple buildkite agents #78373

nixos/buildkite-agents: support multiple buildkite agents #78373

yorickvP commented Jan 23, 2020 •

edited

Loading

mkaito commented Jan 23, 2020

mkaito commented Jan 23, 2020

yorickvP commented Jan 23, 2020

mkaito Jan 23, 2020

yorickvP Jan 24, 2020

flokli Feb 10, 2020

balsoft commented Jan 24, 2020

flokli Jan 24, 2020

yorickvP Jan 24, 2020

mkaito Jan 24, 2020

flokli Jan 24, 2020

kirelagin Feb 3, 2020

flokli Feb 4, 2020

flokli commented Jan 24, 2020

infinisil Jan 27, 2020

yorickvP Jan 29, 2020

infinisil Jan 30, 2020

yorickvP Jan 30, 2020

infinisil Jan 30, 2020

flokli Jan 31, 2020

infinisil Jan 31, 2020

flokli Jan 31, 2020 •

edited

Loading

infinisil Feb 4, 2020

flokli Feb 8, 2020

flokli commented Feb 10, 2020

yorickvP commented Feb 10, 2020

flokli left a comment

infinisil left a comment

grahamc commented Feb 18, 2020

grahamc Feb 19, 2020

	{ assertion = cfg.hooksPath == hooksDir \|\| all (v: v == null) (attrValues cfg.hooks);
	{ assertion = cfg.hooksPath == (hooksDir cfg) \|\| all (v: v == null) (attrValues cfg.hooks);

nixos/buildkite-agents: support multiple buildkite agents #78373

nixos/buildkite-agents: support multiple buildkite agents #78373

Conversation

yorickvP commented Jan 23, 2020 • edited Loading

Motivation for this change

Things done

cc

mkaito commented Jan 23, 2020

mkaito commented Jan 23, 2020

yorickvP commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balsoft commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli commented Feb 10, 2020

yorickvP commented Feb 10, 2020

flokli left a comment

Choose a reason for hiding this comment

infinisil left a comment

Choose a reason for hiding this comment

grahamc commented Feb 18, 2020

Choose a reason for hiding this comment

yorickvP commented Jan 23, 2020 •

edited

Loading

flokli Jan 31, 2020 •

edited

Loading