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

Use common storage config in jsonnet #3257

Merged
merged 15 commits into from Oct 28, 2022
Merged

Use common storage config in jsonnet #3257

merged 15 commits into from Oct 28, 2022

Conversation

colega
Copy link
Contributor

@colega colega commented Oct 19, 2022 •
edited

What this PR does

Switches jsonnet definitions to use common storage config.
Makes compare-helm-with-jsonnet ignore the common.storage. configuration differences for now, as we'll follow up with changes for Helm in future PRs.

Which issue(s) this PR fixes or relates to

Follow up on #2319

Checklist

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

@colega colega force-pushed the use-common-config-in-jsonnet branch 5 times, most recently from ea03090 to 45db1df Compare October 19, 2022 16:52
@colega colega changed the title WIP: Use common storage config in jsonnet Use common storage config in jsonnet Oct 19, 2022
@colega colega requested a review from pracucci October 19, 2022 16:54
@colega colega marked this pull request as ready for review October 19, 2022 16:54
@colega colega requested review from osg-grafana and a team as code owners October 19, 2022 16:54
@colega colega force-pushed the use-common-config-in-jsonnet branch from 45db1df to 2463e20 Compare October 19, 2022 16:55
@colega
Copy link
Contributor Author

colega commented Oct 19, 2022

Hopefully last force-push. Should be ready for review now.

pracucci
pracucci approved these changes Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Massive work, LGTM! 👏

@colega
Copy link
Contributor Author

colega commented Oct 20, 2022

Thank you for your review, @pracucci.
I'll wait for a review from @osg-grafana and anyway this shouldn't be merged until next week.

@colega colega force-pushed the use-common-config-in-jsonnet branch from 654f066 to 2edb19b Compare October 21, 2022 10:57
osg-grafana
osg-grafana reviewed Oct 21, 2022
View reviewed changes
CHANGELOG.md
@@ -29,6 +29,23 @@

### Jsonnet

* [CHANGE] [Common storage configuration](https://grafana.com/docs/mimir/v2.3.x/operators-guide/configure/configure-object-storage-backend/#common-configuration) is now used to configure object storage in all components. This is a breaking change in terms of Jsonnet manifests and also a CLI flag update for components that use object storage, so it will require a rollout of those components. The changes include: #3257
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [CHANGE] [Common storage configuration](https://grafana.com/docs/mimir/v2.3.x/operators-guide/configure/configure-object-storage-backend/#common-configuration) is now used to configure object storage in all components. This is a breaking change in terms of Jsonnet manifests and also a CLI flag update for components that use object storage, so it will require a rollout of those components. The changes include: #3257
* [BREAKING CHANGE] [Common storage configuration](https://grafana.com/docs/mimir/v2.3.x/operators-guide/configure/configure-object-storage-backend/#common-configuration) is now used to configure object storage in all components. This is a breaking change in terms of Jsonnet manifests and also a CLI flag update for components that use object storage, so it will require a rollout of those components. The changes include: #3257

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'm not sure that it's an option in our changelog. I think that all changes are assumed at least breakish, otherwise they're not considered changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to ignore the feedback if it does not apply. I appreciate the note though. :)

osg-grafana
osg-grafana approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

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

Unblocking with nits

@osg-grafana osg-grafana added the type/docs Improvements or additions to documentation label Oct 21, 2022
@colega colega mentioned this pull request Oct 24, 2022
Merged
3 tasks
@colega
Copy link
Contributor Author

colega commented Oct 24, 2022

(Didn't I comment on this? Apparently I forgot to).

@pracucci, @osg-grafana I've added two extra commits after your review, as I felt this change wasn't complete enough.
Thise are:

Can I ask you please to review them too? Thank you.

@colega colega requested a review from pracucci October 26, 2022 08:47
pracucci
pracucci approved these changes Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Amazing work, LGTM! (modulo a nit)

docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/configuring-object-storage-backend.md Outdated

## Azure (`azure`) storage configuration options

Azure storage client requires the `storagE_azurE_account_name` and `storage_azure_account_key` to be configured.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Azure storage client requires the `storagE_azurE_account_name` and `storage_azure_account_key` to be configured.
Azure storage client requires the `storage_azure_account_name` and `storage_azure_account_key` to be configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh God, how did this happen? How didn't I notice it?. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here as file was renamed: 538ad46

osg-grafana
osg-grafana approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

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

Great stuff, @colega. :)

@colega
Copy link
Contributor Author

colega commented Oct 26, 2022

I will merge this on Friday.

colega and others added 15 commits October 28, 2022 09:00
@colega
Use common config in jsonnet
b7a02ef 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Update jsonnet test definitions (but not outputs)
5feb2a5 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Commit results of make check-jsonnet-tests
0dabd8a 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Mark common.storage.backend as irrelevant
3c0396b 
We know helm doesn't set the common storage config yet, so let's mark it
as irrelevant meanwhile.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Update documentation
5013711 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Update CHANGELOG.md
c6c7116 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Don't forget the alertmanager storage config
11f6efe 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Commit jsonnet tests with missing alertmanager cfg
97480aa 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega @osg-grafana
Apply suggestions from code review
a72f13c 
Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
@colega @osg-grafana
Update docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/conf…
a42b48b 
…iguring-object-storage-backend.md

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
@colega @osg-grafana
Update docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/conf…
061954a 
…iguring-object-storage-backend.md

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
@colega
Allow credentials configuration for S3/GCS
91a9072 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega
Improve docs with better examples
b2ae9e5 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@osg-grafana @colega
Rename configuring-object-storage-backend.md to configure-object-stor…
5e6433c 
…age-backend.md (#3308)
@colega
Fix weird case
97511e0 
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega force-pushed the use-common-config-in-jsonnet branch from 538ad46 to 97511e0 Compare October 28, 2022 07:01
@colega colega merged commit 3508a0b into main Oct 28, 2022
@colega colega deleted the use-common-config-in-jsonnet branch October 28, 2022 11:15
masonmei pushed a commit to udmire/mimir that referenced this pull request Nov 4, 2022
@colega @osg-grafana
Use common storage config in jsonnet (grafana#3257)
d4d1aef 
* Use common config in jsonnet

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update jsonnet test definitions (but not outputs)

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Commit results of make check-jsonnet-tests

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Mark common.storage.backend as irrelevant

We know helm doesn't set the common storage config yet, so let's mark it
as irrelevant meanwhile.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update documentation

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update CHANGELOG.md

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Don't forget the alertmanager storage config

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Commit jsonnet tests with missing alertmanager cfg

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Apply suggestions from code review

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

* Update docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/configuring-object-storage-backend.md

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

* Update docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/configuring-object-storage-backend.md

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

* Allow credentials configuration for S3/GCS

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Improve docs with better examples

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Rename configuring-object-storage-backend.md to configure-object-storage-backend.md (grafana#3308)

* Fix weird case

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
masonmei pushed a commit to udmire/mimir that referenced this pull request Nov 4, 2022
@colega @osg-grafana
Use common storage config in jsonnet (grafana#3257)
83cc1e3 
* Use common config in jsonnet

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update jsonnet test definitions (but not outputs)

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Commit results of make check-jsonnet-tests

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Mark common.storage.backend as irrelevant

We know helm doesn't set the common storage config yet, so let's mark it
as irrelevant meanwhile.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update documentation

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update CHANGELOG.md

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Don't forget the alertmanager storage config

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Commit jsonnet tests with missing alertmanager cfg

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Apply suggestions from code review

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

* Update docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/configuring-object-storage-backend.md

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

* Update docs/sources/operators-guide/deploy-grafana-mimir/jsonnet/configuring-object-storage-backend.md

Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>

* Allow credentials configuration for S3/GCS

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Improve docs with better examples

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Rename configuring-object-storage-backend.md to configure-object-storage-backend.md (grafana#3308)

* Fix weird case

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci approved these changes

@osg-grafana osg-grafana osg-grafana approved these changes

Assignees
No one assigned
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@colega @pracucci @osg-grafana