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 VictoriaMetrics package, a time series database. #1512

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gkoh
Copy link
Contributor

@gkoh gkoh commented Aug 21, 2024

Package multiple pieces of the Prometheus compatible VictoriaMetrics suite:

  • victoria-metrics
  • vmagent
  • vmbackup
  • vmrestore
  • vmctl

Whilst here, correct a minor spelling error in functions.sh.

@gkoh
Copy link
Contributor Author

gkoh commented Aug 21, 2024

I need some advice on this package, there are some oddities.

  1. I have claimed uid/gid 93, hope this is correct

  2. Configuration is not via file, it is via command-line argument or equivalent envvar.
    Updating the service manifest to change the command line is crazy, so I think the right answer is by applying service profiles that modify the instance method_environment.
    I have placed examples of such profiles in share/victoriametrics/xxx-profile.xml.

  3. Using an envvar, I have set the storageDataPath for the server victoria-metrics in the default svc manifest, however I believe this will be overwritten by an update. This should probably be applied via profile only. By doing so, the server and agent will require a service profile to be applied or they will not function correctly.

  4. I have added some install notes to cover the profile stuff above, I hope this is sufficient.

Copy link
Member

@citrus-it citrus-it left a comment

Choose a reason for hiding this comment

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

I need some advice on this package, there are some oddities.

  1. I have claimed uid/gid 93, hope this is correct

Yes, and thank you for updating the idlist file. That's where we track allocated UIDs/GIDs across all repositories.
Please could you use tabs instead of spaces so that the new lines match the existing ones?
Same in packages.md.

  1. Configuration is not via file, it is via command-line argument or equivalent envvar.
    Updating the service manifest to change the command line is crazy, so I think the right answer is by applying service profiles that modify the instance method_environment.
    I have placed examples of such profiles in share/victoriametrics/xxx-profile.xml.

That seems like a good solution, and thank for you for the installation notes too.

  1. Using an envvar, I have set the storageDataPath for the server victoria-metrics in the default svc manifest, however I believe this will be overwritten by an update. This should probably be applied via profile only. By doing so, the server and agent will require a service profile to be applied or they will not function correctly.

Have you checked if they are overwritten on update? I don't think they will be if they have ever been modified with svccfg or a profile.

  1. I have added some install notes to cover the profile stuff above, I hope this is sufficient.

Yes, thank you!

Please could you also update doc/baseline.md?

build/victoriametrics/build.sh Outdated Show resolved Hide resolved
build/victoriametrics/build.sh Outdated Show resolved Hide resolved
build/victoriametrics/build.sh Outdated Show resolved Hide resolved
build/victoriametrics/build.sh Outdated Show resolved Hide resolved
build/victoriametrics/files/victoriametrics-template.xml Outdated Show resolved Hide resolved
build/victoriametrics/files/victoriametrics-template.xml Outdated Show resolved Hide resolved
lib/functions.sh Show resolved Hide resolved
gkoh and others added 3 commits September 2, 2024 01:48
Package multiple pieces of the Prometheus compatible VictoriaMetrics suite:
- victoria-metrics
- vmagent
- vmbackup
- vmrestore
- vmctl

Whilst here, correct a minor spelling error in functions.sh.
@gkoh gkoh reopened this Sep 2, 2024
@gkoh
Copy link
Contributor Author

gkoh commented Sep 2, 2024

I need some advice on this package, there are some oddities.

  1. I have claimed uid/gid 93, hope this is correct

Yes, and thank you for updating the idlist file. That's where we track allocated UIDs/GIDs across all repositories. Please could you use tabs instead of spaces so that the new lines match the existing ones? Same in packages.md.

Done.

  1. Using an envvar, I have set the storageDataPath for the server victoria-metrics in the default svc manifest, however I believe this will be overwritten by an update. This should probably be applied via profile only. By doing so, the server and agent will require a service profile to be applied or they will not function correctly.

Have you checked if they are overwritten on update? I don't think they will be if they have ever been modified with svccfg or a profile.

OK, I checked again, properly. You are correct.
On update, values modified by a profile are not overwritten.
Thus, I have left it as-is.

  1. I have added some install notes to cover the profile stuff above, I hope this is sufficient.

Yes, thank you!

Please could you also update doc/baseline.md?

I updated both doc/baseline and doc/baseline.aarch64.
I can't test an aarch64 build (no hardware) but VictoriaMetrics themselves release arch64 binaries.

Thank you for the review.

@gkoh
Copy link
Contributor Author

gkoh commented Sep 2, 2024

Apologies, github didn't like my fat-fingered rebase so some of the review comment context got nuked.
I think I have addressed all review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants