fix: slow vip finalizer operation (#2092)

Co-authored-by: zhangbingbing <zhangbingbing@yealink.com>

dist/images/install.sh

@@ -1190,19 +1190,19 @@ spec:
       additionalPrinterColumns:
       - name: V4IP
         type: string
-        jsonPath: .spec.v4ip
+        jsonPath: .status.v4ip
       - name: PV4IP
         type: string
         jsonPath: .spec.parentV4ip
       - name: Mac
         type: string
-        jsonPath: .spec.macAddress
+        jsonPath: .status.mac
       - name: PMac
         type: string
-        jsonPath: .spec.ParentMac
+        jsonPath: .spec.parentMac
       - name: V6IP
         type: string
-        jsonPath: .spec.v6ip
+        jsonPath: .status.v6ip
       - name: PV6IP
         type: string
         jsonPath: .spec.parentV6ip

pkg/controller/controller.go

@@ -295,9 +295,9 @@ func NewController(config *Configuration) *Controller {
 
 		virtualIpsLister:     virtualIpInformer.Lister(),
 		virtualIpsSynced:     virtualIpInformer.Informer().HasSynced,
-		addVirtualIpQueue:    workqueue.NewNamedRateLimitingQueue(custCrdRateLimiter, "addVirtualIp"),
-		updateVirtualIpQueue: workqueue.NewNamedRateLimitingQueue(custCrdRateLimiter, "updateVirtualIp"),
-		delVirtualIpQueue:    workqueue.NewNamedRateLimitingQueue(custCrdRateLimiter, "delVirtualIp"),
+		addVirtualIpQueue:    workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "addVirtualIp"),
+		updateVirtualIpQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "updateVirtualIp"),
+		delVirtualIpQueue:    workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "delVirtualIp"),
 
 		iptablesEipsLister:     iptablesEipInformer.Lister(),
 		iptablesEipSynced:      iptablesEipInformer.Informer().HasSynced,

pkg/controller/init.go

@@ -404,7 +404,7 @@ func (c *Controller) InitIPAM() error {
 		} else {
 			ipamKey = vip.Name
 		}
-		if _, _, _, err = c.ipam.GetStaticAddress(ipamKey, vip.Name, vip.Spec.V4ip, vip.Spec.MacAddress, vip.Spec.Subnet, false); err != nil {
+		if _, _, _, err = c.ipam.GetStaticAddress(ipamKey, vip.Name, vip.Status.V4ip, vip.Status.Mac, vip.Spec.Subnet, true); err != nil {
 			klog.Errorf("failed to init ipam from vip cr %s: %v", vip.Name, err)
 		}
 	}

pkg/controller/pod.go

@@ -1229,7 +1229,7 @@ func (c *Controller) acquireAddress(pod *v1.Pod, podNet *kubeovnNet) (string, st
 		if err = c.podReuseVip(vipName, portName, isStsPod); err != nil {
 			return "", "", "", podNet.Subnet, err
 		}
-		return vip.Spec.V4ip, vip.Spec.V6ip, vip.Spec.MacAddress, podNet.Subnet, nil
+		return vip.Status.V4ip, vip.Status.V6ip, vip.Status.Mac, podNet.Subnet, nil
 	}
 
 	macStr := pod.Annotations[fmt.Sprintf(util.MacAddressAnnotationTemplate, podNet.ProviderName)]

pkg/controller/vip.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"net"
 	"strings"
-	"time"
 
 	kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1"
 	"github.com/kubeovn/kube-ovn/pkg/ovs"
@@ -17,6 +16,7 @@ import (
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 func (c *Controller) enqueueAddVirtualIp(obj interface{}) {
@@ -183,9 +183,9 @@ func (c *Controller) handleAddVirtualIp(key string) error {
 		// already ok
 		return nil
 	}
+	klog.V(3).Infof("handle add vip %s", key)
 	vip := cachedVip.DeepCopy()
-	klog.V(3).Infof("handle add vip %s", vip.Name)
-	var sourceV4Ip, v4ip, v6ip, mac, subnetName, parentV4ip, parentV6ip, parentMac string
+	var sourceV4Ip, v4ip, v6ip, mac, subnetName string
 	subnetName = vip.Spec.Subnet
 	if subnetName == "" {
 		return fmt.Errorf("failed to create vip '%s', subnet should be set", key)
@@ -205,6 +205,7 @@ func (c *Controller) handleAddVirtualIp(key string) error {
 	if err != nil {
 		return err
 	}
+	var parentV4ip, parentV6ip, parentMac string
 	if vip.Spec.ParentMac != "" {
 		parentV4ip = vip.Spec.ParentV4ip
 		parentV6ip = vip.Spec.ParentV6ip
@@ -214,10 +215,6 @@ func (c *Controller) handleAddVirtualIp(key string) error {
 		klog.Errorf("failed to create or update vip '%s', %v", vip.Name, err)
 		return err
 	}
-	if _, err = c.handleVipFinalizer(vip); err != nil {
-		klog.Errorf("failed to handle vip finalizer, %v", err)
-		return err
-	}
 	if err = c.subnetCountIp(subnet); err != nil {
 		klog.Errorf("failed to count vip '%s' in subnet, %v", vip.Name, err)
 		return err
@@ -237,25 +234,23 @@ func (c *Controller) handleUpdateVirtualIp(key string) error {
 	// should delete
 	if !vip.DeletionTimestamp.IsZero() {
 		// TODO:// clean vip in its parent port aap list
-		// klog.V(3).Infof("todo:// remove this vip '%s' from its parent port aap list", key)
-		v4ip := ""
-		ready := false
-		if err = c.patchVipStatus(key, v4ip, ready); err != nil {
-			return err
-		}
-		vip.Status.Ready = false
-		if _, err = c.handleVipFinalizer(vip); err != nil {
+		if err = c.handleDelVipFinalizer(key); err != nil {
 			klog.Errorf("failed to handle vip finalizer %v", err)
 			return err
 		}
 		return nil
 	}
-	if vip.Status.Mac == "" ||
-		vip.Status.Mac != vip.Spec.MacAddress ||
-		vip.Status.Pmac != vip.Spec.ParentMac {
+	// not support change ip
+	if vip.Status.Mac != "" && vip.Status.Mac != vip.Spec.MacAddress ||
+		vip.Status.V4ip != "" && vip.Status.V4ip != vip.Spec.V4ip ||
+		vip.Status.V6ip != "" && vip.Status.V6ip != vip.Spec.V6ip {
+		err = fmt.Errorf("not support change ip of vip")
+		klog.Errorf("%v", err)
+		return err
+	}
+	// should update
+	if vip.Status.Mac == "" && vip.Spec.MacAddress == "" {
 		// TODO:// add vip in its parent port aap list
-		// klog.V(3).Infof("todo:// add this vip '%s' into its parent port aap list", key)
-
 		if err = c.createOrUpdateCrdVip(key, vip.Namespace, vip.Spec.Subnet,
 			vip.Spec.V6ip, vip.Spec.V6ip, vip.Spec.MacAddress,
 			vip.Spec.ParentV4ip, vip.Spec.ParentV6ip, vip.Spec.MacAddress); err != nil {
@@ -265,11 +260,10 @@ func (c *Controller) handleUpdateVirtualIp(key string) error {
 		if err = c.patchVipStatus(key, vip.Spec.V4ip, ready); err != nil {
 			return err
 		}
-	}
-	vip.Status.Ready = true
-	if _, err = c.handleVipFinalizer(vip); err != nil {
-		klog.Errorf("failed to handle vip finalizer %v", err)
-		return err
+		if err = c.handleAddVipFinalizer(key); err != nil {
+			klog.Errorf("failed to handle vip finalizer %v", err)
+			return err
+		}
 	}
 	return nil
 }
@@ -340,7 +334,7 @@ func (c *Controller) createOrUpdateCrdVip(key, ns, subnet, v4ip, v6ip, mac, pV4i
 	vipCr, err := c.config.KubeOvnClient.KubeovnV1().Vips().Get(context.Background(), key, metav1.GetOptions{})
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
-			_, err := c.config.KubeOvnClient.KubeovnV1().Vips().Create(context.Background(), &kubeovnv1.Vip{
+			if _, err := c.config.KubeOvnClient.KubeovnV1().Vips().Create(context.Background(), &kubeovnv1.Vip{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: key,
 					Labels: map[string]string{
@@ -359,18 +353,7 @@ func (c *Controller) createOrUpdateCrdVip(key, ns, subnet, v4ip, v6ip, mac, pV4i
 					ParentV6ip: pV6ip,
 					ParentMac:  pmac,
 				},
-				Status: kubeovnv1.VipStatus{
-					Ready: true,
-					V4ip:  v4ip,
-					V6ip:  v6ip,
-					Mac:   mac,
-					Pv4ip: pV4ip,
-					Pv6ip: pV6ip,
-					Pmac:  pmac,
-				},
-			}, metav1.CreateOptions{})
-
-			if err != nil {
+			}, metav1.CreateOptions{}); err != nil {
 				errMsg := fmt.Errorf("failed to create crd vip '%s', %v", key, err)
 				klog.Error(errMsg)
 				return errMsg
@@ -384,9 +367,7 @@ func (c *Controller) createOrUpdateCrdVip(key, ns, subnet, v4ip, v6ip, mac, pV4i
 		vip := vipCr.DeepCopy()
 		if vip.Spec.MacAddress == "" && mac != "" {
 			// vip not support to update, just delete and create
-			vip.ObjectMeta.Namespace = ns
 			vip.Spec.Namespace = ns
-			vip.Spec.Subnet = subnet
 			vip.Spec.V4ip = v4ip
 			vip.Spec.V6ip = v6ip
 			vip.Spec.MacAddress = mac
@@ -401,7 +382,6 @@ func (c *Controller) createOrUpdateCrdVip(key, ns, subnet, v4ip, v6ip, mac, pV4i
 			vip.Status.Pv4ip = pV4ip
 			vip.Status.Pv6ip = pV6ip
 			vip.Status.Pmac = pmac
-
 			if _, err := c.config.KubeOvnClient.KubeovnV1().Vips().Update(context.Background(), vip, metav1.UpdateOptions{}); err != nil {
 				errMsg := fmt.Errorf("failed to update vip '%s', %v", key, err)
 				klog.Error(errMsg)
@@ -437,47 +417,6 @@ func (c *Controller) createOrUpdateCrdVip(key, ns, subnet, v4ip, v6ip, mac, pV4i
 	return nil
 }
 
-func (c *Controller) handleVipFinalizer(vip *kubeovnv1.Vip) (bool, error) {
-	if vip.DeletionTimestamp.IsZero() && !util.ContainsString(vip.Finalizers, util.ControllerName) {
-		if len(vip.Finalizers) != 0 {
-			return false, nil
-		}
-		vip.Finalizers = append(vip.Finalizers, util.ControllerName)
-		raw, _ := json.Marshal(vip.Finalizers)
-		patchPayloadTemplate := `[{ "op": "add", "path": "/metadata/finalizers", "value": %s }]`
-		patchPayload := fmt.Sprintf(patchPayloadTemplate, raw)
-		if _, err := c.config.KubeOvnClient.KubeovnV1().Vips().Patch(context.Background(), vip.Name, types.JSONPatchType, []byte(patchPayload), metav1.PatchOptions{}); err != nil {
-			if k8serrors.IsNotFound(err) {
-				return true, nil
-			}
-			klog.Errorf("failed to add finalizer for vip '%s', %v", vip.Name, err)
-			return false, err
-		}
-		// wait local cache ready
-		time.Sleep(2 * time.Second)
-		return false, nil
-	}
-
-	if !vip.DeletionTimestamp.IsZero() && !vip.Status.Ready {
-		if len(vip.Finalizers) == 0 {
-			return true, nil
-		}
-		vip.Finalizers = util.RemoveString(vip.Finalizers, util.ControllerName)
-		raw, _ := json.Marshal(vip.Finalizers)
-		patchPayloadTemplate := `[{ "op": "remove", "path": "/metadata/finalizers", "value": %s }]`
-		patchPayload := fmt.Sprintf(patchPayloadTemplate, raw)
-		if _, err := c.config.KubeOvnClient.KubeovnV1().Vips().Patch(context.Background(), vip.Name, types.JSONPatchType, []byte(patchPayload), metav1.PatchOptions{}); err != nil {
-			if k8serrors.IsNotFound(err) {
-				return true, nil
-			}
-			klog.Errorf("failed to remove finalizer from vip '%s', %v", vip.Name, err)
-			return false, err
-		}
-		return true, nil
-	}
-	return false, nil
-}
-
 func (c *Controller) patchVipStatus(key, v4ip string, ready bool) error {
 	oriVip, err := c.virtualIpsLister.Get(key)
 	if err != nil {
@@ -572,10 +511,70 @@ func (c *Controller) releaseVip(key string) error {
 			klog.Errorf("failed to patch label for vip '%s', %v", vip.Name, err)
 			return err
 		}
-		if _, _, _, err = c.ipam.GetStaticAddress(key, vip.Name, vip.Spec.V4ip, vip.Spec.MacAddress, vip.Spec.Subnet, false); err != nil {
+		if _, _, _, err = c.ipam.GetStaticAddress(key, vip.Name, vip.Status.V4ip, vip.Status.Mac, vip.Spec.Subnet, false); err != nil {
 			klog.Errorf("failed to recover IPAM from VIP CR %s: %v", vip.Name, err)
 		}
 		c.updateSubnetStatusQueue.Add(vip.Spec.Subnet)
 	}
 	return nil
 }
+
+func (c *Controller) handleAddVipFinalizer(key string) error {
+	cachedVip, err := c.virtualIpsLister.Get(key)
+	if err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	if cachedVip.DeletionTimestamp.IsZero() {
+		if util.ContainsString(cachedVip.Finalizers, util.ControllerName) {
+			return nil
+		}
+	}
+	newVip := cachedVip.DeepCopy()
+	controllerutil.AddFinalizer(newVip, util.ControllerName)
+	patch, err := util.GenerateMergePatchPayload(cachedVip, newVip)
+	if err != nil {
+		klog.Errorf("failed to generate patch payload for ovn eip '%s', %v", cachedVip.Name, err)
+		return err
+	}
+	if _, err := c.config.KubeOvnClient.KubeovnV1().Vips().Patch(context.Background(), cachedVip.Name,
+		types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil
+		}
+		klog.Errorf("failed to add finalizer for vip '%s', %v", cachedVip.Name, err)
+		return err
+	}
+	return nil
+}
+
+func (c *Controller) handleDelVipFinalizer(key string) error {
+	cachedVip, err := c.virtualIpsLister.Get(key)
+	if err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	if len(cachedVip.Finalizers) == 0 {
+		return nil
+	}
+	newVip := cachedVip.DeepCopy()
+	controllerutil.RemoveFinalizer(newVip, util.ControllerName)
+	patch, err := util.GenerateMergePatchPayload(cachedVip, newVip)
+	if err != nil {
+		klog.Errorf("failed to generate patch payload for ovn eip '%s', %v", cachedVip.Name, err)
+		return err
+	}
+	if _, err := c.config.KubeOvnClient.KubeovnV1().Vips().Patch(context.Background(), cachedVip.Name,
+		types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil
+		}
+		klog.Errorf("failed to remove finalizer from vip '%s', %v", cachedVip.Name, err)
+		return err
+	}
+	return nil
+}