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

peering: better represent non-passing states during peer check flattening #15615

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Nov 30, 2022

Description

During peer stream replication we flatten checks from the source cluster and build one thin overall check to hide the irrelevant details from the consuming cluster. This flattening logic did correctly flip to non-passing if there were any non-passing checks, but WHICH status it got during that was random (warn/error).

Also it didn't represent "maintenance" operations. There is an api package call AggregatedStatus which more correctly flattened check statuses.

This PR replicated the more complete logic into the peer stream package.

@rboyer rboyer added the theme/cluster-peering Related to Consul's cluster peering feature label Nov 30, 2022
@rboyer rboyer requested review from a team November 30, 2022 16:45
@rboyer rboyer self-assigned this Nov 30, 2022
@rboyer rboyer requested a review from hashi-derek November 30, 2022 16:46
api.HealthPassing: 4,
}

func isStatusBetter(curr, next string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Nit. To simplify mental-effort, I would have this function return the winning string instead of having it return a bool. Also, the name "better" makes me think that it's returning the healthier status, which is a little confusing. I'd probably name it getMostImportantStatus() or something similar.

@rboyer rboyer merged commit 11a277f into main Nov 30, 2022
@rboyer rboyer deleted the fix-peer-check-flattening branch November 30, 2022 17:29
jmurret pushed a commit that referenced this pull request Dec 2, 2022
…ning (#15615)

During peer stream replication we flatten checks from the source cluster and build one thin overall check to hide the irrelevant details from the consuming cluster. This flattening logic did correctly flip to non-passing if there were any non-passing checks, but WHICH status it got during that was random (warn/error).

Also it didn't represent "maintenance" operations. There is an api package call AggregatedStatus which more correctly flattened check statuses.

This PR replicated the more complete logic into the peer stream package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/cluster-peering Related to Consul's cluster peering feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants