Add ECS views #2026

ekimekim · 2016-11-23T00:06:35Z

This PR adds two new ECS-specific topologes, ECSTasks and ECSServices,
and adds a probe which collects info to populate it.

The probe uses the instance's IAM role to look up task and service info. It requires the following permissions:
ListServices
DescribeTasks
DescribeServices

@2opremio: For rendering: I've associated a container with 0 or 1 ECSTasks and ECSServices, under the Node.Parents Sets.
ie. if a container X is associated with task Y and service Z, then rpt.Container.Nodes[X].Parents contains:

{
	"ecstask": [
		"Y;ecstask"
	],
	"ecsservice": [
		"Z;ecsservice"
	]
}

and so any endpoints associated with the container should be assoicated with the given task and service,
so that communication between containers is visible on the tasks view, etc.

TODO:

~~Unit tests~~ (moved to Add ECS views #2026 )
Test on Socks shop: https://microservices-demo.github.io/microservices-demo/deployment/ecs.html
Look for performance regressions (in the socks shop for instance)
~~Document required permissions~~ (No need, created Move ECS AMI documentation to Scope website #2039 , see Add ECS views #2026 (comment) for details)
Add checkpoint flag (like we do for k8s, see Add kubernetes checkpoint flag #1391)
Extend metadata shown in the details panel
Rendering
Make the topology appear along the top (app/api_topology.go)
Actually get the parent stuff described above working - there's a bug I can't work out right now and I need to go sleep. Something to do with the immutable/copying stuff, because I appear to be successfully making the change but it doesn't actually appear in the report.

ekimekim · 2016-11-24T04:53:31Z

more testing is needed since my test environment turned out to be flawed (not enough cpu to run app and probe), but apart from the rendering stuff that @2opremio is working on, this should be ready to go

2opremio · 2016-11-24T08:31:35Z

@abuehrle We are going to need documentation for this. Showing ECS views on Scope requires adding permissions to the AWS machines in which it runs. See above:

The probe uses the instance's IAM role to look up task and service info. It requires the following permissions:
ListServices
DescribeTasks
DescribeServices

@ekimekim knows the details and can draft/write the requirements but it would be great if you can review it (for Re:Invent which is when we plan to release this)

app/merger.go

 		}(<-c, <-c)
 	}
-	return <-c
+	return *<-c


ekimekim · 2016-11-27T08:59:29Z

Tested with sock shop. No obvious performance regressions after looking at CPU load on master (0.70) vs branch with ECS stuff disabled (0.81) vs branch with ECS stuff enabled (0.79). It's a really, really poor test, but we can discount crazy order-of-magnitude changes.

2opremio · 2016-11-28T06:54:43Z

We should probably add a filter for Clusters, pretty much like we do for K8s namespaces.

It is probably not urgent (since a Container Instance can only belong to a cluster) and you can create different organizations for different clusters in Weave Cloud.

@ekimekim Please add the cluster to the metadata (probably to the host instead of the task/service) in order to make this possible.

app/api_topologies.go

@@ -90,81 +96,98 @@ func AddInitialTopologiesToRegistry(registry *Registry) {
 	// be the verb to get to that state
 	registry.Add(
 		APITopologyDesc{
-			id:          processesTopologyDescID,
+			id:          processesID,


app/api_topologies.go

+			parent:      hostsID,
+			renderer:    render.WeaveRenderer,
+			Name:        "weave net",
+			HideIfEmpty: true,


app/merger.go

 			c <- left.Merge(right)
-		}(<-c, <-c)
+		}()


probe/awsecs/client.go

+
+	group := sync.WaitGroup{}
+
+	err := c.client.ListServicesPages(


probe/awsecs/client.go

+				lock.Lock()
+				for _, service := range resp.Services {
+					for _, deployment := range service.Deployments {
+						results[*deployment.Id] = *service.ServiceName


probe/awsecs/client.go

+
+				resp, err := c.client.DescribeServices(&ecs.DescribeServicesInput{
+					Cluster:  &c.cluster,
+					Services: page.ServiceArns,


probe/awsecs/client.go

+// returns a map from deployment ids to service names
+// cannot fail as it will attempt to deliver partial results, though that may end up being no results
+func (c ecsClient) getDeploymentMap() map[string]string {
+	results := make(map[string]string)


probe/awsecs/client.go

+	}
+
+	for _, failure := range resp.Failures {
+		// log the failures but still continue with what succeeded


2opremio · 2016-11-28T14:24:36Z

Fixes #721

probe/awsecs/client.go

+		log.Warnf("Failed to describe ECS task %s, ECS service report may be incomplete: %s", failure.Arn, failure.Reason)
+	}
+
+	results := make(map[string]string)


probe/awsecs/client.go

+		return nil, err
+	}
+
+	results := make(map[string]string)


probe/awsecs/reporter.go

+		if taskArnOk && clusterOk && familyOk {
+			taskMap, ok := results[cluster]
+			if !ok {
+				taskMap = make(map[string]*taskInfo)


probe/awsecs/reporter.go

+		cluster, clusterOk := node.Latest.Lookup(docker.LabelPrefix + "com.amazonaws.ecs.cluster")
+		family, familyOk := node.Latest.Lookup(docker.LabelPrefix + "com.amazonaws.ecs.task-definition-family")
+
+		if taskArnOk && clusterOk && familyOk {


probe/awsecs/reporter.go

+}
+
+// Tag needed for Tagger
+func (r Reporter) Tag(rpt report.Report) (report.Report, error) {


probe/awsecs/reporter.go

+		taskArns := make([]string, 0, len(taskMap))
+		for taskArn := range taskMap {
+			taskArns = append(taskArns, taskArn)
+		}


probe/awsecs/reporter.go

+		}
+
+		// Create all the services first
+		unique := make(map[string]bool)


probe/awsecs/reporter.go

+					log.Warnf("Got task info for non-existent container %v, this shouldn't be able to happen", containerID)
+				}
+			}
+


probe/awsecs/reporter.go

+			}
+
+		}
+


prog/main.go

@@ -282,6 +284,9 @@ func main() {
 	flag.StringVar(&flags.probe.kubernetesConfig.User, "probe.kubernetes.user", "", "The name of the kubeconfig user to use")
 	flag.StringVar(&flags.probe.kubernetesConfig.Username, "probe.kubernetes.username", "", "Username for basic authentication to the API server")

+	// AWS ECS
+	flag.BoolVar(&flags.probe.ecsEnabled, "probe.ecs", false, "collect ecs-related attributes for containers on this node")


tomwilkie · 2016-11-28T14:35:59Z

Lots of nits, but nothing major.

…ents

(not working yet)

spellcheck and required comments and required comment formatting

Turns out that when iterating in go, &loop_var is the same address every time

@rade

This is an alternate way of solving the same problem as 4007a90, but in a nicer way. Compared to using pointers, this approach more obviously preserves the original behaviour, and is arguably more readable than the original code. Credit to @rade for this approach.

Also, refactor some tests and MakeRegistry in api_topologies

errordeveloper · 2016-11-29T15:21:34Z

So you won't see tasks that don't have at least one container running on a probe-monitored instance.

Ah, sure, makes sense with respect to probe's view... Personally, I'd love to see tasks with no scheduled containers, but that'd be a big feature request, and I think I've already filed that as an issue sometime ago in the Kubernetes context or mentioned it elsewhere. Thanks for clarifying, glad we don't have a last minute bug.

2opremio · 2016-11-29T15:22:54Z

LGTM

2opremio · 2016-11-29T15:42:56Z

@pidster Also signed-off offline

ekimekim assigned 2opremio Nov 23, 2016

2opremio removed their assignment Nov 23, 2016

rade reviewed Nov 25, 2016

View reviewed changes

app/merger.go

}(<-c, <-c)

}

return <-c

return *<-c

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

2opremio mentioned this pull request Nov 28, 2016

Release 1.1.0 #2033

Merged