fix(canary): Fix Canary Indefinitely Retrying (#776)

If you were to supply a config with no metrics, orca would loop indefinitely as it waited for non existent stages to finish (#776)
spinnaker · Jul 31, 2020 · f2a3302 · f2a3302
1 parent 44346d1
commit f2a3302
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 8 deletions.
diff --git a/kayenta-core/src/main/java/com/netflix/kayenta/canary/ExecutionMapper.java b/kayenta-core/src/main/java/com/netflix/kayenta/canary/ExecutionMapper.java
@@ -357,6 +357,10 @@ public CanaryExecutionResponse buildExecution(
                 .withTag("canaryConfigName", canaryConfig.getName()))
         .increment();
 
+    if (canaryConfig.getMetrics().size() <= 0) {
+      throw new IllegalArgumentException(
+          "The canary config must specify at least one metric. Otherwise we're not analyzing anything. :)");
+    }
     Set<String> requiredScopes =
         canaryConfig.getMetrics().stream()
             .map(CanaryMetricConfig::getScopeName)

diff --git a/kayenta-web/src/test/java/com/netflix/kayenta/controllers/BaseControllerTest.java b/kayenta-web/src/test/java/com/netflix/kayenta/controllers/BaseControllerTest.java
@@ -4,6 +4,7 @@
 import static org.mockito.Mockito.when;
 import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.kayenta.canary.CanaryJudge;
 import com.netflix.kayenta.canary.ExecutionMapper;
 import com.netflix.kayenta.config.WebConfiguration;
@@ -16,8 +17,11 @@
 import com.netflix.kayenta.storage.StorageService;
 import com.netflix.kayenta.storage.StorageServiceRepository;
 import com.netflix.spectator.api.Registry;
+import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher;
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
+import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import org.junit.Before;
 import org.junit.runner.RunWith;
 import org.mockito.Answers;
@@ -27,6 +31,7 @@
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
+import org.springframework.context.annotation.Scope;
 import org.springframework.test.context.junit4.SpringRunner;
 import org.springframework.test.web.servlet.MockMvc;
 import org.springframework.test.web.servlet.setup.MockMvcBuilders;
@@ -40,11 +45,14 @@
 public abstract class BaseControllerTest {
 
   protected static final String CONFIGS_ACCOUNT = "configs-account";
+  protected static final String METRICS_STORE = "metrics-store";
+  protected static final String OBJECT_STORE = "object-store";
 
-  @Autowired StorageService storageService;
+  @MockBean StorageService storageService;
   @MockBean MetricSetPairListService metricSetPairListService;
   @MockBean ExecutionRepository executionRepository;
-  @MockBean ExecutionMapper executionMapper;
+  @MockBean ExecutionLauncher executionLauncher;
+  @Autowired ExecutionMapper executionMapper;
 
   @MockBean MetricsServiceRepository metricsServiceRepository;
 
@@ -61,6 +69,7 @@ public abstract class BaseControllerTest {
   public void setUp() {
     this.mockMvc =
         MockMvcBuilders.webAppContextSetup(this.webApplicationContext).alwaysDo(print()).build();
+    when(storageService.servicesAccount(CONFIGS_ACCOUNT)).thenReturn(true);
   }
 
   @EnableWebMvc
@@ -74,21 +83,41 @@ StorageServiceRepository storageServiceRepository(List<StorageService> storageSe
     }
 
     @Bean
-    StorageService storageService() {
-      StorageService mock = mock(StorageService.class);
-      when(mock.servicesAccount(CONFIGS_ACCOUNT)).thenReturn(true);
-      return mock;
+    @Scope("prototype")
+    ExecutionMapper executionMapper(
+        ExecutionRepository executionRepository,
+        ExecutionLauncher executionLauncher,
+        Registry registry) {
+      return new ExecutionMapper(
+          new ObjectMapper(),
+          registry,
+          "",
+          Optional.empty(),
+          executionLauncher,
+          executionRepository);
     }
 
     @Bean
     AccountCredentialsRepository accountCredentialsRepository() {
       MapBackedAccountCredentialsRepository repo = new MapBackedAccountCredentialsRepository();
-      repo.save(CONFIGS_ACCOUNT, getCredentials(CONFIGS_ACCOUNT));
+      repo.save(
+          CONFIGS_ACCOUNT,
+          getCredentials(CONFIGS_ACCOUNT, AccountCredentials.Type.CONFIGURATION_STORE));
+      repo.save(
+          METRICS_STORE, getCredentials(METRICS_STORE, AccountCredentials.Type.METRICS_STORE));
+      repo.save(OBJECT_STORE, getCredentials(OBJECT_STORE, AccountCredentials.Type.OBJECT_STORE));
       return repo;
     }
 
-    private static AccountCredentials getCredentials(String accountName) {
+    private static AccountCredentials getCredentials(
+        String accountName, AccountCredentials.Type type) {
+      return getCredentials(accountName, Collections.singletonList(type));
+    }
+
+    private static AccountCredentials getCredentials(
+        String accountName, List<AccountCredentials.Type> types) {
       AccountCredentials credentials = mock(AccountCredentials.class);
+      when(credentials.getSupportedTypes()).thenReturn(types);
       when(credentials.getName()).thenReturn(accountName);
       return credentials;
     }

diff --git a/kayenta-web/src/test/java/com/netflix/kayenta/controllers/CanaryControllerTest.java b/kayenta-web/src/test/java/com/netflix/kayenta/controllers/CanaryControllerTest.java
@@ -0,0 +1,34 @@
+package com.netflix.kayenta.controllers;
+
+import static org.hamcrest.core.StringContains.containsString;
+import static org.mockito.Mockito.when;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
+import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
+
+import com.netflix.kayenta.canary.CanaryConfig;
+import com.netflix.kayenta.storage.ObjectType;
+import org.junit.Test;
+
+public class CanaryControllerTest extends BaseControllerTest {
+
+  private static final String CONFIG_ID = "canary_config_12345";
+
+  @Test
+  public void initiateCanary_failsIfNoMetricsSpecified() throws Exception {
+    CanaryConfig response = CanaryConfig.builder().application("test-app").build();
+    when(storageService.loadObject(CONFIGS_ACCOUNT, ObjectType.CANARY_CONFIG, CONFIG_ID))
+        .thenReturn(response);
+
+    this.mockMvc
+        .perform(
+            post("/canary/{configId}?application=test-app", CONFIG_ID)
+                .contentType("application/json")
+                .content(
+                    "{\"scopes\":{\"default\":{\"controlScope\":{\"scope\":\"testapp-baseline\",\"location\":\"us-east-1\",\"start\":\"2020-07-27T19:17:36Z\",\"end\":\"2020-07-27T19:21:36Z\",\"step\":60,\"extendedScopeParams\":{\"type\":\"cluster\",\"environment\":\"prod\",\"dataset\":\"regional\",\"deployment\":\"main\"}},\"experimentScope\":{\"scope\":\"testapp-canary\",\"location\":\"us-east-1\",\"start\":\"2020-07-27T19:17:36Z\",\"end\":\"2020-07-27T19:21:36Z\",\"step\":60,\"extendedScopeParams\":{\"type\":\"cluster\",\"environment\":\"prod\",\"dataset\":\"regional\",\"deployment\":\"main\"}}}},\"thresholds\":{\"pass\":95.0,\"marginal\":75.0}}"))
+        .andDo(print())
+        .andExpect(status().is4xxClientError())
+        .andExpect(content().contentType("application/json"))
+        .andExpect(jsonPath("$.message").value(containsString("at least one metric")));
+  }
+}