Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the issue that NSX VPC not deleted when the Namespace is terminating #948

Merged
merged 2 commits into from
Dec 13, 2024
+144 −141
Merged

Fix the issue that NSX VPC not deleted when the Namespace is terminating #948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f57080
Fix issues with NetworkInfo reconciler that not delete NSX VPC in time
wenyingd Dec 6, 2024
52387a1
Filter out the Namespace which is in terminating state
wenyingd Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 23 additions & 46 deletions pkg/controllers/networkinfo/networkinfo_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (r *NetworkInfoReconciler) Reconcile(ctx context.Context, req ctrl.Request)
networkInfoCR := &v1alpha1.NetworkInfo{}
if err := r.Client.Get(ctx, req.NamespacedName, networkInfoCR); err != nil {
if apierrors.IsNotFound(err) {
if err := r.deleteVPCsByName(ctx, req.Namespace); err != nil {
if err := r.deleteVPCsByNamespace(ctx, req.Namespace); err != nil {
r.StatusUpdater.DeleteFail(req.NamespacedName, nil, err)
return common.ResultRequeue, err
}
Expand All @@ -150,7 +150,7 @@ func (r *NetworkInfoReconciler) Reconcile(ctx context.Context, req ctrl.Request)
// Check if the CR is marked for deletion
if !networkInfoCR.ObjectMeta.DeletionTimestamp.IsZero() {
r.StatusUpdater.IncreaseDeleteTotal()
if err := r.deleteVPCsByID(ctx, networkInfoCR.GetNamespace(), string(networkInfoCR.UID)); err != nil {
if err := r.deleteVPCsByNamespace(ctx, networkInfoCR.GetNamespace()); err != nil {
r.StatusUpdater.DeleteFail(req.NamespacedName, nil, err)
return common.ResultRequeue, err
}
Expand Down Expand Up @@ -408,8 +408,11 @@ func (r *NetworkInfoReconciler) listNamespaceCRsNameIDSet(ctx context.Context) (
nsSet := sets.Set[string]{}
idSet := sets.Set[string]{}
for _, ns := range namespaces.Items {
nsSet.Insert(ns.Name)
idSet.Insert(string(ns.UID))
// Ignore the terminating Namespaces in the list results.
if ns.ObjectMeta.DeletionTimestamp.IsZero() {
nsSet.Insert(ns.Name)
idSet.Insert(string(ns.UID))
}
}
return nsSet, idSet, nil
}
Expand Down Expand Up @@ -459,36 +462,18 @@ func (r *NetworkInfoReconciler) CollectGarbage(ctx context.Context) {
}
}

func (r *NetworkInfoReconciler) fetchStaleVPCsByNamespace(ctx context.Context, ns string) ([]*model.Vpc, error) {
isShared, err := r.Service.IsSharedVPCNamespaceByNS(ctx, ns)
if err != nil {
if apierrors.IsNotFound(err) {
// if the Namespace has been deleted, we never know whether it`s a shared Namespace, The GC will delete the stale VPCs
log.Info("Namespace does not exist while fetching stale VPCs", "Namespace", ns)
return nil, nil
}
return nil, fmt.Errorf("failed to check if Namespace is shared for NS %s: %w", ns, err)
}
if isShared {
log.Info("Shared Namespace, skipping deletion of NSX VPC", "Namespace", ns)
return nil, nil
func (r *NetworkInfoReconciler) deleteVPCsByNamespace(ctx context.Context, ns string) error {
staleVPCs := r.Service.GetVPCsByNamespace(ns)
timdengyun marked this conversation as resolved.
Show resolved Hide resolved
if len(staleVPCs) == 0 {
return nil
}

return r.Service.GetVPCsByNamespace(ns), nil
}

func (r *NetworkInfoReconciler) deleteVPCsByName(ctx context.Context, ns string) error {
_, idSet, err := r.listNamespaceCRsNameIDSet(ctx)
if err != nil {
log.Error(err, "Failed to list Kubernetes Namespaces")
return fmt.Errorf("failed to list Kubernetes Namespaces while deleting VPCs: %v", err)
}

staleVPCs, err := r.fetchStaleVPCsByNamespace(ctx, ns)
if err != nil {
return err
}

var vpcToDelete []*model.Vpc
for _, nsxVPC := range staleVPCs {
namespaceIDofVPC := filterTagFromNSXVPC(nsxVPC, commonservice.TagScopeNamespaceUID)
Expand All @@ -501,22 +486,6 @@ func (r *NetworkInfoReconciler) deleteVPCsByName(ctx context.Context, ns string)
return r.deleteVPCs(ctx, vpcToDelete, ns)
}

func (r *NetworkInfoReconciler) deleteVPCsByID(ctx context.Context, ns, id string) error {
staleVPCs, err := r.fetchStaleVPCsByNamespace(ctx, ns)
if err != nil {
return err
}

var vpcToDelete []*model.Vpc
for _, nsxVPC := range staleVPCs {
namespaceIDofVPC := filterTagFromNSXVPC(nsxVPC, commonservice.TagScopeNamespaceUID)
if namespaceIDofVPC == id {
vpcToDelete = append(vpcToDelete, nsxVPC)
}
}
return r.deleteVPCs(ctx, vpcToDelete, ns)
}

func (r *NetworkInfoReconciler) deleteVPCs(ctx context.Context, staleVPCs []*model.Vpc, ns string) error {
if len(staleVPCs) == 0 {
log.Info("There is no VPCs found in store, skipping deletion of NSX VPC", "Namespace", ns)
Expand All @@ -538,14 +507,22 @@ func (r *NetworkInfoReconciler) deleteVPCs(ctx context.Context, staleVPCs []*mod
}

// Update the VPCNetworkConfiguration Status
ncName, err := r.Service.GetNetworkconfigNameFromNS(ctx, ns)
if err != nil {
return fmt.Errorf("failed to get VPCNetworkConfiguration for Namespace when deleting stale VPCs %s: %w", ns, err)
vpcNetConfig := r.Service.GetVPCNetworkConfigByNamespace(ns)
if vpcNetConfig != nil {
updateVPCNetworkConfigurationStatusWithAliveVPCs(ctx, r.Client, vpcNetConfig.Name, r.listVPCsByNetworkConfigName)
}
deleteVPCNetworkConfigurationStatus(ctx, r.Client, ncName, staleVPCs, r.Service.ListVPC())
return nil
}

func (r *NetworkInfoReconciler) listVPCsByNetworkConfigName(ncName string) []*model.Vpc {
namespacesUsingNC := r.Service.GetNamespacesByNetworkconfigName(ncName)
aliveVPCs := make([]*model.Vpc, 0)
for _, namespace := range namespacesUsingNC {
aliveVPCs = append(aliveVPCs, r.Service.GetVPCsByNamespace(namespace)...)
wenyingd marked this conversation as resolved.
Show resolved Hide resolved
}
return aliveVPCs
}

func (r *NetworkInfoReconciler) syncPreCreatedVpcIPs(ctx context.Context) {
// Construct a map for the existing NetworkInfo CRs, the key is its Namespace, and the value is
// the NetworkInfo CR.
Expand Down
174 changes: 101 additions & 73 deletions pkg/controllers/networkinfo/networkinfo_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/sets"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/util/workqueue"
controllerruntime "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -154,20 +155,8 @@ func TestNetworkInfoReconciler_Reconcile(t *testing.T) {
{
name: "Empty",
prepareFunc: func(t *testing.T, r *NetworkInfoReconciler, ctx context.Context) (patches *gomonkey.Patches) {
patches = gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
return "", nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
return nil
})
patches.ApplyFunc(deleteVPCNetworkConfigurationStatus, func(ctx context.Context, client client.Client, ncName string, staleVPCs []*model.Vpc, aliveVPCs []model.Vpc) {
return
patches = gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{}
})
return patches
},
Expand Down Expand Up @@ -779,35 +768,41 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
expectErrStr string
}{
{
name: "shared namespace, skip deletion",
name: "no VPC exists for the Namespace,, skip deletion",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return true, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{}
})
return patches
},
},
{
name: "non-shared namespace, no VPCs found",
}, {
name: "NSX VPC is used by a valid Namespace,, skip deletion",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")}}}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return nil
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.New[string]("vpc1"), nil
})
return patches
},
},
{
name: "failed to delete VPC",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath := "/vpc/1"
return []*model.Vpc{{Path: &vpcPath}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.Set[string]{}, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return fmt.Errorf("delete failed")
Expand All @@ -819,26 +814,23 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
{
name: "successful deletion of VPCs",
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath1 := "/vpc/1"
vpcPath2 := "/vpc/2"
displayName1 := "fakeDisplayName"
displayName2 := "fakeDisplayName"
return []*model.Vpc{{Path: &vpcPath1, DisplayName: &displayName1}, {Path: &vpcPath2, DisplayName: &displayName2}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.Set[string]{}, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
vpcPath := "/vpc/1"
displayName := "fakeDisplayName"
return []model.Vpc{{Path: &vpcPath, DisplayName: &displayName}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
return "", nil
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCNetworkConfigByNamespace", func(_ *vpc.VPCService, _ string) *servicecommon.VPCNetworkConfigInfo {
return nil
})
return patches
},
Expand All @@ -860,26 +852,37 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
DisplayName: servicecommon.String("fakeDisplayName1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
vpcPath1 := "/vpc/1"
vpcPath2 := "/vpc/2"
displayName1 := "fakeDisplayName1"
displayName2 := "fakeDisplayName2"
return []*model.Vpc{{Path: &vpcPath1, DisplayName: &displayName1}, {Path: &vpcPath2, DisplayName: &displayName2}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listNamespaceCRsNameIDSet", func(_ *NetworkInfoReconciler, _ context.Context) (sets.Set[string], sets.Set[string], error) {
return sets.Set[string]{}, sets.Set[string]{}, nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return nil
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "ListVPC", func(_ *vpc.VPCService) []model.Vpc {
vpcPath := "/vpc/1"
displayName := "fakeDisplayName1"
return []model.Vpc{{Path: &vpcPath, DisplayName: &displayName}}
patches.ApplyPrivateMethod(reflect.TypeOf(r), "listVPCsByNetworkConfigName", func(_ *NetworkInfoReconciler, _ string) []*model.Vpc {
return []*model.Vpc{
{
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc2"),
DisplayName: servicecommon.String("fakeDisplayName2"),
},
}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetNetworkconfigNameFromNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (string, error) {
return "fakeNetworkconfigName", nil
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCNetworkConfigByNamespace", func(_ *vpc.VPCService, _ string) *servicecommon.VPCNetworkConfigInfo {
return &servicecommon.VPCNetworkConfigInfo{
Name: "fakeNetworkconfigName",
}
})
return patches
},
Expand All @@ -902,7 +905,7 @@ func TestNetworkInfoReconciler_deleteStaleVPCs(t *testing.T) {
defer patches.Reset()
}

err := r.deleteVPCsByName(ctx, namespace)
err := r.deleteVPCsByNamespace(ctx, namespace)

if tc.expectErrStr != "" {
assert.ErrorContains(t, err, tc.expectErrStr)
Expand All @@ -926,28 +929,30 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
{
name: "Delete NetworkInfo and Namespace not existed",
existingNamespace: nil,
expectErrStr: "",
expectRes: common.ResultNormal,
req: reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "testNamespace", Name: "testNetworkInfo"}},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{}
})
return patches
},
expectErrStr: "",
expectRes: common.ResultNormal,
req: reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "testNamespace", Name: "testNetworkInfo"}},
},
{
name: "Delete NetworkInfo with delete stale VPC error",
existingNamespace: &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{Name: "testNamespace", UID: "fakeNamespaceUID"},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
vpcName := "fakeVPCName"
vpcPath := "fakeVPCPath"
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, _ string) []*model.Vpc {
return []*model.Vpc{
{
DisplayName: &vpcName,
Path: &vpcPath,
},
}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "IsSharedVPCNamespaceByNS", func(_ *vpc.VPCService, ctx context.Context, _ string) (bool, error) {
return false, nil
Tags: []model.Tag{
{Scope: servicecommon.String(servicecommon.TagScopeNamespaceUID), Tag: servicecommon.String("vpc1")},
},
Path: servicecommon.String("/orgs/default/projects/default/vpcs/vpc1"),
}}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "DeleteVPC", func(_ *vpc.VPCService, _ string) error {
return fmt.Errorf("delete failed")
Expand Down Expand Up @@ -987,7 +992,7 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
Status: corev1.NamespaceStatus{},
},
prepareFuncs: func(r *NetworkInfoReconciler) *gomonkey.Patches {
return gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service.VpcStore), "GetByIndex", func(_ *vpc.VPCStore, key string, value string) []*model.Vpc {
id := "fakeNamespaceUID"
scope := servicecommon.TagScopeNamespaceUID
tag1 := []model.Tag{
Expand All @@ -1004,6 +1009,10 @@ func TestNetworkInfoReconciler_DeleteNetworkInfo(t *testing.T) {
},
}
})
patches.ApplyPrivateMethod(reflect.TypeOf(r), "deleteVPCs", func(_ *NetworkInfoReconciler, _ context.Context, _ []*model.Vpc, ns string) error {
return fmt.Errorf("failed to get VPCNetworkConfiguration for Namespace when deleting stale VPCs")
})
return patches
},
expectErrStr: "failed to get VPCNetworkConfiguration for Namespace when deleting stale VPCs",
existingNetworkInfo: &v1alpha1.NetworkInfo{
Expand Down Expand Up @@ -1410,3 +1419,22 @@ func TestRegisterAllNetworkInfo(t *testing.T) {
err = r.RegisterAllNetworkInfo(context.Background())
assert.NoError(t, err)
}

func TestListVPCsByNetworkConfigName(t *testing.T) {
r := &NetworkInfoReconciler{
Service: &vpc.VPCService{},
}
patches := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "GetNamespacesByNetworkconfigName", func(_ *vpc.VPCService, _c string) []string {
return []string{"ns1", "ns2"}
})
patches.ApplyMethod(reflect.TypeOf(r.Service), "GetVPCsByNamespace", func(_ *vpc.VPCService, ns string) []*model.Vpc {
if ns == "ns1" {
return []*model.Vpc{{Id: servicecommon.String("vpc1")}}
}
return []*model.Vpc{{Id: servicecommon.String("vpc2")}}
})
defer patches.Reset()
expVPCs := []*model.Vpc{{Id: servicecommon.String("vpc1")}, {Id: servicecommon.String("vpc2")}}
actVPCs := r.listVPCsByNetworkConfigName("nc1")
assert.ElementsMatch(t, expVPCs, actVPCs)
}
Loading
Loading