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

helm: add enabled field for admin-api,compactor,distributor,gateway,ingester,querier,query-frontend,store-gateway #9734

Merged
merged 17 commits into from
Nov 11, 2024

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented Oct 24, 2024

What this PR does

This adds

  • enabled flag for a few components which are normally required for a Mimir/GEM deployment. We need this so that deploying the federation-frontend doesn't end up deploying a ton of unrelated and unnecessary resources.
  • federation_frontend.disableOtherComponents to have the same effect as setting enabled: false on all components.

I've also disabled them in the rendered federation-frontend values. Now all the files when rendering the federation-frontend are these:

operations/helm/tests/test-enterprise-federation-frontend-values-generated
└── mimir-distributed
   └── templates
      ├── federation-frontend
      │  ├── federation-frontend-dep.yaml
      │  ├── federation-frontend-pdb.yaml
      │  └── federation-frontend-svc.yaml
      ├── license-secret.yaml
      ├── mimir-config.yaml
      ├── minio
      │  └── create-bucket-job.yaml
      ├── podsecuritypolicy.yaml
      ├── role.yaml
      ├── rolebinding.yaml
      └── serviceaccount.yaml

Which issue(s) this PR fixes or relates to

Related to #9672

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

Comment on lines -21 to -22
instrumentation:
enabled: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now this is automatically set up

@@ -10,7 +10,7 @@ compactor:
ingester:
schedulerName: my-scheduler

store-gateway:
store_gateway:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated to the PR, but it's wrong test config which i noticed

Comment on lines 292 to 295
{{- if .Values.distributor.enabled }}
distributor:
url: dns:///{{ template "mimir.fullname" . }}-distributor-headless.{{ .Release.Namespace }}.svc.{{ .Values.global.clusterDomain }}:{{ include "mimir.serverGrpcListenPort" . }}
{{- end }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the gateway would try to resolve the dns:/// address before starting. Since it's not there, we shouldn't set it up.

Copy link
Contributor

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Do we still need the gossip-ring service when deploying the federation-frontend? Same question for the runtime ConfigMap.

operations/helm/charts/mimir-distributed/CHANGELOG.md Outdated Show resolved Hide resolved
@dimitarvdimitrov
Copy link
Contributor Author

dimitarvdimitrov commented Oct 25, 2024

Do we still need the gossip-ring service when deploying the federation-frontend? Same question for the runtime ConfigMap.

technically the FF doesn't use either, but it keeps the config consistent with other components. I guess that's not a good enough reason. I'll remove them along with the disableOtherComponents flag

update: they're disabled in eda445e

most of the disableOtherComponents work is done in 6a4f68e

Copy link
Contributor

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

LGTM modulo question about outdated comment below

Base automatically changed from dimitar/helm/federation-frontend to main November 11, 2024 07:58
dimitarvdimitrov and others added 14 commits November 11, 2024 11:57
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/helm/add-enabled-falgs-to-components branch from eda445e to 13cd3b6 Compare November 11, 2024 09:58
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/helm/add-enabled-falgs-to-components branch from 299a2b1 to 383dca1 Compare November 11, 2024 12:57
@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) November 11, 2024 14:28
@dimitarvdimitrov dimitarvdimitrov merged commit e32b5e2 into main Nov 11, 2024
31 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/helm/add-enabled-falgs-to-components branch November 11, 2024 14:35
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