Unmark if excluded (#133)

* chore(logs): remove debug logs
* fix(excluded): unmark all resources that wouldn't be marked this run

swabbie-aws/src/main/kotlin/com/netflix/spinnaker/swabbie/aws/edda/providers/EddaLaunchConfigurationCacheProvider.kt

@@ -97,7 +97,6 @@ data class AmazonLaunchConfigurationCache(
   }
 
   fun getLaunchConfigsByRegionForImage(params: Parameters): Set<AmazonLaunchConfiguration> {
-    log.debug("getting launch configs in ${javaClass.simpleName}")
     if (params.region != "" && params.id != "") {
       return getRefdAmisForRegion(params.region).getOrDefault(params.id, emptySet())
     } else {
@@ -111,7 +110,6 @@ data class AmazonLaunchConfigurationCache(
    * Returns a map of <K: all ami's referenced by a launch config in region, V: set of launch configs referencing K>
    */
   fun getRefdAmisForRegion(region: String): Map<String, Set<AmazonLaunchConfiguration>> {
-    log.debug("getting refd amis in ${javaClass.simpleName}")
     return refdAmisByRegion[region].orEmpty()
   }
 

swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/AbstractResourceTypeHandler.kt

@@ -207,24 +207,24 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
   }
 
   /**
-   * @param candidates: a list of resources that exist in the account/location defined in the work configuration
+   * @param validMarkedResources: a list of resources that were just marked in the account/location defined in
+   *  the work configuration
    * @param workConfiguration: defines the account/location to work with
    *
    * Fetches already marked resources, filters by work configuration namespace, and un-marks any resource whos id
-   * is not present in candidates.
+   * is not present in validMarkedResources.
    */
-  private fun removeDeletedResourcesFromMarkedResources(
-    candidates: List<T>?,
+  private fun unmarkResources(
+    validMarkedResources: Set<String>,
     workConfiguration: WorkConfiguration
   ) {
     resourceRepository
       .getMarkedResources()
       .filter { it.namespace == workConfiguration.namespace }
       .let { markedResourcesToCheck ->
-        val existingResources = candidates?.map { it.resourceId }?.toHashSet() ?: emptySet<String>()
-        for (mr in markedResourcesToCheck) {
-          if (!existingResources.contains(mr.resourceId)) {
-            ensureResourceUnmarked(mr, workConfiguration, "Resource no longer exists")
+        for (resource in markedResourcesToCheck) {
+          if (!validMarkedResources.contains(resource.resourceId)) {
+            ensureResourceUnmarked(resource, workConfiguration, "Resource no longer qualifies to be deleted")
           }
         }
       }
@@ -238,6 +238,7 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
     val violationCounter = AtomicInteger(0)
     val totalResourcesVisitedCounter = AtomicInteger(0)
     val markedResources = mutableListOf<MarkedResource>()
+    val validMarkedIds = mutableSetOf<String>()
 
     try {
       log.info("${javaClass.simpleName} running. Configuration: ", workConfiguration.toLog())
@@ -248,8 +249,6 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
       log.info("Fetched {} resources. Configuration: {}", candidates.size, workConfiguration.toLog())
       totalResourcesVisitedCounter.set(candidates.size)
 
-      removeDeletedResourcesFromMarkedResources(candidates, workConfiguration)
-
       val markedCandidates: List<MarkedResource> = resourceRepository.getMarkedResources()
         .filter { it.namespace == workConfiguration.namespace }
 
@@ -299,10 +298,12 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
               candidateCounter.incrementAndGet()
               violationCounter.addAndGet(violations.size)
               markedResources.add(newMarkedResource)
+              validMarkedIds.add(newMarkedResource.resourceId)
             }
             else -> {
               // already marked, skipping.
               log.debug("Already marked resource " + alreadyMarkedCandidate.resourceId + " ...skipping")
+              validMarkedIds.add(alreadyMarkedCandidate.resourceId)
             }
           }
         } catch (e: Exception) {
@@ -311,6 +312,8 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
         }
       }
 
+      unmarkResources(validMarkedIds, workConfiguration)
+
       printResult(candidateCounter, totalResourcesVisitedCounter, workConfiguration, markedResources, Action.MARK)
     } finally {
       recordMarkMetrics(timerId, workConfiguration, violationCounter, candidateCounter)

swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/ResourceTypeHandlerTest.kt

@@ -463,13 +463,66 @@ object ResourceTypeHandlerTest {
     val markedResource = MarkedResource(
       resource = defaultResource,
       summaries = listOf(Summary("invalid resource", javaClass.simpleName)),
-      namespace = configuration.namespace, //todod eb: fix tests for new marked resource
+      namespace = configuration.namespace,
       projectedDeletionStamp = clock.millis(),
       projectedSoftDeletionStamp = clock.millis().minus(TimeUnit.DAYS.toMillis(1))
     )
 
-    whenever(resourceRepository.getMarkedResources()) doReturn
-      listOf(markedResource)
+    whenever(resourceRepository.getMarkedResources())
+      .thenReturn(listOf(markedResource))
+      .thenReturn(emptyList())
+
+
+    val handler = TestResourceTypeHandler(
+      clock = clock,
+      rules = listOf(
+        TestRule(invalidOn = { true }, summary = null)
+      ),
+      resourceTrackingRepository = resourceRepository,
+      resourceStateRepository = resourceStateRepository,
+      ownerResolver = ownerResolver,
+      exclusionPolicies = listOf(mock()),
+      applicationEventPublisher = applicationEventPublisher,
+      simulatedCandidates = mutableListOf(defaultResource),
+      notifiers = listOf(mock()),
+      lockingService = lockingService,
+      retrySupport = mock(),
+      taskTrackingRepository = InMemoryTaskTrackingRepository(clock),
+      resourceUseTrackingRepository = resourceUseTrackingRepository
+    )
+
+    whenever(ownerResolver.resolve(any())) doReturn "[email protected]"
+    handler.mark(
+      workConfiguration = configuration,
+      postMark = { postAction(listOf(defaultResource)) }
+    )
+
+    verify(applicationEventPublisher, times(1)).publishEvent(
+      check<UnMarkResourceEvent> { event ->
+        Assertions.assertTrue((event.markedResource.resourceId == markedResource.resourceId))
+        Assertions.assertTrue((event.workConfiguration.namespace == configuration.namespace))
+      }
+    )
+    verify(resourceRepository, times(1)).remove(any())
+    verify(resourceRepository, never()).upsert(any(), any(), any())
+  }
+
+  @Test
+  fun `should forget resource if it is excluded after being marked`() {
+    val configuration = workConfiguration()
+    val markedResource = MarkedResource(
+      resource = defaultResource,
+      summaries = listOf(Summary("invalid resource", javaClass.simpleName)),
+      namespace = configuration.namespace,
+      projectedDeletionStamp = clock.millis(),
+      projectedSoftDeletionStamp = clock.millis().minus(TimeUnit.DAYS.toMillis(1))
+    )
+
+    whenever(resourceRepository.getMarkedResources())
+      .thenReturn(emptyList())
+      .thenReturn(listOf(markedResource))
+
+
 
     val handler = TestResourceTypeHandler(
       clock = clock,