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

cluster: storage node addesses discovery, added vmauth #1494

Merged

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Sep 14, 2024

related issue VictoriaMetrics/VictoriaMetrics#7040

@github-actions github-actions bot added the cluster vmcluster helm chart related issue label Sep 14, 2024
@AndrewChubatiuk AndrewChubatiuk changed the title Allow external storage nodes use autodiscovered nodes Allow external storage nodes, use autodiscovered nodes Sep 14, 2024
@AndrewChubatiuk AndrewChubatiuk force-pushed the allow-external-storage-nodes-use-autodiscovered-nodes branch 3 times, most recently from 59f4822 to ffbfc87 Compare September 14, 2024 17:42
@AndrewChubatiuk AndrewChubatiuk changed the title Allow external storage nodes, use autodiscovered nodes cluster: storage node addesses discovery, added vmauth Sep 14, 2024
@github-actions github-actions bot added the single VictoriaMetrics Single node helm chart related issue label Sep 14, 2024
@AndrewChubatiuk AndrewChubatiuk force-pushed the allow-external-storage-nodes-use-autodiscovered-nodes branch from 5c903f2 to ae04a03 Compare September 22, 2024 19:08
@AndrewChubatiuk AndrewChubatiuk force-pushed the allow-external-storage-nodes-use-autodiscovered-nodes branch from ae04a03 to d5dc4e7 Compare September 23, 2024 06:25
@AndrewChubatiuk AndrewChubatiuk force-pushed the allow-external-storage-nodes-use-autodiscovered-nodes branch from d5dc4e7 to adfba40 Compare September 23, 2024 07:18
Copy link
Contributor

@Haleygo Haleygo left a comment

Choose a reason for hiding this comment

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

added optional vmauth in front of vmselect and vminsert. fixes #376

User can only benefit more from vmauth than k8s service when address under url_prefix is pointing to a headless service, that way vmauth can resolve all the backend addresses in advance and proxy request to the least-loaded endpoint. But headless svc is not the default service type for both vmselect and vminsert here.
And I don't think we should add more logic to change the service type when enable the vmauth without making user understand why we do that. What about adding some instructions in readme?


# -- K8s cluster domain suffix, uses for building storage pods' FQDN. Details are [here](https://kubernetes.io/docs/tasks/administer-cluster/dns-custom-nameservers/)
clusterDomainSuffix: cluster.local
# -- Print information after deployment
printNotes: true

# use SRV discovery for storageNode and selectNode flags for enterprise version
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to move it with other .values.license fields for better understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

license can also be defined at global.license

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added failure if no license is defined with autoDiscovery enabled

charts/victoria-metrics-cluster/CHANGELOG.md Outdated Show resolved Hide resolved
@AndrewChubatiuk
Copy link
Contributor Author

added for vmauth.enabled param description about headless services

@Haleygo
Copy link
Contributor

Haleygo commented Sep 24, 2024

Need to set vmselect.name to "" for the new name template to avoid creating new service.

btw, templates seem to complicate debugging and make value modifications more fragile(like the fix needed above, before the service name is not connected with the container name).
I don't understand why we need to consolidate all the component' services into one template yaml, and they have very different port settings in the _helpers.tpl. The previous resources look easier to me only with some duplicated fields.

{{- define "vminsert.ports" -}}
- name: http
port: {{ .service.servicePort }}
protocol: TCP
targetPort: {{ .service.targetPort }}
{{- range .service.extraPorts }}
- name: {{ .name }}
port: {{ .port }}
protocol: TCP
targetPort: {{ .targetPort }}
{{- end }}
{{- with .extraArgs.clusternativeListenAddr }}
- name: cluster-tcp
protocol: TCP
port: {{ include "vm.port.from.flag" (dict "flag" .) }}
targetPort: cluster-tcp
{{- end }}
{{- with .extraArgs.graphiteListenAddr }}
- name: graphite-tcp
protocol: TCP
port: {{ include "vm.port.from.flag" (dict "flag" .) }}
targetPort: graphite-tcp
- name: graphite-udp
protocol: UDP
port: {{ include "vm.port.from.flag" (dict "flag" .) }}
targetPort: graphite-udp
{{- end }}
{{- with .extraArgs.influxListenAddr }}

@AndrewChubatiuk
Copy link
Contributor Author

AndrewChubatiuk commented Sep 24, 2024

btw, templates seem to complicate debugging and make value modifications more fragile.

having single template helps to find issues faster as it will fail in the same places for all apps and implement the same functionality for all apps at the same time

Copy link
Contributor

@zekker6 zekker6 left a comment

Choose a reason for hiding this comment

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

LGTM in general.
Added a few suggestions to fix a typo in configs. I would expect CI to detect this, but it seems like we don't have a test set to cover this templating path. Could you add some tests to cover this part?

charts/victoria-metrics-cluster/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/victoria-metrics-cluster/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/victoria-metrics-cluster/templates/_helpers.tpl Outdated Show resolved Hide resolved
AndrewChubatiuk and others added 2 commits October 3, 2024 21:05
@AndrewChubatiuk
Copy link
Contributor Author

AndrewChubatiuk commented Oct 3, 2024

Could you add some tests to cover this part?

thanks for review. added this scenario

@AndrewChubatiuk AndrewChubatiuk merged commit 2c68c4d into master Oct 3, 2024
8 checks passed
@AndrewChubatiuk AndrewChubatiuk deleted the allow-external-storage-nodes-use-autodiscovered-nodes branch October 3, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster vmcluster helm chart related issue logs-single single VictoriaMetrics Single node helm chart related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using nginx or vmauth for balancing read load to vmselects for cluster chart Multi Retention Setup
3 participants