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
base: master
from

Conversation

Projects
None yet
4 participants
@jbboehr
Copy link
Contributor

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.

Integrate changes recommended by @bdarnell
* 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

This comment has been minimized.

Copy link
Contributor Author

jbboehr commented Apr 15, 2018

@bdarnell Great, thanks!

@thoughtpolice
Copy link
Member

thoughtpolice left a comment

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 {

This comment has been minimized.

@thoughtpolice

thoughtpolice Nov 30, 2018

Member

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.

This comment has been minimized.

@jbboehr

jbboehr Nov 30, 2018

Author Contributor

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.

This comment has been minimized.

@thoughtpolice

thoughtpolice Dec 1, 2018

Member

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

This comment has been minimized.

@thoughtpolice

thoughtpolice Nov 30, 2018

Member

s/MySQL/CockroachDB/

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

This comment has been minimized.

@thoughtpolice

thoughtpolice Nov 30, 2018

Member

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

maintainers = [ ];
};

nodes = {

This comment has been minimized.

@thoughtpolice

thoughtpolice Nov 30, 2018

Member

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)

This comment has been minimized.

@jbboehr

jbboehr Nov 30, 2018

Author Contributor

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.

This comment has been minimized.

@thoughtpolice

thoughtpolice Dec 1, 2018

Member

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

This comment has been minimized.

@thoughtpolice

thoughtpolice Nov 30, 2018

Member

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

@thoughtpolice
Copy link
Member

thoughtpolice left a comment

(Requesting changes based on prior comments)

@jbboehr

This comment has been minimized.

Copy link
Contributor Author

jbboehr commented Nov 30, 2018

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

@thoughtpolice thoughtpolice referenced this pull request Dec 1, 2018

Merged

nixos/cockroachdb: create new service #51306

5 of 5 tasks complete

thoughtpolice added a commit to thoughtpolice/nixpkgs that referenced this pull request Dec 2, 2018

nixos/cockroachdb: create new service
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
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.