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

mimir-distributed: allow components to override their container image #10340

Merged
merged 11 commits into from
Jan 10, 2025

Conversation

narqo
Copy link
Contributor

@narqo narqo commented Jan 3, 2025

What this PR does

The PR updates the Helm chart to allow an individual component to override its container image reference via the component's local values. E.g.

# values.yaml

ingester:
  image:
    repository: registry.example/mimir-ingester
    tag: blah-blah

The order in which the values take precedence is:

  1. take the component's local image or tag if defined
  2. take the image defined in the enterprise section if enterprise is enabled
  3. take the globally-defined image otherwise

Which issue(s) this PR fixes or relates to

Fixes #10237

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.

@narqo narqo requested a review from a team as a code owner January 3, 2025 12:34
@narqo narqo force-pushed the vldmr/helm-component-image-override branch from a650f8a to d559783 Compare January 3, 2025 12:35
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

can you also include the keys in the values.yaml? Since we still want to use the top-level image, then you can make the new <component>.image be a comment. Something like the TSC examples

# -- topologySpreadConstraints allows to customize the default topologySpreadConstraints. This can be either a single dict as shown below or a slice of topologySpreadConstraints.
# labelSelector is taken from the constraint itself (if it exists) or is generated by the chart using the same selectors as for services.
topologySpreadConstraints:
maxSkew: 1
topologyKey: kubernetes.io/hostname
whenUnsatisfiable: ScheduleAnyway
# minDomains: 1
# nodeAffinityPolicy: Honor
# nodeTaintsPolicy: Honor
# matchLabelKeys:
# - pod-template-hash

that's basically the only documentation we have of helm chart features (plus the changelog, which is barely documentation)

@@ -30,6 +30,7 @@ Entries should include a reference to the Pull Request that introduced the chang
## main / unreleased

* [CHANGE] Memcached: Update to Memcached 1.6.34. #10318
* [ENHANCEMENT] Add support for individual components to override their container image. #10340
Copy link
Contributor

Choose a reason for hiding this comment

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

i've been trying to make the changelog enough for anyone to get the gist of a change and know how to use it. Can you give an example of overriding the image for a component? Something like ingester.image.tag and ingester.image.repository. Or explain that there's an image object under all component objects now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rephrased the entry with more details about the change.

@narqo narqo requested a review from dimitarvdimitrov January 8, 2025 15:15
narqo added 7 commits January 8, 2025 16:15
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
@narqo narqo force-pushed the vldmr/helm-component-image-override branch from 089ca5e to 0760e24 Compare January 8, 2025 15:17
@narqo
Copy link
Contributor Author

narqo commented Jan 8, 2025

🆙 I've addressed the comments. PHAL

narqo added 3 commits January 8, 2025 17:03
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
@@ -83,7 +83,7 @@ function generate_manifests() {
helm template "${ARGS[@]}" 1>/dev/null
cp -r "${INTERMEDIATE_OUTPUT_DIR}" "${OUTPUT_DIR}"
rm "${OUTPUT_DIR}/${CHART_NAME}/templates/values-for-rego-tests.yaml"
find "${OUTPUT_DIR}/${CHART_NAME}/templates" -type f -print0 | xargs -0 "${SED}" -E -i -- "/^\s+(checksum\/(alertmanager-fallback-)?config|(helm.sh\/)?chart|app.kubernetes.io\/version|image: \"grafana\/(mimir|mimir-continuous-test|enterprise-metrics)):/d"
find "${OUTPUT_DIR}/${CHART_NAME}/templates" -type f -print0 | xargs -0 "${SED}" -E -i -- "/^\s+(checksum\/(alertmanager-fallback-)?config|(helm.sh\/)?chart|app.kubernetes.io\/version|image: \"?grafana\/(mimir|mimir-continuous-test|enterprise-metrics)):/d"
Copy link
Contributor Author

@narqo narqo Jan 8, 2025

Choose a reason for hiding this comment

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

This one fixes the build script, so it looked for both quoted and non-quoted reference to the default image — both variants are valid to use in the YAML:

image: "grafana/mimir:

and

image: grafana/mimir:

(note the quote ")

That is, we don't want the generated manifests to have the default image, so we didn't have to re-generate them everytime the default image changes (e.g. on every weekly release).

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

values.yaml being the only interface to the chart and the primary source of docs, I try to keep them as explicit as possible. You could also mention that per-component settings exist in the docs for the image and enterprise.image sections.

But I don't need to see this again. Thanks!

@@ -551,6 +551,11 @@ alertmanager:
# E.g. if 'replicas' is set to 4 and there are 3 zones, then 4/3=1.33 and after rounding up it means 2 pods per zone are started.
replicas: 1

# -- Allows to override the container image of the alertmanager component.
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, i may be getting annoying, but can you mention that this takes precedence over the root-level image and enterprise.image sections?

Suggested change
# -- Allows to override the container image of the alertmanager component.
# -- Allows to override the container image of the alertmanager component from the root-level `image` and `enterprise.image` sections.

something along those lines. You should be able to do a find-and-replace of the string.

Signed-off-by: Vladimir Varankin <[email protected]>
@narqo narqo enabled auto-merge (squash) January 10, 2025 14:27
@narqo narqo merged commit 6c186ae into main Jan 10, 2025
31 checks passed
@narqo narqo deleted the vldmr/helm-component-image-override branch January 10, 2025 14:45
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.

Allow override image of individual components in Helm chart
2 participants