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

Fix Cassandra, improve config and tests #59179

Merged
merged 18 commits into from Jun 14, 2019

Conversation

JohnAZoidberg
Copy link
Member

@JohnAZoidberg JohnAZoidberg commented Apr 8, 2019

Motivation for this change
  • It's more intuitive and discoverable when you can configure set some options using Nix attributes - plus we can check it using some assertions.
  • Make it possible to set the variables in cassandra-env.sh.
  • Allow client connections by default because that's what users expect.
  • Fix tests and test more things
  • Make binaries work again (properly wrap them)

The test doesn't work - it doesn't work without this PR. I'll figure out why, when I have time. Seems like a race condition of the VMs starting, it's not a problem with the module itself, rather the tests.
[EDIT]: I think I fixed the tests, please have ofborg run them.

I tested manually on my system that the different configuration options actually have the desired effect.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@JohnAZoidberg
Copy link
Member Author

This PR now also fixes the tests for Cassandra.
They take 5 minutes to run (on my laptop) but that's fine for booting and shutting down 3 VMs and rebalancing a Cassandra cluster.

@jpyen
Copy link

jpyen commented Apr 10, 2019

This is very necessary. Thanks Daniel.

@JohnAZoidberg JohnAZoidberg changed the title More and better configuration for Apache Cassandra module Fix Cassandra and improve config Apr 12, 2019
@JohnAZoidberg JohnAZoidberg mentioned this pull request Apr 12, 2019
10 tasks
@JohnAZoidberg
Copy link
Member Author

JohnAZoidberg commented Apr 12, 2019

I broke some of the binaries in #59090...
I misinterpreted the error when testing some of the binaries. I thought I just used it wrong parameters but turns out I broke some of them while making others work.
This PR (5041afc9690894f1a809ad6fc) fixes it.

@JohnAZoidberg
Copy link
Member Author

Please somebody have a look - Cassandra is currently broken on master.

@aanderse
Copy link
Member

aanderse commented Apr 22, 2019

Hi @JohnAZoidberg,

One delay here is that there is no clear indication of who the cassandra expert is for NixOS. Looking at the git history for both the package and service is a bit sparse. If no one is an expert on cassandra it will take a fair bit of work to review and validate these changes. So the question is... who do we need to get involved for review of this PR before someone with the appropriate access will feel comfortable merging?

@aanderse
Copy link
Member

aanderse commented Apr 22, 2019

@GrahamcOfBorg build cassandra
@GrahamcOfBorg test cassandra

@aanderse
Copy link
Member

@JohnAZoidberg cassandra is missing from all-tests.nix.

@JohnAZoidberg
Copy link
Member Author

Actually yeah, that makes me think I should add my new config options to the tests so that non-Cassandra people can trust that they do the right thing. (And also they're automatically checked.)

@JohnAZoidberg
Copy link
Member Author

Does anybody know whether I can rely on the IPs staying like they are? Or can I force them to be like I assume?

@aanderse
Copy link
Member

Does anybody know whether I can rely on the IPs staying like they are? Or can I force them to be like I assume?

Looking through nixos/tests I'm inclined to believe setting a static IP might be a good solution.

@JohnAZoidberg JohnAZoidberg changed the title Fix Cassandra and improve config Fix Cassandra, improve config and tests Apr 23, 2019
@JohnAZoidberg
Copy link
Member Author

JohnAZoidberg commented Apr 23, 2019

I added some more tests that test almost all config options.
And also made the IPs static.

nixos/tests/all-tests.nix Outdated Show resolved Hide resolved
@JohnAZoidberg
Copy link
Member Author

@fuzzy-id You rewrote the module - do you have any thoughts?

@aanderse
Copy link
Member

Looks like openjdk on arch is broken in master ☹️

hash mismatch in fixed-output derivation '/nix/store/pc2krcggyflqbgpz6zlpzxr7hjyfdlif-OpenJDK11U-jre_aarch64_linux_hotspot_11.0.3_7.tar.gz':
  wanted: sha256:0hisjzgv29x2af0wvpy1c9j027qvv08cd9r9f0hg40wdczbbgxaa
  got:    sha256:0r45f358ih5xmygqm91g6nkq9d6nnjl8zz75kl4xbij00svzlcfy

Unrelated to this change, so I'm going to ignore those aarch64-linux errors.

@fuzzy-id
Copy link
Contributor

On first sight this looks good and promising. @JohnAZoidberg did you run the tests with all versions of cassandra we currently have in nixpkgs?

Allow for more intuitive specifying of seed node addresses with Nix
syntax.
Seems like you can't have a node as its own seed when it's listening on
an interface instead of an IP. At least the way it was done in the
test doesn't work and I can't figure out any other way than to just
listen on the IP address instead.
Would previously overwrite the binary with the wrapper and thus wrap
itself (resulting in an infinite recursion on execution) for the
binaries in /bin.t
If we have the ability to enable remote JMX we should also support
setting credentials for that because they become required if you turn it
on.
If you're on a multi user system you don't want to have the password in
the nix-store. With the new jmxRolesFile option you can specify your own
protected file instead.
Below that it works but only when supplying a custom password file with
restricted permissions (i.e. outside the nix-store). We can't do that
using an absolute path in the tests.
If the `seedAddresses` is not set, don't force `SimpleSeedProvider` to
be in `seed_provider`. This could cause problems in a multi-datacenter
deployment when a different seed provider is preferred.
@JohnAZoidberg
Copy link
Member Author

Oh, I was responsible for those conflicts 😆

@aanderse
Copy link
Member

Thanks @JohnAZoidberg

@aanderse aanderse merged commit fadceeb into NixOS:master Jun 14, 2019
@ivan
Copy link
Member

ivan commented Jun 14, 2019

The NixOS manual doesn't seem to build with this merged:

# nixos-rebuild switch --builders ''
building Nix...
building the system configuration...
trace: warning: Option `programs.captive-browser.package' has no description.
these derivations will be built:
  /nix/store/d9ihpqcd0d6x5dswjfkwrsiwrv1jd2gk-options-db.xml.drv
  /nix/store/33p1av88g1y974qgjbmxz3a53nsv61vp-generated-docbook.drv
  /nix/store/w7zbd8q8akfdcy47vwplpz8n63kwi61b-nixos-manual-combined.drv
  /nix/store/y8z3qqwv7ivskf4m8rzqiz6jpqrgmk1k-manual-olinkdb.drv
  /nix/store/aslfagf5y3c267g41aj2kr3c9j2z8zvm-nixos-manual-html.drv
  /nix/store/dfsssp62vf9963yqdcq9wwc5axljxki2-nixos-help.drv
  /nix/store/rjpb8pnpk4m30dwbi245fpz4bycqb9x0-nixos-manual.desktop.drv
  /nix/store/yfkra26ay0lc76j06cbypj9dmk4c677l-nixos-manpages.drv
  /nix/store/y3qm2sy4y3czpisxdfn67g8vnpn5ccjl-system-path.drv
  /nix/store/n2k8cpgsmzs61v3jhfq4g1v694q1kyjp-dbus-1.drv
  /nix/store/sr7ywq644wvzcdaqm82mha8kf6nf0rw0-unit-dbus.service.drv
  /nix/store/h9dnqlg1pbkhnwx4040cff2z3bx4ab24-user-units.drv
  /nix/store/lija5f5blhlm19clnvlr8l98zpwh4clj-unit-polkit.service.drv
  /nix/store/p7xypymw1czr1rmkiy65rd8ki2xgy4bi-unit-dbus.service.drv
  /nix/store/wqzj1ycaw5va2iy3ddsjdbxckgb01hxj-unit-systemd-fsck-.service.drv
  /nix/store/wrig2rm34mbwq3sfyz5zpzii5xq04hda-system-units.drv
  /nix/store/w31lswqzbb21hgn4hy5inyq697kd7kw8-etc.drv
  /nix/store/0n3zi29zf8k7p1l46qkxy3qn0yh69qh0-nixos-system-ra-19.09.git.412d110.drv
building '/nix/store/d9ihpqcd0d6x5dswjfkwrsiwrv1jd2gk-options-db.xml.drv'...
intermediate.xml:12476: parser error : Opening and ending tag mismatch: literal line 12471 and para
</para></nixos:option-description><para><emphasis>Type:</emphasis> null or strin
       ^
intermediate.xml:12476: parser error : Opening and ending tag mismatch: literal line 12471 and nixos:option-description
</para></nixos:option-description><para><emphasis>Type:</emphasis> null or strin
                                  ^
intermediate.xml:12480: parser error : Opening and ending tag mismatch: para line 12470 and listitem
            </filename></member></simplelist></listitem></varlistentry><varliste
                                                        ^
intermediate.xml:12480: parser error : Opening and ending tag mismatch: option-description line 12470 and varlistentry
            </filename></member></simplelist></listitem></varlistentry><varliste
                                                                       ^
intermediate.xml:60118: parser error : Opening and ending tag mismatch: listitem line 12470 and variablelist
      </filename></member></simplelist></listitem></varlistentry></variablelist>
                                                                               ^
intermediate.xml:60118: parser error : Opening and ending tag mismatch: varlistentry line 12470 and appendix
lename></member></simplelist></listitem></varlistentry></variablelist></appendix
                                                                               ^
intermediate.xml:60120: parser error : Premature end of data in tag variablelist line 3

^
intermediate.xml:60120: parser error : Premature end of data in tag appendix line 3

^
unable to parse intermediate.xml
builder for '/nix/store/d9ihpqcd0d6x5dswjfkwrsiwrv1jd2gk-options-db.xml.drv' failed with exit code 6
cannot build derivation '/nix/store/33p1av88g1y974qgjbmxz3a53nsv61vp-generated-docbook.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/w7zbd8q8akfdcy47vwplpz8n63kwi61b-nixos-manual-combined.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/yfkra26ay0lc76j06cbypj9dmk4c677l-nixos-manpages.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/aslfagf5y3c267g41aj2kr3c9j2z8zvm-nixos-manual-html.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/y3qm2sy4y3czpisxdfn67g8vnpn5ccjl-system-path.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/0n3zi29zf8k7p1l46qkxy3qn0yh69qh0-nixos-system-ra-19.09.git.412d110.drv': 1 dependencies couldn't be built
error: build of '/nix/store/0n3zi29zf8k7p1l46qkxy3qn0yh69qh0-nixos-system-ra-19.09.git.412d110.drv' failed

pull bot pushed a commit to milibopp/nixpkgs that referenced this pull request Jun 14, 2019
Manual build broken by 79f7f89, which
is part of pull request NixOS#59179 (Fix Cassandra, improve config and
tests).

The issue was just a small error because of an unbalanced <literal/>
tag, so only a "/" was missing :-)

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @aanderse
@JohnAZoidberg JohnAZoidberg deleted the cassandra-module branch June 14, 2019 07:54
Copy link
Contributor

@turion turion left a comment

Choose a reason for hiding this comment

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

Just some minor improvement comments, but I can't say I understand nix well enough to judge completely.

cp "$cassandraEnvPkg" "$out/cassandra-env.sh"

# Delete default JMX Port, otherwise we can't set it using env variable
sed -i '/JMX_PORT="7199"/d' "$out/cassandra-env.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

If the source line ever changes, this will just fail silently?

Copy link
Member Author

Choose a reason for hiding this comment

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

If they remove or change this line, sed will not remove it and not issue a warning.
substituteInPlace would warn, but it doesn't have an option to delete an entire line.

I think it'll be fine if I implement the test for jmxPort like you suggested below.

networking.firewall.enable = false;
services.cassandra = cassandraCfg // extra;
networking = {
firewall.allowedTCPPorts = [ 7000 7199 9042 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of 7199, don't you want jmxPort?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, see #65616

ivan added a commit to ivan/nixpkgs that referenced this pull request Jun 18, 2019
The change to "NixOS Test Cluster" in NixOS#59179 broke startup of existing clusters
that used the previously-default cluster name "Test Cluster":

ERROR 23:00:47 Fatal exception during initialization
org.apache.cassandra.exceptions.ConfigurationException: Saved cluster name Test Cluster != configured name NixOS Test Cluster

Fixes NixOS#63388.
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

7 participants