-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 1 #3584
[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 1 #3584
Conversation
// If there is no user-specified host port, ECS Agent will find an available host port | ||
// within the given dynamic host port range. And if no host port is available within the range, | ||
// an error will be returned. | ||
logger.Debug("No user-specified host port, ECS Agent will find an available host port within the given dynamic host port range", logger.Fields{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example ecs-agent.log for line 2415 - 2427
level=info time=xxx msg="Creating container" task=“xxx” container="containerPort"
level=debug time=xxx msg="No user-specified host port, ECS Agent will find an available host port within the given dynamic host port range" container="containerPort" dynamicHostPortRange="50000-50001"
level=debug time=xxx msg="Port 50000 is unavailable or an error occurred while listening on the local tcp network" module=ephemeral_ports.go
level=debug time=xxx msg="Port 50001 is unavailable or an error occurred while listening on the local tcp network" module=ephemeral_ports.go
level=error time=xxx msg="Unable to find a host port for container within the given dynamic host port range" task=“xxx” container="containerPort" dynamicHostPortRange="50000-50001" error="a host port is unavailable"
level=error time=xxx msg="Error transitioning container" runtimeID="" nextState="CREATED" error="error retrieving docker port map: a host port is unavailable" task=“xxx” container="containerPort"
level=info time=xxx msg="Handling container change event" status="CREATED" task=“xxx” container="containerPort" runtimeID=""
level=warn time=xxx msg="Error creating container; marking its desired status as STOPPED" task=“xxx” container="containerPort" error="error retrieving docker port map: a host port is unavailable"
…
level=debug time=xxx msg="Submitted state change to ECS" eventType="task" eventData="TaskChange: [xxx -> STOPPED, Known Sent: STOPPED, PullStartedAt: xxx, PullStoppedAt: xxx, ExecutionStoppedAt: xxx, container change: xxx containerPort -> STOPPED, Reason HostConfigError: error retrieving docker port map: a host port is unavailable, Known Sent: STOPPED] sent: true"
|
||
// VerifyPortsWithinRange returns true if the actualPortRange is within the expectedPortRange; | ||
// otherwise, returns false. | ||
func VerifyPortsWithinRange(actualPortRange, expectedPortRange string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QQ. Is this method used anywhere other than in task_test.go ? If not, could you move it to the test file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do we verify that a port is within range in our taskdef validation or elsewhere? Where is this same validation done outside of our tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QQ. Is this method used anywhere other than in task_test.go ? If not, could you move it to the test file?
Also do we verify that a port is within range in our taskdef validation or elsewhere? Where is this same validation done outside of our tests?
This method is used in both agent/utils/ephemeral_ports_test.go
and agent/api/task/task_test.go
in this PR. As it's using in different packages, I have to make it as a public method.
And as fierlion@ suggested, we should validate a host port or a host port range assigned by ECS Agent after calling getHostPort()
or getHostPortRange()
. Therefore, I would like to keep VerifyPortsWithinRange()
in agent/utils/ephemeral_ports.go
, and have a follow up PR to implement host port validations outside of our tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
// VerifyPortsWithinRange returns true if the actualPortRange is within the expectedPortRange; | ||
// otherwise, returns false. | ||
func VerifyPortsWithinRange(actualPortRange, expectedPortRange string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do we verify that a port is within range in our taskdef validation or elsewhere? Where is this same validation done outside of our tests?
1. Add GetHostPort() and update unit tests #3570 2. Upudate dockerPortMap() in task.go with dynamic host port range support part 1 #3584 3. Upudate dockerPortMap() in task.go with dynamic host port range support part 2 #3589 4. Validate the host port/host port range found by ECS Agent before returning it #3589
1. Add GetHostPort() and update unit tests aws#3570 2. Upudate dockerPortMap() in task.go with dynamic host port range support part 1 aws#3584 3. Upudate dockerPortMap() in task.go with dynamic host port range support part 2 aws#3589 4. Validate the host port/host port range found by ECS Agent before returning it aws#3589 5. Refactor buildPortMapWithSCIngressConfig() in task.go aws#3600
1. Add GetHostPort() and update unit tests #3570 2. Upudate dockerPortMap() in task.go with dynamic host port range support part 1 #3584 3. Upudate dockerPortMap() in task.go with dynamic host port range support part 2 #3589 4. Validate the host port/host port range found by ECS Agent before returning it #3589 5. Refactor buildPortMapWithSCIngressConfig() in task.go #3600
Summary
DHPA - Dynamic Host Port Assignment
The target branch of this PR is feature/dynamicHostPortAssignment.
This is a follow-up PR for [DHPA] Add GetHostPort() and update unit tests to utilize function
GetHostPort()
indockerPortMap()
to construct a docker port map for an application container with the user-specified container port but without an user-defined host port running in bridge network mode.Comparing to the current dockerPortMap(), main changes in this PR include:
GetHostPort()
to get a host port from the givendynamicHostPortRange
when (a) it's a bridge network mode task and (b) there is the defined container port but no defined host in the port mapping configured by customers. With this change, ECS Agent will be the single source of truth for host port assignment. We will not fall back to Docker dynamic host port assignment if no host port can be found by ECS Agent within the givendynamicHostPortRange
.TestDockerHostConfigPortBinding
to validate changes made in this PRPortIsInRange()
andVerifyPortsWithinRange()
publicdynamicHostPortRange
toString()
for Agent config.goImplementation details
agent/api/task/task.go
dockerPortMap()
to include ECS Agent dynamic host port assignment for a singular port caseagent/api/task/task_test.go
TestDockerHostConfigPortBinding
to test changesagent/utils/ephemeral_ports.go
VerifyPortsWithinRange()
andPortIsInRange()
from the test file and make them public for reusing them in task_test.goagent/utils/ephemeral_ports_test.go
VerifyPortsWithinRange()
andPortIsInRange()
agent/config/config.go
dynamicHostPortRange
to functionString()
Testing
New tests cover the changes: yes
Unit test
TestDockerHostConfigPortBinding
TestPortIsInRange
TestVerifyPortsWithinRange
Manual test
Setup
aws-cli/2.10.0 Python/3.9.11 Linux/4.14.301-224.520.amzn2.x86_64 exe/x86_64.amzn.2 prompt/off
on the hostTest cases
Description for the changelog
[Enhancement] Support user-specified dynamic host port range for a singular port - part 1
Related PRs
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.