Skip to content

Commit

Permalink
Remove staticroute finalizer
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@TaoZou1
TaoZou1 committed Oct 8, 2024
1 parent b910006 commit e3f6489
Show file tree
Hide file tree
Showing 4 changed files with 201 additions and 85 deletions.
111 changes: 68 additions & 43 deletions pkg/controllers/staticroute/staticroute_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,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"

Expand Down Expand Up @@ -72,60 +71,86 @@ 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(name, ns string) error {
CRPolicySet, err := r.listStaticRouteCRIDs()
if err != nil {
return err
}
nsxStaticRoutes := r.Service.ListStaticRouteByName(name, ns)
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) deleteStaticRoute(ctx context.Context, req ctrl.Request, obj *v1alpha1.StaticRoute) (ctrl.Result, error) {

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

}
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 {
log.Error(err, "unable to fetch static route CR", "req", req.NamespacedName)
return ResultNormal, client.IgnoreNotFound(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)
if client.IgnoreNotFound(err) == nil {
if err := r.deleteStaticRouteByName(req.Namespace, req.Name); err != nil {
return ResultRequeue, err
} else {
return ResultNormal, nil
}
log.V(1).Info("added finalizer on staticroute CR", "staticroute", req.NamespacedName)
}
log.Error(err, "unable to fetch static route CR", "req", req.NamespacedName)
return ResultRequeue, err
}

if err := r.Service.CreateOrUpdateStaticRoute(req.Namespace, obj); err != nil {
updateFail(r, ctx, obj, &err)
// TODO: if error is not retriable, not requeue
apierror, errortype := util.DumpAPIError(err)
if apierror != nil {
log.Info("create or update static route failed", "error", apierror, "error type", errortype)
}
return ResultRequeue, err
}
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)
}
if !obj.ObjectMeta.DeletionTimestamp.IsZero() {
return r.deleteStaticRoute(ctx, req, obj)
}

metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, common.MetricResTypeStaticRoute)
if err := r.Service.CreateOrUpdateStaticRoute(req.Namespace, obj); err != nil {
updateFail(r, ctx, obj, &err)
// TODO: if error is not retriable, not requeue
apierror, errortype := util.DumpAPIError(err)
if apierror != nil {
log.Info("create or update static route failed", "error", apierror, "error type", errortype)
}
return ResultRequeue, err
}
updateSuccess(r, ctx, obj)
return ResultNormal, nil
}

Expand Down
161 changes: 121 additions & 40 deletions pkg/controllers/staticroute/staticroute_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -30,6 +31,7 @@ import (
"github.com/vmware-tanzu/nsx-operator/pkg/nsx"
_ "github.com/vmware-tanzu/nsx-operator/pkg/nsx/ratelimiter"
"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
commonservice "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"

Check failure on line 34 in pkg/controllers/staticroute/staticroute_controller_test.go

View workflow job for this annotation

GitHub Actions / build 

duplicated-imports: Package "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common" already imported (revive)
"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/staticroute"
)

Expand Down Expand Up @@ -175,23 +177,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()
Expand All @@ -200,74 +193,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()
Expand Down Expand Up @@ -366,3 +339,111 @@ 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(commonservice.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(commonservice.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()
}
1 change: 0 additions & 1 deletion pkg/nsx/services/common/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ const (

SecurityPolicyFinalizerName = "securitypolicy.crd.nsx.vmware.com/finalizer"
NetworkPolicyFinalizerName = "networkpolicy.crd.nsx.vmware.com/finalizer"
StaticRouteFinalizerName = "staticroute.crd.nsx.vmware.com/finalizer"
SubnetFinalizerName = "subnet.crd.nsx.vmware.com/finalizer"
SubnetSetFinalizerName = "subnetset.crd.nsx.vmware.com/finalizer"
SubnetPortFinalizerName = "subnetport.crd.nsx.vmware.com/finalizer"
Expand Down
Loading

0 comments on commit e3f6489

Please sign in to comment.