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.

Sign up for GitHub

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

Jump to bottom

distributed: major refactoring, use common templates #1520

Merged
merged 4 commits into from
Nov 5, 2024
Merged

distributed: major refactoring, use common templates #1520

merged 4 commits into from
Nov 5, 2024

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Sep 20, 2024 •
edited
Loading

  • added default common.<component>.spec, which is merged with all *.<component>.spec, e.g: common.vmagent.spec is merged with availabilityZones[1].vmagent.spec
  • vmauthIngestGlobal was changed to write.global.vmauth
  • vmauthQueryGlobal was changed to read.global.vmauth
  • availabilityZones[*].allowIngest was changed to availabilityZones[*].write.allow
  • availabilityZones[*].allowRead was changed to availabilityZones[*].read.allow
  • availabilityZones[*].nodeSelector was moved to availabilityZones[*].common.spec.nodeSelector
  • availabilityZones[*].extraAffinity was moved to availabilityZones[*].common.spec.affinity
  • availabilityZones[*].topologySpreadConstraints was moved to availabilityZones[*].common.spec.topologySpreadConstraints
  • availabilityZones[*].vmauthIngest was moved to availabilityZones[*].write.vmauth
  • availabilityZones[*].vmauthQueryPerZone was moved to availabilityZones[*].read.perZone.vmauth
  • availabilityZones[*].vmauthCrossAZQuery was moved to availabilityZones[*].read.crossZone.vmauth

Haleygo
Haleygo reviewed Sep 24, 2024
View reviewed changes
charts/victoria-metrics-distributed/CHANGELOG.md
Comment on lines +3 to +4
- `vmauthIngestGlobal` was changed to `write.global.vmauth`
- `vmauthQueryGlobal` was changed to `read.global.vmauth`
Copy link
Contributor

Choose a reason for hiding this comment

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

How about moving them into .global as global.vmauthIngest and global.vmauthQuery?
Since there shouldn't be other fields under write.global or global.write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

global section in helm is special one, it's shared across all charts in release

charts/victoria-metrics-distributed/templates/per-az/vmagent.yaml Outdated Show resolved Hide resolved
charts/victoria-metrics-distributed/values.yaml
-
# -- Availability zone name
name: zone-eu-1
common:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think components shouldn't share other spec except nodeSelector/affinitytopologySpreadConstraints, which could lead to misuse and it's better to not lead to that way.
How about changing this as a plain commonSpec instead of having common.spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

common.spec allows having then common.annotations and common.labels

charts/victoria-metrics-distributed/values.yaml Outdated
# Available parameters can be found [here](https://docs.victoriametrics.com/operator/api/index.html#vmagentspec)
spec:
remoteWriteSettings:
useMultiTenantMode: true
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to not set useMultiTenantMode=true by default, since it degrades vmagent performance.

Copy link
Contributor Author

@AndrewChubatiuk AndrewChubatiuk Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

updated this

charts/victoria-metrics-distributed/values.yaml
vminsert:
serviceSpec:
spec:
clusterIP: None
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this setting?

Copy link
Contributor Author

@AndrewChubatiuk AndrewChubatiuk Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was hardcoded in templates, looks like it's needed to provide to vmauth pod ips and move balancing to vmauth

charts/victoria-metrics-distributed/templates/per-az/vmauth-read-proxy.yaml Show resolved Hide resolved
charts/victoria-metrics-distributed/values.yaml
whenUnsatisfiable: ScheduleAnyway
write:
# -- Allow data ingestion to this zone
allow: true
Copy link
Contributor

Choose a reason for hiding this comment

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

How about unifying them to enable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

enable is for resource creation and allow is for traffic routing

@AndrewChubatiuk AndrewChubatiuk force-pushed the refactor-distributed-chart branch 2 times, most recently from dcc3720 to 3c05054 Compare October 10, 2024 11:10
@github-actions github-actions bot added k8s-stack k8s-stack helm chart related issue alert vmalert helm chart related issue auth vmauth helm chart related issue cluster vmcluster helm chart related issue operator operator helm chart related issue single VictoriaMetrics Single node helm chart related issue anomaly vmanomaly labels Nov 5, 2024
zekker6
zekker6 previously approved these changes Nov 5, 2024
View reviewed changes
@AndrewChubatiuk AndrewChubatiuk removed k8s-stack k8s-stack helm chart related issue alert vmalert helm chart related issue auth vmauth helm chart related issue cluster vmcluster helm chart related issue single VictoriaMetrics Single node helm chart related issue anomaly vmanomaly logs-single labels Nov 5, 2024
@AndrewChubatiuk AndrewChubatiuk merged commit d9dc9d3 into master Nov 5, 2024
14 checks passed
@AndrewChubatiuk AndrewChubatiuk deleted the refactor-distributed-chart branch November 5, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zekker6 zekker6 zekker6 approved these changes

@f41gh7 f41gh7 Awaiting requested review from f41gh7 f41gh7 is a code owner

@Haleygo Haleygo Awaiting requested review from Haleygo Haleygo is a code owner

Assignees
No one assigned
Labels
operator operator helm chart related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndrewChubatiuk @zekker6 @Haleygo