Repository: spidernet-io/spiderpool (go)

Issue:
IPAM allocation blocked
**Describe the version**
version about:
1. spiderpool: `v0.8.0-rc1`

**Describe the bug**
1:1 allocation case for performance test.

1 IPPool with 1000 IP addresses and create 1 deployment with 1000 replicas. At the end, the IPAM blocked and one Pod can't start correctly with no IP address. But you can notice the IPPool resource already record the pod and allocated its IP. What's more, the pod corresponding SpiderEndpoint not exist.


With the upper description, I image the possibility to reproduce it:
step 1: In the allocation steps, the IPPool allocate IPs correctly.
step 2: Failed to create a SpiderEndpoint object, the spiderpool-agent record this allocation IPs in the memory db.
step3: The spiderpool-agent restart
step4: In the  reallocation, the spiderpool-agent component try to find the previous allocation details in the memory db but find nothing.
step5: The spiderpool-agent component try to allocate a new IP address but found the whole IPPool IPs were allocated.

Notice: For spiderpool-controller GC system, the pod is still exist but can't find the SpiderEndpoint resource. So, the GC scanAll will skip to release the previous allocated IPs.



Pull request description:
fix ipam block with ip address used out
As issue #2517 described, we need try to circulate traversal the whole IPPool allocated IPs to find the previous allocated IP address once the whole IPPool IP addresses used out.

It might reduce performance, but this is the simplest and cause less impaction to solve this problem.

And we only do this when we meet IP addresses used out case. In other case, the spiderpool-agent component still find new free IP address to allocate for the pod and the spiderpool-controller component will use GC scanAll to release the previous IP addresses.


Signed-off-by: Icarus9913 [icaruswu66@qq.com](mailto:icaruswu66@qq.com)

**What this PR does / why we need it**:
fix bug

**Which issue(s) this PR fixes**:
close #2517 



Patch:
diff --git a/pkg/ippoolmanager/ippool_manager.go b/pkg/ippoolmanager/ippool_manager.go
index 0c3bd7dd0..b8e23b2b9 100644
--- a/pkg/ippoolmanager/ippool_manager.go
+++ b/pkg/ippoolmanager/ippool_manager.go
@@ -140,6 +140,20 @@ func (im *ipPoolManager) AllocateIP(ctx context.Context, poolName, nic string, p
 }
 
 func (im *ipPoolManager) genRandomIP(ctx context.Context, nic string, ipPool *spiderpoolv2beta1.SpiderIPPool, pod *corev1.Pod, podController types.PodTopController) (net.IP, error) {
+	logger := logutils.FromContext(ctx)
+
+	var tmpPod *corev1.Pod
+	if im.config.EnableKubevirtStaticIP && podController.APIVersion == kubevirtv1.SchemeGroupVersion.String() && podController.Kind == constant.KindKubevirtVMI {
+		tmpPod = pod.DeepCopy()
+		tmpPod.SetName(podController.Name)
+	} else {
+		tmpPod = pod
+	}
+	key, err := cache.MetaNamespaceKeyFunc(tmpPod)
+	if err != nil {
+		return nil, err
+	}
+
 	reservedIPs, err := im.rIPManager.AssembleReservedIPs(ctx, *ipPool.Spec.IPVersion)
 	if err != nil {
 		return nil, err
@@ -166,22 +180,20 @@ func (im *ipPoolManager) genRandomIP(ctx context.Context, nic string, ipPool *sp
 
 	availableIPs := spiderpoolip.IPsDiffSet(totalIPs, append(reservedIPs, usedIPs...), false)
 	if len(availableIPs) == 0 {
-		return nil, constant.ErrIPUsedOut
-	}
-	resIP := availableIPs[0]
-
-	var tmpPod *corev1.Pod
-	if im.config.EnableKubevirtStaticIP && podController.APIVersion == kubevirtv1.SchemeGroupVersion.String() && podController.Kind == constant.KindKubevirtVMI {
-		tmpPod = pod.DeepCopy()
-		tmpPod.SetName(podController.Name)
-	} else {
-		tmpPod = pod
-	}
+		// traverse the usedIPs to find the previous allocated IPs if there be
+		// reference issue: https://github.com/spidernet-io/spiderpool/issues/2517
+		allocatedIPFromRecords, hasFound := findAllocatedIPFromRecords(allocatedRecords, nic, key, string(pod.UID))
+		if !hasFound {
+			return nil, constant.ErrIPUsedOut
+		}
 
-	key, err := cache.MetaNamespaceKeyFunc(tmpPod)
-	if err != nil {
-		return nil, err
+		availableIPs, err = spiderpoolip.ParseIPRange(*ipPool.Spec.IPVersion, allocatedIPFromRecords)
+		if nil != err {
+			return nil, err
+		}
+		logger.Sugar().Warnf("find previous IP '%s' from IPPool '%s' recorded IP allocations", allocatedIPFromRecords, ipPool.Name)
 	}
+	resIP := availableIPs[0]
 
 	if allocatedRecords == nil {
 		allocatedRecords = spiderpoolv2beta1.PoolIPAllocations{}
diff --git a/pkg/ippoolmanager/utils.go b/pkg/ippoolmanager/utils.go
index 47d9235d2..8ae9ffcd9 100644
--- a/pkg/ippoolmanager/utils.go
+++ b/pkg/ippoolmanager/utils.go
@@ -123,3 +123,17 @@ func (b ByPoolPriority) Less(i, j int) bool {
 
 	return false
 }
+
+// findAllocatedIPFromRecords try to find pod NIC previous allocated IP from the IPPool.Status.AllocatedIPs
+// this function serves for the issue: https://github.com/spidernet-io/spiderpool/issues/2517
+func findAllocatedIPFromRecords(allocatedRecords spiderpoolv2beta1.PoolIPAllocations, nic, namespacedName, podUID string) (previousIP string, hasFound bool) {
+	for tmpIP, poolIPAllocation := range allocatedRecords {
+		if poolIPAllocation.NIC == nic &&
+			poolIPAllocation.NamespacedName == namespacedName &&
+			poolIPAllocation.PodUID == podUID {
+			return tmpIP, true
+		}
+	}
+
+	return "", false
+}


Test patch:
diff --git a/pkg/ippoolmanager/ippool_manager_test.go b/pkg/ippoolmanager/ippool_manager_test.go
index 2415de070..28ef29840 100644
--- a/pkg/ippoolmanager/ippool_manager_test.go
+++ b/pkg/ippoolmanager/ippool_manager_test.go
@@ -5,6 +5,7 @@ package ippoolmanager_test
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net"
 	"sync/atomic"
@@ -19,6 +20,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/uuid"
+	"k8s.io/client-go/tools/cache"
 	"k8s.io/utils/pointer"
 	kubevirtv1 "kubevirt.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -410,6 +412,60 @@ var _ = Describe("IPPoolManager", Label("ippool_manager_test"), func() {
 				Expect(res.Gateway).To(Equal(gateway))
 				Expect(res.Vlan).To(Equal(vlan))
 			})
+
+			It("allocate IP address from the previous records", func() {
+				mockRIPManager.EXPECT().
+					AssembleReservedIPs(gomock.Eq(ctx), gomock.Eq(constant.IPv4)).
+					Return(nil, nil).
+					Times(1)
+
+				ipVersion := constant.IPv4
+				allocatedIP := "172.18.40.40/24"
+				gateway := "172.18.40.1"
+				vlan := int64(0)
+
+				ip, ipNet, err := net.ParseCIDR(allocatedIP)
+				Expect(err).NotTo(HaveOccurred())
+
+				ipPoolT.Spec.IPVersion = pointer.Int64(ipVersion)
+				ipPoolT.Spec.Subnet = ipNet.String()
+				ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs, ip.String())
+				ipPoolT.Spec.Gateway = pointer.String(gateway)
+				ipPoolT.Spec.Vlan = pointer.Int64(vlan)
+
+				key, err := cache.MetaNamespaceKeyFunc(podT)
+				Expect(err).NotTo(HaveOccurred())
+
+				records := spiderpoolv2beta1.PoolIPAllocations{
+					ip.String(): spiderpoolv2beta1.PoolIPAllocation{
+						NIC:            nic,
+						NamespacedName: key,
+						PodUID:         string(podT.UID),
+					},
+				}
+				allocatedIPs, err := json.Marshal(records)
+				Expect(err).NotTo(HaveOccurred())
+
+				ipPoolT.Status = spiderpoolv2beta1.IPPoolStatus{
+					AllocatedIPs:     pointer.String(string(allocatedIPs)),
+					TotalIPCount:     pointer.Int64(1),
+					AllocatedIPCount: pointer.Int64(1),
+				}
+
+				err = fakeClient.Create(ctx, ipPoolT)
+				Expect(err).NotTo(HaveOccurred())
+				err = tracker.Add(ipPoolT)
+				Expect(err).NotTo(HaveOccurred())
+
+				res, err := ipPoolManager.AllocateIP(ctx, ipPoolName, nic, podT, spiderpooltypes.PodTopController{})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(*res.Nic).To(Equal(nic))
+				Expect(*res.Version).To(Equal(ipVersion))
+				Expect(*res.Address).To(Equal(allocatedIP))
+				Expect(res.IPPool).To(Equal(ipPoolT.Name))
+				Expect(res.Gateway).To(Equal(gateway))
+				Expect(res.Vlan).To(Equal(vlan))
+			})
 		})
 
 		Describe("ReleaseIP", func() {
diff --git a/pkg/ippoolmanager/utils_test.go b/pkg/ippoolmanager/utils_test.go
index 4249a65dd..8e517f624 100644
--- a/pkg/ippoolmanager/utils_test.go
+++ b/pkg/ippoolmanager/utils_test.go
@@ -11,6 +11,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	types2 "k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/uuid"
 
 	"github.com/spidernet-io/spiderpool/pkg/constant"
 	spiderpoolv2beta1 "github.com/spidernet-io/spiderpool/pkg/k8s/apis/spiderpool.spidernet.io/v2beta1"
@@ -290,4 +291,48 @@ var _ = Describe("IPPoolManager-utils", Label("ippool_manager_utils"), func() {
 			Expect(byPoolPriority).Should(Equal(ByPoolPriority{pool2, pool1}))
 		})
 	})
+
+	Context("Test findAllocatedIPFromRecords", func() {
+		var allocatedRecords spiderpoolv2beta1.PoolIPAllocations
+		var ip, nic, namespacedName, podUID string
+
+		BeforeEach(func() {
+			ip = "172.18.40.40"
+			nic = "eth0"
+			namespacedName = "default/testPod"
+			podUID = string(uuid.NewUUID())
+			allocatedRecords = spiderpoolv2beta1.PoolIPAllocations{
+				ip: spiderpoolv2beta1.PoolIPAllocation{
+					NIC:            nic,
+					NamespacedName: namespacedName,
+					PodUID:         podUID,
+				},
+			}
+		})
+
+		It("find previous allocated IP in the records", func() {
+			_, hasFound := findAllocatedIPFromRecords(allocatedRecords, nic, namespacedName, podUID)
+			Expect(hasFound).To(BeTrue())
+		})
+
+		It("no previous allocated IP in the records", func() {
+			_, hasFound := findAllocatedIPFromRecords(allocatedRecords, "eth1", "kube-system/testPod1", string(uuid.NewUUID()))
+			Expect(hasFound).To(BeFalse())
+		})
+
+		It("no previous allocated IP in records due to different nic", func() {
+			_, hasFound := findAllocatedIPFromRecords(allocatedRecords, "eth1", namespacedName, podUID)
+			Expect(hasFound).To(BeFalse())
+		})
+
+		It("no previous allocated IP in records due to different NamespacedName", func() {
+			_, hasFound := findAllocatedIPFromRecords(allocatedRecords, nic, "kube-system/testPod1", podUID)
+			Expect(hasFound).To(BeFalse())
+		})
+
+		It("no previous allocated IP in records due to different podUID", func() {
+			_, hasFound := findAllocatedIPFromRecords(allocatedRecords, nic, namespacedName, string(uuid.NewUUID()))
+			Expect(hasFound).To(BeFalse())
+		})
+	})
 })
diff --git a/test/e2e/coordinator/macvlan-overlay-one/macvlan_overlay_one_test.go b/test/e2e/coordinator/macvlan-overlay-one/macvlan_overlay_one_test.go
index 094b7b7ac..6330599f7 100644
--- a/test/e2e/coordinator/macvlan-overlay-one/macvlan_overlay_one_test.go
+++ b/test/e2e/coordinator/macvlan-overlay-one/macvlan_overlay_one_test.go
@@ -272,11 +272,10 @@ var _ = Describe("MacvlanOverlayOne", Label("overlay", "one-nic", "coordinator")
 
 			ctx, cancel := context.WithTimeout(context.Background(), common.PodStartTimeout)
 			defer cancel()
-
 			depObject, err := frame.WaitDeploymentReady(depName, namespace, ctx)
-			Expect(err).NotTo(HaveOccurred(), "waiting for deploy ready failed:  %v ", err)
+			Expect(err).NotTo(HaveOccurred(), "waiting for deploy ready failed, error is: %v ", err)
 			podList, err := frame.GetPodListByLabel(depObject.Spec.Template.Labels)
-			Expect(err).NotTo(HaveOccurred(), "failed to get podList: %v ", err)
+			Expect(err).NotTo(HaveOccurred(), "failed to get podList, error is: %v ", err)
 
 			// Check pod's mac address prefix
 			commandString := fmt.Sprintf("ip link show dev %s | awk '/ether/ {print substr($2,0,5)}'", common.NIC2)
diff --git a/test/e2e/ifacer/ifacer_test.go b/test/e2e/ifacer/ifacer_test.go
index 2e85957c8..e2e0dc03d 100644
--- a/test/e2e/ifacer/ifacer_test.go
+++ b/test/e2e/ifacer/ifacer_test.go
@@ -5,6 +5,7 @@ package ifacer_test
 import (
 	"context"
 	"fmt"
+	"math/rand"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -29,7 +30,8 @@ var _ = Describe("test ifacer", Label("ifacer"), func() {
 		namespace = "ns" + tools.RandomName()
 		spiderMultusNadName = "test-multus-" + common.GenerateString(10, true)
 
-		vlanInterface = 50
+		r := rand.New(rand.NewSource(time.Now().UnixNano()))
+		vlanInterface = r.Intn(100)
 		GinkgoWriter.Println("Generate vlan ID of sub-interface:", vlanInterface)
 
 		err := frame.CreateNamespaceUntilDefaultServiceAccountReady(namespace, common.ServiceAccountReadyTimeout)
@@ -119,7 +121,7 @@ var _ = Describe("test ifacer", Label("ifacer"), func() {
 			for _, node := range frame.Info.KindNodeList {
 				showMasterResult, err := frame.DockerExecCommand(ctx, node, checkMasterUPString)
 				if err != nil {
-					GinkgoWriter.Printf("Failed to execute command %s on the node %s : %v \n", checkMasterUPString, node, showMasterResult)
+					GinkgoWriter.Printf("Failed to execute command on the node %s : %v \n", node, err)
 					return false
 				}
 
@@ -130,7 +132,7 @@ var _ = Describe("test ifacer", Label("ifacer"), func() {
 
 				showResult, err := frame.DockerExecCommand(ctx, node, checkIPLinkString)
 				if err != nil {
-					GinkgoWriter.Printf("Failed to execute %s on the node %s: %v \n", checkIPLinkString, node, showResult)
+					GinkgoWriter.Printf("Failed to execute command on the node %s: %v \n", node, err)
 					return false
 				}
 
@@ -150,7 +152,7 @@ var _ = Describe("test ifacer", Label("ifacer"), func() {
 		setDownString := fmt.Sprintf("ip link set %s.%d down", common.NIC1, vlanInterface)
 		for _, node := range frame.Info.KindNodeList {
 			_, err := frame.DockerExecCommand(ctx, node, setDownString)
-			Expect(err).NotTo(HaveOccurred(), "Failed to execute  %s on the node %s:  %v", setDownString, node, err)
+			Expect(err).NotTo(HaveOccurred(), "Failed to execute down %s on the node %s:  %v", vlanInterface, err)
 		}
 		GinkgoWriter.Println("Restart all pods")
 		podList, err := frame.GetPodListByLabel(dsObject.Spec.Template.Labels)
@@ -182,51 +184,45 @@ var _ = Describe("test ifacer", Label("ifacer"), func() {
 			return true
 		}, common.ResourceDeleteTimeout, common.ForcedWaitingTime).Should(BeTrue())
 
-		// macvlan issue: https://github.com/containernetworking/plugins/pull/954
-		// When the master interface on the node has been deleted, and loadConf tries
-		// to get the MTU, This causes cmdDel to return a linkNotFound error to the
-		// runtime.
-		// TODO(cyclinder): undo the following comment if cni-plugins has new release
-		//	GinkgoWriter.Println("After the host is restarted, the sub-interface is lost. Restarting the Pod will refresh the sub-interface.")
-		//	ctx, cancel = context.WithTimeout(context.Background(), common.ExecCommandTimeout)
-		//	defer cancel()
-		//
-		//	deleteIPLinkString := fmt.Sprintf("ip link delete dev %s.%d", common.NIC1, vlanInterface)
-		//	Eventually(func() bool {
-		//		for _, node := range frame.Info.KindNodeList {
-		//			_, err := frame.DockerExecCommand(ctx, node, deleteIPLinkString)
-		//			Expect(err).NotTo(HaveOccurred(), "Failed to execute the delete sub-interface command on the node %s %v", node, err)
-		//		}
-		//		return true
-		//	}, common.ResourceDeleteTimeout, common.ForcedWaitingTime).Should(BeTrue())
-		//
-		//	GinkgoWriter.Println("After deleting the sub-interface, restart all Pods")
-		//	podList, err = frame.GetPodListByLabel(dsObject.Spec.Template.Labels)
-		//	Expect(err).NotTo(HaveOccurred(), "failed to get Pod list, Pod list is %v", len(podList.Items))
-		//	Expect(frame.DeletePodList(podList)).NotTo(HaveOccurred())
-		//
-		//	GinkgoWriter.Println("The sub-interfaces of all nodes are automatically rebuilt, and the status is UP")
-		//	ctx, cancel = context.WithTimeout(context.Background(), common.ExecCommandTimeout)
-		//	defer cancel()
-		//
-		//	err = frame.WaitPodListRunning(dsObject.Spec.Template.Labels, 2, ctx)
-		//	Expect(err).NotTo(HaveOccurred())
-		//
-		//	checkIPLinkString = fmt.Sprintf("ip link show up %s.%d", common.NIC1, vlanInterface)
-		//	Eventually(func() bool {
-		//		for _, node := range frame.Info.KindNodeList {
-		//			showResult, err := frame.DockerExecCommand(ctx, node, checkIPLinkUpString)
-		//			if err != nil {
-		//				GinkgoWriter.Printf("failed to check if subinterfaces is up on node %s: %v \n", node, err)
-		//				return false
-		//			}
-		//
-		//			if len(showResult) == 0 {
-		//				GinkgoWriter.Printf("sub-vlan interfaces is down, waiting...")
-		//				return false
-		//			}
-		//		}
-		//		return true
-		//	}, common.ResourceDeleteTimeout, common.ForcedWaitingTime).Should(BeTrue())
+		GinkgoWriter.Println("After the host is restarted, the sub-interface is lost. Restarting the Pod will refresh the sub-interface.")
+		ctx, cancel = context.WithTimeout(context.Background(), common.ExecCommandTimeout)
+		defer cancel()
+		deleteIPLinkString := fmt.Sprintf("ip link delete dev %s.%d", common.NIC1, vlanInterface)
+		Eventually(func() bool {
+			for _, node := range frame.Info.KindNodeList {
+				_, err := frame.DockerExecCommand(ctx, node, deleteIPLinkString)
+				Expect(err).NotTo(HaveOccurred(), "Failed to execute the delete sub-interface command on the node %s %v", node, err)
+			}
+			return true
+		}, common.ResourceDeleteTimeout, common.ForcedWaitingTime).Should(BeTrue())
+
+		GinkgoWriter.Println("After deleting the sub-interface, restart all Pods")
+		podList, err = frame.GetPodListByLabel(dsObject.Spec.Template.Labels)
+		Expect(err).NotTo(HaveOccurred(), "failed to get Pod list, Pod list is %v", len(podList.Items))
+		Expect(frame.DeletePodList(podList)).NotTo(HaveOccurred())
+
+		GinkgoWriter.Println("The sub-interfaces of all nodes are automatically rebuilt, and the status is UP")
+		ctx, cancel = context.WithTimeout(context.Background(), common.ExecCommandTimeout)
+		defer cancel()
+
+		err = frame.WaitPodListRunning(dsObject.Spec.Template.Labels, 2, ctx)
+		Expect(err).NotTo(HaveOccurred())
+
+		checkIPLinkString = fmt.Sprintf("ip link show up %s.%d", common.NIC1, vlanInterface)
+		Eventually(func() bool {
+			for _, node := range frame.Info.KindNodeList {
+				showResult, err := frame.DockerExecCommand(ctx, node, checkIPLinkUpString)
+				if err != nil {
+					GinkgoWriter.Printf("failed to check if subinterfaces is up on node %s: %v \n", node, err)
+					return false
+				}
+
+				if len(showResult) == 0 {
+					GinkgoWriter.Printf("sub-vlan interfaces is down, waiting...")
+					return false
+				}
+			}
+			return true
+		}, common.ResourceDeleteTimeout, common.ForcedWaitingTime).Should(BeTrue())
 	})
 })
diff --git a/test/e2e/performance/performance_test.go b/test/e2e/performance/performance_test.go
index 6bcb7967d..58f142345 100644
--- a/test/e2e/performance/performance_test.go
+++ b/test/e2e/performance/performance_test.go
@@ -145,14 +145,15 @@ var _ = Describe("performance test case", Serial, Label("performance"), func() {
 				defer cancel()
 				var curlCheckString string
 				if frame.Info.IpV6Enabled && !frame.Info.IpV4Enabled {
-					curlCheckString = fmt.Sprintf("curl --retry 10 -m 1 -g http://[%s]:80 --insecure", pod.Status.PodIP)
+					curlCheckString = fmt.Sprintf("curl -I -m 1 -g http://[%s]:80 --insecure", pod.Status.PodIP)
 				} else {
-					curlCheckString = fmt.Sprintf("curl --retry 10 -m 1 http://%s:80 --insecure", pod.Status.PodIP)
+					curlCheckString = fmt.Sprintf("curl -I -m 1 http://%s:80 --insecure", pod.Status.PodIP)
 				}
 
 				for _, node := range frame.Info.KindNodeList {
 					curlReturnResult, errCurl := frame.DockerExecCommand(ctx, node, curlCheckString)
-					Expect(errCurl).NotTo(HaveOccurred(), "Failed to execute the command %s on the node:  %v", curlCheckString, string(curlReturnResult))
+					Expect(errCurl).NotTo(HaveOccurred())
+					GinkgoWriter.Printf("Failed to execute the curl command on the node, error is %v", string(curlReturnResult))
 				}
 			}