fix (ecs): Unstable deployments when there are more than one containe…

…r in task definition (#5544)
spinnaker · Mar 12, 2020 · 9ec282f · 9ec282f
1 parent 35b8d96
commit 9ec282f
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 20 deletions.
diff --git a/...ain/java/com/netflix/spinnaker/clouddriver/ecs/provider/agent/TaskHealthCachingAgent.java b/...ain/java/com/netflix/spinnaker/clouddriver/ecs/provider/agent/TaskHealthCachingAgent.java
@@ -134,17 +134,24 @@ protected List<TaskHealth> getItems(AmazonECS ecs, ProviderCache providerCache)
           continue;
         }
 
-        TaskHealth taskHealth;
-        if (task.getContainers().get(0).getNetworkBindings().size() >= 1) {
-          taskHealth =
-              inferHealthNetworkBindedContainer(
-                  targetHealthCacheClient,
-                  task,
-                  containerInstance,
-                  serviceName,
-                  service,
-                  taskDefinition);
-        } else {
+        TaskHealth taskHealth = null;
+        Collection<Container> containers = task.getContainers();
+
+        for (Container container : containers) {
+          if (container.getNetworkBindings().size() >= 1) {
+            taskHealth =
+                inferHealthNetworkBindedContainer(
+                    targetHealthCacheClient,
+                    task,
+                    containerInstance,
+                    serviceName,
+                    service,
+                    taskDefinition);
+            break;
+          }
+        }
+
+        if (taskHealth == null) {
           taskHealth =
               inferHealthNetworkInterfacedContainer(
                   targetHealthCacheClient, task, serviceName, service, taskDefinition);
@@ -191,7 +198,15 @@ private TaskHealth inferHealthNetworkInterfacedContainer(
         continue;
       }
 
-      NetworkInterface networkInterface = task.getContainers().get(0).getNetworkInterfaces().get(0);
+      Collection<Container> containers = task.getContainers();
+      NetworkInterface networkInterface = null;
+
+      for (Container container : containers) {
+        if (container.getNetworkInterfaces().size() >= 1) {
+          networkInterface = container.getNetworkInterfaces().get(0);
+          break;
+        }
+      }
 
       overallTaskHealth =
           describeTargetHealth(
@@ -382,17 +397,29 @@ private boolean isContainerMissingNetworking(Task task) {
   }
 
   private boolean isTaskMissingNetworkBindings(Task task) {
-    return task.getContainers().isEmpty()
-        || task.getContainers().get(0).getNetworkBindings() == null
-        || task.getContainers().get(0).getNetworkBindings().isEmpty()
-        || task.getContainers().get(0).getNetworkBindings().get(0) == null;
+    Collection<Container> containers = task.getContainers();
+
+    for (Container container : containers) {
+      if (!(container.getNetworkBindings() == null
+          || container.getNetworkBindings().isEmpty()
+          || container.getNetworkBindings().get(0) == null)) {
+        return false;
+      }
+    }
+    return true;
   }
 
   private boolean isTaskMissingNetworkInterfaces(Task task) {
-    return task.getContainers().isEmpty()
-        || task.getContainers().get(0).getNetworkInterfaces() == null
-        || task.getContainers().get(0).getNetworkInterfaces().isEmpty()
-        || task.getContainers().get(0).getNetworkInterfaces().get(0) == null;
+    Collection<Container> containers = task.getContainers();
+
+    for (Container container : containers) {
+      if (!(container.getNetworkInterfaces() == null
+          || container.getNetworkInterfaces().isEmpty()
+          || container.getNetworkInterfaces().get(0) == null)) {
+        return false;
+      }
+    }
+    return true;
   }
 
   @Override

diff --git a/...vy/com/netflix/spinnaker/clouddriver/ecs/provider/agent/TaskHealthCachingAgentSpec.groovy b/...vy/com/netflix/spinnaker/clouddriver/ecs/provider/agent/TaskHealthCachingAgentSpec.groovy
@@ -327,6 +327,53 @@ class TaskHealthCachingAgentSpec extends Specification {
     taskHealthList == []
   }
 
+  def 'should get task health for task with some non-networked containers'() {
+    given:
+    ObjectMapper mapper = new ObjectMapper()
+    Map<String, Object> containerMap1 = mapper.convertValue(new Container().withName('noports'), Map.class)
+    Map<String, Object> containerMap2 = mapper.convertValue(new Container().withName('withports').withNetworkBindings(
+      new NetworkBinding().withContainerPort(1338).withHostPort(1338)
+    ), Map.class)
+
+    def taskAttributes = [
+      taskId              : CommonCachingAgent.TASK_ID_1,
+      taskArn             : CommonCachingAgent.TASK_ARN_1,
+      taskDefinitionArn   : CommonCachingAgent.TASK_DEFINITION_ARN_1,
+      startedAt           : new Date().getTime(),
+      containerInstanceArn: CommonCachingAgent.CONTAINER_INSTANCE_ARN_1,
+      group               : 'service:' + CommonCachingAgent.SERVICE_NAME_1,
+      containers          : [containerMap1, containerMap2]
+    ]
+    def taskKey = Keys.getTaskKey(CommonCachingAgent.ACCOUNT, CommonCachingAgent.REGION, CommonCachingAgent.TASK_ID_1)
+    def taskCacheData = new DefaultCacheData(taskKey, taskAttributes, Collections.emptyMap())
+    providerCache.getAll(TASKS.toString(), _) >> Collections.singletonList(taskCacheData)
+
+    Map<String, Object> containerDefinitionMap1 = mapper.convertValue(new ContainerDefinition().withName('noports'), Map.class)
+    Map<String, Object> containerDefinitionMap2 = mapper.convertValue(new ContainerDefinition().withName('withports').withPortMappings(
+      new PortMapping().withHostPort(1338)
+    ), Map.class)
+    def taskDefAttributes = [
+      taskDefinitionArn    : CommonCachingAgent.TASK_DEFINITION_ARN_1,
+      containerDefinitions : [ containerDefinitionMap1, containerDefinitionMap2 ]
+    ]
+    def taskDefKey = Keys.getTaskDefinitionKey(CommonCachingAgent.ACCOUNT, CommonCachingAgent.REGION, CommonCachingAgent.TASK_DEFINITION_ARN_1)
+    def taskDefCacheData = new DefaultCacheData(taskDefKey, taskDefAttributes, Collections.emptyMap())
+    providerCache.get(TASK_DEFINITIONS.toString(), taskDefKey) >> taskDefCacheData
+
+    when:
+    def taskHealthList = agent.getItems(ecs, providerCache)
+
+    then:
+    taskHealthList.size() == 1
+    TaskHealth taskHealth = taskHealthList.get(0)
+    taskHealth.getState() == 'Up'
+    taskHealth.getType() == 'loadBalancer'
+    taskHealth.getInstanceId() == CommonCachingAgent.TASK_ARN_1
+    taskHealth.getServiceName() == CommonCachingAgent.SERVICE_NAME_1
+    taskHealth.getTaskArn() == CommonCachingAgent.TASK_ARN_1
+    taskHealth.getTaskId() == CommonCachingAgent.TASK_ID_1
+  }
+
   def 'should generate fresh data'() {
     given:
     def taskIds = [CommonCachingAgent.TASK_ID_1, CommonCachingAgent.TASK_ID_2]