Skip to content

Add admin page for HA tracker information. #1546

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
Merged
merged 11 commits into from
Aug 14, 2019
Merged

Add admin page for HA tracker information. #1546

merged 11 commits into from
Aug 14, 2019

Conversation

cstyan
Copy link
Contributor

@cstyan cstyan commented Jul 31, 2019

It wasn't clear to me what the right way to serve the page was, and whether for other pages (ex: the ring pages) if we serve the page from every distributor/ingester or not.

Signed-off-by: Callum Styan [email protected]

@cstyan
Copy link
Contributor Author

cstyan commented Jul 31, 2019

cc @tomwilkie @gouthamve @jtlisi

tomwilkie
tomwilkie reviewed Aug 6, 2019
View reviewed changes
tomwilkie
tomwilkie reviewed Aug 6, 2019
View reviewed changes
tomwilkie
tomwilkie reviewed Aug 6, 2019
View reviewed changes
tomwilkie
tomwilkie reviewed Aug 6, 2019
View reviewed changes
@tomwilkie
Copy link
Contributor

Looks really good - couple minor nits, but hopefully merge tomorrow.

@cstyan cstyan marked this pull request as ready for review August 7, 2019 03:08
tomwilkie
tomwilkie reviewed Aug 7, 2019
View reviewed changes
tomwilkie
tomwilkie reviewed Aug 7, 2019
View reviewed changes
tomwilkie
tomwilkie reviewed Aug 7, 2019
View reviewed changes
@tomwilkie
Copy link
Contributor

Couple more nits.

@cstyan
Copy link
Contributor Author

cstyan commented Aug 7, 2019

@tomwilkie PTAL. In the most recent commit I've reverted the removal of/moved the EnableHATracker cfg flag to the HA tracker cfg from the Distributor cfg, added a prefix flag to the HA tracker flag registration (so we don't have to manually prefix each of it's flags with distributor.), and moved any checks for if EnableHATracker from distributor.go to ha_tracker.go.

This means the HA Tracker struct is always created, but it only creates the client to it's KV store if EnableHATracker == true. Also if there's a misconfiguration of EnableHATracker == false but acceptHASamples == true for a user ID, we'll still accept samples that contain both HA tracking labels (cluster/replica), the HA tracker checkReplica will tell the distributor to accept any sample if EnableHATracker == false.

tomwilkie
tomwilkie reviewed Aug 7, 2019
View reviewed changes
@cstyan
Copy link
Contributor Author

cstyan commented Aug 8, 2019

test failures look like a flake?

@bboreham
Copy link
Contributor

bboreham commented Aug 8, 2019

TestChunkStoreRandom failures noted at #1522

It could be a real problem, but not one introduced by this PR.

cstyan
cstyan commented Aug 8, 2019
View reviewed changes
@tomwilkie tomwilkie requested a review from gouthamve August 12, 2019 15:43
gouthamve
gouthamve approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM with the issue.

Also, please update the changelog.

cstyan and others added 11 commits August 14, 2019 10:28
@cstyan @tomwilkie
Add admin page for HA tracker information.
615a547 
Signed-off-by: Callum Styan <[email protected]>
@cstyan @tomwilkie
Turn on HA tracker by default, remove EnableHATracker config. Accepting
640c5b0 
HA samples is still false by default for a User ID.

Signed-off-by: Callum Styan <[email protected]>
@cstyan @tomwilkie
Add Update and Failover times to the HA tracker HTTP page.
7845c2c 
Signed-off-by: Callum Styan <[email protected]>
@cstyan @tomwilkie
Prefix HA tracker flags, check HA enable only in HA code, and only
3b61a30 
create the client to the HA trackers KV store if HA tracking is enabled.

Signed-off-by: Callum Styan <[email protected]>
@cstyan @tomwilkie
Prefix HA tracker flags, check HA enable only in HA code, and only
6a66e47 
create the client to the HA trackers KV store if HA tracking is enabled.

Signed-off-by: Callum Styan <[email protected]>
@cstyan @tomwilkie
Move creation of HA tracker to before creation of distributor struct in
1702bb9 
distributor constructor.

Signed-off-by: Callum Styan <[email protected]>
@cstyan @tomwilkie
Sort results for HA tracker admin page.
468bb3f 
Signed-off-by: Callum Styan <[email protected]>
@tomwilkie
Nits.
1855a1f 
- Init replicas in the struct.
- Remove old comment.
- More intuitive sort.

Signed-off-by: Tom Wilkie <[email protected]>
@tomwilkie
Add changelog entry.
3b280e1 
Signed-off-by: Tom Wilkie <[email protected]>
@tomwilkie
Unlock immediately after use.
f55e509 
Signed-off-by: Tom Wilkie <[email protected]>
@tomwilkie
Fix rebase.
c87a062 
Signed-off-by: Tom Wilkie <[email protected]>
@tomwilkie tomwilkie merged commit 1c57ecb into cortexproject:master Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomwilkie tomwilkie tomwilkie approved these changes

@gouthamve gouthamve gouthamve approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cstyan @tomwilkie @bboreham @gouthamve