Improve WARM_IP_TARGET handling

config/v1.3/aws-k8s-cni.yaml

@@ -69,7 +69,7 @@ spec:
       tolerations:
       - operator: Exists
       containers:
-      - image: 602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni:v1.3.3
+      - image: 602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni:v1.3.4
         imagePullPolicy: Always
         ports:
         - containerPort: 61678
@@ -126,5 +126,3 @@ spec:
     plural: eniconfigs
     singular: eniconfig
     kind: ENIConfig
-
-

config/v1.4/calico.yaml

@@ -62,6 +62,10 @@ spec:
             # Set Felix endpoint to host default action to ACCEPT.
             - name: FELIX_DEFAULTENDPOINTTOHOSTACTION
               value: "ACCEPT"
+            # This will make Felix honor AWS VPC CNI's mangle table
+            # rules.
+            - name: FELIX_IPTABLESMANGLEALLOWACTION
+              value: Return
             # Disable IPV6 on Kubernetes.
             - name: FELIX_IPV6SUPPORT
               value: "false"
@@ -86,9 +90,6 @@ spec:
               value: "true"
           securityContext:
             privileged: true
-          resources:
-            requests:
-              cpu: 250m
           livenessProbe:
             httpGet:
               path: /liveness
@@ -451,6 +452,10 @@ spec:
               value: "1"
             - name: TYPHA_HEALTHENABLED
               value: "true"
+            # This will make Felix honor AWS VPC CNI's mangle table
+            # rules.
+            - name: FELIX_IPTABLESMANGLEALLOWACTION
+              value: Return
           livenessProbe:
             exec:
               command:

ipamd/datastore/data_store.go

@@ -319,21 +319,42 @@ func (ds *DataStore) GetStats() (int, int) {
 	return ds.total, ds.assigned
 }
 
-func (ds *DataStore) getDeletableENI() *ENIIPPool {
+// IsRequiredForWarmIPTarget determines if this ENI has warm IPs that are required to fulfill whatever WARM_IP_TARGET is
+// set to.
+func (ds *DataStore) isRequiredForWarmIPTarget(warmIPTarget int, eni *ENIIPPool) bool {
+	otherWarmIPs := 0
+	for _, other := range ds.eniIPPools {
+		if other.ID != eni.ID {
+			otherWarmIPs += len(other.IPv4Addresses) - other.AssignedIPv4Addresses
+		}
+	}
+	return otherWarmIPs < warmIPTarget
+}
+
+func (ds *DataStore) getDeletableENI(warmIPTarget int) *ENIIPPool {
 	for _, eni := range ds.eniIPPools {
 		if eni.IsPrimary {
+			log.Debugf("ENI %s cannot be deleted because it is primary", eni.ID)
+			continue
+		}
+
+		if eni.isTooYoung() {
+			log.Debugf("ENI %s cannot be deleted because it is too young", eni.ID)
 			continue
 		}
 
-		if time.Now().Sub(eni.createTime) < minLifeTime {
+		if eni.hasIPInCooling() {
+			log.Debugf("ENI %s cannot be deleted because has IPs in cooling", eni.ID)
 			continue
 		}
 
-		if time.Now().Sub(eni.lastUnassignedTime) < addressENICoolingPeriod {
+		if eni.hasPods() {
+			log.Debugf("ENI %s cannot be deleted because it has pods assigned", eni.ID)
 			continue
 		}
 
-		if eni.AssignedIPv4Addresses != 0 {
+		if warmIPTarget != 0 && ds.isRequiredForWarmIPTarget(warmIPTarget, eni) {
+			log.Debugf("ENI %s cannot be deleted because it is required for WARM_IP_TARGET: %d", eni.ID, warmIPTarget)
 			continue
 		}
 
@@ -343,6 +364,21 @@ func (ds *DataStore) getDeletableENI() *ENIIPPool {
 	return nil
 }
 
+// IsTooYoung returns true if the ENI hasn't been around long enough to be deleted.
+func (e *ENIIPPool) isTooYoung() bool {
+	return time.Now().Sub(e.createTime) < minLifeTime
+}
+
+// HasIPInCooling returns true if an IP address was unassigned recently.
+func (e *ENIIPPool) hasIPInCooling() bool {
+	return time.Now().Sub(e.lastUnassignedTime) < addressENICoolingPeriod
+}
+
+// HasPods returns true if the ENI has pods assigned to it.
+func (e *ENIIPPool) hasPods() bool {
+	return e.AssignedIPv4Addresses != 0
+}
+
 // GetENINeedsIP finds an ENI in the datastore that needs more IP addresses allocated
 func (ds *DataStore) GetENINeedsIP(maxIPperENI int, skipPrimary bool) *ENIIPPool {
 	for _, eni := range ds.eniIPPools {
@@ -362,11 +398,11 @@ func (ds *DataStore) GetENINeedsIP(maxIPperENI int, skipPrimary bool) *ENIIPPool
 // RemoveUnusedENIFromStore removes a deletable ENI from the data store.
 // It returns the name of the ENI which has been removed from the data store and needs to be deleted,
 // or empty string if no ENI could be removed.
-func (ds *DataStore) RemoveUnusedENIFromStore() string {
+func (ds *DataStore) RemoveUnusedENIFromStore(warmIPTarget int) string {
 	ds.lock.Lock()
 	defer ds.lock.Unlock()
 
-	deletableENI := ds.getDeletableENI()
+	deletableENI := ds.getDeletableENI(warmIPTarget)
 	if deletableENI == nil {
 		log.Debugf("No ENI can be deleted at this time")
 		return ""

ipamd/datastore/data_store_test.go

@@ -287,13 +287,15 @@ func TestPodIPv4Address(t *testing.T) {
 	assert.Equal(t, len(ds.eniIPPools["eni-2"].IPv4Addresses), 1)
 	assert.Equal(t, ds.eniIPPools["eni-2"].AssignedIPv4Addresses, 0)
 
+	noWarmIPTarget := 0
+
 	// should not able to free this eni
-	eni := ds.RemoveUnusedENIFromStore()
+	eni := ds.RemoveUnusedENIFromStore(noWarmIPTarget)
 	assert.True(t, eni == "")
 
 	ds.eniIPPools["eni-2"].createTime = time.Time{}
 	ds.eniIPPools["eni-2"].lastUnassignedTime = time.Time{}
-	eni = ds.RemoveUnusedENIFromStore()
+	eni = ds.RemoveUnusedENIFromStore(noWarmIPTarget)
 	assert.Equal(t, eni, "eni-2")
 
 	assert.Equal(t, ds.total, 2)

ipamd/introspect.go

@@ -70,7 +70,6 @@ func (c *IPAMContext) ServeIntrospection() {
 }
 
 func (c *IPAMContext) setupIntrospectionServer() *http.Server {
-	// If enabled, add introspection endpoints
 	serverFunctions := map[string]func(w http.ResponseWriter, r *http.Request){
 		"/v1/enis":                      eniV1RequestHandler(c),
 		"/v1/eni-configs":               eniConfigRequestHandler(c),
@@ -87,7 +86,7 @@ func (c *IPAMContext) setupIntrospectionServer() *http.Server {
 	availableCommandResponse, err := json.Marshal(&availableCommands)
 
 	if err != nil {
-		log.Error("Failed to marshal: %v", err)
+		log.Errorf("Failed to marshal: %v", err)
 	}
 
 	defaultHandler := func(w http.ResponseWriter, r *http.Request) {