From 469f3c49be1ae379ab197ce8b478521f2011953e Mon Sep 17 00:00:00 2001
From: Mark Vulfson <mvulfson@netflix.com>
Date: Mon, 6 Apr 2020 19:53:17 -0700
Subject: [PATCH] fix(peering): don't reset last updated timestamp if there is
 nothing to copy (#3576)

This fixes an issue where when there are no [new] completed executions to copy to the peer the peer would reset its "lastUpdated" timestamp causing a full diff on the DB next peering cycle.
Instead, we should just leave the "lastUpdated" timestamp unchanged. Also, fixed the test that didn't but should've exposed this issue (the test was incorrect)
---
 .../spinnaker/orca/peering/PeeringAgent.kt    |  2 +-
 .../orca/peering/PeeringAgentSpec.groovy      | 23 ++++++++++++++-----
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/orca-peering/src/main/kotlin/com/netflix/spinnaker/orca/peering/PeeringAgent.kt b/orca-peering/src/main/kotlin/com/netflix/spinnaker/orca/peering/PeeringAgent.kt
index 748cb384db..2860d428d0 100644
--- a/orca-peering/src/main/kotlin/com/netflix/spinnaker/orca/peering/PeeringAgent.kt
+++ b/orca-peering/src/main/kotlin/com/netflix/spinnaker/orca/peering/PeeringAgent.kt
@@ -203,7 +203,7 @@ class PeeringAgent(
       .map { it.id }
 
     fun getLatestCompletedUpdatedTime() =
-      (completedPipelineKeys.map { it.updated_at }.max() ?: 0)
+      (completedPipelineKeys.map { it.updated_at }.max() ?: updatedAfter)
 
     if (pipelineIdsToDelete.isEmpty() && pipelineIdsToMigrate.isEmpty()) {
       log.debug("No completed $executionType executions to copy for peering")
diff --git a/orca-peering/src/test/groovy/com/netflix/spinnaker/orca/peering/PeeringAgentSpec.groovy b/orca-peering/src/test/groovy/com/netflix/spinnaker/orca/peering/PeeringAgentSpec.groovy
index ddf63e8d3e..afc8f808cc 100644
--- a/orca-peering/src/test/groovy/com/netflix/spinnaker/orca/peering/PeeringAgentSpec.groovy
+++ b/orca-peering/src/test/groovy/com/netflix/spinnaker/orca/peering/PeeringAgentSpec.groovy
@@ -139,10 +139,17 @@ class PeeringAgentSpec extends Specification {
   def "updates the most recent timestamp even when there is nothing to copy"() {
     def peeringAgent = constructPeeringAgent()
 
-    def correctMax = Math.max(0, (srcKeys.max { it.updated_at }?.updated_at ?: 0) - clockDrift)
+    def correctMax = Math.max(0, (srcKeys.max { it.updated_at }?.updated_at ?: mostRecentTimeStamp) - clockDrift)
 
     when:
-    peeringAgent.peerExecutions(executionType)
+    if (executionType == PIPELINE) {
+      peeringAgent.completedPipelinesMostRecentUpdatedTime = mostRecentTimeStamp
+      peeringAgent.completedOrchestrationsMostRecentUpdatedTime = 0
+    } else {
+      peeringAgent.completedPipelinesMostRecentUpdatedTime = 0
+      peeringAgent.completedOrchestrationsMostRecentUpdatedTime = mostRecentTimeStamp
+    }
+    peeringAgent.peerCompletedExecutions(executionType)
 
     then:
     1 * src.getCompletedExecutionIds(executionType, "peeredId", mostRecentTimeStamp) >> srcKeys
@@ -176,10 +183,14 @@ class PeeringAgentSpec extends Specification {
     // Note: since the logic for executions and orchestrations should be the same, it's overkill to have the same set of tests for each
     // but it's easy so why not?
     executionType | mostRecentTimeStamp | srcKeys           | destKeys                           || toDelete | toCopy
-    PIPELINE      | 0                   | []                | []                                 || []       | []
-    PIPELINE      | 0                   | []                | [key("ID1", 100)]                  || ["ID1"]  | []
-    PIPELINE      | 0                   | [key("ID1", 100)] | [key("ID1", 100)]                  || []       | []
-    PIPELINE      | 0                   | [key("ID1", 100)] | [key("ID1", 100), key("ID2", 200)] || ["ID2"]  | []
+    PIPELINE      | 20                  | []                | []                                 || []       | []
+    PIPELINE      | 21                  | []                | [key("ID1", 100)]                  || ["ID1"]  | []
+    PIPELINE      | 22                  | [key("ID1", 100)] | [key("ID1", 100)]                  || []       | []
+    PIPELINE      | 23                  | [key("ID1", 100)] | [key("ID1", 100), key("ID2", 200)] || ["ID2"]  | []
+    ORCHESTRATION | 20                  | []                | []                                 || []       | []
+    ORCHESTRATION | 21                  | []                | [key("ID1", 100)]                  || ["ID1"]  | []
+    ORCHESTRATION | 22                  | [key("ID1", 100)] | [key("ID1", 100)]                  || []       | []
+    ORCHESTRATION | 23                  | [key("ID1", 100)] | [key("ID1", 100), key("ID2", 200)] || ["ID2"]  | []
   }
 
   @Unroll