From 8b58b87e3f41c645f84a5824da43415a1bd2da3e Mon Sep 17 00:00:00 2001 From: Tao Zou Date: Tue, 8 Oct 2024 10:12:30 +0800 Subject: [PATCH] Remove staticroute finalizer The finalizer may block the deletion since the NSX resource may take long time to delete. Remove the finalizer so the deleting operation will return immediately --- .../staticroute/staticroute_controller.go | 93 +++++----- .../staticroute_controller_test.go | 162 +++++++++++++----- pkg/nsx/services/common/types.go | 1 - pkg/nsx/services/staticroute/staticroute.go | 14 ++ pkg/nsx/services/staticroute/store.go | 15 +- 5 files changed, 202 insertions(+), 83 deletions(-) diff --git a/pkg/controllers/staticroute/staticroute_controller.go b/pkg/controllers/staticroute/staticroute_controller.go index 265ff29ad..b1855d5e8 100644 --- a/pkg/controllers/staticroute/staticroute_controller.go +++ b/pkg/controllers/staticroute/staticroute_controller.go @@ -11,6 +11,7 @@ import ( "strings" v1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apimachineryruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" @@ -18,9 +19,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1" @@ -72,28 +70,64 @@ func deleteSuccess(r *StaticRouteReconciler, _ context.Context, o *v1alpha1.Stat metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteSuccessTotal, common.MetricResTypeStaticRoute) } +func (r *StaticRouteReconciler) listStaticRouteCRIDs() (sets.Set[string], error) { + staticRouteList := &v1alpha1.StaticRouteList{} + err := r.Client.List(context.Background(), staticRouteList) + if err != nil { + log.Error(err, "failed to list StaticRoute CRs") + return nil, err + } + + CRStaticRouteSet := sets.New[string]() + for _, staticroute := range staticRouteList.Items { + CRStaticRouteSet.Insert(string(staticroute.UID)) + } + return CRStaticRouteSet, nil +} + +func (r *StaticRouteReconciler) deleteStaticRouteByName(ns, name string) error { + CRPolicySet, err := r.listStaticRouteCRIDs() + if err != nil { + return err + } + nsxStaticRoutes := r.Service.ListStaticRouteByName(ns, name) + for _, item := range nsxStaticRoutes { + uid := util.FindTag(item.Tags, commonservice.TagScopeStaticRouteCRUID) + if CRPolicySet.Has(uid) { + log.Info("skipping deletion, StaticRoute CR still exists in K8s", "staticrouteUID", uid, "nsxStatciRouteId", *item.Id) + continue + } + + log.Info("deleting StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id) + path := strings.Split(*item.Path, "/") + if err := r.Service.DeleteStaticRouteByPath(path[2], path[4], path[6], *item.Id); err != nil { + log.Error(err, "failed to delete StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id) + return err + } + log.Info("successfully deleted StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id) + } + return nil +} + func (r *StaticRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { obj := &v1alpha1.StaticRoute{} log.Info("reconciling staticroute CR", "staticroute", req.NamespacedName) metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerSyncTotal, common.MetricResTypeStaticRoute) if err := r.Client.Get(ctx, req.NamespacedName, obj); err != nil { + if apierrors.IsNotFound(err) { + if err := r.deleteStaticRouteByName(req.Namespace, req.Name); err != nil { + return ResultRequeue, err + } else { + return ResultNormal, nil + } + } log.Error(err, "unable to fetch static route CR", "req", req.NamespacedName) - return ResultNormal, client.IgnoreNotFound(err) + return ResultRequeue, err } if obj.ObjectMeta.DeletionTimestamp.IsZero() { metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, common.MetricResTypeStaticRoute) - if !controllerutil.ContainsFinalizer(obj, commonservice.StaticRouteFinalizerName) { - controllerutil.AddFinalizer(obj, commonservice.StaticRouteFinalizerName) - if err := r.Client.Update(ctx, obj); err != nil { - log.Error(err, "add finalizer", "staticroute", req.NamespacedName) - updateFail(r, ctx, obj, &err) - return ResultRequeue, err - } - log.V(1).Info("added finalizer on staticroute CR", "staticroute", req.NamespacedName) - } - if err := r.Service.CreateOrUpdateStaticRoute(req.Namespace, obj); err != nil { updateFail(r, ctx, obj, &err) // TODO: if error is not retriable, not requeue @@ -105,27 +139,14 @@ func (r *StaticRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) } updateSuccess(r, ctx, obj) } else { - if controllerutil.ContainsFinalizer(obj, commonservice.StaticRouteFinalizerName) { - metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, common.MetricResTypeStaticRoute) - // TODO, update the value from 'default' to actual value， get OrgID, ProjectID, VPCID depending on obj.Namespace from vpc store - if err := r.Service.DeleteStaticRoute(obj); err != nil { - log.Error(err, "delete failed, would retry exponentially", "staticroute", req.NamespacedName) - deleteFail(r, ctx, obj, &err) - return ResultRequeue, err - } - controllerutil.RemoveFinalizer(obj, commonservice.StaticRouteFinalizerName) - if err := r.Client.Update(ctx, obj); err != nil { - deleteFail(r, ctx, obj, &err) - return ResultRequeue, err - } - log.V(1).Info("removed finalizer", "staticroute", req.NamespacedName) - deleteSuccess(r, ctx, obj) - } else { - // only print a message because it's not a normal case - log.Info("finalizers cannot be recognized", "staticroute", req.NamespacedName) + metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, common.MetricResTypeStaticRoute) + if err := r.Service.DeleteStaticRoute(obj); err != nil { + log.Error(err, "delete failed, would retry exponentially", "staticroute", req.NamespacedName) + deleteFail(r, ctx, obj, &err) + return ResultRequeue, err } + deleteSuccess(r, ctx, obj) } - return ResultNormal, nil } @@ -198,12 +219,6 @@ func getExistingConditionOfType(conditionType v1alpha1.StaticRouteStatusConditio func (r *StaticRouteReconciler) setupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.StaticRoute{}). - WithEventFilter(predicate.Funcs{ - DeleteFunc: func(e event.DeleteEvent) bool { - // Suppress Delete events to avoid filtering them out in the Reconcile function - return false - }, - }). WithOptions( controller.Options{ MaxConcurrentReconciles: common.NumReconcile(), diff --git a/pkg/controllers/staticroute/staticroute_controller_test.go b/pkg/controllers/staticroute/staticroute_controller_test.go index 695f03ed4..110fa9762 100644 --- a/pkg/controllers/staticroute/staticroute_controller_test.go +++ b/pkg/controllers/staticroute/staticroute_controller_test.go @@ -18,6 +18,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" controllerruntime "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -175,23 +176,14 @@ func TestStaticRouteReconciler_Reconcile(t *testing.T) { ctx := context.Background() req := controllerruntime.Request{NamespacedName: types.NamespacedName{Namespace: "dummy", Name: "dummy"}} - // not found - errNotFound := errors.New("not found") - k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errNotFound) + // get error + errUnknowError := errors.New("unknown error") + k8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(errUnknowError) _, err := r.Reconcile(ctx, req) - assert.Equal(t, err, errNotFound) + assert.Equal(t, err, errUnknowError) - // DeletionTimestamp.IsZero = ture, client update failed sp := &v1alpha1.StaticRoute{} - k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil) - err = errors.New("Update failed") - k8sClient.EXPECT().Update(ctx, gomock.Any(), gomock.Any()).Return(err) fakewriter := fakeStatusWriter{} - k8sClient.EXPECT().Status().Return(fakewriter) - _, ret := r.Reconcile(ctx, req) - assert.Equal(t, err, ret) - - // DeletionTimestamp.IsZero = false, Finalizers doesn't include util.FinalizerName k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.StaticRoute) time := metav1.Now() @@ -200,74 +192,54 @@ func TestStaticRouteReconciler_Reconcile(t *testing.T) { }) patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error { - assert.FailNow(t, "should not be called") return nil }) - k8sClient.EXPECT().Update(ctx, gomock.Any(), gomock.Any()).Return(nil) - _, ret = r.Reconcile(ctx, req) - assert.Equal(t, ret, nil) - patch.Reset() - - // DeletionTimestamp.IsZero = false, Finalizers include util.FinalizerName - k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { - v1sp := obj.(*v1alpha1.StaticRoute) - time := metav1.Now() - v1sp.ObjectMeta.DeletionTimestamp = &time - v1sp.Finalizers = []string{common.StaticRouteFinalizerName} - return nil - }) - patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error { - return nil - }) - _, ret = r.Reconcile(ctx, req) + _, ret := r.Reconcile(ctx, req) assert.Equal(t, ret, nil) patch.Reset() - // DeletionTimestamp.IsZero = false, Finalizers include util.FinalizerName, DeleteStaticRoute fail + // DeletionTimestamp.IsZero = false, DeleteStaticRoute fail k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.StaticRoute) time := metav1.Now() v1sp.ObjectMeta.DeletionTimestamp = &time - v1sp.Finalizers = []string{common.StaticRouteFinalizerName} return nil }) patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRoute", func(_ *staticroute.StaticRouteService, obj *v1alpha1.StaticRoute) error { return errors.New("delete failed") }) - - k8sClient.EXPECT().Status().Times(2).Return(fakewriter) + k8sClient.EXPECT().Status().Times(1).Return(fakewriter) _, ret = r.Reconcile(ctx, req) assert.NotEqual(t, ret, nil) patch.Reset() - // DeletionTimestamp.IsZero = true, Finalizers include util.FinalizerName, CreateorUpdateStaticRoute fail + // DeletionTimestamp.IsZero = true, CreateorUpdateStaticRoute fail k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.StaticRoute) v1sp.ObjectMeta.DeletionTimestamp = nil - v1sp.Finalizers = []string{common.StaticRouteFinalizerName} return nil }) patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "CreateOrUpdateStaticRoute", func(_ *staticroute.StaticRouteService, namespace string, obj *v1alpha1.StaticRoute) error { return errors.New("create failed") }) + k8sClient.EXPECT().Status().Times(1).Return(fakewriter) _, ret = r.Reconcile(ctx, req) assert.NotEqual(t, ret, nil) patch.Reset() - // DeletionTimestamp.IsZero = true, Finalizers include util.FinalizerName, CreateorUpdateStaticRoute succ + // DeletionTimestamp.IsZero = true, CreateorUpdateStaticRoute succ k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error { v1sp := obj.(*v1alpha1.StaticRoute) v1sp.ObjectMeta.DeletionTimestamp = nil - v1sp.Finalizers = []string{common.StaticRouteFinalizerName} return nil }) + k8sClient.EXPECT().Status().Times(1).Return(fakewriter) patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "CreateOrUpdateStaticRoute", func(_ *staticroute.StaticRouteService, namespace string, obj *v1alpha1.StaticRoute) error { return nil }) - k8sClient.EXPECT().Status().Times(1).Return(fakewriter) _, ret = r.Reconcile(ctx, req) assert.Equal(t, ret, nil) patch.Reset() @@ -366,3 +338,113 @@ func TestStaticRouteReconciler_Start(t *testing.T) { err := r.Start(mgr) assert.NotEqual(t, err, nil) } + +func TestStaticRouteReconciler_listStaticRouteCRIDs(t *testing.T) { + mockCtl := gomock.NewController(t) + k8sClient := mock_client.NewMockClient(mockCtl) + r := &StaticRouteReconciler{ + Client: k8sClient, + Scheme: nil, + } + + ctx := context.Background() + + // list returns an error + errList := errors.New("list error") + k8sClient.EXPECT().List(ctx, gomock.Any()).Return(errList) + _, err := r.listStaticRouteCRIDs() + assert.Equal(t, err, errList) + + // list returns no error, but no items + k8sClient.EXPECT().List(ctx, gomock.Any()).DoAndReturn(func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { + staticRouteList := list.(*v1alpha1.StaticRouteList) + staticRouteList.Items = []v1alpha1.StaticRoute{} + return nil + }) + crIDs, err := r.listStaticRouteCRIDs() + assert.NoError(t, err) + assert.Equal(t, 0, crIDs.Len()) + + // list returns items + k8sClient.EXPECT().List(ctx, gomock.Any()).DoAndReturn(func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { + staticRouteList := list.(*v1alpha1.StaticRouteList) + staticRouteList.Items = []v1alpha1.StaticRoute{ + {ObjectMeta: metav1.ObjectMeta{UID: "uid1"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "uid2"}}, + } + return nil + }) + crIDs, err = r.listStaticRouteCRIDs() + assert.NoError(t, err) + assert.Equal(t, 2, crIDs.Len()) + assert.True(t, crIDs.Has("uid1")) + assert.True(t, crIDs.Has("uid2")) +} + +func TestStaticRouteReconciler_deleteStaticRouteByName(t *testing.T) { + mockCtl := gomock.NewController(t) + defer mockCtl.Finish() + + k8sClient := mock_client.NewMockClient(mockCtl) + mockStaticRouteClient := mocks.NewMockStaticRoutesClient(mockCtl) + + service := &staticroute.StaticRouteService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + StaticRouteClient: mockStaticRouteClient, + }, + NSXConfig: &config.NSXOperatorConfig{ + NsxConfig: &config.NsxConfig{ + EnforcementPoint: "vmc-enforcementpoint", + }, + }, + }, + } + + r := &StaticRouteReconciler{ + Client: k8sClient, + Scheme: nil, + Service: service, + } + + // listStaticRouteCRIDs returns an error + errList := errors.New("list error") + patch := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "listStaticRouteCRIDs", func(_ *StaticRouteReconciler) (sets.Set[string], error) { + return nil, errList + }) + defer patch.Reset() + + err := r.deleteStaticRouteByName("dummy-name", "dummy-ns") + assert.Equal(t, err, errList) + + // listStaticRouteCRIDs returns items, and deletion fails + patch.Reset() + patch.ApplyPrivateMethod(reflect.TypeOf(r), "listStaticRouteCRIDs", func(_ *StaticRouteReconciler) (sets.Set[string], error) { + return sets.New[string]("uid1"), nil + }) + patch.ApplyMethod(reflect.TypeOf(service), "ListStaticRouteByName", func(_ *staticroute.StaticRouteService, _ string, _ string) []*model.StaticRoutes { + return []*model.StaticRoutes{ + { + Id: pointy.String("route-id-1"), + Path: pointy.String("/orgs/org123/projects/pro123/vpcs/vpc123/static-routes/route-id-1"), + Tags: []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String("uid1")}}, + }, + { + Id: pointy.String("route-id-2"), + Path: pointy.String("/orgs/org123/projects/pro123/vpcs/vpc123/static-routes/route-id-2"), + Tags: []model.Tag{{Scope: pointy.String(common.TagScopeStaticRouteCRUID), Tag: pointy.String("uid2")}}, + }, + } + }) + + patch.ApplyMethod(reflect.TypeOf(service), "DeleteStaticRouteByPath", func(_ *staticroute.StaticRouteService, orgId string, projectId string, vpcId string, uid string) error { + if uid == "route-id-2" { + return errors.New("delete failed") + } + return nil + }) + + err = r.deleteStaticRouteByName("dummy-name", "dummy-ns") + assert.Error(t, err) + patch.Reset() +} diff --git a/pkg/nsx/services/common/types.go b/pkg/nsx/services/common/types.go index 02a559e67..7abe2cc4f 100644 --- a/pkg/nsx/services/common/types.go +++ b/pkg/nsx/services/common/types.go @@ -104,7 +104,6 @@ const ( NSXServiceAccountFinalizerName = "nsxserviceaccount.nsx.vmware.com/finalizer" T1SecurityPolicyFinalizerName = "securitypolicy.nsx.vmware.com/finalizer" - StaticRouteFinalizerName = "staticroute.crd.nsx.vmware.com/finalizer" SubnetPortFinalizerName = "subnetport.crd.nsx.vmware.com/finalizer" NetworkInfoFinalizerName = "networkinfo.crd.nsx.vmware.com/finalizer" PodFinalizerName = "pod.crd.nsx.vmware.com/finalizer" diff --git a/pkg/nsx/services/staticroute/staticroute.go b/pkg/nsx/services/staticroute/staticroute.go index d09b37c4a..4a007b538 100644 --- a/pkg/nsx/services/staticroute/staticroute.go +++ b/pkg/nsx/services/staticroute/staticroute.go @@ -40,6 +40,7 @@ func InitializeStaticRoute(commonService common.Service, vpcService common.VPCSe staticRouteStore := &StaticRouteStore{} staticRouteStore.Indexer = cache.NewIndexer(keyFunc, cache.Indexers{ common.TagScopeStaticRouteCRUID: indexFunc, + common.TagScopeNamespace: indexStaticRouteNamespace, }) staticRouteStore.BindingType = model.StaticRoutesBindingType() staticRouteService.StaticRouteStore = staticRouteStore @@ -148,6 +149,19 @@ func (service *StaticRouteService) DeleteStaticRoute(obj *v1alpha1.StaticRoute) return service.DeleteStaticRouteByPath(vpcResourceInfo.OrgID, vpcResourceInfo.ProjectID, vpcResourceInfo.VPCID, id) } +func (service *StaticRouteService) ListStaticRouteByName(ns, name string) []*model.StaticRoutes { + var result []*model.StaticRoutes + staticroutes := service.StaticRouteStore.GetByIndex(common.TagScopeNamespace, ns) + for _, staticroute := range staticroutes { + sr := staticroute.(*model.StaticRoutes) + tagname := nsxutil.FindTag(sr.Tags, common.TagScopeStaticRouteCRName) + if tagname == name { + result = append(result, staticroute.(*model.StaticRoutes)) + } + } + return result +} + func (service *StaticRouteService) ListStaticRoute() []*model.StaticRoutes { staticRoutes := service.StaticRouteStore.List() staticRouteSet := []*model.StaticRoutes{} diff --git a/pkg/nsx/services/staticroute/store.go b/pkg/nsx/services/staticroute/store.go index b4d13153d..1d64ce2a2 100644 --- a/pkg/nsx/services/staticroute/store.go +++ b/pkg/nsx/services/staticroute/store.go @@ -29,17 +29,26 @@ func indexFunc(obj interface{}) ([]string, error) { res := make([]string, 0, 5) switch v := obj.(type) { case *model.StaticRoutes: - return filterTag(v.Tags), nil + return filterTag(v.Tags, common.TagScopeStaticRouteCRUID), nil default: break } return res, nil } -var filterTag = func(v []model.Tag) []string { +func indexStaticRouteNamespace(obj interface{}) ([]string, error) { + switch o := obj.(type) { + case *model.StaticRoutes: + return filterTag(o.Tags, common.TagScopeNamespace), nil + default: + return nil, errors.New("indexByStaticRouteNamespace doesn't support unknown type") + } +} + +var filterTag = func(v []model.Tag, tagScope string) []string { res := make([]string, 0, 5) for _, tag := range v { - if *tag.Scope == common.TagScopeStaticRouteCRUID { + if *tag.Scope == tagScope { res = append(res, *tag.Tag) } }