-
Notifications
You must be signed in to change notification settings - Fork 101
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
Expose etcdbr_snapshot_required metric #193
Expose etcdbr_snapshot_required metric #193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be handling of metrics at https://github.com/gardener/etcd-backup-restore/blob/master/pkg/snapshot/snapshotter/snapshotter.go#L354 ?
@swapnilgm That check is only to see if a delta snapshot is required, so it doesn't check whether a full snapshot is required there (atleast at the moment). And again, this code path is taken only when we decide to start with delta snapshot, but not for the case where we start with full snapshot. |
fe51e75
to
8eb44cb
Compare
@swapnilgm Thanks for your comments. I've updated the PR. PTAL. |
How about post restoration, if triggered? Shouldn't the metric be reset appropriately? |
8eb44cb
to
0dc6124
Compare
@PadmaB Once restoration is complete, the snapshotter will start again, and we set the appropriate metric values there. So there's no need to explicitly reset the metrics after restoration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Couple of more minor changes.
Signed-off-by: Shreyas Rao <[email protected]>
0dc6124
to
c94dece
Compare
@swapnilgm Thanks for your review. I've addressed your comments. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for quick resolve. I have suggested some more changes please address those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for answers
Signed-off-by: Shreyas Rao [email protected]
What this PR does / why we need it:
This PR exposes new metric
etcdbr_snapshot_required
that exposes a Gauge metric to indicate whether a snapshot is required to be taken, based on whether a new etcd revision is observed.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Also contains minor linting corrections.
Release note: