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/influxdb2: add provisioning and nixos tests #249502

Merged
merged 3 commits into from
Aug 24, 2023

Conversation

oddlama
Copy link
Contributor

@oddlama oddlama commented Aug 16, 2023

Description of changes

This PR adds provisioning options and nixos tests to influxdb2. This allows skipping the initial setup page and automatically creates/deletes the following entities:

  • organizations
  • buckets
  • users
  • api tokens (auths)

Originally initial setup and entity provisioning was a single PR which has been split into two to reduce scope and discuss details of the implementation. This is the second part, which needs to be rebased the first part once merged:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@oddlama
Copy link
Contributor Author

oddlama commented Aug 16, 2023

In summary, @NickCao was asking whether the setup script can be simplified (and thus was split to this separate PR):

Overall I like the idea of declarative setup, but the implementation looks overly complicated. Can we first get the part that can be done with influx setup in and figure out better ways to do the rest?

I'm currently not seeing how this could be made simpler, but I am open for suggestions.

@oddlama
Copy link
Contributor Author

oddlama commented Aug 16, 2023

Rebased on master.

@oddlama oddlama force-pushed the feat-influxdb-provision-full branch from 4eb485a to 80b25dd Compare August 17, 2023 14:45
@oddlama
Copy link
Contributor Author

oddlama commented Aug 17, 2023

Restructured the provisioning script. All scripts are now defined on the respective option for increased consistency and readability. Also added the option to provision the secret value of api tokens.

@oddlama oddlama force-pushed the feat-influxdb-provision-full branch from 80b25dd to 5da9a19 Compare August 17, 2023 15:17
@oddlama oddlama marked this pull request as ready for review August 17, 2023 15:35
Copy link
Member

@NickCao NickCao left a comment

Choose a reason for hiding this comment

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

And I thinks that a better API might look like:

provisioning.resources = {
  organization = {
    acme-corp = {
      present = true; # the default
      some-attribute = "bar";
      bucket = {
        example = {
          present = false; # indicates removal
        };
      };
    };
  };
}

nixos/modules/services/databases/influxdb2.nix Outdated Show resolved Hide resolved
nixos/modules/services/databases/influxdb2.nix Outdated Show resolved Hide resolved
nixos/modules/services/databases/influxdb2.nix Outdated Show resolved Hide resolved
@NickCao
Copy link
Member

NickCao commented Aug 18, 2023

And as there are various influxdb modules for ansible (https://docs.ansible.com/ansible/latest/collections/community/general/influxdb_database_module.html), we can generate a ansible-playbook to carry out the actual operation.

@oddlama
Copy link
Contributor Author

oddlama commented Aug 18, 2023

And I thinks that a better API might look like: [...]

Fully agree, I'll refactor this.

And as there are various influxdb modules for ansible (https://docs.ansible.com/ansible/latest/collections/community/general/influxdb_database_module.html), we can generate a ansible-playbook to carry out the actual operation.

I fear that is a module for influx v1 not v2. I've also searched for alternatives, but I cannot find any that support provisioning remotes, replications or authentication tokens.

@NickCao
Copy link
Member

NickCao commented Aug 18, 2023

Another thought: we may use https://github.com/influxdata/influxdb-client-python of other sdks instead of the cli, which would be easier/more readable than bash

@oddlama
Copy link
Contributor Author

oddlama commented Aug 18, 2023

Another thought: we may use https://github.com/influxdata/influxdb-client-python of other sdks instead of the cli, which would be easier/more readable than bash

Looks good initially, I'll see how we can utilize that

@oddlama
Copy link
Contributor Author

oddlama commented Aug 19, 2023

Summary of the last update:

  • Switched to suggested configuration schema
  • I've switched to using an external python script that is utilizing influxdb-client and applies the desired state which is passed via json.
  • For now, I've removed provisioning of remotes and replications from the scope of this PR, because it will require more work (the library has the capability but doesn't contain all required classes to do so easily). Maybe for a future PR.
  • Added extensive testing
  • Shortly mention this change in the release notes
  • Rebased on master

@oddlama oddlama force-pushed the feat-influxdb-provision-full branch from 4f8b810 to 55d3b33 Compare August 23, 2023 09:58
@oddlama
Copy link
Contributor Author

oddlama commented Aug 23, 2023

Update:

  • Use lib.mdDoc for descriptions
  • Move organizations submodule declaration to top let block
  • Use /run/current-system/systemd/bin/systemctl instead of systemctl

@NickCao
Copy link
Member

NickCao commented Aug 23, 2023

And be sure to split the changes into individual commits. (I did not check the correctness of the assertions e.t.c. but I'm sure you can do a better job than me in that regard)

@oddlama oddlama force-pushed the feat-influxdb-provision-full branch from 55d3b33 to ef4e3ae Compare August 23, 2023 10:20
@oddlama
Copy link
Contributor Author

oddlama commented Aug 23, 2023

Split in to 3 commits (1 for each package, one for the module update)

@NickCao
Copy link
Member

NickCao commented Aug 23, 2023

influxdb2-token-manipulator is failing build due to hash mismatch.

@oddlama
Copy link
Contributor Author

oddlama commented Aug 23, 2023

influxdb2-token-manipulator is failing build due to hash mismatch.

Ah sorry, that hash seems to have been present in my local store from a previous version so I didn't notice :/
Updated now.

@oddlama oddlama force-pushed the feat-influxdb-provision-full branch from ef4e3ae to 8b5b7de Compare August 23, 2023 12:46
@NickCao
Copy link
Member

NickCao commented Aug 24, 2023

@ofborg test influxdb2

NickCao

This comment was marked as outdated.

Copy link
Member

@NickCao NickCao left a comment

Choose a reason for hiding this comment

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

Tested with nixos tests, also tested evaluation with provision disabled.

@NickCao NickCao merged commit 8d524e6 into NixOS:master Aug 24, 2023
12 of 13 checks passed
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.

2 participants