Merge branch 'pks-repl-logging-inconsistency' into 'master'

coordinator: Reflect primary's dirty-status when logging repl cause

See merge request gitlab-org/gitaly!3607

1 files changed, 16 insertions, 7 deletions

diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index afd59271c..f56cdd347 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io/ioutil"
 	"sync"
 	"time"
 
@@ -783,15 +784,23 @@ func getUpdatedAndOutdatedSecondaries(
 	primaryDirtied = transaction.DidCommitAnySubtransaction() ||
 		(transaction.CountSubtransactions() == 0 && primaryErr == nil)
 
+	// If the primary wasn't dirtied, then we never replicate any changes. While this is
+	// duplicates logic defined elsewhere, it's probably good enough given that we only talk
+	// about metrics here.
 	recordReplication := func(reason string, replicationCount int) {
-		// If the primary wasn't dirtied, then we never replicate any changes. While this is
-		// duplicates logic defined elsewhere, it's probably good enough given that we only
-		// talk about metrics here.
 		if primaryDirtied && replicationCount > 0 {
 			replicationCountMetric.WithLabelValues(reason).Add(float64(replicationCount))
 		}
 	}
 
+	// Same as above, we discard log entries in case the primary wasn't dirtied.
+	logReplication := ctxlogrus.Extract(ctx)
+	if !primaryDirtied {
+		discardLogger := logrus.New()
+		discardLogger.Out = ioutil.Discard
+		logReplication = logrus.NewEntry(discardLogger)
+	}
+
 	// Replication targets were not added to the transaction, most likely because they are
 	// either not healthy or out of date. We thus need to make sure to create replication jobs
 	// for them.
@@ -801,7 +810,7 @@ func getUpdatedAndOutdatedSecondaries(
 	// If the primary errored, then we need to assume that it has modified on-disk state and
 	// thus need to replicate those changes to secondaries.
 	if primaryErr != nil {
-		ctxlogrus.Extract(ctx).WithError(primaryErr).Info("primary failed transaction")
+		logReplication.WithError(primaryErr).Info("primary failed transaction")
 		outdated = append(outdated, routerNodesToStorages(route.Secondaries)...)
 		recordReplication("primary-failed", len(route.Secondaries))
 		return
@@ -812,7 +821,7 @@ func getUpdatedAndOutdatedSecondaries(
 	// no changes were done and the nodes hit an error prior to voting. If the primary processed
 	// the RPC successfully, we assume the RPC is not correctly voting and replicate everywhere.
 	if transaction.CountSubtransactions() == 0 {
-		ctxlogrus.Extract(ctx).Info("transaction did not create subtransactions")
+		logReplication.Info("transaction did not create subtransactions")
 		outdated = append(outdated, routerNodesToStorages(route.Secondaries)...)
 		recordReplication("no-votes", len(route.Secondaries))
 		return
@@ -822,7 +831,7 @@ func getUpdatedAndOutdatedSecondaries(
 	// safe route and just replicate to all secondaries.
 	nodeStates, err := transaction.State()
 	if err != nil {
-		ctxlogrus.Extract(ctx).WithError(err).Error("could not get transaction state")
+		logReplication.WithError(err).Error("could not get transaction state")
 		outdated = append(outdated, routerNodesToStorages(route.Secondaries)...)
 		recordReplication("missing-tx-state", len(route.Secondaries))
 		return
@@ -833,7 +842,7 @@ func getUpdatedAndOutdatedSecondaries(
 	// but it's what we got. So in order to ensure a consistent state, we need to replicate.
 	if state := nodeStates[route.Primary.Storage]; state != transactions.VoteCommitted {
 		if state == transactions.VoteFailed {
-			ctxlogrus.Extract(ctx).Error("transaction: primary failed vote")
+			logReplication.Error("transaction: primary failed vote")
 		}
 		outdated = append(outdated, routerNodesToStorages(route.Secondaries)...)
 		recordReplication("primary-not-committed", len(route.Secondaries))