feat(sql): compositeStorageService.read gauge

Altered write behavior such that it'll ensure a successful write to
`previous` before attempting `primary`.

If `primary` fails, the expectation is that the `StorageServiceMigrator`
will subsequent migrate from `previous` to `primary`.

Few more tests for `loadObject()` behaviors.

front50-sql/front50-sql.gradle

@@ -17,12 +17,10 @@ dependencies {
   // runtimeOnly "mysql:mysql-connector-java"
 
   testImplementation "io.mockk:mockk"
-  testImplementation "com.ninja-squad:springmockk:1.1.0"
   testImplementation "mysql:mysql-connector-java"
   testImplementation "dev.minutest:minutest"
   testImplementation "org.testcontainers:mysql"
   testImplementation "org.junit.jupiter:junit-jupiter-api"
-  testImplementation "com.netflix.spinnaker.kork:kork-sql-test"
 }
 
 test {

front50-sql/src/main/kotlin/com/netflix/spinnaker/config/CompositeStorageServiceConfiguration.kt

@@ -35,10 +35,12 @@ class CompositeStorageServiceConfiguration() {
   @Primary
   @ConditionalOnProperty("spinnaker.migration.compositeStorageService.enabled")
   fun compositeStorageService(dynamicConfigService: DynamicConfigService,
+                              registry: Registry,
                               properties: StorageServiceMigratorConfigurationProperties,
                               storageServices: List<StorageService>) =
     CompositeStorageService(
       dynamicConfigService,
+      registry,
       storageServices.first { it.javaClass.canonicalName.equals(properties.primaryClass) },
       storageServices.first { it.javaClass.canonicalName.equals(properties.previousClass) }
     )

front50-sql/src/main/kotlin/com/netflix/spinnaker/front50/model/CompositeStorageService.kt

@@ -16,13 +16,15 @@
 
 package com.netflix.spinnaker.front50.model
 
+import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.front50.exception.NotFoundException
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import org.slf4j.LoggerFactory
 import org.springframework.scheduling.annotation.Scheduled
 
 class CompositeStorageService(
   private val dynamicConfigService: DynamicConfigService,
+  private val registry: Registry,
   private val primary: StorageService,
   private val previous: StorageService
 ) : StorageService {
@@ -31,6 +33,17 @@ class CompositeStorageService(
     private val log = LoggerFactory.getLogger(CompositeStorageService::class.java)
   }
 
+  var primaryReadStatusGauge = registry.gauge(
+    registry.createId("compositeStorageService.read")
+      .withTag("type", "primary")
+      .withTag("class", primary.javaClass.simpleName)
+  )
+  var previousReadStatusGauge = registry.gauge(
+    registry.createId("compositeStorageService.read")
+      .withTag("type", "previous")
+      .withTag("class", previous.javaClass.simpleName)
+  )
+
   @Scheduled(fixedDelay = 60000L)
   fun status() {
     log.debug(
@@ -40,6 +53,9 @@ class CompositeStorageService(
       previous.javaClass.simpleName,
       isPreviousReadEnabled()
     )
+
+    primaryReadStatusGauge.set(if (isPrimaryReadEnabled()) 1.0 else 0.0)
+    previousReadStatusGauge.set(if (isPreviousReadEnabled()) 1.0 else 0.0)
   }
 
   override fun ensureBucketExists() {
@@ -86,36 +102,38 @@ class CompositeStorageService(
   }
 
   override fun <T : Timestamped?> storeObject(objectType: ObjectType?, objectKey: String?, item: T) {
-    var exception: Exception? = null
-
     try {
-      primary.storeObject(objectType, objectKey, item)
+      /*
+       * Ensure that writes are first successful against the current source of truth (aka 'previous').
+       *
+       * The migration process (StorageServiceMigrator) is capable of detecting and migrating any records
+       * that failed the subsequent 'primary' write.
+       */
+      previous.storeObject(objectType, objectKey, item)
     } catch (e: Exception) {
-      exception = e
       log.error(
         "{}.storeObject({}, {}) failed",
-        primary.javaClass.simpleName,
+        previous.javaClass.simpleName,
         objectType,
         objectKey,
         e
       )
+
+      throw e
     }
 
     try {
-      previous.storeObject(objectType, objectKey, item)
+      primary.storeObject(objectType, objectKey, item)
     } catch (e: Exception) {
-      exception = e
       log.error(
         "{}.storeObject({}, {}) failed",
-        previous.javaClass.simpleName,
+        primary.javaClass.simpleName,
         objectType,
         objectKey,
         e
       )
-    }
 
-    if (exception != null) {
-      throw exception
+      throw e
     }
   }
 

front50-sql/src/test/kotlin/com/netflix/spinnaker/front50/model/CompositeStorageServiceTests.kt

@@ -16,28 +16,32 @@
 
 package com.netflix.spinnaker.front50.model
 
+import com.netflix.spectator.api.NoopRegistry
+import com.netflix.spinnaker.front50.exception.NotFoundException
+import com.netflix.spinnaker.front50.model.application.Application
 import com.netflix.spinnaker.front50.model.tag.EntityTags
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
 import dev.minutest.junit.JUnit5Minutests
 import dev.minutest.rootContext
 import io.mockk.*
 import strikt.api.expectThat
+import strikt.api.expectThrows
 import strikt.assertions.isEqualTo
 
 internal object CompositeStorageServiceTests : JUnit5Minutests {
   val dynamicConfigService: DynamicConfigService = mockk(relaxUnitFun = true)
   val primary: StorageService = mockk(relaxUnitFun = true)
   val previous: StorageService = mockk(relaxUnitFun = true)
 
-  val subject = CompositeStorageService(dynamicConfigService, primary, previous)
+  val subject = CompositeStorageService(dynamicConfigService, NoopRegistry(), primary, previous)
 
   fun tests() = rootContext {
     after {
       clearMocks(dynamicConfigService, primary, previous)
     }
 
-    context("Entity Tags") {
-      test("should always load from 'previous'") {
+    context("loadObject()") {
+      test("should always load EntityTags from 'previous'") {
         every {
           previous.loadObject<EntityTags>(ObjectType.ENTITY_TAGS, "id-entitytags001")
         } returns EntityTags().apply { id = "id-entitytags001" }
@@ -51,6 +55,71 @@ internal object CompositeStorageServiceTests : JUnit5Minutests {
           previous.loadObject<Timestamped>(ObjectType.ENTITY_TAGS, "id-entitytags001")
         }
       }
+
+      test("should favor 'primary'") {
+        every {
+          primary.loadObject<Application>(ObjectType.APPLICATION, "application001")
+        } returns Application().apply { name = "application001" }
+
+        every {
+          dynamicConfigService.getConfig(Boolean::class.java, any(), any())
+        } returns true
+
+        expectThat(
+          subject.loadObject<Application>(ObjectType.APPLICATION, "application001").id
+        ).isEqualTo("application001")
+
+        verifyAll {
+          primary.loadObject<Timestamped>(ObjectType.APPLICATION, "application001")
+          previous wasNot Called
+        }
+      }
+
+      test("should fallback to 'previous' when 'primary' fails") {
+        every {
+          primary.loadObject<Application>(ObjectType.APPLICATION, "application001")
+        } throws NotFoundException("Object not found")
+
+        every {
+          previous.loadObject<Application>(ObjectType.APPLICATION, "application001")
+        } returns Application().apply { name = "application001" }
+
+        every {
+          dynamicConfigService.getConfig(Boolean::class.java, any(), any())
+        } returns true
+
+        expectThat(
+          subject.loadObject<Application>(ObjectType.APPLICATION, "application001").id
+        ).isEqualTo("application001")
+
+        verifyAll {
+          primary.loadObject<Timestamped>(ObjectType.APPLICATION, "application001")
+          previous.loadObject<Timestamped>(ObjectType.APPLICATION, "application001")
+        }
+      }
+
+      test("should propagate exception if 'primary' fails and 'previous' not enabled") {
+        every {
+          primary.loadObject<Application>(ObjectType.APPLICATION, "application001")
+        } throws NotFoundException("Object not found")
+
+        every {
+          dynamicConfigService.getConfig(Boolean::class.java, match { it.contains("primary") }, any())
+        } returns true
+
+        every {
+          dynamicConfigService.getConfig(Boolean::class.java, match { it.contains("previous") }, any())
+        } returns false
+
+        expectThrows<NotFoundException> {
+          subject.loadObject<Application>(ObjectType.APPLICATION, "application001").id
+        }
+
+        verifyAll {
+          primary.loadObject<Timestamped>(ObjectType.APPLICATION, "application001")
+          previous wasNot Called
+        }
+      }
     }
   }
 }