Enable controller to be aware of number of available IP addresses #31
Conversation
|
|
||
| #go tool vet | ||
| vet: | ||
| go tool vet ./pkg/awsutils | ||
| go tool vet ./plugins/routed-eni | ||
| go tool vet ./pkg/k8sapi | ||
| go tool vet ./pkg/networkutils | ||
| go tool vet controller |
There was a problem hiding this comment.
I don't think this is working. go vet tool works on files. go vet works on packages.
| } | ||
| } | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
Should we split the main() into another file?
| defer log.Flush() | ||
| logger.SetupLogger(logger.GetLogFileLocation(defaultLogFilePath)) | ||
|
|
||
| nodeName := os.Getenv("MY_NODE_NAME") |
There was a problem hiding this comment.
Something more descriptive for the node name K8s_NODE_NAME, KUBERNETES_NODE_NAME, AMZN_CNI_NODE_NAME. Not sure where this comes from but it should be more descriptive than MY.
| log.Infof("Starting eni-ip-controller on %s: %s ...", nodeName, version) | ||
|
|
||
| // creates the in-cluster config | ||
| config, err := rest.InClusterConfig() |
There was a problem hiding this comment.
Should we support options to allow the config to be provided from different places? For instance a config file?
| ) | ||
|
|
||
| // Interface is the interface for vpcipresource | ||
| type Interface interface { |
There was a problem hiding this comment.
I think Interface for vpcipresource reads more cleanly. Interface is the interface for ... seems to stutter a bit.
Also what does the interface actually provide? How is it used.
| Update(client kubernetes.Interface, nodeName string, ipCount int) error | ||
| } | ||
|
|
||
| // VPCIPResource is the obect for VPC IP resource |
There was a problem hiding this comment.
s/obect/object/
This comment is fairly self descriptive. Is there any more insight you can provide for someone not familiar with what VPCIPResource is. What does it do? Why do we need it?
| @@ -0,0 +1,94 @@ | |||
| // Copyright 2017-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| } | ||
| } | ||
|
|
||
| func addNode(nodeStore cache.Store, instanceType string, key string) *v1.Node { |
There was a problem hiding this comment.
This method is not called anywhere.
| return node | ||
| } | ||
|
|
||
| func TestAddNode(t *testing.T) { |
There was a problem hiding this comment.
Can you break the test cases into their own tests.
|
|
||
| c := NewENIIPController(clientset, fakeVPCIP, "dummyNode") | ||
|
|
||
| return &eniIPController{ |
There was a problem hiding this comment.
I am confused as to why we are encapsalating the ENIIPController in the eniIPController in this test file.
| } | ||
|
|
||
| // NewENIIPController creates ENIIPController | ||
| func NewENIIPController(client kubernetes.Interface, vpcIP vpcipresource.Interface, nodeName string) *ENIIPController { |
There was a problem hiding this comment.
Take a look at your coverage. There is a lot of code in this file not covered by test.
|
This PR has amazon-ecs-agent code and some irrelevant aws packages imported - can you do something similiar to #43 and send out new review? |
| return nil | ||
| } | ||
|
|
||
| if strings.Compare(key, c.nodeName) == 0 { |
There was a problem hiding this comment.
This seems wrong. If it is a running tally of IPs, then we should be doing it for all pods that are already running on the node. But its called IP limit, so that means to me it should be a limit, and should have nothing to do with any pods currently running on the instance.
There was a problem hiding this comment.
This assumes eni-controller pod is deployed before any other workload Pods and all other Pods have specified using resource of vpc.amazonaws.com/ipv4 resources.
|
Any progress on this? Definitely need this incorporated so the scheduler is aware of IPs as a resource. I keep running into nodes with not enough available IPs and the scheduler does not try to move the pod to a node with available IPs. |
|
+1 |
|
Any update about this? We have serious problems trying to setup a production quality EKS cluster because of this. |
|
We are working on a solution to this problem. It will be sent out in another PR (no ETA yet but I would say soonish). Going to close this one. |
This PR addresses issue #18 .
It defines vpc.amazonaws.com/ipv4 as a kubernetes's extended resources to represent the number of available IP addresses on each node.
See #26 for more info