Skip to content

Commit

Permalink
Merge pull request #445 from Zyqsempai/419-init-env-vars
Browse files Browse the repository at this point in the history 
Moved all GetEnv's calls to init step
  • Loading branch information
Claes Mogren authored May 28, 2019
2 parents 3e39a2a + fa2b95d commit 73ba53b
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 149 deletions.
88 changes: 40 additions & 48 deletions ipamd/ipamd.go
View file
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 @@ -578,29 +582,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 @@ -819,15 +815,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 @@ -836,10 +832,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 @@ -871,17 +867,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 @@ -905,17 +899,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 @@ -995,7 +988,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 @@ -1088,8 +1081,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 @@ -1098,12 +1090,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
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 {
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

0 comments on commit 73ba53b

Please sign in to comment.