Skip to content
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

bugfix for container port range #3494

Conversation

singholt
Copy link
Contributor

@singholt singholt commented Nov 29, 2022

Summary

This PR makes 2 bug-fixes:

  1. fix ports information logged in agent logs.
  2. fix port search for case when 1 container requests multiple container port ranges.

Implementation details

  1. We currently log ports information in agent logs here and here. With this project, we made the containerPort and containerPortRange pointer to int/string values. So the logger is now logging the addresses of variables, rather than the values themselves. We have 2 options to fix this:
    a) Encode the ports into JSON and then print it. example
    b) Change the references back to values. The reason why these were references in the first place is because we wanted to perform nil checks. Since we know that containerPort: 0 and containerPortRange: "" are not valid (validated on ECS side), we can stick to non-references.
    We're doing option (b) in this PR, since its simple and also avoids a lot of type conversions in unit tests.

  2. Fixing concurrent host port search when a container requests multiple port mappings, each with a container port range. Currently, we lock the method where we do host port search. Once we identify host ports, we send it to docker which then maps it. A case where the same container requests multiple container port ranges, would get the same host ports picked because we do the search (for both ranges) before sending a create-container call to docker. This PR fixes it by tracking which host port was last picked and locking read/updates to this tracker. Note that the method to get host ports is also concurrent-safe to prevent two task-level go routines get ports at the same time.

Testing

  1. Modified existing unit tests based on the refactor.
  2. Tested the changes manually by building an AMI with these changes and running 50 tasks at the same time, each with a container requesting multiple container port ranges.

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.

@singholt singholt marked this pull request as ready for review November 29, 2022 18:34
@singholt singholt requested a review from a team as a code owner November 29, 2022 18:34
agent/utils/ephemeral_ports.go Outdated Show resolved Hide resolved
@singholt singholt force-pushed the feature/container-port-range branch 2 times, most recently from 430ff15 to 074b1a9 Compare November 30, 2022 00:16
amogh09
amogh09 previously approved these changes Nov 30, 2022
agent/utils/ephemeral_ports.go Outdated Show resolved Hide resolved
@singholt singholt dismissed stale reviews from YashdalfTheGray and amogh09 via 9868c82 November 30, 2022 22:56
@singholt singholt force-pushed the feature/container-port-range branch from 074b1a9 to 9868c82 Compare November 30, 2022 22:56
Copy link
Contributor

@YashdalfTheGray YashdalfTheGray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapproving after testing fix rebase.

amogh09
amogh09 previously approved these changes Nov 30, 2022
Copy link
Contributor

@amogh09 amogh09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the rebase.

@singholt singholt dismissed stale reviews from amogh09 and YashdalfTheGray via 0ddd2c1 December 1, 2022 18:44
@singholt singholt force-pushed the feature/container-port-range branch from 9868c82 to 0ddd2c1 Compare December 1, 2022 18:44
@singholt singholt force-pushed the feature/container-port-range branch from 0ddd2c1 to 66ef2bb Compare December 1, 2022 23:18
Copy link
Contributor

@amogh09 amogh09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. As discussed offline, the loop-back may result in Agent finding overlapping ranges for a single container in some cases (which would result in container failing to start) but that's a risk we are willing to take for the potential benefit of finding contiguous port ranges.

@singholt singholt merged commit 648f96e into aws:feature/container-port-range Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants