Attempt to fix ipamd detached ENI issues (#1)

* seems about right

* force removal tests

Author: tatatodd

ipamd/datastore/data_store.go

@@ -78,6 +78,18 @@ var (
 			Help: "The number of IP addresses assigned to pods",
 		},
 	)
+	forceRemovedENIs = prometheus.NewCounter(
+		prometheus.CounterOpts{
+			Name: "awscni_force_removed_enis",
+			Help: "The number of ENIs force removed while they had assigned pods",
+		},
+	)
+	forceRemovedIPs = prometheus.NewCounter(
+		prometheus.CounterOpts{
+			Name: "awscni_force_removed_ips",
+			Help: "The number of IPs force removed while they had assigned pods",
+		},
+	)
 	prometheusRegistered = false
 )
 
@@ -146,6 +158,8 @@ func prometheusRegister() {
 		prometheus.MustRegister(enis)
 		prometheus.MustRegister(totalIPs)
 		prometheus.MustRegister(assignedIPs)
+		prometheus.MustRegister(forceRemovedENIs)
+		prometheus.MustRegister(forceRemovedIPs)
 		prometheusRegistered = true
 	}
 }
@@ -209,7 +223,7 @@ func (ds *DataStore) AddIPv4AddressToStore(eniID string, ipv4 string) error {
 }
 
 // DelIPv4AddressFromStore delete an IP of ENI from datastore
-func (ds *DataStore) DelIPv4AddressFromStore(eniID string, ipv4 string) error {
+func (ds *DataStore) DelIPv4AddressFromStore(eniID string, ipv4 string, force bool) error {
 	ds.lock.Lock()
 	defer ds.lock.Unlock()
 	log.Debugf("Deleting ENI(%s)'s IPv4 address %s from datastore", eniID, ipv4)
@@ -226,7 +240,18 @@ func (ds *DataStore) DelIPv4AddressFromStore(eniID string, ipv4 string) error {
 	}
 
 	if ipAddr.Assigned {
-		return errors.New(IPInUseError)
+		if !force {
+			return errors.New(IPInUseError)
+		}
+		log.Warnf("Force deleting assigned ip %s on eni %s", ipv4, eniID)
+		forceRemovedIPs.Inc()
+		decrementAssignedCount(ds, curENI, ipAddr)
+		for key, info := range ds.podsIP {
+			if info.IP == ipv4 {
+				delete(ds.podsIP, key)
+				break
+			}
+		}
 	}
 
 	ds.total--
@@ -307,6 +332,17 @@ func incrementAssignedCount(ds *DataStore, eni *ENIIPPool, addr *AddressInfo) {
 	assignedIPs.Set(float64(ds.assigned))
 }
 
+func decrementAssignedCount(ds *DataStore, eni *ENIIPPool, addr *AddressInfo) {
+	ds.assigned--
+	eni.AssignedIPv4Addresses--
+	addr.Assigned = false
+	curTime := time.Now()
+	eni.lastUnassignedTime = curTime
+	addr.UnassignedTime = curTime
+	// Prometheus gauge
+	assignedIPs.Set(float64(ds.assigned))
+}
+
 // GetStats returns total number of IP addresses and number of assigned IP addresses
 func (ds *DataStore) GetStats() (int, int) {
 	return ds.total, ds.assigned
@@ -431,7 +467,7 @@ func (ds *DataStore) RemoveUnusedENIFromStore(warmIPTarget int, minimumIPTarget
 }
 
 // RemoveENIFromDataStore removes an ENI from the datastore.  It return nil on success or an error.
-func (ds *DataStore) RemoveENIFromDataStore(eni string) error {
+func (ds *DataStore) RemoveENIFromDataStore(eni string, force bool) error {
 	ds.lock.Lock()
 	defer ds.lock.Unlock()
 
@@ -440,9 +476,26 @@ func (ds *DataStore) RemoveENIFromDataStore(eni string) error {
 		return errors.New(UnknownENIError)
 	}
 
-	// Only unused ENIs can be deleted
-	if eniIPPool.AssignedIPv4Addresses != 0 {
-		return errors.New(ENIInUseError)
+	if eniIPPool.hasPods() {
+		if !force {
+			return errors.New(ENIInUseError)
+		}
+		// This scenario can occur if the reconciliation process discovered this eni was detached
+		// from the EC2 instance outside of the control of ipamd.  If this happens, there's nothing
+		// we can do other than force all pods to be unassigned from the IPs on this eni.
+		log.Warnf("Force removing eni %s with %d assigned pods", eni, eniIPPool.AssignedIPv4Addresses)
+		forceRemovedENIs.Inc()
+		forceRemovedIPs.Add(float64(eniIPPool.AssignedIPv4Addresses))
+		for _, addr := range eniIPPool.IPv4Addresses {
+			if addr.Assigned {
+				decrementAssignedCount(ds, eniIPPool, addr)
+			}
+		}
+		for key, info := range ds.podsIP {
+			if info.DeviceNumber == eniIPPool.DeviceNumber {
+				delete(ds.podsIP, key)
+			}
+		}
 	}
 
 	ds.total -= len(eniIPPool.IPv4Addresses)
@@ -478,13 +531,7 @@ func (ds *DataStore) UnassignPodIPv4Address(k8sPod *k8sapi.K8SPodInfo) (ip strin
 	for _, eni := range ds.eniIPPools {
 		ip, ok := eni.IPv4Addresses[ipAddr.IP]
 		if ok && ip.Assigned {
-			ip.Assigned = false
-			ds.assigned--
-			assignedIPs.Set(float64(ds.assigned))
-			eni.AssignedIPv4Addresses--
-			curTime := time.Now()
-			ip.UnassignedTime = curTime
-			eni.lastUnassignedTime = curTime
+			decrementAssignedCount(ds, eni, ip)
 			log.Infof("UnassignPodIPv4Address: pod (Name: %s, NameSpace %s Sandbox %s)'s ipAddr %s, DeviceNumber%d",
 				k8sPod.Name, k8sPod.Namespace, k8sPod.Sandbox, ip.Address, eni.DeviceNumber)
 			delete(ds.podsIP, podKey)

ipamd/datastore/data_store_test.go

@@ -54,17 +54,37 @@ func TestDeleteENI(t *testing.T) {
 	eniInfos := ds.GetENIInfos()
 	assert.Equal(t, len(eniInfos.ENIIPPools), 3)
 
-	err = ds.RemoveENIFromDataStore("eni-2")
+	err = ds.RemoveENIFromDataStore("eni-2", false)
 	assert.NoError(t, err)
 
 	eniInfos = ds.GetENIInfos()
 	assert.Equal(t, len(eniInfos.ENIIPPools), 2)
 
-	err = ds.RemoveENIFromDataStore("unknown-eni")
+	err = ds.RemoveENIFromDataStore("unknown-eni", false)
 	assert.Error(t, err)
 
 	eniInfos = ds.GetENIInfos()
 	assert.Equal(t, len(eniInfos.ENIIPPools), 2)
+
+	// Add an IP and assign a pod.
+	err = ds.AddIPv4AddressFromStore("eni-1", "1.1.1.1")
+	assert.NoError(t, err)
+	podInfo := &k8sapi.K8SPodInfo{
+		Name:      "pod-1",
+		Namespace: "ns-1",
+		IP:        "1.1.1.1",
+	}
+	ip, device, err := ds.AssignPodIPv4Address(podInfo)
+	assert.NoError(t, err)
+	assert.Equal(t, "1.1.1.1", ip)
+	assert.Equal(t, 1, device)
+
+	// Test force removal.  The first call fails because eni-1 has an IP with a pod assigned to it,
+	// but the second call force-removes it and succeeds.
+	err = ds.RemoveENIFromDataStore("eni-1", false)
+	assert.Error(t, err)
+	err = ds.RemoveENIFromDataStore("eni-1", true)
+	assert.NoError(t, err)
 }
 
 func TestAddENIIPv4Address(t *testing.T) {
@@ -158,16 +178,39 @@ func TestDelENIIPv4Address(t *testing.T) {
 	assert.Equal(t, ds.total, 3)
 	assert.Equal(t, len(ds.eniIPPools["eni-1"].IPv4Addresses), 3)
 
-	err = ds.DelIPv4AddressFromStore("eni-1", "1.1.1.2")
+	err = ds.DelIPv4AddressFromStore("eni-1", "1.1.1.2", false)
 	assert.NoError(t, err)
 	assert.Equal(t, ds.total, 2)
 	assert.Equal(t, len(ds.eniIPPools["eni-1"].IPv4Addresses), 2)
 
 	// delete a unknown IP
-	err = ds.DelIPv4AddressFromStore("eni-1", "10.10.10.10")
+	err = ds.DelIPv4AddressFromStore("eni-1", "10.10.10.10", false)
+	assert.Error(t, err)
+	assert.Equal(t, ds.total, 2)
+	assert.Equal(t, len(ds.eniIPPools["eni-1"].IPv4Addresses), 2)
+
+	// Assign a pod.
+	podInfo := &k8sapi.K8SPodInfo{
+		Name:      "pod-1",
+		Namespace: "ns-1",
+		IP:        "1.1.1.1",
+	}
+	ip, device, err := ds.AssignPodIPv4Address(podInfo)
+	assert.NoError(t, err)
+	assert.Equal(t, "1.1.1.1", ip)
+	assert.Equal(t, 1, device)
+
+	// Test force removal.  The first call fails because the IP has a pod assigned to it, but the
+	// second call force-removes it and succeeds.
+	err = ds.DelIPv4AddressFromStore("eni-1", "1.1.1.1", false)
 	assert.Error(t, err)
 	assert.Equal(t, ds.total, 2)
 	assert.Equal(t, len(ds.eniIPPools["eni-1"].IPv4Addresses), 2)
+
+	err = ds.DelIPv4AddressFromStore("eni-1", "1.1.1.1", true)
+	assert.NoError(t, err)
+	assert.Equal(t, ds.total, 1)
+	assert.Equal(t, len(ds.eniIPPools["eni-1"].IPv4Addresses), 1)
 }
 
 func TestPodIPv4Address(t *testing.T) {

ipamd/ipamd.go

@@ -550,7 +550,10 @@ func (c *IPAMContext) tryUnassignIPsFromAll() {
 			// Delete IPs from datastore
 			var deletedIPs []string
 			for _, toDelete := range ips {
-				err := c.dataStore.DelIPv4AddressFromStore(eniID, toDelete)
+				// Don't force the delete, since a freeable IP might have been assigned to a pod
+				// before we get around to deleting it.
+				const force = false
+				err := c.dataStore.DelIPv4AddressFromStore(eniID, toDelete, force)
 				if err != nil {
 					log.Warnf("Failed to delete IP %s on ENI %s from datastore: %s", toDelete, eniID, err)
 					ipamdErrInc("decreaseIPPool")
@@ -991,7 +994,10 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) {
 	// Sweep phase: since the marked ENI have been removed, the remaining ones needs to be sweeped
 	for eni := range curENIs.ENIIPPools {
 		log.Infof("Reconcile and delete detached ENI %s", eni)
-		err = c.dataStore.RemoveENIFromDataStore(eni)
+		// Force the delete, since aws local metadata has told us that this ENI is no longer
+		// attached, so any IPs assigned from this ENI will no longer work.
+		const force = true
+		err = c.dataStore.RemoveENIFromDataStore(eni, force)
 		if err != nil {
 			log.Errorf("IP pool reconcile: Failed to delete ENI during reconcile: %v", err)
 			ipamdErrInc("eniReconcileDel")
@@ -1063,7 +1069,10 @@ func (c *IPAMContext) eniIPPoolReconcile(ipPool map[string]*datastore.AddressInf
 	// Sweep phase, delete remaining IPs
 	for existingIP := range ipPool {
 		log.Debugf("Reconcile and delete IP %s on ENI %s", existingIP, eni)
-		err := c.dataStore.DelIPv4AddressFromStore(eni, existingIP)
+		// Force the delete, since aws local metadata has told us that this ENI is no longer
+		// attached, so any IPs assigned from this ENI will no longer work.
+		const force = true
+		err := c.dataStore.DelIPv4AddressFromStore(eni, existingIP, force)
 		if err != nil {
 			log.Errorf("Failed to reconcile and delete IP %s on ENI %s, %v", existingIP, eni, err)
 			ipamdErrInc("ipReconcileDel")