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
Conversation
* Require that either insecure or certsDir be specified * Escape percents in arguments to systemd * Use Type=notify in systemd unit to improve startup process
@bdarnell Great, thanks! |
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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)
@thoughtpolice Feel free to take this over if you'd like. |
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>
Motivation for this change
Ease of configuring CockroachDB on NixOS
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)Naming isn't quite consistent - the binary is called
cockroach
but the package is calledcockroachdb
. For consistency, I've usedcockroachdb
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.