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

mimirtool: retain old default of blocks_storage.backend #1626

Merged
merged 10 commits into from
Apr 22, 2022

Conversation

dimitarvdimitrov
Copy link
Contributor

What this PR does

When passing --update-defaults to mimirtool config convert and the
value of blocks_storage.backend is using the old default of "s3", the
value in the resulting config is updated to "filesystem." It is better
if the value is not updated.

Which issue(s) this PR fixes or relates to

Fixes #1622

Checklist

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

When passing `--update-defaults` to mimirtool config convert and the
value of blocks_storage.backend is using the old default of "s3", the
value in the resulting config is updated to "filesystem." It is better
if the value is not updated.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

What about other storages? (Alertmanager, ruler) I would expect the update path to be the same for all of them (if alertmanager / ruler were used in the first place).

@pstibrany
Copy link
Member

I think this now deserves a CHANGELOG entry too.

When passing `--update-defaults` to mimirtool config convert and the
value of *_storage.backend is using the old default of "s3", the
value in the resulting config is updated to "filesystem." It is better
if the value is not updated.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov added the type/docs Improvements or additions to documentation label Apr 5, 2022
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov
Copy link
Contributor Author

nice call @pstibrany. I've added (ruler|alertmanager)_storage.backend to the list of retained old defaults in 740878d and added a changelog entry in fbbc355

I also added some docs to mention the special behaviour for these defaults in a075db0. Also added type/docs label to get some advice from Docs squad on that paragraph.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Looks good to me. There is no good / easy way checking if alertmanager and ruler were used before (in Cortex) :(

docs/sources/operators-guide/tools/mimirtool.md Outdated Show resolved Hide resolved
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
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.

I'm not much familiar with the config conversion tool, but to my understanding the changes make sense.

@dimitarvdimitrov
Copy link
Contributor Author

dimitarvdimitrov commented Apr 14, 2022

thank you for reviewing. For visibility - I'm waiting for the docs squad to get to this PR so they can review the added docs to mimirtool.

There is other work related to this that I'd like to do - some filesystem paths (like /data/compactor changed default values, and we can employ a similar approach as in this PR to provide a smoother migration.

I want these to be included in the 2.1 release, so if I don't get a docs review in the next week. We can extract the docs in a separate PR and move on with the code 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 feedback, which I would like in part to be technically vetted for accuracy.

docs/sources/operators-guide/tools/mimirtool.md Outdated Show resolved Hide resolved
@@ -655,6 +655,22 @@ It supports converting both CLI flags and [YAML configuration files]({{< relref
| `-v`, `--verbose` | If you set this flag, the CLI flags and YAML paths from the old configuration that do not exist in the new configuration are printed to `stderr`. This flag also prints default values that have changed between the old and the new configuration. |
| `--gem` | If you set this flag, the tool will convert from Grafana Metrics Enterprise (GEM) v1.7.x to v2.0.0. |

##### A note on default values

This command is designed to aid a migration from Cortex to Grafana Mimir. There are changes to the default values of
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
This command is designed to aid a migration from Cortex to Grafana Mimir. There are changes to the default values of
The `mimirtool` command helps you migrate from Cortex to Grafana Mimir. There are changes to the default values of

I assume we are talking about the mimirtool here.

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 am talking about the mimirtool config convert command. Which is part of the mimirtool tool. I pushed a change in cecbdd2 to follow your advice

docs/sources/operators-guide/tools/mimirtool.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/tools/mimirtool.md Outdated Show resolved Hide resolved
Comment on lines 670 to 672
This command will output the Cortex default value even when the configuration parameter is not explicitly set in the input
configuration. If the Cortex default value is explicitly set in the input configuration, and you have provided
the `--update-defaults` flag, the command will not update the value to the Mimir default.
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
This command will output the Cortex default value even when the configuration parameter is not explicitly set in the input
configuration. If the Cortex default value is explicitly set in the input configuration, and you have provided
the `--update-defaults` flag, the command will not update the value to the Mimir default.
The `mimirtool` command outputs the Cortex default value even when the configuration parameter is not explicitly set in the input
configuration. If in the input configuration you explicitly set the Cortex default value, and you have provide
the `--update-defaults` flag, the `mimirtool` command will not update the value to the Mimir default.

Please vet for technical accuracy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed in cecbdd2, can you take another look?

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.

See previous comment. :)

dimitarvdimitrov and others added 2 commits April 21, 2022 14:31
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
@pracucci pracucci merged commit 6f5563f into main Apr 22, 2022
@pracucci pracucci deleted the dimitar/mimirtool-fix-blocks-storage-backend branch April 22, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component/mimirtool type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mimirtool: Failed to pick the correct backend when running convert
4 participants