Added ability to poll for container stats instead of constant stream #1646

yhlee-aws · 2018-10-24T19:46:37Z

Summary

Push blocked until next agent release
Allows for container stat polling instead of forcing constant stream. This allows for a significant decrease in CPU usage when a lot of containers are deployed. Duplicate/fix of #1475

addresses issue #1422

Implementation details

merging PR #1475 and fixing conflicts, and errors in unit tests.

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes: yes

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ub.com/inmar/amazon-ecs-agent into inmar-add-polling-option-for-getting-metrics

adnxn

changes lgtm, thanks @jcbowman

@yunhee-l: does this need an integ test to test the polling behavior?

linkar-ec2

Looks good overall.

linkar-ec2 · 2018-10-25T19:12:20Z

README.md

@@ -104,6 +104,8 @@ additional details on each available environment variable.
 | `ECS_UPDATES_ENABLED` | &lt;true &#124; false&gt; | Whether to exit for an updater to apply updates when requested. | false | false |
 | `ECS_UPDATE_DOWNLOAD_DIR` | /cache               | Where to place update tarballs within the container. | | |
 | `ECS_DISABLE_METRICS`     | &lt;true &#124; false&gt;  | Whether to disable metrics gathering for tasks. | false | true |
+| `ECS_POLL_METRICS`     | &lt;true &#124; false&gt;  | Whether to poll or stream when gathering metrics for tasks. | false | false |


Is this an Agent-specific decision to be able to turn off metrics completely? (How does this affect TACS if two Agents in a cluster poll at different frequencies?)

seems that way, if this is turned off we fall back to the streaming

linkar-ec2 · 2018-10-25T19:43:53Z

agent/dockerclient/dockerapi/docker_client.go

+	} else {
+		seelog.Infof("DockerGoClient: Starting to Poll for metrics")
+		//Sleep for a random period time up to the polling interval. This will help make containers ask for stats at different times
+		time.Sleep(time.Second * time.Duration(rand.Intn(int(dg.config.PollingMetricsWaitDuration.Seconds()))))


Good intuition but I don't think this is needed. The only time all containers of a Task would poll at the same time is if Agent stops and starts while the Task is running. Otherwise, assuming go routines run at approximately the same speed (or even randomly), containers are added to the stats engine iteratively so they will already be staggered to some degree.

I think there is value in adding extra jitter here.

sharanyad

please add unit tests

sharanyad · 2018-10-25T20:34:33Z

agent/config/config.go

 	cfg.platformOverrides()

 	return nil
 }

+func (cfg *Config) pollMetricsOverrides() {
+	if cfg.PollMetrics {
+


nit: extra line

sharanyad · 2018-10-25T20:35:25Z

agent/config/config.go

@@ -81,6 +85,14 @@ const (
 	// 'stuck' in the pull / unpack step. Very small values are unsafe and lead to high failure rate.
 	minimumImagePullInactivityTimeout = 1 * time.Minute

+	// minimumPollingMetricsWaitDuration specifies the minimum duration to wait before polling for new stats
+	// from docker. This is only used when PollMetrics is set to true
+	minimumPollingMetricsWaitDuration = 1 * time.Second


just curious, how did we come up with the min and max values?

customer did, there are some testing/benchmarking info in the original PR

sharanyad · 2018-10-25T20:36:38Z

CHANGELOG.md

@@ -1,4 +1,7 @@
 # Changelog
+## 1.22.1-dev
+* Enhancement - Configurable poll duration for container stats [@jcbowman](https://github.com/jcbowman) [#1646](https://github.com/aws/amazon-ecs-agent/pull/1646)


Should this be a feature?

sharanyad · 2018-10-25T20:39:00Z

agent/config/config_test.go

@@ -184,6 +189,12 @@ func TestInvalidLoggingDriver(t *testing.T) {
 	assert.Error(t, conf.validateAndOverrideBounds(), "Should be error with invalid-logging-driver")
 }

+func TestDefaultPollMetricsWithoutECSDataDir(t *testing.T) {


what does WithoutECSDataDir indicate? without config..?

I think that's what it means (see TestDefaultCheckpointWithoutECSDataDir below), if we do not have config file it should take default value.

sharanyad · 2018-10-25T20:40:09Z

agent/dockerclient/dockerapi/docker_client.go

+			}
+		}()
+	} else {
+		seelog.Infof("DockerGoClient: Starting to Poll for metrics")


can we also log the container id here?

sharanyad · 2018-10-25T20:42:35Z

agent/dockerclient/dockerapi/docker_client.go

-			seelog.Infof("DockerGoClient: Unable to retrieve stats for container %s: %v",
-				id, statsErr)
+	if !dg.config.PollMetrics {
+		seelog.Infof("DockerGoClient: Starting to Stream for metrics")


seelog.Info

added container ID to the logging line instead

sharanyad · 2018-10-25T20:45:54Z

agent/dockerclient/dockerapi/docker_client.go

+	} else {
+		seelog.Infof("DockerGoClient: Starting to Poll for metrics")
+		//Sleep for a random period time up to the polling interval. This will help make containers ask for stats at different times
+		time.Sleep(time.Second * time.Duration(rand.Intn(int(dg.config.PollingMetricsWaitDuration.Seconds()))))


Does this mean this could end up sleeping for the whole wait duration, skipping a cycle of poll metrics? not sure if this could lead to any unexpected behavior

at this point the polling cycle has not started, this is adding jitter before starting it.

changelog updated

yhlee-aws · 2018-10-29T21:58:48Z

BLOCKED: Holding off on adding unit tests, as sdk migration will end up in more conflicts. I'll update this PR again after migration branch is merged.

increasing fnl test timeout by 2 mins

yhlee-aws · 2018-12-20T17:33:35Z

Integ and functional tests have been added.

fenxiong · 2018-12-20T23:27:31Z

agent/functional_tests/tests/functionaltests_test.go

+		},
+	}
+	params.StartTime = aws.Time(RoundTimeUp(time.Now(), time.Minute).UTC())
+	params.EndTime = aws.Time((*params.StartTime).Add(waitMetricsInCloudwatchDuration).UTC())


i don't think the params here are used?

Good catch! I shortened this test's checks and didn't remove the set up part. Removed.

fenxiong · 2018-12-20T23:34:41Z

agent/stats/engine_integ_test.go

+func TestStatsEngineWithNewContainersWithPolling(t *testing.T) {
+	// additional config fields to use polling instead of stream
+	cfg.PollMetrics = true
+	cfg.PollingMetricsWaitDuration = 1 * time.Second


the cfg object is shared by many tests in this package.. we probably should set the value back if we are changing it for this test?

I tried to clean up how cfg is shared by all integ tests, but it's a bigger refactoring job. I'll work on it separately, but for this one I reset it to false/default duration at the end of the test.

yumex93 · 2018-12-21T00:59:09Z

Is this pr for merging dev to inmar-add-polling-option-for-getting-metrics or the other way?

yhlee-aws · 2018-12-21T01:04:00Z

merging into dev

jcbowman and others added 3 commits August 30, 2018 10:45

Added ability to poll for container stats instead of constant stream

062d013

Converting docker container stat polling from timer to ticker

f0270e7

Merge branch 'add-polling-option-for-getting-metrics' of https://gith…

45e9e1b

…ub.com/inmar/amazon-ecs-agent into inmar-add-polling-option-for-getting-metrics

yhlee-aws added this to the 1.22.1 milestone Oct 24, 2018

yhlee-aws requested a review from a team October 24, 2018 19:46

yhlee-aws mentioned this pull request Oct 24, 2018

Added ability to poll for container stats instead of constant stream #1475

Closed

8 tasks

yhlee-aws force-pushed the inmar-add-polling-option-for-getting-metrics branch from 07d1d5d to 3a831a5 Compare October 24, 2018 20:00

adnxn reviewed Oct 25, 2018

View reviewed changes

linkar-ec2 reviewed Oct 25, 2018

View reviewed changes

sharanyad reviewed Oct 25, 2018

View reviewed changes

fixing polling duration defaulting logic test and gocyclo violation

9902dd9

changelog updated

yhlee-aws force-pushed the inmar-add-polling-option-for-getting-metrics branch from 95e0023 to 9902dd9 Compare October 29, 2018 20:15

Merge branch 'dev' into inmar-add-polling-option-for-getting-metrics

5e8ec5f

Merge branch 'dev' into inmar-add-polling-option-for-getting-metrics

87e896d

yhlee-aws modified the milestones: 1.22.1, 1.24.0 Dec 18, 2018

yhlee-aws force-pushed the inmar-add-polling-option-for-getting-metrics branch 3 times, most recently from a7ffa9b to 5d2e394 Compare December 20, 2018 00:47

yhlee-aws closed this Dec 20, 2018

yhlee-aws reopened this Dec 20, 2018

Adding integ and functional tests for stats polling

d079426

increasing fnl test timeout by 2 mins

yhlee-aws force-pushed the inmar-add-polling-option-for-getting-metrics branch from 973d1fc to d079426 Compare December 20, 2018 17:25

Merge branch 'dev' into inmar-add-polling-option-for-getting-metrics

0abc2b7

shubham2892 approved these changes Dec 20, 2018

View reviewed changes

fenxiong reviewed Dec 20, 2018

View reviewed changes

clean up

360baba

fenxiong mentioned this pull request Dec 21, 2018

Fixing agent functional tests #1751

Merged

8 tasks

fenxiong approved these changes Dec 21, 2018

View reviewed changes

yhlee-aws merged commit 20cdb82 into aws:dev Dec 21, 2018

adnxn mentioned this pull request Jan 18, 2019

High CPU Gathering Stats for Lots of Containers #1422

Closed

yhlee-aws deleted the inmar-add-polling-option-for-getting-metrics branch July 3, 2019 17:47

sharanyad mentioned this pull request Jan 13, 2020

Remove the constraint of queue data points #2335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to poll for container stats instead of constant stream #1646

Added ability to poll for container stats instead of constant stream #1646

yhlee-aws commented Oct 24, 2018 •

edited

Loading

adnxn left a comment

linkar-ec2 left a comment

linkar-ec2 Oct 25, 2018

yhlee-aws Oct 29, 2018

linkar-ec2 Oct 25, 2018

yhlee-aws Oct 29, 2018

sharanyad left a comment

sharanyad Oct 25, 2018

yhlee-aws Oct 29, 2018

sharanyad Oct 25, 2018

yhlee-aws Oct 29, 2018

sharanyad Oct 25, 2018

yhlee-aws Dec 20, 2018

sharanyad Oct 25, 2018

yhlee-aws Oct 29, 2018

sharanyad Oct 25, 2018

yhlee-aws Oct 29, 2018

sharanyad Oct 25, 2018

yhlee-aws Oct 29, 2018

sharanyad Oct 25, 2018

yhlee-aws Oct 29, 2018

yhlee-aws commented Oct 29, 2018 •

edited

Loading

yhlee-aws commented Dec 20, 2018

fenxiong Dec 20, 2018

yhlee-aws Dec 20, 2018

fenxiong Dec 20, 2018

yhlee-aws Dec 21, 2018

yumex93 commented Dec 21, 2018

yhlee-aws commented Dec 21, 2018

Added ability to poll for container stats instead of constant stream #1646

Added ability to poll for container stats instead of constant stream #1646

Conversation

yhlee-aws commented Oct 24, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

adnxn left a comment

Choose a reason for hiding this comment

linkar-ec2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhlee-aws commented Oct 29, 2018 • edited Loading

yhlee-aws commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yumex93 commented Dec 21, 2018

yhlee-aws commented Dec 21, 2018

yhlee-aws commented Oct 24, 2018 •

edited

Loading

yhlee-aws commented Oct 29, 2018 •

edited

Loading