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

Extend alertstore to support read/write of clusterpb.FullState. #4020

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

stevesg
Copy link
Contributor

@stevesg stevesg commented Mar 26, 2021

What this PR does:
Extend alertstore to support read/write of clusterpb.FullState.

Required to support the work for ring-based/sharding replication.

Due to the underlying clusterpb.FullState type being an upstream type
defined in alertmanager, it is wrapped in a separate message type
(alertspb.FullStateDesc). This can be used to cope with backwards
incompatible changes to the upstream data format, if ever necessary.

Assuming we want to store the state alongside the existing state for the
alertmanager, then it makes sense to extend the alertstore. This avoids
having to plumb through a different storage widget or configuration.

The slight complication is that the existing store uses an object path
of <prefix>/<user>, so we have to use a different prefix in order to
use the desired object path of <prefix>/<user>/<object>.

Checklist

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

Required to support the work for ring-based/sharding replication.

Due to the underlying `clusterpb.FullState` type being an upstream type
defined in alertmanager, it is wrapped in a separate message type
(`alertspb.FullStateDesc`). This can be used to cope with backwards
incompatible changes to the upstream data format, if ever necessary.

Assuming we want to store the state alongside the existing state for the
alertmanager, then it makes sense to extend the alertstore. This avoids
having to plumb through a different storage widget or configuration.

The slight complication is that the existing store uses an object path
of `<prefix>/<user>`, so we have to use a different prefix in order to
use the desired object path of `<prefix>/<user>/<object>`.

Signed-off-by: Steve Simpson <[email protected]>
Copy link
Contributor

@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.

Thanks for working on this preliminary PR for the alertmanager state persistency. LGTM (modulo a couple of nits)!

pkg/alertmanager/alertstore/store.go Show resolved Hide resolved
pkg/alertmanager/alertstore/objectclient/store.go Outdated Show resolved Hide resolved
pkg/alertmanager/alertstore/local/store.go Outdated Show resolved Hide resolved
pkg/alertmanager/alertstore/configdb/store.go Outdated Show resolved Hide resolved
pkg/alertmanager/alertstore/bucketclient/bucket_client.go Outdated Show resolved Hide resolved
Signed-off-by: Steve Simpson <[email protected]>
Copy link
Contributor

@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.

LGTM, thanks.

@pracucci pracucci merged commit 8924550 into cortexproject:master Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants