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

Add sanity check at startup to ensure the configured filesystem directories don't overlap for different components #2828

Merged
merged 5 commits into from
Aug 26, 2022

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Aug 24, 2022

What this PR does

While answering the question #2698 (reply in thread) I've realized that if you set overlapping local directories for different purposes you could end up with weird behaviour (e.g. alertmanager deleting the alerts store content). These errors are quite difficult to detect, so in this PR I'm proposing to introduce a sanity check to ensure the configured filesystem directories don't overlap for different components (checking only the running components).

Thought: the logic encoded in this new check is non trivial, but that's one more reason why we should encode it instead of letting our users figuring out through a try-and-error process.

Which issue(s) this PR fixes or relates to

N/A

Checklist

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

@pracucci pracucci force-pushed the add-local-paths-sanity-check branch from aa10858 to 7f3d94e Compare August 24, 2022 13:39
@pracucci pracucci marked this pull request as draft August 24, 2022 14:04
@pracucci
Copy link
Collaborator Author

I need to improve the checks on the filesystem bucket, because ruler and alertmanager use a different prefix so they don't conflict (and blocks storage could have an optional prefix too).

@pracucci pracucci force-pushed the add-local-paths-sanity-check branch from 7f3d94e to 484f90d Compare August 25, 2022 06:53
@pracucci pracucci marked this pull request as ready for review August 25, 2022 08:16
@colega colega self-requested a review August 25, 2022 13:56
Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

I've got just one small nitpick: now we have two completely different paths to validate the bucket config:

  • When config is just parsed, we call .Validate() and it checks bucket configs: that is basically ensuring that if two buckets are the same, they should have different prefixes.
  • Then your new logic is triggered as part of the SanityCheck module.

Can we move the former call to validateBucketConfigs somewhere near the call to checkFilesystemPathsOvelapping? Maybe checkFilesystemPathsOvelapping should be called from validateBucketConfigs?

Otherwise we have two completely paths for validating two aspects of the same config.

pkg/mimir/sanity_check.go Outdated Show resolved Hide resolved
pracucci and others added 4 commits August 26, 2022 10:02
…tories don't overlap for different components

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com>
@pracucci pracucci force-pushed the add-local-paths-sanity-check branch from 720ef95 to 7fe2550 Compare August 26, 2022 08:02
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci
Copy link
Collaborator Author

Can we move the former call to validateBucketConfigs somewhere near the call to checkFilesystemPathsOvelapping? Maybe checkFilesystemPathsOvelapping should be called from validateBucketConfigs?

This is a very good feedback. This new check doesn't need to be a sanity check (it doesn't run anything against the real setup) but it's just a static config check. I moved it to be a function of the Config, like validateBucketConfigs().

Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@pracucci pracucci merged commit 6fef60a into main Aug 26, 2022
@pracucci pracucci deleted the add-local-paths-sanity-check branch August 26, 2022 09:02
@pstibrany pstibrany mentioned this pull request Sep 21, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants