From b06fee8c0fa4da7899ca3ff0434553eb355b1d7b Mon Sep 17 00:00:00 2001 From: Mike Lang Date: Mon, 28 Nov 2016 08:55:10 -0800 Subject: [PATCH] Review feedback --- probe/awsecs/client.go | 15 +++++------- probe/awsecs/reporter.go | 53 +++++++++++++++++++++++----------------- prog/main.go | 2 +- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/probe/awsecs/client.go b/probe/awsecs/client.go index 8d56a4fd3..cac336603 100644 --- a/probe/awsecs/client.go +++ b/probe/awsecs/client.go @@ -33,7 +33,7 @@ func newClient(cluster string) (*ecsClient, error) { // 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) + results := map[string]string{} lock := sync.Mutex{} // lock mediates access to results group := sync.WaitGroup{} @@ -43,21 +43,20 @@ func (c ecsClient) getDeploymentMap() map[string]string { func(page *ecs.ListServicesOutput, lastPage bool) bool { // describe each page of 10 (the max for one describe command) concurrently group.Add(1) + serviceArns := page.ServiceArns go func() { defer group.Done() resp, err := c.client.DescribeServices(&ecs.DescribeServicesInput{ Cluster: &c.cluster, - Services: page.ServiceArns, + Services: serviceArns, }) if err != nil { - // rather than trying to propagate errors up, just log a warning here log.Warnf("Error describing some ECS services, ECS service report may be incomplete: %v", err) return } for _, failure := range resp.Failures { - // log the failures but still continue with what succeeded log.Warnf("Failed to describe ECS service %s, ECS service report may be incomplete: %s", failure.Arn, failure.Reason) } @@ -75,7 +74,6 @@ func (c ecsClient) getDeploymentMap() map[string]string { group.Wait() if err != nil { - // We want to still return partial results if we have any, so just log a warning log.Warnf("Error listing ECS services, ECS service report may be incomplete: %v", err) } return results @@ -99,11 +97,10 @@ func (c ecsClient) getTaskDeployments(taskArns []string) (map[string]string, err } for _, failure := range resp.Failures { - // log the failures but still continue with what succeeded log.Warnf("Failed to describe ECS task %s, ECS service report may be incomplete: %s", failure.Arn, failure.Reason) } - results := make(map[string]string) + results := make(map[string]string, len(resp.Tasks)) for _, task := range resp.Tasks { results[*task.TaskArn] = *task.StartedBy } @@ -112,7 +109,7 @@ func (c ecsClient) getTaskDeployments(taskArns []string) (map[string]string, err // returns a map from task ARNs to service names func (c ecsClient) getTaskServices(taskArns []string) (map[string]string, error) { - deploymentMapChan := make(chan map[string]string) + deploymentMapChan := chan map[string]string{} go func() { deploymentMapChan <- c.getDeploymentMap() }() @@ -125,7 +122,7 @@ func (c ecsClient) getTaskServices(taskArns []string) (map[string]string, error) return nil, err } - results := make(map[string]string) + results := map[string]string{} for taskArn, depID := range taskDeployments { // Note not all tasks map to a deployment, or we could otherwise mismatch due to races. // It's safe to just ignore all these cases and consider them "non-service" tasks. diff --git a/probe/awsecs/reporter.go b/probe/awsecs/reporter.go index 8752a07b5..8537c4611 100644 --- a/probe/awsecs/reporter.go +++ b/probe/awsecs/reporter.go @@ -18,29 +18,38 @@ type taskInfo struct { // return map from cluster to map of task arns to task infos func getLabelInfo(rpt report.Report) map[string]map[string]*taskInfo { - results := make(map[string]map[string]*taskInfo) + results := map[string]map[string]*taskInfo{} log.Debug("scanning for ECS containers") for nodeID, node := range rpt.Container.Nodes { - taskArn, taskArnOk := node.Latest.Lookup(docker.LabelPrefix + "com.amazonaws.ecs.task-arn") - 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 { - taskMap, ok := results[cluster] - if !ok { - taskMap = make(map[string]*taskInfo) - results[cluster] = taskMap - } - - task, ok := taskMap[taskArn] - if !ok { - task = &taskInfo{containerIDs: make([]string, 0), family: family} - taskMap[taskArn] = task - } - - task.containerIDs = append(task.containerIDs, nodeID) + taskArn, ok := node.Latest.Lookup(docker.LabelPrefix + "com.amazonaws.ecs.task-arn") + if !ok { + continue } + + cluster, ok := node.Latest.Lookup(docker.LabelPrefix + "com.amazonaws.ecs.cluster") + if !ok { + continue + } + + family, ok := node.Latest.Lookup(docker.LabelPrefix + "com.amazonaws.ecs.task-definition-family") + if !ok { + continue + } + + taskMap, ok := results[cluster] + if !ok { + taskMap = map[string]*taskInfo{} + results[cluster] = taskMap + } + + task, ok := taskMap[taskArn] + if !ok { + task = &taskInfo{containerIDs: []string{}, family: family} + taskMap[taskArn] = task + } + + task.containerIDs = append(task.containerIDs, nodeID) } log.Debug("Got ECS container info: %v", results) return results @@ -51,7 +60,7 @@ type Reporter struct { } // Tag needed for Tagger -func (r Reporter) Tag(rpt report.Report) (report.Report, error) { +func (Reporter) Tag(rpt report.Report) (report.Report, error) { rpt = rpt.Copy() clusterMap := getLabelInfo(rpt) @@ -75,7 +84,7 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { } // Create all the services first - unique := make(map[string]bool) + unique := map[string]bool{} for _, serviceName := range taskServices { if !unique[serviceName] { serviceID := report.MakeECSServiceNodeID(serviceName) @@ -99,7 +108,6 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { serviceID := report.MakeECSServiceNodeID(serviceName) parentsSets = parentsSets.Add(report.ECSService, report.MakeStringSet(serviceID)) } - for _, containerID := range info.containerIDs { if containerNode, ok := rpt.Container.Nodes[containerID]; ok { rpt.Container.Nodes[containerID] = containerNode.WithParents(parentsSets) @@ -107,7 +115,6 @@ func (r Reporter) Tag(rpt report.Report) (report.Report, error) { log.Warnf("Got task info for non-existent container %v, this shouldn't be able to happen", containerID) } } - } } diff --git a/prog/main.go b/prog/main.go index 2f8999968..0a679970c 100644 --- a/prog/main.go +++ b/prog/main.go @@ -285,7 +285,7 @@ func main() { 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") + flag.BoolVar(&flags.probe.ecsEnabled, "probe.ecs", false, "Collect ecs-related attributes for containers on this node") // Weave flag.StringVar(&flags.probe.weaveAddr, "probe.weave.addr", "127.0.0.1:6784", "IP address & port of the Weave router")