From 3b5d53e91f873d5f697397e09a73e4aa6d4e527a Mon Sep 17 00:00:00 2001 From: bobz965 Date: Mon, 14 Aug 2023 17:50:29 +0800 Subject: [PATCH 1/4] fix ovn nat not clean --- pkg/controller/ovn_dnat.go | 58 +++++++++++++------------------------- pkg/controller/ovn_fip.go | 52 +++++++++++++--------------------- pkg/controller/ovn_snat.go | 55 ++++++++++++++++-------------------- 3 files changed, 62 insertions(+), 103 deletions(-) diff --git a/pkg/controller/ovn_dnat.go b/pkg/controller/ovn_dnat.go index 53e89eee1bd..6e7e3e81f49 100644 --- a/pkg/controller/ovn_dnat.go +++ b/pkg/controller/ovn_dnat.go @@ -26,7 +26,7 @@ func (c *Controller) enqueueAddOvnDnatRule(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue add ovn dnat %s", key) + klog.Infof("enqueue add ovn dnat %s", key) c.addOvnDnatRuleQueue.Add(key) } @@ -40,11 +40,6 @@ func (c *Controller) enqueueUpdateOvnDnatRule(old, new interface{}) { oldDnat := old.(*kubeovnv1.OvnDnatRule) newDnat := new.(*kubeovnv1.OvnDnatRule) - if !newDnat.DeletionTimestamp.IsZero() { - klog.Infof("enqueue del ovn dnat %s", key) - c.delOvnDnatRuleQueue.Add(key) - return - } if oldDnat.Spec.OvnEip != newDnat.Spec.OvnEip { c.resetOvnEipQueue.Add(oldDnat.Spec.OvnEip) @@ -67,8 +62,9 @@ func (c *Controller) enqueueDelOvnDnatRule(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue delete ovn dnat %s", key) - c.delOvnDnatRuleQueue.Add(key) + klog.Infof("enqueue delete ovn dnat %s", key) + dnat := obj.(*kubeovnv1.OvnDnatRule) + c.delOvnDnatRuleQueue.Add(dnat) } func (c *Controller) runAddOvnDnatRuleWorker() { @@ -154,16 +150,16 @@ func (c *Controller) processNextDeleteOvnDnatRuleWorkItem() bool { err := func(obj interface{}) error { defer c.delOvnDnatRuleQueue.Done(obj) - var key string + var dnat *kubeovnv1.OvnDnatRule var ok bool - if key, ok = obj.(string); !ok { + if dnat, ok = obj.(*kubeovnv1.OvnDnatRule); !ok { c.delOvnDnatRuleQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected string in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnDnatRule(key); err != nil { - c.delOvnDnatRuleQueue.AddRateLimited(key) - return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) + if err := c.handleDelOvnDnatRule(dnat); err != nil { + c.delOvnDnatRuleQueue.AddRateLimited(obj) + return fmt.Errorf("error syncing '%s': %s, requeuing", dnat.Name, err.Error()) } c.delOvnDnatRuleQueue.Forget(obj) return nil @@ -212,8 +208,7 @@ func (c *Controller) handleAddOvnDnatRule(key string) error { // already ok return nil } - klog.V(3).Infof("handle add dnat %s", key) - + klog.Infof("handle add dnat %s", key) var internalV4Ip, mac, subnetName string if cachedDnat.Spec.IpType == util.Vip { internalVip, err := c.virtualIpsLister.Get(cachedDnat.Spec.IpName) @@ -315,32 +310,17 @@ func (c *Controller) handleAddOvnDnatRule(key string) error { return nil } -func (c *Controller) handleDelOvnDnatRule(key string) error { - klog.V(3).Infof("handle del ovn dnat %s", key) - cachedDnat, err := c.ovnDnatRulesLister.Get(key) - if err != nil { - if k8serrors.IsNotFound(err) { - return nil - } - klog.Error(err) - return err - } - - eipName := cachedDnat.Spec.OvnEip - if len(eipName) == 0 { - err := fmt.Errorf("failed to create fip rule, should set eip") - klog.Error(err) - return err - } - - if cachedDnat.Status.Vpc != "" && cachedDnat.Status.V4Eip != "" && cachedDnat.Status.ExternalPort != "" { - if err = c.DelDnatRule(cachedDnat.Status.Vpc, cachedDnat.Name, - cachedDnat.Status.V4Eip, cachedDnat.Status.ExternalPort); err != nil { +func (c *Controller) handleDelOvnDnatRule(dnat *kubeovnv1.OvnDnatRule) error { + if dnat.Status.Vpc != "" && dnat.Status.V4Eip != "" && dnat.Status.ExternalPort != "" { + if err := c.DelDnatRule(dnat.Status.Vpc, dnat.Name, + dnat.Status.V4Eip, dnat.Status.ExternalPort); err != nil { klog.Errorf("failed to delete dnat, %v", err) return err } } - c.resetOvnEipQueue.Add(cachedDnat.Spec.OvnEip) + if dnat.Spec.OvnEip != "" { + c.resetOvnEipQueue.Add(dnat.Spec.OvnEip) + } return nil } @@ -354,7 +334,7 @@ func (c *Controller) handleUpdateOvnDnatRule(key string) error { return err } - klog.V(3).Infof("handle update dnat %s", key) + klog.Infof("handle update dnat %s", key) var internalV4Ip, mac, subnetName string if cachedDnat.Spec.IpType == util.Vip { internalVip, err := c.virtualIpsLister.Get(cachedDnat.Spec.IpName) @@ -421,7 +401,7 @@ func (c *Controller) handleUpdateOvnDnatRule(key string) error { dnat := cachedDnat.DeepCopy() if dnat.Status.Ready { - klog.V(3).Infof("dnat change ip, old ip '%s', new ip %s", dnat.Status.V4Ip, cachedEip.Status.V4Ip) + klog.Infof("dnat change ip, old ip '%s', new ip %s", dnat.Status.V4Ip, cachedEip.Status.V4Ip) if err = c.DelDnatRule(dnat.Status.Vpc, dnat.Name, dnat.Status.V4Eip, dnat.Status.ExternalPort); err != nil { klog.Errorf("failed to delete dnat, %v", err) return err diff --git a/pkg/controller/ovn_fip.go b/pkg/controller/ovn_fip.go index 0da5c12f6c1..50e097f578d 100644 --- a/pkg/controller/ovn_fip.go +++ b/pkg/controller/ovn_fip.go @@ -28,7 +28,7 @@ func (c *Controller) enqueueAddOvnFip(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue add ovn fip %s", key) + klog.Infof("enqueue add ovn fip %s", key) c.addOvnFipQueue.Add(key) } @@ -41,11 +41,6 @@ func (c *Controller) enqueueUpdateOvnFip(old, new interface{}) { } oldFip := old.(*kubeovnv1.OvnFip) newFip := new.(*kubeovnv1.OvnFip) - if !newFip.DeletionTimestamp.IsZero() { - klog.V(3).Infof("enqueue del ovn fip %s", key) - c.delOvnFipQueue.Add(key) - return - } if oldFip.Spec.OvnEip != newFip.Spec.OvnEip { // enqueue to reset eip to be clean klog.Infof("enqueue reset old ovn eip %s", oldFip.Spec.OvnEip) @@ -66,8 +61,9 @@ func (c *Controller) enqueueDelOvnFip(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue del ovn fip %s", key) - c.delOvnFipQueue.Add(key) + klog.Infof("enqueue del ovn fip %s", key) + fip := obj.(*kubeovnv1.OvnFip) + c.delOvnFipQueue.Add(fip) } func (c *Controller) runAddOvnFipWorker() { @@ -150,16 +146,16 @@ func (c *Controller) processNextDeleteOvnFipWorkItem() bool { } err := func(obj interface{}) error { defer c.delOvnFipQueue.Done(obj) - var key string + var fip *kubeovnv1.OvnFip var ok bool - if key, ok = obj.(string); !ok { + if fip, ok = obj.(*kubeovnv1.OvnFip); !ok { c.delOvnFipQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected fip in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnFip(key); err != nil { - c.delOvnFipQueue.AddRateLimited(key) - return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) + if err := c.handleDelOvnFip(fip); err != nil { + c.delOvnFipQueue.AddRateLimited(obj) + return fmt.Errorf("error syncing '%s': %s, requeuing", fip.Name, err.Error()) } c.delOvnFipQueue.Forget(obj) return nil @@ -202,7 +198,7 @@ func (c *Controller) handleAddOvnFip(key string) error { // already ok return nil } - klog.V(3).Infof("handle add fip %s", key) + klog.Infof("handle add fip %s", key) var internalV4Ip, mac, subnetName string if cachedFip.Spec.IpType == util.Vip { internalVip, err := c.virtualIpsLister.Get(cachedFip.Spec.IpName) @@ -316,7 +312,7 @@ func (c *Controller) handleUpdateOvnFip(key string) error { } return err } - klog.V(3).Infof("handle update fip %s", key) + klog.Infof("handle update fip %s", key) var internalV4Ip, mac, subnetName string if cachedFip.Spec.IpType == util.Vip { internalVip, err := c.virtualIpsLister.Get(cachedFip.Spec.IpName) @@ -380,7 +376,7 @@ func (c *Controller) handleUpdateOvnFip(key string) error { fip := cachedFip.DeepCopy() // fip change eip if c.ovnFipChangeEip(fip, cachedEip) { - klog.V(3).Infof("fip change ip, old ip '%s', new ip %s", fip.Status.V4Ip, cachedEip.Status.V4Ip) + klog.Infof("fip change ip, old ip '%s', new ip %s", fip.Status.V4Ip, cachedEip.Status.V4Ip) if err = c.ovnClient.DeleteNat(vpcName, ovnnb.NATTypeDNATAndSNAT, fip.Status.V4Ip, internalV4Ip); err != nil { klog.Errorf("failed to create fip, %v", err) return err @@ -410,29 +406,19 @@ func (c *Controller) handleUpdateOvnFip(key string) error { return nil } -func (c *Controller) handleDelOvnFip(key string) error { - klog.V(3).Infof("handle del ovn fip %s", key) - cachedFip, err := c.ovnFipsLister.Get(key) - if err != nil { - if k8serrors.IsNotFound(err) { - return nil - } - klog.Error(err) - return err - } - eipName := cachedFip.Spec.OvnEip - if len(eipName) == 0 { - klog.Errorf("failed to delete ovn fip, should set eip") - } +func (c *Controller) handleDelOvnFip(fip *kubeovnv1.OvnFip) error { + klog.Infof("handle del ovn fip %s", fip.Name) // ovn delete fip - if cachedFip.Status.Vpc != "" && cachedFip.Status.V4Eip != "" && cachedFip.Status.V4Ip != "" { - if err = c.ovnClient.DeleteNat(cachedFip.Status.Vpc, ovnnb.NATTypeDNATAndSNAT, cachedFip.Status.V4Eip, cachedFip.Status.V4Ip); err != nil { + if fip.Status.Vpc != "" && fip.Status.V4Eip != "" && fip.Status.V4Ip != "" { + if err := c.ovnClient.DeleteNat(fip.Status.Vpc, ovnnb.NATTypeDNATAndSNAT, fip.Status.V4Eip, fip.Status.V4Ip); err != nil { klog.Errorf("failed to delete fip, %v", err) return err } } // reset eip - c.resetOvnEipQueue.Add(cachedFip.Spec.OvnEip) + if fip.Spec.OvnEip != "" { + c.resetOvnEipQueue.Add(fip.Spec.OvnEip) + } return nil } diff --git a/pkg/controller/ovn_snat.go b/pkg/controller/ovn_snat.go index 47faa0a55ed..59bbedcd407 100644 --- a/pkg/controller/ovn_snat.go +++ b/pkg/controller/ovn_snat.go @@ -37,11 +37,6 @@ func (c *Controller) enqueueUpdateOvnSnatRule(old, new interface{}) { } oldSnat := old.(*kubeovnv1.OvnSnatRule) newSnat := new.(*kubeovnv1.OvnSnatRule) - if !newSnat.DeletionTimestamp.IsZero() { - klog.V(3).Infof("enqueue reset old ovn eip %s", oldSnat.Spec.OvnEip) - c.updateOvnSnatRuleQueue.Add(key) - return - } if oldSnat.Spec.OvnEip != newSnat.Spec.OvnEip { // enqueue to reset eip to be clean c.resetOvnEipQueue.Add(oldSnat.Spec.OvnEip) @@ -62,8 +57,9 @@ func (c *Controller) enqueueDelOvnSnatRule(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue del ovn snat %s", key) - c.delOvnSnatRuleQueue.Add(key) + klog.Infof("enqueue del ovn snat %s", key) + snat := obj.(*kubeovnv1.OvnSnatRule) + c.delOvnSnatRuleQueue.Add(snat) } func (c *Controller) runAddOvnSnatRuleWorker() { @@ -149,16 +145,16 @@ func (c *Controller) processNextDeleteOvnSnatRuleWorkItem() bool { err := func(obj interface{}) error { defer c.delOvnSnatRuleQueue.Done(obj) - var key string + var snat *kubeovnv1.OvnSnatRule var ok bool - if key, ok = obj.(string); !ok { + if snat, ok = obj.(*kubeovnv1.OvnSnatRule); !ok { c.delOvnSnatRuleQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected snat in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnSnatRule(key); err != nil { - c.delOvnSnatRuleQueue.AddRateLimited(key) - return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) + if err := c.handleDelOvnSnatRule(snat); err != nil { + c.delOvnSnatRuleQueue.AddRateLimited(obj) + return fmt.Errorf("error syncing '%s': %s, requeuing", snat.Name, err.Error()) } c.delOvnSnatRuleQueue.Forget(obj) return nil @@ -184,7 +180,7 @@ func (c *Controller) handleAddOvnSnatRule(key string) error { // already ok return nil } - klog.V(3).Infof("handle add ovn snat %s", key) + klog.Infof("handle add ovn snat %s", key) eipName := cachedSnat.Spec.OvnEip if len(eipName) == 0 { err := fmt.Errorf("failed to create fip rule, should set eip") @@ -278,7 +274,7 @@ func (c *Controller) handleUpdateOvnSnatRule(key string) error { klog.Error(err) return err } - klog.V(3).Infof("handle update ovn snat %s", key) + klog.Infof("handle update ovn snat %s", key) eipName := cachedSnat.Spec.OvnEip if len(eipName) == 0 { err := fmt.Errorf("failed to create fip rule, should set eip") @@ -290,20 +286,6 @@ func (c *Controller) handleUpdateOvnSnatRule(key string) error { klog.Errorf("failed to get eip, %v", err) return err } - // should delete - if !cachedSnat.DeletionTimestamp.IsZero() { - klog.V(3).Infof("ovn delete snat %s", key) - // ovn delete snat - if cachedSnat.Status.Vpc != "" && cachedSnat.Status.V4Eip != "" && cachedSnat.Status.V4IpCidr != "" { - if err = c.ovnClient.DeleteNat(cachedSnat.Status.Vpc, ovnnb.NATTypeSNAT, cachedSnat.Status.V4Eip, cachedSnat.Status.V4IpCidr); err != nil { - klog.Errorf("failed to delete snat, %v", err) - return err - } - } - // reset eip - c.resetOvnEipQueue.Add(cachedSnat.Spec.OvnEip) - return nil - } if cachedEip.Spec.Type == util.Lsp { // eip is using by ecmp nexthop lsp, nat can not use @@ -344,7 +326,7 @@ func (c *Controller) handleUpdateOvnSnatRule(key string) error { } // snat change eip if c.ovnSnatChangeEip(cachedSnat, cachedEip) { - klog.V(3).Infof("snat change ip, old ip %s, new ip %s", cachedEip.Status.V4Ip, cachedEip.Spec.V4Ip) + klog.Infof("snat change ip, old ip %s, new ip %s", cachedEip.Status.V4Ip, cachedEip.Spec.V4Ip) if err = c.ovnClient.DeleteNat(vpcName, ovnnb.NATTypeSNAT, cachedEip.Status.V4Ip, v4IpCidr); err != nil { klog.Errorf("failed to delte snat, %v", err) return err @@ -371,8 +353,19 @@ func (c *Controller) handleUpdateOvnSnatRule(key string) error { return nil } -func (c *Controller) handleDelOvnSnatRule(key string) error { - klog.V(3).Infof("deleted ovn snat %s", key) +func (c *Controller) handleDelOvnSnatRule(snat *kubeovnv1.OvnSnatRule) error { + klog.Infof("deleted ovn snat %s", snat.Name) + // ovn delete snat + if snat.Status.Vpc != "" && snat.Status.V4Eip != "" && snat.Status.V4IpCidr != "" { + if err := c.ovnClient.DeleteNat(snat.Status.Vpc, ovnnb.NATTypeSNAT, snat.Status.V4Eip, snat.Status.V4IpCidr); err != nil { + klog.Errorf("failed to delete snat, %v", err) + return err + } + } + // reset eip + if snat.Spec.OvnEip != "" { + c.resetOvnEipQueue.Add(snat.Spec.OvnEip) + } return nil } From 2b25faff63d405a01089467782b1b5f00c30e50b Mon Sep 17 00:00:00 2001 From: bobz965 Date: Tue, 15 Aug 2023 12:50:01 +0800 Subject: [PATCH 2/4] fix eip nat clean --- pkg/controller/ovn_dnat.go | 95 +++++++++++++++++++++++++++++++------ pkg/controller/ovn_eip.go | 38 +++++++-------- pkg/controller/ovn_fip.go | 97 +++++++++++++++++++++++++++++++------- pkg/controller/ovn_snat.go | 94 ++++++++++++++++++++++++++++++------ pkg/util/const.go | 1 - 5 files changed, 259 insertions(+), 66 deletions(-) diff --git a/pkg/controller/ovn_dnat.go b/pkg/controller/ovn_dnat.go index 6e7e3e81f49..ba618fb9ed0 100644 --- a/pkg/controller/ovn_dnat.go +++ b/pkg/controller/ovn_dnat.go @@ -14,6 +14,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" kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1" "github.com/kubeovn/kube-ovn/pkg/util" @@ -63,8 +64,7 @@ func (c *Controller) enqueueDelOvnDnatRule(obj interface{}) { return } klog.Infof("enqueue delete ovn dnat %s", key) - dnat := obj.(*kubeovnv1.OvnDnatRule) - c.delOvnDnatRuleQueue.Add(dnat) + c.delOvnDnatRuleQueue.Add(key) } func (c *Controller) runAddOvnDnatRuleWorker() { @@ -150,16 +150,16 @@ func (c *Controller) processNextDeleteOvnDnatRuleWorkItem() bool { err := func(obj interface{}) error { defer c.delOvnDnatRuleQueue.Done(obj) - var dnat *kubeovnv1.OvnDnatRule + var key string var ok bool - if dnat, ok = obj.(*kubeovnv1.OvnDnatRule); !ok { + if key, ok = obj.(string); !ok { c.delOvnDnatRuleQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected string in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnDnatRule(dnat); err != nil { - c.delOvnDnatRuleQueue.AddRateLimited(obj) - return fmt.Errorf("error syncing '%s': %s, requeuing", dnat.Name, err.Error()) + if err := c.handleDelOvnDnatRule(key); err != nil { + c.delOvnDnatRuleQueue.AddRateLimited(key) + return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) } c.delOvnDnatRuleQueue.Forget(obj) return nil @@ -274,7 +274,7 @@ func (c *Controller) handleAddOvnDnatRule(key string) error { return err } - if err = c.handleAddOvnEipFinalizer(cachedEip, util.OvnEipFinalizer); err != nil { + if err = c.handleAddOvnEipFinalizer(cachedEip, util.ControllerName); err != nil { klog.Errorf("failed to add finalizer for ovn eip, %v", err) return err } @@ -285,6 +285,11 @@ func (c *Controller) handleAddOvnDnatRule(key string) error { return err } + if err := c.handleAddOvnDnatFinalizer(cachedDnat, util.ControllerName); err != nil { + klog.Errorf("failed to add finalizer for ovn dnat %s, %v", cachedDnat.Name, err) + return err + } + // patch dnat eip relationship if err = c.natLabelAndAnnoOvnEip(eipName, cachedDnat.Name, vpcName); err != nil { klog.Errorf("failed to label dnat '%s' in eip %s, %v", cachedDnat.Name, eipName, err) @@ -310,16 +315,29 @@ func (c *Controller) handleAddOvnDnatRule(key string) error { return nil } -func (c *Controller) handleDelOvnDnatRule(dnat *kubeovnv1.OvnDnatRule) error { - if dnat.Status.Vpc != "" && dnat.Status.V4Eip != "" && dnat.Status.ExternalPort != "" { - if err := c.DelDnatRule(dnat.Status.Vpc, dnat.Name, - dnat.Status.V4Eip, dnat.Status.ExternalPort); err != nil { - klog.Errorf("failed to delete dnat, %v", err) +func (c *Controller) handleDelOvnDnatRule(key string) error { + klog.Infof("handle delete ovn dnat %s", key) + cachedDnat, err := c.ovnDnatRulesLister.Get(key) + if err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Error(err) + return err + } + if cachedDnat.Status.Vpc != "" && cachedDnat.Status.V4Eip != "" && cachedDnat.Status.ExternalPort != "" { + if err = c.DelDnatRule(cachedDnat.Status.Vpc, cachedDnat.Name, + cachedDnat.Status.V4Eip, cachedDnat.Status.ExternalPort); err != nil { + klog.Errorf("failed to delete dnat %s, %v", key, err) return err } } - if dnat.Spec.OvnEip != "" { - c.resetOvnEipQueue.Add(dnat.Spec.OvnEip) + if err = c.handleDelOvnDnatFinalizer(cachedDnat, util.ControllerName); err != nil { + klog.Errorf("failed to remove finalizer for ovn dnat %s, %v", cachedDnat.Name, err) + return err + } + if cachedDnat.Spec.OvnEip != "" { + c.resetOvnEipQueue.Add(cachedDnat.Spec.OvnEip) } return nil } @@ -592,3 +610,50 @@ func (c *Controller) DelDnatRule(vpcName, dnatName, externalIp, externalPort str return nil } + +func (c *Controller) handleAddOvnDnatFinalizer(cachedDnat *kubeovnv1.OvnDnatRule, finalizer string) error { + if cachedDnat.DeletionTimestamp.IsZero() { + if util.ContainsString(cachedDnat.Finalizers, finalizer) { + return nil + } + } + newDnat := cachedDnat.DeepCopy() + controllerutil.AddFinalizer(newDnat, finalizer) + patch, err := util.GenerateMergePatchPayload(cachedDnat, newDnat) + if err != nil { + klog.Errorf("failed to generate patch payload for ovn dnat '%s', %v", cachedDnat.Name, err) + return err + } + if _, err := c.config.KubeOvnClient.KubeovnV1().OvnDnatRules().Patch(context.Background(), cachedDnat.Name, + types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Errorf("failed to add finalizer for ovn dnat '%s', %v", cachedDnat.Name, err) + return err + } + return nil +} + +func (c *Controller) handleDelOvnDnatFinalizer(cachedDnat *kubeovnv1.OvnDnatRule, finalizer string) error { + if len(cachedDnat.Finalizers) == 0 { + return nil + } + var err error + newDnat := cachedDnat.DeepCopy() + controllerutil.RemoveFinalizer(newDnat, finalizer) + patch, err := util.GenerateMergePatchPayload(cachedDnat, newDnat) + if err != nil { + klog.Errorf("failed to generate patch payload for ovn dnat '%s', %v", cachedDnat.Name, err) + return err + } + if _, err := c.config.KubeOvnClient.KubeovnV1().OvnDnatRules().Patch(context.Background(), cachedDnat.Name, + types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Errorf("failed to remove finalizer from ovn dnat '%s', %v", cachedDnat.Name, err) + return err + } + return nil +} diff --git a/pkg/controller/ovn_eip.go b/pkg/controller/ovn_eip.go index d1ce97555a1..67e9b47ea21 100644 --- a/pkg/controller/ovn_eip.go +++ b/pkg/controller/ovn_eip.go @@ -3,7 +3,6 @@ package controller import ( "context" "encoding/json" - "errors" "fmt" "reflect" "strings" @@ -49,7 +48,7 @@ func (c *Controller) enqueueUpdateOvnEip(old, new interface{}) { return } else { klog.Infof("enqueue del ovn eip %s", key) - c.delOvnEipQueue.Add(newEip) + c.delOvnEipQueue.Add(key) return } } @@ -74,8 +73,7 @@ func (c *Controller) enqueueDelOvnEip(obj interface{}) { return } klog.Infof("enqueue del ovn eip %s", key) - eip := obj.(*kubeovnv1.OvnEip) - c.delOvnEipQueue.Add(eip) + c.delOvnEipQueue.Add(key) } func (c *Controller) runAddOvnEipWorker() { @@ -189,16 +187,16 @@ func (c *Controller) processNextDeleteOvnEipWorkItem() bool { } err := func(obj interface{}) error { defer c.delOvnEipQueue.Done(obj) - var eip *kubeovnv1.OvnEip + var key string var ok bool - if eip, ok = obj.(*kubeovnv1.OvnEip); !ok { + if key, ok = obj.(string); !ok { c.delOvnEipQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected ovn eip in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnEip(eip); err != nil { - c.delOvnEipQueue.AddRateLimited(obj) - return fmt.Errorf("error syncing '%s': %s, requeuing", eip.Name, err.Error()) + if err := c.handleDelOvnEip(key); err != nil { + c.delOvnEipQueue.AddRateLimited(key) + return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) } c.delOvnEipQueue.Forget(obj) return nil @@ -285,7 +283,7 @@ func (c *Controller) handleUpdateOvnEip(key string) error { klog.Error(err) return err } - klog.V(3).Infof("handle update ovn eip %s", cachedEip.Name) + klog.Infof("handle update ovn eip %s", cachedEip.Name) if !cachedEip.DeletionTimestamp.IsZero() { subnetName := cachedEip.Spec.ExternalSubnet if subnetName == "" { @@ -332,28 +330,30 @@ func (c *Controller) handleResetOvnEip(key string) error { return nil } -func (c *Controller) handleDelOvnEip(eip *kubeovnv1.OvnEip) error { - klog.V(3).Infof("handle del ovn eip %s", eip.Name) - if len(eip.Finalizers) > 1 { - err := errors.New("eip is referenced, it cannot be deleted directly") - klog.Errorf("failed to delete eip %s, %v", eip.Name, err) +func (c *Controller) handleDelOvnEip(key string) error { + klog.Infof("handle del ovn eip %s", key) + eip, err := c.ovnEipsLister.Get(key) + if err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Error(err) return err } - - if err := c.handleDelOvnEipFinalizer(eip, util.OvnEipFinalizer); err != nil { + if err = c.handleDelOvnEipFinalizer(eip, util.ControllerName); err != nil { klog.Errorf("failed to handle remove ovn eip finalizer , %v", err) return err } if eip.Spec.Type == util.Lsp { - if err := c.ovnClient.DeleteLogicalSwitchPort(eip.Name); err != nil { + if err = c.ovnClient.DeleteLogicalSwitchPort(eip.Name); err != nil { klog.Errorf("failed to delete lsp %s, %v", eip.Name, err) return err } } if eip.Spec.Type == util.Lrp { - if err := c.ovnClient.DeleteLogicalRouterPort(eip.Name); err != nil { + if err = c.ovnClient.DeleteLogicalRouterPort(eip.Name); err != nil { klog.Errorf("failed to delete lrp %s, %v", eip.Name, err) return err } diff --git a/pkg/controller/ovn_fip.go b/pkg/controller/ovn_fip.go index 50e097f578d..fa3ed429276 100644 --- a/pkg/controller/ovn_fip.go +++ b/pkg/controller/ovn_fip.go @@ -14,6 +14,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" kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1" "github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb" @@ -62,8 +63,7 @@ func (c *Controller) enqueueDelOvnFip(obj interface{}) { return } klog.Infof("enqueue del ovn fip %s", key) - fip := obj.(*kubeovnv1.OvnFip) - c.delOvnFipQueue.Add(fip) + c.delOvnFipQueue.Add(key) } func (c *Controller) runAddOvnFipWorker() { @@ -146,16 +146,16 @@ func (c *Controller) processNextDeleteOvnFipWorkItem() bool { } err := func(obj interface{}) error { defer c.delOvnFipQueue.Done(obj) - var fip *kubeovnv1.OvnFip + var key string var ok bool - if fip, ok = obj.(*kubeovnv1.OvnFip); !ok { + if key, ok = obj.(string); !ok { c.delOvnFipQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected fip in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnFip(fip); err != nil { - c.delOvnFipQueue.AddRateLimited(obj) - return fmt.Errorf("error syncing '%s': %s, requeuing", fip.Name, err.Error()) + if err := c.handleDelOvnFip(key); err != nil { + c.delOvnFipQueue.AddRateLimited(key) + return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) } c.delOvnFipQueue.Forget(obj) return nil @@ -272,7 +272,7 @@ func (c *Controller) handleAddOvnFip(key string) error { klog.Errorf("failed to patch status for fip %s, %v", key, err) return err } - if err = c.handleAddOvnEipFinalizer(cachedEip, util.OvnEipFinalizer); err != nil { + if err = c.handleAddOvnEipFinalizer(cachedEip, util.ControllerName); err != nil { klog.Errorf("failed to add finalizer for ovn eip, %v", err) return err } @@ -283,6 +283,12 @@ func (c *Controller) handleAddOvnFip(key string) error { klog.Errorf("failed to create v4 fip, %v", err) return err } + + if err = c.handleAddOvnFipFinalizer(cachedFip, util.ControllerName); err != nil { + klog.Errorf("failed to add finalizer for ovn fip, %v", err) + return err + } + // patch fip eip relationship if err = c.natLabelAndAnnoOvnEip(eipName, cachedFip.Name, vpcName); err != nil { klog.Errorf("failed to label fip '%s' in eip %s, %v", cachedFip.Name, eipName, err) @@ -406,18 +412,30 @@ func (c *Controller) handleUpdateOvnFip(key string) error { return nil } -func (c *Controller) handleDelOvnFip(fip *kubeovnv1.OvnFip) error { - klog.Infof("handle del ovn fip %s", fip.Name) - // ovn delete fip - if fip.Status.Vpc != "" && fip.Status.V4Eip != "" && fip.Status.V4Ip != "" { - if err := c.ovnClient.DeleteNat(fip.Status.Vpc, ovnnb.NATTypeDNATAndSNAT, fip.Status.V4Eip, fip.Status.V4Ip); err != nil { - klog.Errorf("failed to delete fip, %v", err) +func (c *Controller) handleDelOvnFip(key string) error { + klog.Infof("handle del ovn fip %s", key) + cachedFip, err := c.ovnFipsLister.Get(key) + if err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Error(err) + return err + } + // ovn delete fip nat + if cachedFip.Status.Vpc != "" && cachedFip.Status.V4Eip != "" && cachedFip.Status.V4Ip != "" { + if err = c.ovnClient.DeleteNat(cachedFip.Status.Vpc, ovnnb.NATTypeDNATAndSNAT, cachedFip.Status.V4Eip, cachedFip.Status.V4Ip); err != nil { + klog.Errorf("failed to delete fip %s, %v", key, err) return err } } + if err = c.handleDelOvnFipFinalizer(cachedFip, util.ControllerName); err != nil { + klog.Errorf("failed to remove finalizer for ovn fip %s, %v", cachedFip.Name, err) + return err + } // reset eip - if fip.Spec.OvnEip != "" { - c.resetOvnEipQueue.Add(fip.Spec.OvnEip) + if cachedFip.Spec.OvnEip != "" { + c.resetOvnEipQueue.Add(cachedFip.Spec.OvnEip) } return nil } @@ -544,3 +562,50 @@ func (c *Controller) GetOvnEip(eipName string) (*kubeovnv1.OvnEip, error) { } return cachedEip, nil } + +func (c *Controller) handleAddOvnFipFinalizer(cachedFip *kubeovnv1.OvnFip, finalizer string) error { + if cachedFip.DeletionTimestamp.IsZero() { + if util.ContainsString(cachedFip.Finalizers, finalizer) { + return nil + } + } + newFip := cachedFip.DeepCopy() + controllerutil.AddFinalizer(newFip, finalizer) + patch, err := util.GenerateMergePatchPayload(cachedFip, newFip) + if err != nil { + klog.Errorf("failed to generate patch payload for ovn fip '%s', %v", cachedFip.Name, err) + return err + } + if _, err := c.config.KubeOvnClient.KubeovnV1().OvnFips().Patch(context.Background(), cachedFip.Name, + types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Errorf("failed to add finalizer for ovn fip '%s', %v", cachedFip.Name, err) + return err + } + return nil +} + +func (c *Controller) handleDelOvnFipFinalizer(cachedFip *kubeovnv1.OvnFip, finalizer string) error { + if len(cachedFip.Finalizers) == 0 { + return nil + } + var err error + newFip := cachedFip.DeepCopy() + controllerutil.RemoveFinalizer(newFip, finalizer) + patch, err := util.GenerateMergePatchPayload(cachedFip, newFip) + if err != nil { + klog.Errorf("failed to generate patch payload for ovn fip '%s', %v", cachedFip.Name, err) + return err + } + if _, err := c.config.KubeOvnClient.KubeovnV1().OvnFips().Patch(context.Background(), cachedFip.Name, + types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Errorf("failed to remove finalizer from ovn fip '%s', %v", cachedFip.Name, err) + return err + } + return nil +} diff --git a/pkg/controller/ovn_snat.go b/pkg/controller/ovn_snat.go index 59bbedcd407..c8f515244b4 100644 --- a/pkg/controller/ovn_snat.go +++ b/pkg/controller/ovn_snat.go @@ -11,6 +11,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" kubeovnv1 "github.com/kubeovn/kube-ovn/pkg/apis/kubeovn/v1" "github.com/kubeovn/kube-ovn/pkg/ovsdb/ovnnb" @@ -58,8 +59,7 @@ func (c *Controller) enqueueDelOvnSnatRule(obj interface{}) { return } klog.Infof("enqueue del ovn snat %s", key) - snat := obj.(*kubeovnv1.OvnSnatRule) - c.delOvnSnatRuleQueue.Add(snat) + c.delOvnSnatRuleQueue.Add(key) } func (c *Controller) runAddOvnSnatRuleWorker() { @@ -145,16 +145,16 @@ func (c *Controller) processNextDeleteOvnSnatRuleWorkItem() bool { err := func(obj interface{}) error { defer c.delOvnSnatRuleQueue.Done(obj) - var snat *kubeovnv1.OvnSnatRule + var key string var ok bool - if snat, ok = obj.(*kubeovnv1.OvnSnatRule); !ok { + if key, ok = obj.(string); !ok { c.delOvnSnatRuleQueue.Forget(obj) utilruntime.HandleError(fmt.Errorf("expected snat in workqueue but got %#v", obj)) return nil } - if err := c.handleDelOvnSnatRule(snat); err != nil { - c.delOvnSnatRuleQueue.AddRateLimited(obj) - return fmt.Errorf("error syncing '%s': %s, requeuing", snat.Name, err.Error()) + if err := c.handleDelOvnSnatRule(key); err != nil { + c.delOvnSnatRuleQueue.AddRateLimited(key) + return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) } c.delOvnSnatRuleQueue.Forget(obj) return nil @@ -237,7 +237,7 @@ func (c *Controller) handleAddOvnSnatRule(key string) error { } // create snat - if err = c.handleAddOvnEipFinalizer(cachedEip, util.OvnEipFinalizer); err != nil { + if err = c.handleAddOvnEipFinalizer(cachedEip, util.ControllerName); err != nil { klog.Errorf("failed to add finalizer for ovn eip, %v", err) return err } @@ -246,6 +246,10 @@ func (c *Controller) handleAddOvnSnatRule(key string) error { klog.Errorf("failed to create snat, %v", err) return err } + if err := c.handleAddOvnSnatFinalizer(cachedSnat, util.ControllerName); err != nil { + klog.Errorf("failed to add finalizer for ovn snat %s, %v", cachedSnat.Name, err) + return err + } if err = c.natLabelAndAnnoOvnEip(eipName, cachedSnat.Name, vpcName); err != nil { klog.Errorf("failed to label snat '%s' in eip %s, %v", cachedSnat.Name, eipName, err) return err @@ -353,18 +357,31 @@ func (c *Controller) handleUpdateOvnSnatRule(key string) error { return nil } -func (c *Controller) handleDelOvnSnatRule(snat *kubeovnv1.OvnSnatRule) error { - klog.Infof("deleted ovn snat %s", snat.Name) +func (c *Controller) handleDelOvnSnatRule(key string) error { + klog.Infof("handle delete ovn snat %s", key) + cachedSnat, err := c.ovnSnatRulesLister.Get(key) + if err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Error(err) + return err + } // ovn delete snat - if snat.Status.Vpc != "" && snat.Status.V4Eip != "" && snat.Status.V4IpCidr != "" { - if err := c.ovnClient.DeleteNat(snat.Status.Vpc, ovnnb.NATTypeSNAT, snat.Status.V4Eip, snat.Status.V4IpCidr); err != nil { - klog.Errorf("failed to delete snat, %v", err) + if cachedSnat.Status.Vpc != "" && cachedSnat.Status.V4Eip != "" && cachedSnat.Status.V4IpCidr != "" { + if err = c.ovnClient.DeleteNat(cachedSnat.Status.Vpc, ovnnb.NATTypeSNAT, + cachedSnat.Status.V4Eip, cachedSnat.Status.V4IpCidr); err != nil { + klog.Errorf("failed to delete snat %s, %v", key, err) return err } } + if err = c.handleDelOvnSnatFinalizer(cachedSnat, util.ControllerName); err != nil { + klog.Errorf("failed to remove finalizer for ovn snat %s, %v", cachedSnat.Name, err) + return err + } // reset eip - if snat.Spec.OvnEip != "" { - c.resetOvnEipQueue.Add(snat.Spec.OvnEip) + if cachedSnat.Spec.OvnEip != "" { + c.resetOvnEipQueue.Add(cachedSnat.Spec.OvnEip) } return nil } @@ -477,3 +494,50 @@ func (c *Controller) ovnSnatChangeEip(snat *kubeovnv1.OvnSnatRule, eip *kubeovnv } return false } + +func (c *Controller) handleAddOvnSnatFinalizer(cachedSnat *kubeovnv1.OvnSnatRule, finalizer string) error { + if cachedSnat.DeletionTimestamp.IsZero() { + if util.ContainsString(cachedSnat.Finalizers, finalizer) { + return nil + } + } + newSnat := cachedSnat.DeepCopy() + controllerutil.AddFinalizer(newSnat, finalizer) + patch, err := util.GenerateMergePatchPayload(cachedSnat, newSnat) + if err != nil { + klog.Errorf("failed to generate patch payload for ovn snat '%s', %v", cachedSnat.Name, err) + return err + } + if _, err := c.config.KubeOvnClient.KubeovnV1().OvnSnatRules().Patch(context.Background(), cachedSnat.Name, + types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Errorf("failed to add finalizer for ovn snat '%s', %v", cachedSnat.Name, err) + return err + } + return nil +} + +func (c *Controller) handleDelOvnSnatFinalizer(cachedSnat *kubeovnv1.OvnSnatRule, finalizer string) error { + if len(cachedSnat.Finalizers) == 0 { + return nil + } + var err error + newSnat := cachedSnat.DeepCopy() + controllerutil.RemoveFinalizer(newSnat, finalizer) + patch, err := util.GenerateMergePatchPayload(cachedSnat, newSnat) + if err != nil { + klog.Errorf("failed to generate patch payload for ovn snat '%s', %v", cachedSnat.Name, err) + return err + } + if _, err := c.config.KubeOvnClient.KubeovnV1().OvnSnatRules().Patch(context.Background(), cachedSnat.Name, + types.MergePatchType, patch, metav1.PatchOptions{}, ""); err != nil { + if k8serrors.IsNotFound(err) { + return nil + } + klog.Errorf("failed to remove finalizer from ovn snat '%s', %v", cachedSnat.Name, err) + return err + } + return nil +} diff --git a/pkg/util/const.go b/pkg/util/const.go index 83ad8a71a28..1885ef1c94b 100644 --- a/pkg/util/const.go +++ b/pkg/util/const.go @@ -20,7 +20,6 @@ const ( FipNameAnnotation = "ovn.kubernetes.io/fip_name" FipEnableAnnotation = "ovn.kubernetes.io/enable_fip" FipFinalizer = "ovn.kubernetes.io/fip" - OvnEipFinalizer = "ovn.kubernetes.io/ovn_eip" VipAnnotation = "ovn.kubernetes.io/vip" ChassisAnnotation = "ovn.kubernetes.io/chassis" From 1d7c24b16f4e1dd84d5d7a2f0149cd78b4814d59 Mon Sep 17 00:00:00 2001 From: bobz965 Date: Tue, 15 Aug 2023 15:23:53 +0800 Subject: [PATCH 3/4] fix e2e --- pkg/controller/ovn_dnat.go | 9 ++++++--- pkg/controller/ovn_eip.go | 2 +- pkg/controller/ovn_fip.go | 7 ++++++- pkg/controller/ovn_snat.go | 7 ++++++- pkg/controller/vip.go | 6 +++--- test/e2e/ovn-vpc-nat-gw/e2e_test.go | 30 +++++++++++++---------------- 6 files changed, 35 insertions(+), 26 deletions(-) diff --git a/pkg/controller/ovn_dnat.go b/pkg/controller/ovn_dnat.go index ba618fb9ed0..15626271335 100644 --- a/pkg/controller/ovn_dnat.go +++ b/pkg/controller/ovn_dnat.go @@ -38,10 +38,13 @@ func (c *Controller) enqueueUpdateOvnDnatRule(old, new interface{}) { utilruntime.HandleError(err) return } - - oldDnat := old.(*kubeovnv1.OvnDnatRule) newDnat := new.(*kubeovnv1.OvnDnatRule) - + if !newDnat.DeletionTimestamp.IsZero() { + klog.Infof("enqueue del ovn dnat %s", key) + c.delOvnDnatRuleQueue.Add(key) + return + } + oldDnat := old.(*kubeovnv1.OvnDnatRule) if oldDnat.Spec.OvnEip != newDnat.Spec.OvnEip { c.resetOvnEipQueue.Add(oldDnat.Spec.OvnEip) } diff --git a/pkg/controller/ovn_eip.go b/pkg/controller/ovn_eip.go index 67e9b47ea21..ef9d406975e 100644 --- a/pkg/controller/ovn_eip.go +++ b/pkg/controller/ovn_eip.go @@ -40,7 +40,6 @@ func (c *Controller) enqueueUpdateOvnEip(old, new interface{}) { utilruntime.HandleError(err) return } - oldEip := old.(*kubeovnv1.OvnEip) newEip := new.(*kubeovnv1.OvnEip) if newEip.DeletionTimestamp != nil { if len(newEip.Finalizers) == 0 { @@ -52,6 +51,7 @@ func (c *Controller) enqueueUpdateOvnEip(old, new interface{}) { return } } + oldEip := old.(*kubeovnv1.OvnEip) if oldEip.Spec.V4Ip != "" && oldEip.Spec.V4Ip != newEip.Spec.V4Ip || oldEip.Spec.MacAddress != "" && oldEip.Spec.MacAddress != newEip.Spec.MacAddress { klog.Infof("not support change ip or mac for eip %s", key) diff --git a/pkg/controller/ovn_fip.go b/pkg/controller/ovn_fip.go index fa3ed429276..afdbb50e654 100644 --- a/pkg/controller/ovn_fip.go +++ b/pkg/controller/ovn_fip.go @@ -40,8 +40,13 @@ func (c *Controller) enqueueUpdateOvnFip(old, new interface{}) { utilruntime.HandleError(err) return } - oldFip := old.(*kubeovnv1.OvnFip) newFip := new.(*kubeovnv1.OvnFip) + if !newFip.DeletionTimestamp.IsZero() { + klog.Infof("enqueue del ovn fip %s", key) + c.delOvnFipQueue.Add(key) + return + } + oldFip := old.(*kubeovnv1.OvnFip) if oldFip.Spec.OvnEip != newFip.Spec.OvnEip { // enqueue to reset eip to be clean klog.Infof("enqueue reset old ovn eip %s", oldFip.Spec.OvnEip) diff --git a/pkg/controller/ovn_snat.go b/pkg/controller/ovn_snat.go index c8f515244b4..385ee8bde98 100644 --- a/pkg/controller/ovn_snat.go +++ b/pkg/controller/ovn_snat.go @@ -36,8 +36,13 @@ func (c *Controller) enqueueUpdateOvnSnatRule(old, new interface{}) { utilruntime.HandleError(err) return } - oldSnat := old.(*kubeovnv1.OvnSnatRule) newSnat := new.(*kubeovnv1.OvnSnatRule) + if !newSnat.DeletionTimestamp.IsZero() { + klog.Infof("enqueue del ovn snat %s", key) + c.delOvnSnatRuleQueue.Add(key) + return + } + oldSnat := old.(*kubeovnv1.OvnSnatRule) if oldSnat.Spec.OvnEip != newSnat.Spec.OvnEip { // enqueue to reset eip to be clean c.resetOvnEipQueue.Add(oldSnat.Spec.OvnEip) diff --git a/pkg/controller/vip.go b/pkg/controller/vip.go index c47479dd290..c106f1e9f77 100644 --- a/pkg/controller/vip.go +++ b/pkg/controller/vip.go @@ -28,7 +28,7 @@ func (c *Controller) enqueueAddVirtualIp(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue add vip %s", key) + klog.Infof("enqueue add vip %s", key) c.addVirtualIpQueue.Add(key) } @@ -46,7 +46,7 @@ func (c *Controller) enqueueUpdateVirtualIp(old, new interface{}) { oldVip.Spec.ParentMac != newVip.Spec.ParentMac || oldVip.Spec.ParentV4ip != newVip.Spec.ParentV4ip || oldVip.Spec.V4ip != newVip.Spec.V4ip { - klog.V(3).Infof("enqueue update vip %s", key) + klog.Infof("enqueue update vip %s", key) c.updateVirtualIpQueue.Add(key) } } @@ -58,7 +58,7 @@ func (c *Controller) enqueueDelVirtualIp(obj interface{}) { utilruntime.HandleError(err) return } - klog.V(3).Infof("enqueue del vip %s", key) + klog.Infof("enqueue del vip %s", key) vip := obj.(*kubeovnv1.Vip) c.delVirtualIpQueue.Add(vip) } diff --git a/test/e2e/ovn-vpc-nat-gw/e2e_test.go b/test/e2e/ovn-vpc-nat-gw/e2e_test.go index bb807c0800c..c5584e2a856 100644 --- a/test/e2e/ovn-vpc-nat-gw/e2e_test.go +++ b/test/e2e/ovn-vpc-nat-gw/e2e_test.go @@ -307,12 +307,21 @@ var _ = framework.Describe("[group:ovn-vpc-nat-gw]", func() { ovnEipClient.DeleteSync(dnatEipName) ginkgo.By("Deleting ovn eip " + snatEipName) ovnEipClient.DeleteSync(snatEipName) - ginkgo.By("Deleting ovn share eip " + sharedEipName) - ovnEipClient.DeleteSync(sharedEipName) - ginkgo.By("Deleting ovn vip " + arpProxyVip1Name) + ginkgo.By("Deleting ovn arp proxy vip " + arpProxyVip1Name) vipClient.DeleteSync(arpProxyVip1Name) - ginkgo.By("Deleting ovn vip " + arpProxyVip2Name) + ginkgo.By("Deleting ovn arp proxy vip " + arpProxyVip2Name) + + // clean up share eip case resource + ginkgo.By("Deleting share ovn dnat " + sharedEipDnatName) + ovnDnatRuleClient.DeleteSync(sharedEipDnatName) + ginkgo.By("Deleting share ovn fip " + sharedEipFipShoudOkName) + ovnFipClient.DeleteSync(sharedEipFipShoudOkName) + ginkgo.By("Deleting share ovn fip " + sharedEipFipShoudFailName) + ovnFipClient.DeleteSync(sharedEipFipShoudFailName) + ginkgo.By("Deleting share ovn snat " + sharedEipSnatName) + ovnSnatRuleClient.DeleteSync(sharedEipSnatName) + vipClient.DeleteSync(arpProxyVip2Name) ginkgo.By("Deleting ovn vip " + dnatVipName) vipClient.DeleteSync(dnatVipName) @@ -323,16 +332,13 @@ var _ = framework.Describe("[group:ovn-vpc-nat-gw]", func() { ginkgo.By("Deleting subnet " + noBfdSubnetName) subnetClient.DeleteSync(noBfdSubnetName) - ginkgo.By("Deleting subnet " + bfdSubnetName) subnetClient.DeleteSync(bfdSubnetName) - ginkgo.By("Deleting underlay subnet " + underlaySubnetName) subnetClient.DeleteSync(underlaySubnetName) ginkgo.By("Deleting no bfd custom vpc " + noBfdVpcName) vpcClient.DeleteSync(noBfdVpcName) - ginkgo.By("Deleting bfd custom vpc " + bfdVpcName) vpcClient.DeleteSync(bfdVpcName) @@ -489,16 +495,6 @@ var _ = framework.Describe("[group:ovn-vpc-nat-gw]", func() { framework.ExpectContainSubstring(vlanSubnetGw, route.NextHopIP) } - // clean up share eip case resource - ginkgo.By("Deleting share ovn fip " + sharedEipFipShoudOkName) - ovnFipClient.DeleteSync(sharedEipFipShoudOkName) - ginkgo.By("Deleting share ovn fip " + sharedEipFipShoudFailName) - ovnFipClient.DeleteSync(sharedEipFipShoudFailName) - ginkgo.By("Deleting share ovn dnat " + sharedEipDnatName) - ovnDnatRuleClient.DeleteSync(sharedEipDnatName) - ginkgo.By("Deleting share ovn snat " + sharedEipSnatName) - ovnSnatRuleClient.DeleteSync(sharedEipSnatName) - ginkgo.By("2. Creating custom vpc enable external and bfd") for _, nodeName := range nodeNames { ginkgo.By("Creating ovn node-ext-gw type eip on node " + nodeName) From 01b389a31922abbbae487ae06fa359076ee606c7 Mon Sep 17 00:00:00 2001 From: bobz965 Date: Tue, 15 Aug 2023 15:45:04 +0800 Subject: [PATCH 4/4] fix log --- pkg/controller/ovn_dnat.go | 11 ++++++++--- pkg/controller/ovn_fip.go | 11 ++++++++--- pkg/controller/ovn_snat.go | 11 ++++++++--- pkg/controller/vip.go | 2 +- 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/pkg/controller/ovn_dnat.go b/pkg/controller/ovn_dnat.go index 15626271335..34372d30f7f 100644 --- a/pkg/controller/ovn_dnat.go +++ b/pkg/controller/ovn_dnat.go @@ -40,9 +40,14 @@ func (c *Controller) enqueueUpdateOvnDnatRule(old, new interface{}) { } newDnat := new.(*kubeovnv1.OvnDnatRule) if !newDnat.DeletionTimestamp.IsZero() { - klog.Infof("enqueue del ovn dnat %s", key) - c.delOvnDnatRuleQueue.Add(key) - return + if len(newDnat.Finalizers) == 0 { + // avoid delete twice + return + } else { + klog.Infof("enqueue del ovn dnat %s", key) + c.delOvnDnatRuleQueue.Add(key) + return + } } oldDnat := old.(*kubeovnv1.OvnDnatRule) if oldDnat.Spec.OvnEip != newDnat.Spec.OvnEip { diff --git a/pkg/controller/ovn_fip.go b/pkg/controller/ovn_fip.go index afdbb50e654..ff8250bf548 100644 --- a/pkg/controller/ovn_fip.go +++ b/pkg/controller/ovn_fip.go @@ -42,9 +42,14 @@ func (c *Controller) enqueueUpdateOvnFip(old, new interface{}) { } newFip := new.(*kubeovnv1.OvnFip) if !newFip.DeletionTimestamp.IsZero() { - klog.Infof("enqueue del ovn fip %s", key) - c.delOvnFipQueue.Add(key) - return + if len(newFip.Finalizers) == 0 { + // avoid delete twice + return + } else { + klog.Infof("enqueue del ovn fip %s", key) + c.delOvnFipQueue.Add(key) + return + } } oldFip := old.(*kubeovnv1.OvnFip) if oldFip.Spec.OvnEip != newFip.Spec.OvnEip { diff --git a/pkg/controller/ovn_snat.go b/pkg/controller/ovn_snat.go index 385ee8bde98..bacd2d02191 100644 --- a/pkg/controller/ovn_snat.go +++ b/pkg/controller/ovn_snat.go @@ -38,9 +38,14 @@ func (c *Controller) enqueueUpdateOvnSnatRule(old, new interface{}) { } newSnat := new.(*kubeovnv1.OvnSnatRule) if !newSnat.DeletionTimestamp.IsZero() { - klog.Infof("enqueue del ovn snat %s", key) - c.delOvnSnatRuleQueue.Add(key) - return + if len(newSnat.Finalizers) == 0 { + // avoid delete twice + return + } else { + klog.Infof("enqueue del ovn snat %s", key) + c.delOvnSnatRuleQueue.Add(key) + return + } } oldSnat := old.(*kubeovnv1.OvnSnatRule) if oldSnat.Spec.OvnEip != newSnat.Spec.OvnEip { diff --git a/pkg/controller/vip.go b/pkg/controller/vip.go index c106f1e9f77..68d5b01cb43 100644 --- a/pkg/controller/vip.go +++ b/pkg/controller/vip.go @@ -298,7 +298,7 @@ func (c *Controller) handleUpdateVirtualIp(key string) error { } func (c *Controller) handleDelVirtualIp(vip *kubeovnv1.Vip) error { - klog.Infof("delete vip %s", vip.Name) + klog.Infof("handle delete vip %s", vip.Name) // TODO:// clean vip in its parent port aap list if vip.Spec.Type == util.SwitchLBRuleVip { subnet, err := c.subnetsLister.Get(vip.Spec.Subnet)