Add config option for number of ENIs get preallocated by liwenwu-amazon · Pull Request #68 · aws/amazon-vpc-cni-k8s

liwenwu-amazon · 2018-05-15T20:05:24Z

Description of changes

Current Behavior
Today, ipamD dynamically allocate and free ENIs based on the number Pods running on the node.
It allocates one more ENI when the number of available IP addresses goes below the per ENI's secondary IP limit. It can free an ENI when the number of available IP addresses grows more than 2 * per ENI's secondary IP limit.

For example, for c3.xlarge, each ENI have 14 secondary IPs. When there is one ENI attached to the instance(e.g. when the instance is first booted) and after 8 pods just get scheduled to run on this node. The available IPs becomes 6 (14-8) and this will trigger ipamD allocating a new ENI.

Requirements
There are 3 different deployment scenarios requires 3 different triggering threshold:

scenario-1: default and same as today's behavior. Let ipamD dynamically allocate and free one ENI at time based on the available IPs
scenario-2: allocate all available ENIs and IPs right after the instance is initialized. Once all ENIs are allocated on all nodes, there will be no delay on Pod deployment (compare to scenario-1, that some pods may have to wait for nodes to get new ENI and IPs)
scenario-3: do NOT allocate any ENI. And only use secondary IPs from instance's primary ENI.

New Behavior
This PR add config option which allow user to define the number of ENIs get allocated. If the config option is not defined, it is default to today's behavior

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

dbenhur

mostly nits, but I'm confused by the threshold. I think we need an issue to make the warm address management driven by desired pool size and move away from eni centric logic.

dbenhur · 2018-05-17T18:32:11Z

ipamd/ipamd.go

 //TODO need to break this function down(comments from CR)
 func (c *IPAMContext) nodeInit() error {
+	ipamdActionsInprogress.WithLabelValues("nodeInit").Add(float64(1))
+	defer ipamdActionsInprogress.WithLabelValues("nodeInit").Sub(float64(1))


Nice. Glad to see we're tracking pending actions now.

dbenhur · 2018-05-17T18:40:20Z

ipamd/ipamd.go

 	}
 	if (c.maxENI > 0) && (c.maxENI == c.dataStore.GetENIs()) {
+		if c.maxENI < maxENIs {
+			errString := "desired: " + strconv.FormatInt(int64(maxENIs), 10) + "current: " + strconv.FormatInt(int64(c.maxENI), 10)


this would be simpler with sprintf. eg:

errString = fmt.sprintf("desired: %d, current: %d", maxENIs, c.maxENI)

dbenhur · 2018-05-17T18:42:21Z

ipamd/ipamd.go

+		total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI, preAllocENIs)

-	return ((total - used) <= c.currentMaxAddrsPerENI)
+	return ((total - used) <= c.currentMaxAddrsPerENI*preAllocENIs)


pls put spaces around * for readability.

dbenhur · 2018-05-17T18:43:53Z

ipamd/ipamd.go

+	preAllocENIs := getPreAllocENILimit()
 	total, used := c.dataStore.GetStats()
+	log.Debugf("IP pool stats: total=%d, used=%d, c.currentMaxAddrsPerENI =%d, c.maxAddrsPerENI = %d, preAllocENIs",
+		total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI, preAllocENIs)


We produce this IP pool stats message in multiple locations. Please extract to a function so it stays consistent everywhere.

dbenhur · 2018-05-17T18:44:16Z

ipamd/ipamd.go

-		total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI)
-
-	return (total-used > 2*c.currentMaxAddrsPerENI)
+	return (total-used > (preAllocENIs+1)*c.currentMaxAddrsPerENI)


add some whitespace

It's hard to understand why this is the threshold. Why does pre allocation effect whether we have too many available addrs other than setting a floor? I still think all this management would be better expressed with a desired address-warm-pool-buffer size, and derive eni requirements from that.

dbenhur · 2018-05-18T00:46:25Z

ipamd/ipamd.go

+	ipMax = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "ip_max",
+			Help: "The number of maximum IPs can be allocated to the instance",


s/number of maximum IPs can/maximum number of IP addresses that can/

please correct as advised. this is user facing description, let's get the grammar right.

These are "private IPv4 addresses". Not "IPs". Even the internal-facing strings/objects should be properly named.

dbenhur · 2018-05-18T00:49:32Z

ipamd/ipamd.go

+}
+
+func logPoolStats(total, used, currentMaxAddrsPerENI, maxAddrsPerENI int) {
+	log.Debugf("IP pool stats: total=%d, used=%d, c.currentMaxAddrsPerENI =%d, c.maxAddrsPerENI = %d",


be consistent on your use of spaces around =

dbenhur · 2018-05-18T00:52:03Z

ipamd/ipamd.go

+	logPoolStats(total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI)

-	return ((total - used) <= c.currentMaxAddrsPerENI)
+	return ((total - used) <= c.currentMaxAddrsPerENI*warmENITarget)


this would be clearer if you said available := total - used then made the comparison against available. add space around ' * '.

go fmt removes spaces around '*'

dbenhur · 2018-05-18T00:52:15Z

ipamd/ipamd.go

-		total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI)
-
-	return (total-used > 2*c.currentMaxAddrsPerENI)
+	return (total-used > (warmENITarget+1)*c.currentMaxAddrsPerENI)


dbenhur · 2018-05-18T00:55:03Z

ipamd/ipamd.go

 	maxENIs, err := c.awsClient.GetENILimit()
 	enisMax.Set(float64(maxENIs))
+	maxIPs, err := c.awsClient.GetENIipLimit()
+	ipMax.Set(float64(maxIPs * int64(maxENIs)))


These can get an error which the code completely ignores.

dbenhur · 2018-05-18T21:15:37Z

ipamd/ipamd.go

+	ipMax = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "ip_max",
+			Help: "The number of maximum IPs can be allocated to the instance",


please correct as advised. this is user facing description, let's get the grammar right.

dbenhur

Ship it!

liwenwu-amazon requested a review from dbenhur May 16, 2018 00:22

dbenhur suggested changes May 17, 2018

View reviewed changes

liwenwu-amazon force-pushed the preallocate-eni branch from e5a9976 to 1d2873c Compare May 17, 2018 22:51

dbenhur suggested changes May 18, 2018

View reviewed changes

liwenwu-amazon force-pushed the preallocate-eni branch from 1d2873c to 26a41ba Compare May 18, 2018 17:19

dbenhur suggested changes May 18, 2018

View reviewed changes

liwenwu-amazon force-pushed the preallocate-eni branch 2 times, most recently from 3960987 to 96d2d7a Compare May 18, 2018 21:37

Add config option for number of warm ENIs get allocated

90c3243

liwenwu-amazon force-pushed the preallocate-eni branch from 96d2d7a to 90c3243 Compare May 18, 2018 21:40

dbenhur approved these changes May 18, 2018

View reviewed changes

liwenwu-amazon merged commit 5d6757f into aws:master May 18, 2018

liwenwu-amazon mentioned this pull request Jul 17, 2018

Add WARM_IP_TARGET support #125

Merged

tvalasek mentioned this pull request Jun 19, 2019

aws-cni on vanilla K8s #508

Closed

Conversation

liwenwu-amazon commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Uh oh!

dbenhur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofiliz May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbenhur left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liwenwu-amazon commented May 15, 2018 •

edited

Loading

ofiliz May 18, 2018 •

edited

Loading