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

Enable controller to be aware of number of available IP addresses #23

Closed

Conversation

liwenwu-amazon
Copy link
Contributor

This PR addresses issue #18 .

It defines eks.amazon.com/ip as a kubernetes' extended resources to represent the number of available of available IP addresses on each node.

@mattlandis
Copy link
Contributor

This plugin is usable outside of an eks cluster so the resource should not be scoped to eks. Maybe vpc-cni.amazon.com/ip or vpc-cni.aws.amazon.com/ip?

@aaithal
Copy link

aaithal commented Jan 30, 2018

I second @mattlandis comment here. Also, if you have 3611 files in a single commit, it's impossible for anyone to review it effectively.

@ofiliz
Copy link

ofiliz commented Jan 30, 2018

I am not sure if this resource would ever make sense outside of EKS. If we name it generically, we risk confusing people who do not need it into thinking they do. This resource exists only because a specific implementation detail of vpc-cni-k8s plugin requires it. Other CNI plugins (at least as of today) do not need it.

Also, the name of the resource should be something like "private-ipv4-address". "ip" is the name of the protocol, and even if it is commonly (and incorrectly) understood to be an address, let's be clear on what type of address it really is.

@bmoylan
Copy link

bmoylan commented Jan 30, 2018

We deploy a custom-configured Kubernetes cluster on EC2 and would love to have this feature generically available instead of tied directly to EKS.

@liwenwu-amazon
Copy link
Contributor Author

how about vpc.amazonaws.com/ip?

@aaithal
Copy link

aaithal commented Jan 30, 2018

how about vpc.amazonaws.com/ipv4 to state explicitly that this is for ipv4 addresses?

Copy link

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Please do at least the following:

  • Break the dep/vendor changes into a separate commit so they can be reviewed separately
  • Run dep prune to ensure you're only vendoring packages that are actually used

@@ -21,13 +21,17 @@
# version = "2.4.0"


[[constraint]]
name = "github.com/aws/amazon-ecs-agent"

Choose a reason for hiding this comment

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

Why did you vendor the ECS agent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, it uses some code from amazon-ecs-agent and amazon-ecs-cni-plugins. We are planning to move these shared code out into another library repo and only vendor this new library repo.

Choose a reason for hiding this comment

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

What code are you using? It's worth evaluating whether to factor it out or to just stop using the code; not everything in the ECS agent is code that we'd recommend reusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened issue #27 to track this. Thanks

revision = "ea58fddf8462034ff2034aa9788b02341d7702e3"
version = "v1.15.2"
revision = "ea383f2ff70290029ee69c7089160b10941baeb6"
version = "v1.16.1"

[[projects]]
branch = "master"

Choose a reason for hiding this comment

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

Why did you vendor github.com/aws/amazon-ecs-cni-plugins?

@liwenwu-amazon
Copy link
Contributor Author

Move this to a new PR #31 where it is broken down into multiple commit

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.

6 participants