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

[prometheus] Bump chart dependencies #3422

Merged

Conversation

zeritti
Copy link
Member

@zeritti zeritti commented May 23, 2023

What this PR does / why we need it

This PR brings the following changes:

  • Updates chart dependencies as follows:
    • alertmanager 0.33.1
    • kube-state-metrics 5.8.1 (major change)
    • prometheus-node-exporter 4.18.1
    • prometheus-pushgateway 2.3.0
  • Minor corrections in README

Which issue this PR fixes

None

Special notes for your reviewer

Included is a major (breaking) change, upgrade notes have been added.

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

@zeritti zeritti marked this pull request as ready for review May 23, 2023 22:14
| [prometheus-pushgateway](https://github.com/prometheus-community/helm-charts/releases/tag/prometheus-pushgateway-2.1.6) | 2.1.6 | 2.0.4 |

Users are advised to review changes in the corresponding chart releases before upgrading.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zeritti
do we need to provide some way to tell how to export the user current data and import to the new version?

Copy link
Member

Choose a reason for hiding this comment

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

which data should that be?

Copy link
Contributor

Choose a reason for hiding this comment

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

@monotek
templates/pvc.yaml, in case the user enable PVC.

Copy link
Member Author

@zeritti zeritti Jun 30, 2023

Choose a reason for hiding this comment

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

@zanhsieh There is no issue related to the pushgateway's data stored in a persistent volume during the upgrade, the volume/its contents are not affected and no change on the pushgateway's side in this respect.

The same is true for Prometheus's PV - no side efects have come up in my tests.

However, whilst upgrading the pushgateway subchart with deployment and persistent volume enabled is straightforward, not so with statefulset and persistent volume - the latter cannot be patched due to changing labels and the upgrade fails. Fo this scenario, I'll have to add a note on upgrading - deleting the pushgateway's statefulset is needed before upgrading.

For prometheus-pushgateway, we should consider whether including the full label set from prometheus-pushgateway.defaultLabels (with app.kubernetes.io/version) in statefulset's spec.volmeClaimTemplates is reasonable as it leads to requiring manual intervention at an upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zeritti
Thank you for testing that out. I think put some warning text as for pushgateway subchart sts with pvc as you suggested should be fine. We only do due diligence since in that case the user decided to go with sts - that's their choice.

@zeritti zeritti force-pushed the feat/prometheus-subcharts-bump branch 2 times, most recently from 4a573e9 to 9a74bc8 Compare May 26, 2023 12:43
@zeritti zeritti marked this pull request as draft May 26, 2023 12:47
@arukiidou

This comment was marked as resolved.

Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

Having a second look, major version update is ok, as dependency had major version update too.

The infos about this (containing lik to depnedency update notes) should be added to the upgrading section though.

@zeritti zeritti force-pushed the feat/prometheus-subcharts-bump branch from 9a74bc8 to af0bfaa Compare June 21, 2023 16:55
alertmanager              0.33.1
kube-state-metrics        4.32.0
prometheus-node-exporter  4.18.0
prometheus-pushgateway    2.3.0

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
@zeritti zeritti force-pushed the feat/prometheus-subcharts-bump branch from af0bfaa to 46362be Compare June 21, 2023 21:51
Signed-off-by: André Bauer <monotek@users.noreply.github.com>
version: 4.30.0
version: 4.32.0
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, latest k-s-m is now 5.8.1

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
@zeritti zeritti marked this pull request as ready for review June 26, 2023 08:38
@zeritti zeritti requested review from zanhsieh and monotek June 26, 2023 08:39
zanhsieh
zanhsieh previously approved these changes Jun 26, 2023
Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

what @nicholascapo says.
update ksm to latest version please and raise chart version to 23.0.0

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
@zeritti
Copy link
Member Author

zeritti commented Jun 27, 2023

what @nicholascapo says. update ksm to latest version please and raise chart version to 23.0.0

Changed, upgrade notes added

@arukiidou
Copy link
Contributor

what @nicholascapo says. update ksm to latest version please and raise chart version to 23.0.0

Changed, upgrade notes added

Thank you! Sorry to take so your time.

zanhsieh and others added 2 commits June 28, 2023 23:00
Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
@zeritti
Copy link
Member Author

zeritti commented Jul 2, 2023

@monotek @zanhsieh I notice opposite views regarding upgrade notes on minor changes (b3dd332 vs 54f650f). Shall we agree on handling these?

@monotek
Copy link
Member

monotek commented Jul 3, 2023

@monotek @zanhsieh I notice opposite views regarding upgrade notes on minor changes (b3dd332 vs 54f650f). Shall we agree on handling these?

Please don't add minor update changes in the upgrading instructions.
We only add major version changes to the readmes in this repo.

It makes no sense as there is no action item for the user.
If there would be one it's a breaking / major change anyway.

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
@zanhsieh zanhsieh merged commit 644e23d into prometheus-community:main Jul 6, 2023
4 checks passed
@zeritti zeritti deleted the feat/prometheus-subcharts-bump branch July 11, 2023 06:36
Matiasmct pushed a commit to giffgaff/prometheus-charts that referenced this pull request Aug 25, 2023
* Bump chart dependencies

alertmanager              0.33.1
kube-state-metrics        4.32.0
prometheus-node-exporter  4.18.0
prometheus-pushgateway    2.3.0

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>

* remove minor version changes 

Signed-off-by: André Bauer <monotek@users.noreply.github.com>

* Update dependencies with node exporter chart patch

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>

* Include the latest KSM release

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>

* Clear trailing whitespaces

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>

* Update README file

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>

---------

Signed-off-by: zeritti <47476160+zeritti@users.noreply.github.com>
Signed-off-by: André Bauer <monotek@users.noreply.github.com>
Co-authored-by: André Bauer <monotek@users.noreply.github.com>
Co-authored-by: MH <zanhsieh@gmail.com>
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.

5 participants