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.

Sign up for GitHub

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

Jump to bottom

nodeInit() unnecessarily calls DescribeNetworkInterfaces() IMDS more than once #808

Closed
jaypipes opened this issue Jan 28, 2020 · 2 comments
Closed

nodeInit() unnecessarily calls DescribeNetworkInterfaces() IMDS more than once #808

jaypipes opened this issue Jan 28, 2020 · 2 comments
Assignees
@jaypipes
Labels
bug

Comments

@jaypipes
Copy link
Contributor

nodeInit() - https:/aws/amazon-vpc-cni-k8s/blob/master/ipamd/ipamd.go#L271) function grabs all the ENIs for the worker node, then grabs all pods on the worker node and loops through all the pods setting up iptables rules.

This piece of code: https:/aws/amazon-vpc-cni-k8s/blob/master/ipamd/ipamd.go#L373-L378

can and should be pulled out of that loop since it's not dependent on the contents of the loop iteration. This would eliminate N-1 calls to IMDS to get the subnets associated with the node, where N == #pods
@jaypipes jaypipes added the bug label Jan 28, 2020
@jaypipes
Copy link
Contributor Author

Actually, it's even less efficient than that... We already call GetVPCIPv4CIDRs() on line 305 and throw away the result :(

https:/aws/amazon-vpc-cni-k8s/blob/master/ipamd/ipamd.go#L305

@jaypipes jaypipes self-assigned this Jan 28, 2020
@jaypipes
Copy link
Contributor Author

nodeInit() is making all these calls to DescribeNetworkInterfaces(), but it's not actually the GetVPCIPv4CIDRs() call that is doing it. It's all the callers of IPAMContext.getENIAddresses():

amazon-vpc-cni-k8s/ipamd/ipamd.go

Line 792 in 4bf10a0

func (c *IPAMContext) getENIaddresses(eni string) ([]*ec2.NetworkInterfacePrivateIpAddress, string, error) {

IPAMContext.getENIAddresses() calls the awsClient.DescribeENI() method, which bypasses the "cache" and calls the IMDS's DescribeNetworkInterfaces() API call.
[January 28, 2020, 8:48 AM] Pipes, Jay: getENIAddresses() is called from two places: IPAMContext.tryAssignIPs() and IPAMContext.setupENI(). Both of those locations are called in nodeInit(), with the setupENI() called inside a loop across each ENI for the instance type and tryAssignIPs() doing yet another loop across ENIs for the instance type.

In short, the solution here is to call DescribeNetworkInterfaces() once at the start of nodeInit() and pass the results of that call along to subroutines like setupENI() and tryAssignIPs().

@jaypipes jaypipes changed the title nodeInit() unnecessarily calls GetVPCIPv4CIDRs() for each pod nodeInit() unnecessarily calls DescribeNetworkInterfaces() IMDS more than once Jan 28, 2020
jaypipes added a commit to jaypipes/amazon-vpc-cni-k8s that referenced this issue Jan 28, 2020
@jaypipes
[ipamd] call GetVPCIPv4CIDRs() once in nodeInit
63b30f9 
Even though the call to IPAMContext.awsClient.GetVPCIPv4CIDRs() simply
returns the cached list of CIDRs for the node, IPAMContext.nodeInit()
was unnecessarily calling it N+1 times, where N = #pods on the instance.

This patch changes nodeInit() to call GetVPCIPv4CIDRs() once and then
pass the CIDRs into the inner loops.

Issue aws#808
jaypipes added a commit to jaypipes/amazon-vpc-cni-k8s that referenced this issue Jan 28, 2020
@jaypipes
[ipamd] de-dup calls to DescribeNetworkInterfaces
62d787f 
In IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile(), the
IPAMContext.setupENI() method was being called, passing in an
ENIMetadata struct that contained the "local" IPv4 addresses that had
been queried from IMDS. IPAMContext.setupENI() called the
IPAMContext.getENIAddresses() method, however, which in turn calls the
AWS EC2 DescribeNetworkInterfaces API. The DescribeNetworkInterfaces API
call is expensive and throttled, and so we need to be careful how often
we call it.

In the case of IPAMContext.nodeInit() and
IPAMContext.nodeIPPoolReconcile(), they were both calling
IPAMContext.setupENI() and passing along an ENIMetadata struct that both
methods had previously constructed using the
EC2InstanceMetadataCache.GetAttachedENIs() method, which in turn called
EC2InstanceMetadataCache.getENIMetadata().
EC2InstanceMetadataCache.getENIMetadata() itself ended up calling the
DescribeNetworkInterfaces EC2 API for each ENI on the host machine. So,
the slice of ENIMetadata structs returned from
EC2InstanceMetadataCache.GetAttachedENIs() -- which was being called by
IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile() *already
had the most up-to-date information from the DescribeNetworkInterfaces
EC2 API call*.

Therefore, what this patch does is simply modify the ENIMetadata struct
to store the slice of pointer to ec2.NetworkInterfacePrivateIpAddress
structs that is returned from the DescribeNetworkInterfaces EC2 API
call. That way, for methods like IPAMContext.setupENI(), which passes in
an ENIMetadata struct, we can remove the call to
IPAMContext.getENIAddresses() which cuts down on the number of duplicate
calls to DescribeNetworkInterfaces EC2 API.

Issue aws#808
@jaypipes jaypipes mentioned this issue Jan 28, 2020
Merged
mogren pushed a commit that referenced this issue Jan 29, 2020
@jaypipes @mogren
[ipamd] call GetVPCIPv4CIDRs() once in nodeInit
fb00d8b 
Even though the call to IPAMContext.awsClient.GetVPCIPv4CIDRs() simply
returns the cached list of CIDRs for the node, IPAMContext.nodeInit()
was unnecessarily calling it N+1 times, where N = #pods on the instance.

This patch changes nodeInit() to call GetVPCIPv4CIDRs() once and then
pass the CIDRs into the inner loops.

Issue #808
mogren pushed a commit that referenced this issue Jan 29, 2020
@jaypipes @mogren
[ipamd] de-dup calls to DescribeNetworkInterfaces
30d63c6 
In IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile(), the
IPAMContext.setupENI() method was being called, passing in an
ENIMetadata struct that contained the "local" IPv4 addresses that had
been queried from IMDS. IPAMContext.setupENI() called the
IPAMContext.getENIAddresses() method, however, which in turn calls the
AWS EC2 DescribeNetworkInterfaces API. The DescribeNetworkInterfaces API
call is expensive and throttled, and so we need to be careful how often
we call it.

In the case of IPAMContext.nodeInit() and
IPAMContext.nodeIPPoolReconcile(), they were both calling
IPAMContext.setupENI() and passing along an ENIMetadata struct that both
methods had previously constructed using the
EC2InstanceMetadataCache.GetAttachedENIs() method, which in turn called
EC2InstanceMetadataCache.getENIMetadata().
EC2InstanceMetadataCache.getENIMetadata() itself ended up calling the
DescribeNetworkInterfaces EC2 API for each ENI on the host machine. So,
the slice of ENIMetadata structs returned from
EC2InstanceMetadataCache.GetAttachedENIs() -- which was being called by
IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile() *already
had the most up-to-date information from the DescribeNetworkInterfaces
EC2 API call*.

Therefore, what this patch does is simply modify the ENIMetadata struct
to store the slice of pointer to ec2.NetworkInterfacePrivateIpAddress
structs that is returned from the DescribeNetworkInterfaces EC2 API
call. That way, for methods like IPAMContext.setupENI(), which passes in
an ENIMetadata struct, we can remove the call to
IPAMContext.getENIAddresses() which cuts down on the number of duplicate
calls to DescribeNetworkInterfaces EC2 API.

Issue #808
mogren pushed a commit to mogren/amazon-vpc-cni-k8s that referenced this issue Jan 31, 2020
@jaypipes
[ipamd] call GetVPCIPv4CIDRs() once in nodeInit
714f528 
Even though the call to IPAMContext.awsClient.GetVPCIPv4CIDRs() simply
returns the cached list of CIDRs for the node, IPAMContext.nodeInit()
was unnecessarily calling it N+1 times, where N = #pods on the instance.

This patch changes nodeInit() to call GetVPCIPv4CIDRs() once and then
pass the CIDRs into the inner loops.

Issue aws#808

(cherry picked from commit fb00d8b)
mogren pushed a commit to mogren/amazon-vpc-cni-k8s that referenced this issue Jan 31, 2020
@jaypipes
[ipamd] de-dup calls to DescribeNetworkInterfaces
e3fc3f7 
In IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile(), the
IPAMContext.setupENI() method was being called, passing in an
ENIMetadata struct that contained the "local" IPv4 addresses that had
been queried from IMDS. IPAMContext.setupENI() called the
IPAMContext.getENIAddresses() method, however, which in turn calls the
AWS EC2 DescribeNetworkInterfaces API. The DescribeNetworkInterfaces API
call is expensive and throttled, and so we need to be careful how often
we call it.

In the case of IPAMContext.nodeInit() and
IPAMContext.nodeIPPoolReconcile(), they were both calling
IPAMContext.setupENI() and passing along an ENIMetadata struct that both
methods had previously constructed using the
EC2InstanceMetadataCache.GetAttachedENIs() method, which in turn called
EC2InstanceMetadataCache.getENIMetadata().
EC2InstanceMetadataCache.getENIMetadata() itself ended up calling the
DescribeNetworkInterfaces EC2 API for each ENI on the host machine. So,
the slice of ENIMetadata structs returned from
EC2InstanceMetadataCache.GetAttachedENIs() -- which was being called by
IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile() *already
had the most up-to-date information from the DescribeNetworkInterfaces
EC2 API call*.

Therefore, what this patch does is simply modify the ENIMetadata struct
to store the slice of pointer to ec2.NetworkInterfacePrivateIpAddress
structs that is returned from the DescribeNetworkInterfaces EC2 API
call. That way, for methods like IPAMContext.setupENI(), which passes in
an ENIMetadata struct, we can remove the call to
IPAMContext.getENIAddresses() which cuts down on the number of duplicate
calls to DescribeNetworkInterfaces EC2 API.

Issue aws#808

(cherry picked from commit 30d63c6)
jaypipes added a commit that referenced this issue Jan 31, 2020
@jaypipes
[ipamd] call GetVPCIPv4CIDRs() once in nodeInit
75d8fcc 
Even though the call to IPAMContext.awsClient.GetVPCIPv4CIDRs() simply
returns the cached list of CIDRs for the node, IPAMContext.nodeInit()
was unnecessarily calling it N+1 times, where N = #pods on the instance.

This patch changes nodeInit() to call GetVPCIPv4CIDRs() once and then
pass the CIDRs into the inner loops.

Issue #808

(cherry picked from commit fb00d8b)
jaypipes added a commit that referenced this issue Jan 31, 2020
@jaypipes
[ipamd] de-dup calls to DescribeNetworkInterfaces
a5dfb55 
In IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile(), the
IPAMContext.setupENI() method was being called, passing in an
ENIMetadata struct that contained the "local" IPv4 addresses that had
been queried from IMDS. IPAMContext.setupENI() called the
IPAMContext.getENIAddresses() method, however, which in turn calls the
AWS EC2 DescribeNetworkInterfaces API. The DescribeNetworkInterfaces API
call is expensive and throttled, and so we need to be careful how often
we call it.

In the case of IPAMContext.nodeInit() and
IPAMContext.nodeIPPoolReconcile(), they were both calling
IPAMContext.setupENI() and passing along an ENIMetadata struct that both
methods had previously constructed using the
EC2InstanceMetadataCache.GetAttachedENIs() method, which in turn called
EC2InstanceMetadataCache.getENIMetadata().
EC2InstanceMetadataCache.getENIMetadata() itself ended up calling the
DescribeNetworkInterfaces EC2 API for each ENI on the host machine. So,
the slice of ENIMetadata structs returned from
EC2InstanceMetadataCache.GetAttachedENIs() -- which was being called by
IPAMContext.nodeInit() and IPAMContext.nodeIPPoolReconcile() *already
had the most up-to-date information from the DescribeNetworkInterfaces
EC2 API call*.

Therefore, what this patch does is simply modify the ENIMetadata struct
to store the slice of pointer to ec2.NetworkInterfacePrivateIpAddress
structs that is returned from the DescribeNetworkInterfaces EC2 API
call. That way, for methods like IPAMContext.setupENI(), which passes in
an ENIMetadata struct, we can remove the call to
IPAMContext.getENIAddresses() which cuts down on the number of duplicate
calls to DescribeNetworkInterfaces EC2 API.

Issue #808

(cherry picked from commit 30d63c6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jaypipes jaypipes

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@jaypipes