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.

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

Add structure warning to replication-analysis when all masters are read_only #878

Merged
merged 22 commits into from
Jun 20, 2019
Merged

Add structure warning to replication-analysis when all masters are read_only #878

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f2f7402
Add structure warning to `replication-analysis` when all nodes in the…
May 8, 2019
0848d25
format
May 8, 2019
bd7d8e8
Update analysis_dao.go
May 8, 2019
2739486
Only warn on RO master
May 8, 2019
48ed4eb
merge
May 8, 2019
eed30fc
fix indentation
May 8, 2019
895a011
Ensure intermediate master is not flagged for R/O
May 10, 2019
b3ccd73
Only warn if all co-masters are R/O
May 10, 2019
4a5b67f
merge conflict
May 10, 2019
1977891
Delete init_docker.sh
May 10, 2019
65d83f9
Fix indentations
May 10, 2019
ff13e0b
Fix small issue with counts
May 10, 2019
fa8c923
go fmt
May 10, 2019
a1b3080
Remove extra script
May 10, 2019
f0078eb
merge conflict
May 10, 2019
eb75598
formatting again
May 10, 2019
e45511a
Simplify logic
Jun 3, 2019
a2bbdce
Merge branch 'master' into jfudally-ro-structure-warning
Jun 3, 2019
7943eac
Fix indentation
Jun 5, 2019
42f0747
UI fix
Jun 10, 2019
940cc59
Merge branch 'jfudally-ro-structure-warning' of https://github.com/gi…
Jun 10, 2019
b7e0df8
Auto-merged master into jfudally-ro-structure-warning on deployment
Jun 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions go/inst/analysis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const (
DifferentGTIDModesStructureWarning = "DifferentGTIDModesStructureWarning"
ErrantGTIDStructureWarning = "ErrantGTIDStructureWarning"
NoFailoverSupportStructureWarning = "NoFailoverSupportStructureWarning"
NoWriteableMasterStructureWarning = "NoWriteableMasterStructureWarning"
)

type InstanceAnalysis struct {
Expand Down
25 changes: 21 additions & 4 deletions go/inst/analysis_dao.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func GetReplicationAnalysis(clusterName string, hints *ReplicationAnalysisHints)

args := sqlutils.Args(ValidSecondsFromSeenToLastAttemptedCheck(), config.Config.ReasonableReplicationLagSeconds, clusterName)
analysisQueryReductionClause := ``

if config.Config.ReduceReplicationAnalysisCount {
analysisQueryReductionClause = `
HAVING
Expand Down Expand Up @@ -90,15 +91,17 @@ func GetReplicationAnalysis(clusterName string, hints *ReplicationAnalysisHints)
SELECT
master_instance.hostname,
master_instance.port,
MIN(master_instance.data_center) AS data_center,
MIN(master_instance.physical_environment) AS physical_environment,
master_instance.read_only AS read_only,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

SUM(master_instance.is_co_master) AS co_master_count,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will be either 0 or 1, not what you'd expect. See later discussion.

MIN(master_instance.data_center) AS data_center,
MIN(master_instance.physical_environment) AS physical_environment,
MIN(master_instance.master_host) AS master_host,
MIN(master_instance.master_port) AS master_port,
MIN(master_instance.cluster_name) AS cluster_name,
MIN(IFNULL(cluster_alias.alias, master_instance.cluster_name)) AS cluster_alias,
MIN(
master_instance.last_checked <= master_instance.last_seen
and master_instance.last_attempted_check <= master_instance.last_seen + interval ? second
master_instance.last_checked <= master_instance.last_seen
and master_instance.last_attempted_check <= master_instance.last_seen + interval ? second
) = 1 AS is_last_check_valid,
MIN(master_instance.last_check_partial_success) as last_check_partial_success,
MIN(master_instance.master_host IN ('' , '_')
Expand Down Expand Up @@ -241,6 +244,8 @@ func GetReplicationAnalysis(clusterName string, hints *ReplicationAnalysisHints)
is_cluster_master DESC,
count_replicas DESC
`, analysisQueryReductionClause)

coMasterROCount := 0
err := db.QueryOrchestrator(query, args, func(m sqlutils.RowMap) error {
a := ReplicationAnalysis{
Analysis: NoProblem,
Expand Down Expand Up @@ -296,6 +301,10 @@ func GetReplicationAnalysis(clusterName string, hints *ReplicationAnalysisHints)
a.CountDelayedReplicas = m.GetUint("count_delayed_replicas")
a.CountLaggingReplicas = m.GetUint("count_lagging_replicas")

if a.IsCoMaster && m.GetUint("read_only") == 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please read m.GetUint("read_only") into a variable first. In fact, it's really interesting information, make that a member in the ReplicationAnalysis struct

coMasterROCount++
Copy link
Collaborator

Choose a reason for hiding this comment

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

This look can iterate over multiple clusters. You only want to count the number of co masters per cluster. So this trick of coMasterROCount++ won't work.

A cluster with two co-masters appears in orchestrator as a single cluster, not as two clusters. One of the two co-masters is deemed to be the "primary" master. If one is read-only and another is writable, it's the writable. If both are read-only or both are writable, I believe the choice is alphabetic.

So, as I'm writing this, I'm rethinking our earlier discussion. if the master is a co_master and is read_only, then the other co-master is also read_only (or else IT would have been the primary master!). This simplifies things and you can get rid of coMasterROCount.

}

if !a.LastCheckValid {
analysisMessage := fmt.Sprintf("analysis: IsMaster: %+v, LastCheckValid: %+v, LastCheckPartialSuccess: %+v, CountReplicas: %+v, CountValidReplicatingReplicas: %+v, CountLaggingReplicas: %+v, CountDelayedReplicas: %+v, ",
a.IsMaster, a.LastCheckValid, a.LastCheckPartialSuccess, a.CountReplicas, a.CountValidReplicatingReplicas, a.CountLaggingReplicas, a.CountDelayedReplicas,
Expand Down Expand Up @@ -477,6 +486,14 @@ func GetReplicationAnalysis(clusterName string, hints *ReplicationAnalysisHints)
if a.MaxReplicaGTIDErrant != "" {
a.StructureAnalysis = append(a.StructureAnalysis, ErrantGTIDStructureWarning)
}

if a.IsMaster && m.GetUint("read_only") == 1 && m.GetString("master_host") == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

"master_host" is already read in a.AnalyzedInstanceMasterKey. You can ask if a.AnalyzedInstanceMasterKey.IsValid() (== non empty)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given earlier discussion, I think we can skip the check about master_host altogether.

a.StructureAnalysis = append(a.StructureAnalysis, NoWriteableMasterStructureWarning)
}
if a.IsCoMaster && coMasterROCount == m.GetInt("co_master_count") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given earlier discussion, I think we can skip this block altogether.

a.StructureAnalysis = append(a.StructureAnalysis, NoWriteableMasterStructureWarning)
}

}
appendAnalysis(&a)

Expand Down