-
-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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/curator: init elasticsearch curator #44340
Conversation
|
||
options.services.curator = { | ||
|
||
enable = 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.
Maybe use mkEnableOption here.
}; | ||
in { | ||
|
||
options.services.curator = { |
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 we should name the module elasticsearch-curator
instead of just curator
because it's more descriptive and consistent with the name of the Python package.
|
||
config = mkIf cfg.enable { | ||
|
||
systemd.services.curator = { |
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.
As with the module name I think we should also name the systemd unit elasticsearch-curator
instead of just curator
.
enable = cfg.enable; | ||
startAt = cfg.interval; | ||
serviceConfig = { | ||
ExecStart = ''${pkgs.python36Packages.elasticsearch-curator}/bin/curator --config ${curatorConfig} ${curatorAction}''; |
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.
<nitpicking> could you indent this line? </nitpicking>
# not a Python "NoneType" | ||
client: | ||
hosts: ${builtins.toJSON cfg.hosts} | ||
port: 9200 |
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.
Shouldn't the port also be configurable? We should probably default it to services.elasticsearch.port.
@basvandijk I have updated the PR according to your PR, do I need to squash the extra commit? |
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.
Could you also rename the file to elasticsearch-curator.nix
?
with lib; | ||
|
||
let | ||
cfg = config.services.curator; |
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 should now be: config.services.elasticsearch-curator
}; | ||
port = mkOption { | ||
description = "the port that elasticsearch is listening on"; | ||
type = types.int |
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.
You're missing a semicolon here.
fixed. should I squash @basvandijk ? |
@shmish111 thank you. A squash would be nice. Any chance you could add a NixOS test for this? Maybe extend the existing ELK test. |
@basvandijk do you think it is ok to add it to the ELK test? I feel like it would be unnecessary overhead to have a test that boots up another ES machine when I could just add in some test cases to the existing ELK test. I suppose I would just create an index in ES, run a curator action that should delete it and then check it has been deleted. |
@shmish111 extending the existing ELK test like you suggest is the way to go IMHO. It would not only test curator but also elasticsearch itself. |
@basvandijk how do I run the nixpkgs tests? I can't find documentation anywhere :-( |
@basvandijk I have squashed. Also, I can't get the test passing on
This fails to connect to elasticsearch. It seems that elasticsearch has failed to start but I can't work out how to debug in order to find out why. |
See the following on how to run tests interactively: |
@basvandijk I've tried to play around but it's difficult to debug as I can't get into a shell in the running VM. All I can see is the output of Have you managed to run these tests successfully? I am trying to run these on a NixOS machine on AWS with the image I think the test maintainers need to look at this, does this need to prevent my PR? I'm happy to add a test but the existing test needs to be fixed first and at the moment that is beyond me. |
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.
Regarding the test, I can successfully run nix-build nixos/tests/elk.nix
on my Mac Book Pro mid 2015 running NixOS 18.03. Maybe a t2.xlarge
can't run VMs?
Do you have some statements available that create an index on elasticsearch and an actionYAML
definition that deletes that index? Then I can try running that test to see if it works.
config = mkIf cfg.enable { | ||
|
||
systemd.services.elasticsearch-curator = { | ||
enable = cfg.enable; |
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.
You don't need to set this since enable
is true
by default and the whole definition is already under a mkIf
.
@basvandijk ok, I've fixed the |
Great we're getting closer! I pushed some more fixes to your branch. In the end you might want to squash all of them. Could you look into fixing the
I see we don't have a package for |
You might also want to add yourself as maintainer of the module using:
and add |
8c8862c
to
50dd177
Compare
@GrahamcOfBorg test elk.ELK-5 elk.ELK-6 |
No attempt on aarch64-linux (full log) The following builds were skipped because they don't evaluate on aarch64-linux: tests.elk.ELK-5, tests.elk.ELK-6 Partial log (click to expand)
|
Timed out, unknown build status on x86_64-linux (full log) Attempted: tests.elk.ELK-5, tests.elk.ELK-6 Partial log (click to expand)
|
TODO: we're still missing the requests-aws4auth>=0.9 dependency
Needed by elasticsearch-curator.
…elasticsearch-curator
|
||
propagatedBuildInputs = [ requests ]; | ||
|
||
doCheck = false; |
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.
Why are the tests disabled? Please include a comment.
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.
The test hang. I'm not sure why. Good point about adding a comment. Will do.
See elasticsearch curator
Motivation for this change
I needed to use this tool on my NixOS ES cluster
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)