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

Add Prometheus 2 service in parallel with 1.x version (continuation) #58255

Merged
merged 13 commits into from Apr 9, 2019

Conversation

jbgi
Copy link
Contributor

@jbgi jbgi commented Mar 25, 2019

Motivation for this change

Continuation of the work done by @azazel75 in #49802, addressing remaining review points.

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.

azazel75 and others added 5 commits March 25, 2019 14:36
As the configuration for the exporters and alertmanager is unchanged
between the two major versions this patch tries to minimize
duplication while at the same time as there's no upgrade path from 1.x
to 2.x, it allows running the two services in parallel. See also NixOS#56037
@jbgi jbgi requested a review from infinisil as a code owner March 25, 2019 13:46
This uses fewer lines of code and one less process.
Previously the prometheus.service file looked like:

  ExecStart=/nix/store/wjkhfw3xgkmavz1akkqir99w4lbqhak7-prometheus-1.8.2-bin/bin/prometheus -storage.local.path=/var/lib/prometheus/metrics \
    -config.file=/nix/store/zsnvzw51mk3n1cxjd0351bj39k1j6j27-prometheus.yml-check-config-checked \
    -web.listen-address=0.0.0.0:9090 \
    -alertmanager.notification-queue-capacity=10000 \
    -alertmanager.timeout=10s \
     \

  Restart=always

Now it's:

  ExecStart=/nix/store/wjkhfw3xgkmavz1akkqir99w4lbqhak7-prometheus-1.8.2-bin/bin/prometheus \
    -storage.local.path=/var/lib/prometheus/metrics \
    -config.file=/nix/store/zsnvzw51mk3n1cxjd0351bj39k1j6j27-prometheus.yml-check-config-checked \
    -web.listen-address=0.0.0.0:9090 \
    -alertmanager.notification-queue-capacity=10000 \
    -alertmanager.timeout=10s
  Restart=always
@basvandijk
Copy link
Member

@jbgi I just pushed a commit which registers the prometheus2 test. So now we should be able to instruct:

@GrahamcOfBorg test prometheus prometheus2

@basvandijk
Copy link
Member

I added the changes around Prometheus to the 19.09 release notes. I think this is ready to be merged now. If nobody objects I'll merge after the checks are green.

@basvandijk
Copy link
Member

basvandijk commented Apr 8, 2019

@jbgi @fpletz I was thinking of adding 60507cc to this PR to add a bit more backwards compatibility by keeping the services.prometheus.dataDir directory. This might allow this PR to be backported to 19.03. What do you think? If you agree with the commit, I'll add it to the PR and update the 19.09 release notes accordingly.

@jbgi
Copy link
Contributor Author

jbgi commented Apr 9, 2019

@basvandijk thanks! backporting this PR would be great indeed. Just a minor nitpick about isNull being deprecated.

@basvandijk
Copy link
Member

basvandijk commented Apr 9, 2019

@jbgi thanks for the heads-up regarding the deprecation of isNull. What should I use instead?

@jbgi
Copy link
Contributor Author

jbgi commented Apr 9, 2019

@basvandijk just == null or != null.

This is to ensure more backwards compatibility. Note this is not 100%
backwards compatible because we now require dataDir to begin with /var/lib/.
@basvandijk
Copy link
Member

Thanks. I just pushed these commits to this PR. Will merge after the checks are green.

@GrahamcOfBorg test prometheus prometheus2

@basvandijk basvandijk merged commit 2f2e297 into NixOS:master Apr 9, 2019
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

4 participants