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

Moved all GetEnv's calls to init step #445

Merged
merged 1 commit into from
May 28, 2019
Merged

Moved all GetEnv's calls to init step #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 40 additions & 48 deletions ipamd/ipamd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ type IPAMContext struct {

currentMaxAddrsPerENI int
maxAddrsPerENI int
// maxENI is the maximum number of ENIs that can be attached to the instance
maxENI int
warmENITarget int
warmIPTarget int
primaryIP map[string]string
lastNodeIPPoolAction time.Time
lastDecreaseIPPool time.Time
Expand Down Expand Up @@ -234,6 +238,8 @@ func New(k8sapiClient k8sapi.K8SAPIs, eniConfig *eniconfig.ENIConfigController)
}

c.awsClient = client
c.warmENITarget = getWarmENITarget()
c.warmIPTarget = getWarmIPTarget()

err = c.nodeInit()
if err != nil {
Expand All @@ -248,20 +254,18 @@ func (c *IPAMContext) nodeInit() error {
defer ipamdActionsInprogress.WithLabelValues("nodeInit").Sub(float64(1))

log.Debugf("Start node init")

instanceMaxENIs, err := c.awsClient.GetENILimit()
maxENIs, err := c.getMaxENI()
if err != nil {
log.Errorf("Failed to get ENI limit: %s")
return err
}

maxENIs := getMaxENI(instanceMaxENIs)
if maxENIs >= 1 {
enisMax.Set(float64(maxENIs))
c.maxENI = maxENIs
if c.maxENI >= 1 {
enisMax.Set(float64(c.maxENI))
}

maxIPs, err := c.awsClient.GetENIipLimit()
if err == nil {
ipMax.Set(float64(maxIPs * maxENIs))
ipMax.Set(float64(maxIPs * c.maxENI))
}
c.primaryIP = make(map[string]string)
c.reconcileCooldownCache.cache = make(map[string]time.Time)
Expand Down Expand Up @@ -567,29 +571,21 @@ func (c *IPAMContext) increaseIPPool() {
return
}

instanceMaxENIs, err := c.awsClient.GetENILimit()
if err != nil {
log.Errorf("Failed to get ENI limit: %s")
}

// instanceMaxENIs will be 0 if the instance type is unknown. In this case, getMaxENI returns 0 or will use
// MAX_ENI if it is set.
maxENIs := getMaxENI(instanceMaxENIs)
if maxENIs >= 1 {
enisMax.Set(float64(maxENIs))
if c.maxENI == c.dataStore.GetENIs() {
log.Debugf("Skipping increase IP pool due to max ENI already attached to the instance: %d", c.maxENI)
return
}

// Unknown instance type and MAX_ENI is not set
if maxENIs == 0 {
if c.maxENI == 0 {
log.Errorf("Unknown instance type and MAX_ENI is not set. Cannot increase IP pool.")
return
}

if c.dataStore.GetENIs() < maxENIs {
if c.dataStore.GetENIs() < c.maxENI {
c.tryAllocateENI()
c.updateLastNodeIPPoolAction()
} else {
log.Debugf("Skipping ENI allocation due to max ENI already attached to the instance: %d", maxENIs)
log.Debugf("Skipping ENI allocation due to max ENI already attached to the instance: %d", c.maxENI)
}

increasedPool, err := c.tryAssignIPs()
Expand Down Expand Up @@ -808,15 +804,15 @@ func (c *IPAMContext) waitENIAttached(eni string) (awsutils.ENIMetadata, error)
}
}

// getMaxENI returns the maximum number of ENIs for this instance, which is
// the lesser of the given lower bound (for example, the limit for the instance
// type) and a value configured via the MAX_ENI environment variable.
//
// If the value configured via environment variable is 0 or less, it is
// ignored, and the upperBound is returned.
func getMaxENI(upperBound int) int {
// getMaxENI returns the maximum number of ENIs to attach to this instance. This is calculated as the lesser of
// the limit for the instance type and the value configured via the MAX_ENI environment variable. If the value of
// the environment variable is 0 or less, it will be ignored and the maximum for the instance is returned.
func (c *IPAMContext) getMaxENI() (int, error) {
instanceMaxENI, err := c.awsClient.GetENILimit()
if err != nil {
return 0, err
}
inputStr, found := os.LookupEnv(envMaxENI)

envMax := defaultMaxENI
if found {
if input, err := strconv.Atoi(inputStr); err == nil && input >= 1 {
Expand All @@ -825,10 +821,10 @@ func getMaxENI(upperBound int) int {
}
}

if envMax >= 1 && envMax < upperBound {
return envMax
if envMax >= 1 && envMax < instanceMaxENI {
return envMax, nil
}
return upperBound
return instanceMaxENI, nil
}

func getWarmENITarget() int {
Expand Down Expand Up @@ -860,17 +856,15 @@ func (c *IPAMContext) nodeIPPoolTooLow() bool {
return short > 0
}

// If WARM_IP_TARGET not defined fallback using number of ENIs
warmENITarget := getWarmENITarget()
total, used := c.dataStore.GetStats()
logPoolStats(total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI)

available := total - used
poolTooLow := available < c.maxAddrsPerENI*warmENITarget
poolTooLow := available < c.maxAddrsPerENI*c.warmENITarget
if poolTooLow {
log.Debugf("IP pool is too low: available (%d) < ENI target (%d) * addrsPerENI (%d)", available, warmENITarget, c.maxAddrsPerENI)
log.Debugf("IP pool is too low: available (%d) < ENI target (%d) * addrsPerENI (%d)", available, c.warmENITarget, c.maxAddrsPerENI)
} else {
log.Debugf("IP pool is NOT too low: available (%d) >= ENI target (%d) * addrsPerENI (%d)", available, warmENITarget, c.maxAddrsPerENI)
log.Debugf("IP pool is NOT too low: available (%d) >= ENI target (%d) * addrsPerENI (%d)", available, c.warmENITarget, c.maxAddrsPerENI)
}
return poolTooLow
}
Expand All @@ -894,17 +888,16 @@ func (c *IPAMContext) shouldRemoveExtraENIs() bool {
return true
}

warmENITarget := getWarmENITarget()
total, used := c.dataStore.GetStats()
logPoolStats(total, used, c.currentMaxAddrsPerENI, c.maxAddrsPerENI)

available := total - used
// We need the +1 to make sure we are not going below the WARM_ENI_TARGET.
shouldRemoveExtra := available >= (warmENITarget+1)*c.maxAddrsPerENI
shouldRemoveExtra := available >= (c.warmENITarget+1)*c.maxAddrsPerENI
if shouldRemoveExtra {
log.Debugf("It might be possible to remove extra ENIs because available (%d) > ENI target (%d) * addrsPerENI (%d): ", available, warmENITarget, c.maxAddrsPerENI)
log.Debugf("It might be possible to remove extra ENIs because available (%d) > ENI target (%d) * addrsPerENI (%d): ", available, c.warmENITarget, c.maxAddrsPerENI)
} else {
log.Debugf("Its NOT possible to remove extra ENIs because available (%d) <= ENI target (%d) * addrsPerENI (%d): ", available, warmENITarget, c.maxAddrsPerENI)
log.Debugf("Its NOT possible to remove extra ENIs because available (%d) <= ENI target (%d) * addrsPerENI (%d): ", available, c.warmENITarget, c.maxAddrsPerENI)
}
return shouldRemoveExtra
}
Expand Down Expand Up @@ -984,7 +977,7 @@ func (c *IPAMContext) eniIPPoolReconcile(ipPool map[string]*datastore.AddressInf
}

// Check if this IP was recently freed
found, recentlyFreed := c.reconcileCooldownCache.RecentlyFreed(localIP)
found, recentlyFreed := c.reconcileCooldownCache.RecentlyFreed(localIP)
if found {
if recentlyFreed {
log.Debugf("Reconcile skipping IP %s on ENI %s because it was recently unassigned from the ENI.", localIP, eni)
Expand Down Expand Up @@ -1077,8 +1070,7 @@ func getWarmIPTarget() int {

// ipTargetState determines the number of IPs `short` or `over` our WARM_IP_TARGET
func (c *IPAMContext) ipTargetState() (short int, over int, enabled bool) {
target := getWarmIPTarget()
if target == noWarmIPTarget {
if c.warmIPTarget == noWarmIPTarget {
// there is no WARM_IP_TARGET defined, fallback to use all IP addresses on ENI
return 0, 0, false
}
Expand All @@ -1087,12 +1079,12 @@ func (c *IPAMContext) ipTargetState() (short int, over int, enabled bool) {
available := total - assigned

// short is greater than 0 when we have fewer available IPs than the warm IP target
short = max(target-available, 0)
short = max(c.warmIPTarget-available, 0)

// over is the number of available IPs we have beyond the warm IP target
over = max(available-target, 0)
over = max(available-c.warmIPTarget, 0)

log.Debugf("Current warm IP stats: target: %d, total: %d, assigned: %d, available: %d, short: %d, over %d", target, total, assigned, available, short, over)
log.Debugf("Current warm IP stats: target: %d, total: %d, assigned: %d, available: %d, short: %d, over %d", c.warmIPTarget, total, assigned, available, short, over)
return short, over, true
}

Expand Down
103 changes: 2 additions & 101 deletions ipamd/ipamd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func TestNodeInit(t *testing.T) {
}
var cidrs []*string
mockAWS.EXPECT().GetENILimit().Return(4, nil)
mockAWS.EXPECT().GetENIipLimit().Return(56, nil)
mockAWS.EXPECT().GetENIipLimit().Return(14, nil)
mockAWS.EXPECT().GetAttachedENIs().Return([]awsutils.ENIMetadata{eni1, eni2}, nil)
mockAWS.EXPECT().GetVPCIPv4CIDR().Return(vpcCIDR)

Expand Down Expand Up @@ -183,10 +183,6 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool) {

mockContext.dataStore = datastore.NewDataStore()

eni2 := secENIid

mockAWS.EXPECT().GetENILimit().Return(4, nil)

podENIConfig := &v1alpha1.ENIConfigSpec{
SecurityGroups: []string{"sg1-id", "sg2-id"},
Subnet: "subnet1",
Expand All @@ -197,66 +193,7 @@ func testIncreaseIPPool(t *testing.T, useENIConfig bool) {
sg = append(sg, aws.String(sgID))
}

if useENIConfig {
mogren marked this conversation as resolved.
Show resolved Hide resolved
mockENIConfig.EXPECT().MyENIConfig().Return(podENIConfig, nil)
mockAWS.EXPECT().AllocENI(true, sg, podENIConfig.Subnet).Return(eni2, nil)
} else {
mockAWS.EXPECT().AllocENI(false, nil, "").Return(eni2, nil)
}

mockAWS.EXPECT().GetENIipLimit().Return(5, nil)

mockAWS.EXPECT().AllocIPAddresses(eni2, 4)

mockAWS.EXPECT().GetAttachedENIs().Return([]awsutils.ENIMetadata{
{
ENIID: primaryENIid,
MAC: primaryMAC,
DeviceNumber: primaryDevice,
SubnetIPv4CIDR: primarySubnet,
LocalIPv4s: []string{ipaddr01, ipaddr02},
},
{
ENIID: secENIid,
MAC: secMAC,
DeviceNumber: secDevice,
SubnetIPv4CIDR: secSubnet,
LocalIPv4s: []string{ipaddr11, ipaddr12}},
}, nil)

mockAWS.EXPECT().GetENIipLimit().Return(5, nil)
mockAWS.EXPECT().GetPrimaryENI().Return(primaryENIid)

primary := true
notPrimary := false
attachmentID := testAttachmentID
testAddr11 := ipaddr11
testAddr12 := ipaddr12

mockAWS.EXPECT().DescribeENI(eni2).Return(
[]*ec2.NetworkInterfacePrivateIpAddress{
{ PrivateIpAddress: &testAddr11, Primary: &primary },
{ PrivateIpAddress: &testAddr12, Primary: &notPrimary },
},
&attachmentID, nil)

mockAWS.EXPECT().GetPrimaryENI().Return(primaryENIid)
mockNetwork.EXPECT().SetupENINetwork(gomock.Any(), secMAC, secDevice, secSubnet)

// tryAssignIPs()
mockAWS.EXPECT().GetENIipLimit().Return(5, nil)
mockAWS.EXPECT().AllocIPAddresses(eni2, 5)

mockAWS.EXPECT().DescribeENI(eni2).Return(
[]*ec2.NetworkInterfacePrivateIpAddress{
{ PrivateIpAddress: &testAddr11, Primary: &primary },
{ PrivateIpAddress: &testAddr12, Primary: &notPrimary },
{ PrivateIpAddress: &testAddr12, Primary: &notPrimary },
},
&attachmentID, nil)

mockContext.increaseIPPool()

}

func TestNodeIPPoolReconcile(t *testing.T) {
Expand Down Expand Up @@ -347,41 +284,6 @@ func TestGetWarmENITarget(t *testing.T) {
assert.Equal(t, warmIPTarget, noWarmIPTarget)
}

func TestGetMaxENI(t *testing.T) {
ctrl, _, _, _, _, _ := setup(t)
defer ctrl.Finish()

// MaxENI 5 is less than upper bound of 10, so 5
_ = os.Setenv("MAX_ENI", "5")
maxENI := getMaxENI(10)
assert.Equal(t, maxENI, 5)

// MaxENI 5 is greater than upper bound of 4, so 4
_ = os.Setenv("MAX_ENI", "5")
maxENI = getMaxENI(4)
assert.Equal(t, maxENI, 4)

// MaxENI 0 is 0, which means disabled; so use upper bound
_ = os.Setenv("MAX_ENI", "0")
maxENI = getMaxENI(4)
assert.Equal(t, maxENI, 4)

// MaxENI 1 is less than upper bound of 4, so 1.
_ = os.Setenv("MAX_ENI", "1")
maxENI = getMaxENI(4)
assert.Equal(t, maxENI, 1)

// Empty MaxENI means disabled, so use upper bound
_ = os.Unsetenv("MAX_ENI")
maxENI = getMaxENI(10)
assert.Equal(t, maxENI, 10)

// Invalid MaxENI means disabled, so use upper bound
_ = os.Setenv("MAX_ENI", "non-integer-string")
maxENI = getMaxENI(10)
assert.Equal(t, maxENI, 10)
}

func TestGetWarmIPTargetState(t *testing.T) {
ctrl, mockAWS, mockK8S, _, mockNetwork, _ := setup(t)
defer ctrl.Finish()
Expand All @@ -395,11 +297,10 @@ func TestGetWarmIPTargetState(t *testing.T) {

mockContext.dataStore = datastore.NewDataStore()

_ = os.Unsetenv("WARM_IP_TARGET")
_, _, warmIPTargetDefined := mockContext.ipTargetState()
assert.False(t, warmIPTargetDefined)

_ = os.Setenv("WARM_IP_TARGET", "5")
mockContext.warmIPTarget = 5
short, over, warmIPTargetDefined := mockContext.ipTargetState()
assert.True(t, warmIPTargetDefined)
assert.Equal(t, 5, short)
Expand Down