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/cockroachdb: create service #38665

Closed
wants to merge 2 commits into from

Conversation

jbboehr
Copy link
Contributor

@jbboehr jbboehr commented Apr 10, 2018

Motivation for this change

Ease of configuring CockroachDB on NixOS

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Naming isn't quite consistent - the binary is called cockroach but the package is called cockroachdb. For consistency, I've used cockroachdb in all cases here.

I added one very basic test that checks if an insecure single-node cluster can boot and run SHOW ALL CLUSTER SETTINGS.

I wasn't quite sure who to put as the maintainer.

nixos/tests/cockroachdb.nix Outdated Show resolved Hide resolved
nixos/modules/services/databases/cockroachdb.nix Outdated Show resolved Hide resolved
* Require that either insecure or certsDir be specified
* Escape percents in arguments to systemd
* Use Type=notify in systemd unit to improve startup process
@jbboehr
Copy link
Contributor Author

jbboehr commented Apr 15, 2018

@bdarnell Great, thanks!

Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

I'd like to see this go in since I was testing CockroachDB. I'd be willing to fix these problems and upstream it myself, but first, a review.

";
};

package = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a package option here, honestly? Realistically, CockroachDB doesn't support multiple versions in an LTS manner like MySQL or PostgreSQL. They probably will in the future but I think that's a bridge we can cross later on. What this means is that every time there's an upgrade, we're probably going to just move to it directly, for security/bugfixes that won't exist in prior versions, for example, as I recently did in c90a45a. Unless we start packaging multiple versions, there's no easy way to even create a new expression without just overriding it in your configuration.nix or writing/copy pasting your own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems pretty standard to include this option. I think it's useful enough to be able to override the package with a user-supplied package (maybe you need 2.1 plus commit xyz). Yes, you could also override the service with a user-supplied service. I can't say whether or not it's worth the extra option, but the harm in having it seems minimal since it doesn't have any significant code to go with it.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, Cockroach has one important distinction that separates it from the other stuff: it has the fully FOSS version and the "Cockroach Community License" (CCL) version with Enterprise features. It's reasonable to assume someone might want, and build, an enterprise copy for themselves. (Whether or not they'd use this module to configure it is a different matter, though).

type = types.path;
default = "/var/lib/cockroachdb";
example = "/var/lib/cockroachdb";
description = "Location where MySQL stores its table files";
Copy link
Member

Choose a reason for hiding this comment

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

s/MySQL/CockroachDB/

users.extraUsers.cockroachdb = {
description = "CockroachDB server user";
group = "cockroachdb";
uid = config.ids.uids.cockroachdb;
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to give this user a shell as well so you can sudo to it easily.

maintainers = [ ];
};

nodes = {
Copy link
Member

Choose a reason for hiding this comment

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

CockroachDB comes with a new workload subsystem in 2.1. I think having these tests do a cluster-wide workload test (say, for 1 minute) would be much better (and hopefully rattle out bugs on new platforms, like aarch64)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be overkill for nixpkgs CI, depending on the exact specifics of workload. The test I wrote is simply to boot a cluster and verify it's working at all, mainly the systemd configuration. I think sniffing for arch problems is probably better handled upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Workload testing only happens for 1 minute and takes little RAM in my tests, so I think it's fine. See #51306 for more on that.

Booting a single node is fine for testing, but generally I feel NixOS tests are already very weak in a number of areas -- I do not think a cluster test is overkill at all. Having an actual cluster test is arguably the minimal amount of testing we can do if we're going to offer and support a clustering database to our users, IMO! It's basically the primary feature of Cockroach. And doing an actual workload test with built-in tools is about as easy as you can ask for, I think.

I am also a strong supporter of trying to offer reasonable multi-architecture feature parity for NixOS, especially as far as packages-with-a-service-module are concerned. In other words, unless there's strong, obvious reason to reject it (e.g. significant complexity in the derivation, completely unstable and unusable) I generally aspire to offer feature-parity for services like this since they are part of NixOS proper. My indication from looking around the Cockroach upstream is that while they don't explicitly advertise aarch64 as "production ready", it seems to work fine -- it probably just doesn't undergo the same release-stress-testing they put the x86_64 release through, I'm guessing. (And, in fact, the only reason the clustering test doesn't work for us on AArch64 isn't due to Cockroach, but NTP shenanigans, and that could be fixed, probably.)

Regardless, there's a lot of interacting complexity in a modern system, architecture issues aside -- and being thorough and testing things (even things upstream has control over) will save people a lot of headache later on!

secureArg = insecure: certsDir:
if insecure then "--insecure " else
if (!isNull certsDir) then "--certs-dir=${certsDir} " else
throw "error: cockroachdb must specify either insecure or certsDir";
Copy link
Member

Choose a reason for hiding this comment

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

This is better done as an assertion in the module implementation, IMO.

Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

(Requesting changes based on prior comments)

@jbboehr
Copy link
Contributor Author

jbboehr commented Nov 30, 2018

@thoughtpolice Feel free to take this over if you'd like.

thoughtpolice added a commit to thoughtpolice/nixpkgs that referenced this pull request Dec 2, 2018
This also includes a full end-to-end CockroachDB clustering test to
ensure everything basically works. However, this test is not currently
enabled by default, though it can be run manually. See the included
comments in the test for more information.

Closes NixOS#51306. Closes NixOS#38665.

Co-authored-by: Austin Seipp <aseipp@pobox.com>
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants