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

allow env variables for numa settings #152

Merged
merged 6 commits into from
Jan 28, 2025
Merged

allow env variables for numa settings #152

merged 6 commits into from
Jan 28, 2025

Conversation

koomie
Copy link
Collaborator

@koomie koomie commented Jan 27, 2025

No description provided.

OMNISTAT_EXPORTER_COREBINDING, OMNISTAT_VICSERVER_COREBINDING

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
)

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
@@ -132,6 +132,9 @@ def startVictoriaServer(self):

vm_logfile = self.runtimeConfig[section].get("victoria_logfile", "victoria_server.log")
vm_corebinding = self.runtimeConfig[section].getint("victoria_corebinding", None)
# corebinding can also be overridden by separate env variable
if "OMNISTAT_VICSERVER_COREBINDING" in os.environ:
Copy link
Collaborator

@jordap jordap Jan 27, 2025

Choose a reason for hiding this comment

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

This is minor, but we use victoria_corebinding and OMNISTAT_VICSERVER_COREBINDING, and internally we use vm for the variables. It would be nice to settle on one name for Victoria Metrics.

Victoria Metrics itself uses either victoria-metrics, victoriametrics, or vm. I understand victoriametrics is long, and vm can be confused with virtual machine.

Any thoughts? Even if vm makes me think of virtual machine, there's enough context and it could work. We could also use vmserver: OMNISTAT_VMSERVER_COREBINDING and vmserver_corebinding.

@koomie
Copy link
Collaborator Author

koomie commented Jan 27, 2025

I'm a bit hesitant to change the existing nomenclature in the runtime config file which goes beyond a core binding setting, e.g.

victoria_binary = /path-to-victoria-metrics-install/victoria-metrics-prod
victoria_datadir = data_prom
victoria_logfile = vic_server.log

I'm all for consistency but we have also advertised OMNISTAT_VICSERVER_DATADIR to override the datadir setting so there are multiple things that would need to change for consistency if we were to do it.

Seems we have the following options:

  1. continue to have a slightly different convention in env variable name vs runtime file setting
  2. update all environment variable override name options to match runtime file counterparts exactly with an OMNISTAT prefix
  3. change runtime variable names and environment variable names to some new, consistent schema

Options 2. and 3. would introduce a version incompatibility for the runtime file and environment variable settings. Granted, most folks are using the supplied runtime file, so now would be the time to do it but I confess to wondering if it's that useful to change things now.

Thoughts?

@koomie
Copy link
Collaborator Author

koomie commented Jan 27, 2025

I'm a bit hesitant to change the existing nomenclature in the runtime config file which goes beyond a core binding setting, e.g.

victoria_binary = /path-to-victoria-metrics-install/victoria-metrics-prod
victoria_datadir = data_prom
victoria_logfile = vic_server.log

I'm all for consistency but we have also advertised OMNISTAT_VICSERVER_DATADIR to override the datadir setting so there are multiple things that would need to change for consistency if we were to do it.

Seems we have the following options:

  1. continue to have a slightly different convention in env variable name vs runtime file setting
  2. update all environment variable override name options to match runtime file counterparts exactly with an OMNISTAT prefix
  3. change runtime variable names and environment variable names to some new, consistent schema

Options 2. and 3. would introduce a version incompatibility for the runtime file and environment variable settings. Granted, most folks are using the supplied runtime file, so now would be the time to do it but I confess to wondering if it's that useful to change things now.

Thoughts?

Option 4 (assuming we don't care about env variable name consistency with prometheus settings in user mode which are likely to get deprecated at some point)

  1. change OMNISTAT_VICSERVER_DATADIR and OMNISTAT_VICSERVER_COREBINDING to
  • OMNISTAT_VICTORIA_DATADIR
  • OMNISTAT_VICTORIA_COREBINDING

This would make them consistent with the runtime variable names. Since the corebinding variable is new, that won't affect anyone and we could continue to support OMNISTAT_VICSERVER_DATADIR with a warning to phase out in future release.

…put slightly

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
@koomie koomie added this to the 1.3 milestone Jan 27, 2025
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
@koomie
Copy link
Collaborator Author

koomie commented Jan 27, 2025

Via direct comms, Option 4 above is the preference.

OMNISTAST_VICTORIA_COREBINDING; also update datadir override to
OMNISTAT_VICTORIA_DATADIR. The older OMNISTAT_VICSERVER_DATADIR
setting is still supported for the time being but a warning is thrown
to use the new name.

Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
Signed-off-by: Karl W Schulz <karl.schulz@amd.com>
@koomie koomie merged commit 84addd2 into main Jan 28, 2025
9 checks passed
@koomie koomie deleted the issue_151 branch January 28, 2025 22:03
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