DISTMYSQL-208: Orchestrator GUI incorrectly shows recovery option for

intermediate database in chained replication

https://jira.percona.com/browse/DISTMYSQL-208

Problem:
If we've got replication chain A->B->C, and C is down, GUI shows
'Recover' dropdown for node B, but there is no possible recovery action
available in such a case.
The problem is well visible on upstream and Percona Orchestrator up to
version v3.2.6-3. For next versions it is still there, but is hard to
reproduce.

Cause:
The cause for hard reproduction on versions > v3.2.6-6 is caused by fix
for DISTMYSQL-182. In case of the instance being unreachable, it causes
skip of instance.Key storing, which is later needed for Orchestrator's
backend update. This caused that 'last_checked' and 'last_seen'
timestamps were updated incorrectly.

The root cause of the main problem is the analysis logic in
Analysis_dao.go:GetReplicationAnalysis(). The condition for setting
AllIntermediateMasterReplicasNotReplicating does not check if there are
any replicas reachable. So the case when all replicas are dead
(no recovery action possible) and the case when some replicas are still
reachable, but are not replicating (recovery action possible) are
undistingushable.

Solution:
1. Move instance.Key assignment before Ping
2. Improve the analysis logic. Report
AllIntermediateMasterReplicasNotReplicating only if all replicas are not
replicating, but there are still some reachable replicas.

Author: kamil-holubicki

go/inst/analysis_dao.go

@@ -608,9 +608,9 @@ func GetReplicationAnalysis(clusterName string, hints *ReplicationAnalysisHints)
 				a.Analysis = AllIntermediateMasterReplicasFailingToConnectOrDead
 				a.Description = "Intermediate master is reachable but all of its replicas are failing to connect"
 				//
-			} else if !a.IsMaster && a.LastCheckValid && a.CountReplicas > 0 && a.CountValidReplicatingReplicas == 0 {
+			} else if !a.IsMaster && a.LastCheckValid && a.CountReplicas > 0 && a.CountValidReplicas > 0 && a.CountValidReplicatingReplicas == 0 {
 				a.Analysis = AllIntermediateMasterReplicasNotReplicating
-				a.Description = "Intermediate master is reachable but none of its replicas is replicating"
+				a.Description = "Intermediate master is reachable but none of its reachable replicas is replicating"
 				//
 			} else if a.IsBinlogServer && a.IsFailingToConnectToMaster {
 				a.Analysis = BinlogServerFailingToConnectToMaster

go/inst/instance_dao.go

@@ -384,14 +384,17 @@ func ReadTopologyInstanceBufferable(instanceKey *InstanceKey, bufferWrites bool,
 		latency.Stop("instance")
 		goto Cleanup
 	}
+
+	// Even if the instance is dead, we need its key below to update
+	// the backend database's timestamps
+	instance.Key = *instanceKey
+
 	err = db.Ping()
 	if err != nil {
 		goto Cleanup
 	}
 	latency.Stop("instance")
 
-	instance.Key = *instanceKey
-
 	if isMaxScale, resolvedHostname, err = instance.checkMaxScale(db, latency); err != nil {
 		// We do not "goto Cleanup" here, although it should be the correct flow.
 		// Reason is 5.7's new security feature that requires GRANTs on performance_schema.session_variables.

tests/system/intermediate-master-replica-failure/01-relocate/expect_output

@@ -0,0 +1,6 @@
+127.0.0.1:10113<127.0.0.1:10112
+127.0.0.1:10114<127.0.0.1:10113
+127.0.0.1:10111      |0s|ok
++ 127.0.0.1:10112    |0s|ok
+  + 127.0.0.1:10113  |0s|ok
+    + 127.0.0.1:10114|0s|ok

tests/system/intermediate-master-replica-failure/01-relocate/run

@@ -0,0 +1,5 @@
+# 10111 -> 10112 -> 10113 -> 10114
+orchestrator-client -c relocate -i 127.0.0.1:10113 -d 127.0.0.1:10112
+orchestrator-client -c relocate -i 127.0.0.1:10114 -d 127.0.0.1:10113
+
+orchestrator-client -c topology-tabulated -alias ci | cut -d'|' -f 1,2,3

tests/system/intermediate-master-replica-failure/02-down-slave-3/expect_output

@@ -0,0 +1 @@
+orchestrator-client -c replication-analysis

tests/system/intermediate-master-replica-failure/02-down-slave-3/run

@@ -0,0 +1,6 @@
+#!/bin/bash
+
+set -e
+
+mysqladmin -uci -pci -h 127.0.0.1 --port=10114 shutdown
+sleep 20

tests/system/intermediate-master-replica-failure/02-down-slave-3/setup

@@ -193,7 +193,7 @@ test_single() {
   fi
 
   # test steps:
-  find "$tests_path/$test_name" ! -path . -type d -mindepth 1 -maxdepth 1 | sort | cut -d "/" -f 5 | while read test_step_name ; do
+  find "$tests_path/$test_name" -mindepth 1 -maxdepth 1 ! -path . -type d | sort | cut -d "/" -f 5 | while read test_step_name ; do
     [ "$test_step_name" == "." ] && continue
     test_step "$tests_path/$test_name/$test_step_name" "$test_name" "$test_step_name"
     if [ $? -ne 0 ] ; then
@@ -307,7 +307,7 @@ test_all() {
   while [ -s $tests_todo_file ] ; do
     echo -n > $tests_todo_file
 
-    find $tests_path ! -path . -type d -mindepth 1 -maxdepth 1 | xargs ls -td1 | cut -d "/" -f 4 | egrep "$test_pattern" | while read test_name ; do
+    find $tests_path -mindepth 1 -maxdepth 1 ! -path . -type d | xargs ls -td1 | cut -d "/" -f 4 | egrep "$test_pattern" | while read test_name ; do
       if ! test_listed_as_attempted "$test_name" ; then
         echo "$test_name" >> $tests_todo_file
       fi
@@ -329,7 +329,7 @@ test_all() {
       fi
     done || return 1
   done
-  find $tests_path ! -path . -type d -mindepth 1 -maxdepth 1 | xargs ls -td1 | cut -d "/" -f 4 | egrep "$test_pattern" | while read test_name ; do
+  find $tests_path -mindepth 1 -maxdepth 1 ! -path . -type d | xargs ls -td1 | cut -d "/" -f 4 | egrep "$test_pattern" | while read test_name ; do
     if ! test_listed_as_attempted "$test_name" ; then
       echo "# ERROR: tests completed by $test_name seems to have been skipped"
       exit 1