Increase steady state and burst throttles for task endpoints #1240

sharanyad · 2018-02-17T00:02:24Z

Summary

This addresses #1231

Implementation details

default steady state throttle of 40 rps and burst of 60 rps.
values configurable through ECS_TASK_METADATA_RPS_LIMIT environment variable.

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

This contribution is under the terms of the Apache 2.0 License: yes

yhlee-aws · 2018-02-17T00:23:02Z

looks like gocyclo failed? if/else statements with || tends to push the cyclomatic complexity up, which we currently fail if a method has complexity > 15.

aaithal · 2018-02-19T16:53:25Z

agent/config/config.go

+	rpsLimitEnvVal := os.Getenv("ECS_TASK_METADATA_RPS_LIMIT")
+	rpsLimitSplits := strings.Split(rpsLimitEnvVal, ",")
+	if len(rpsLimitSplits) != 2 {
+		seelog.Warnf("Invalid format for \"ECS_TASK_METADATA_RPS_LIMIT\", expected: \"rateLimit,burst\"")


nit: you can also do something like this to avoid these escapes:

seelog.Warnf(`Invalid format for "ECS_TASK_METADATA_RPS_LIMIT", expected: "rateLimit,burst"`)

Also, you can just use seelog.Warn() here.

aaithal · 2018-02-19T16:54:10Z

agent/config/config.go

+	rpsLimitSplits := strings.Split(rpsLimitEnvVal, ",")
+	if len(rpsLimitSplits) != 2 {
+		seelog.Warnf("Invalid format for \"ECS_TASK_METADATA_RPS_LIMIT\", expected: \"rateLimit,burst\"")
+	} else {


just return here so that you don't need an additional else block. Avoids one level of indentation

This block got moved into this method from the original environmentConfig() and was never refactored. Will make this change.

aaithal · 2018-02-19T16:57:11Z

agent/config/config.go

+	} else {
+		rpsLimitVal, err := strconv.Atoi(strings.TrimSpace(rpsLimitSplits[0]))
+		rpsBurstLimitVal, err1 := strconv.Atoi(strings.TrimSpace(rpsLimitSplits[1]))
+		if err != nil || err1 != nil {


This kind of error handling makes it harder to know which field exactly caused the error. Doing something like this is much simpler imo:

rpsLimit, err := strconv.Atoi(strings.TrimSpace(rpsLimitSplits[0])) if err != nil { ... return 0, 0 } rpsBurstLimit, err := strconv.Atoi(strings.TrimSpace(rpsLimitSplits[1])) if err != nil { ... return 0, 0 } return rpsLimit, rpsBurstLimit

aaithal · 2018-02-19T16:58:52Z

agent/config/config.go

+	// DefaultRpsLimit is set as 40. This is arrived from our benchmarking
+	// results where task endpoint can handle 4000 rps effectively. Here, 100 containers
+	// will be able to send out 40 rps.
+	DefaultRpsLimit = 40


DefaultRpsLimit is a bit vague. Similar to above. Can you please rename this to something like DefaultTaskMetadataSteadyStateRate?

aaithal · 2018-02-19T16:59:13Z

agent/config/config.go

+	DefaultRpsLimit = 40
+
+	// DefaultRpsBurstLimit is set to handle 60 burst requests at once
+	DefaultRpsBurstLimit = 60


Similar to above. Can you please rename this to something like DefaultTaskMetadataBurstRate?

aaithal · 2018-02-19T17:00:02Z

agent/config/config.go

@@ -434,6 +445,25 @@ func getTaskCPUMemLimitEnabled() Conditional {
 	return taskCPUMemLimitEnabled
 }

+func getTaskMetatadataRpsLimits() (int, int) {


getTaskMetadataThrottles() is a better name for this.

aaithal · 2018-02-19T17:01:42Z

agent/config/types.go

@@ -198,4 +198,10 @@ type Config struct {
 	// CgroupPath is the path expected by the agent, defaults to
 	// '/sys/fs/cgroup'
 	CgroupPath string
+
+	// RpsLimit specifies the steady state throttle for the task metadata endpoint
+	RpsLimit int


Similar to above. Can you please rename this to something like TaskMetadataSteadyStateRate?

aaithal · 2018-02-19T17:02:07Z

agent/config/types.go

+	RpsLimit int
+
+	// RpsBurstLimit specifies the burst rate throttle for the task metadata endpoint
+	RpsBurstLimit int


Similar to above. Can you please rename this to something like TaskMetadataBurstRate?

aaithal · 2018-02-19T17:03:53Z

agent/config/config.go

@@ -346,6 +354,7 @@ func environmentConfig() (Config, error) {
 	}
 	containerMetadataEnabled := utils.ParseBool(os.Getenv("ECS_ENABLE_CONTAINER_METADATA"), false)
 	dataDirOnHost := os.Getenv("ECS_HOST_DATA_DIR")
+	rpsLimit, rpsBurstLimit := getTaskMetatadataRpsLimits()


steadyStateRate and burstRate are better names imo

aaithal · 2018-02-19T17:04:30Z

agent/config/config.go

@@ -434,6 +445,25 @@ func getTaskCPUMemLimitEnabled() Conditional {
 	return taskCPUMemLimitEnabled
 }

+func getTaskMetatadataRpsLimits() (int, int) {
+	var rpsLimit, rpsBurstLimit int
+	rpsLimitEnvVal := os.Getenv("ECS_TASK_METADATA_RPS_LIMIT")


Can you also add an entry in README and changelog for this?

adnxn · 2018-02-19T17:07:44Z

agent/config/config.go

@@ -434,6 +445,25 @@ func getTaskCPUMemLimitEnabled() Conditional {
 	return taskCPUMemLimitEnabled
 }

+func getTaskMetatadataRpsLimits() (int, int) {


maybe i missed an earlier conversation. why don't we want two separate env variables for this? ECS_TASK_METADATA_RPS_LIMIT and ECS_TASK_METADATA_RPS_BURST_LIMIT?

Since steady state and burst throttles both are needed and related, it would be convenient for customers to have a single environment variable.

hm i don't feel strongly either way, my concern was mostly if it would add extra overhead of enforcing ordering of the throttle limits (for the customer) and extra validation code (for us). but what you said about "needed and related" also makes sense. okay thanks.

adnxn

changes lgtm, minor nits only.

adnxn · 2018-02-19T19:18:24Z

agent/config/config.go

+	rpsLimitSplits := strings.Split(rpsLimitEnvVal, ",")
+	if len(rpsLimitSplits) != 2 {
+		seelog.Warn(`Invalid format for "ECS_TASK_METADATA_RPS_LIMIT", expected: "rateLimit,burst"`)
+		return steadyStateRate, burstRate


nit: for consistency, should we explicitly return 0, 0 as you do for errors reading input in the two cases below?

adnxn · 2018-02-19T19:24:59Z

README.md

@@ -177,6 +177,8 @@ additional details on each available environment variable.
 | `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted. We use this to determine the source mount path for container metadata files in the case the ECS Agent is running as a container. We do not use this value in Windows because the ECS Agent is not running as container in Windows. | `/var/lib/ecs` | `Not used` |
 | `ECS_ENABLE_TASK_CPU_MEM_LIMIT` | `true` | Whether to enable task-level cpu and memory limits | `true` | `false` |
 | `ECS_CGROUP_PATH` | `/sys/fs/cgroup` | The root cgroup path that is expected by the ECS agent. This is the path that accessible from the agent mount. | `/sys/fs/cgroup` | Not applicable |
+| `ECS_TASK_METADATA_RPS_LIMIT` | `100,150` | Comma separated values for steady state and burst throttle limits for task metadata endpoint | `40,60` | `40,60` |


nit: specify integer value w/ "Comma separated integer values for..."

…endpoints This fixes aws#1231

sharanyad requested review from aaithal, adnxn and yhlee-aws February 17, 2018 00:02

sharanyad force-pushed the fix1231 branch from 28a70f1 to 1c4a6d8 Compare February 17, 2018 00:42

aaithal reviewed Feb 19, 2018

View reviewed changes

adnxn reviewed Feb 19, 2018

View reviewed changes

sharanyad force-pushed the fix1231 branch 2 times, most recently from dc61d25 to 91dc8fa Compare February 19, 2018 19:03

sharanyad added this to the 1.17.1 milestone Feb 19, 2018

adnxn approved these changes Feb 19, 2018

View reviewed changes

config, handlers: Increase steady state and burst throttles for task …

163f185

…endpoints This fixes aws#1231

sharanyad force-pushed the fix1231 branch from 91dc8fa to 163f185 Compare February 19, 2018 20:02

aaithal approved these changes Feb 19, 2018

View reviewed changes

sharanyad merged commit ea8ea05 into aws:dev Feb 19, 2018

richardpen mentioned this pull request Feb 19, 2018

v1.17.1 stage #1244

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase steady state and burst throttles for task endpoints #1240

Increase steady state and burst throttles for task endpoints #1240

sharanyad commented Feb 17, 2018 •

edited

yhlee-aws commented Feb 17, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

sharanyad Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

aaithal Feb 19, 2018

adnxn Feb 19, 2018

sharanyad Feb 19, 2018

adnxn Feb 19, 2018

adnxn left a comment

adnxn Feb 19, 2018

adnxn Feb 19, 2018

Increase steady state and burst throttles for task endpoints #1240

Increase steady state and burst throttles for task endpoints #1240

Conversation

sharanyad commented Feb 17, 2018 • edited

Summary

Implementation details

Testing

Description for the changelog

Licensing

yhlee-aws commented Feb 17, 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

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

adnxn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharanyad commented Feb 17, 2018 •

edited