Remove id comparison when deleting nsx resource by NamespacedName

Signed-off-by: Yanjun Zhou <[email protected]>

pkg/controllers/networkpolicy/networkpolicy_controller.go

@@ -177,19 +177,9 @@ func (r *NetworkPolicyReconciler) CollectGarbage(ctx context.Context) {
 }
 
 func (r *NetworkPolicyReconciler) deleteNetworkPolicyByName(ns, name string) error {
-	CRPolicySet, err := r.listNetworkPolciyCRIDs()
-	if err != nil {
-		return err
-	}
-
 	nsxSecurityPolicies := r.Service.ListNetworkPolicyByName(ns, name)
 	for _, item := range nsxSecurityPolicies {
 		uid := nsxutil.FindTag(item.Tags, servicecommon.TagScopeNetworkPolicyUID)
-		if CRPolicySet.Has(uid) {
-			log.Info("Skipping deletion, NetworkPolicy CR still exists in K8s", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
-			continue
-		}
-
 		log.Info("Deleting NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
 		if err := r.Service.DeleteSecurityPolicy(types.UID(uid), false, false, servicecommon.ResourceTypeNetworkPolicy); err != nil {
 			log.Error(err, "Failed to delete NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)

pkg/controllers/networkpolicy/networkpolicy_controller_test.go

@@ -452,20 +452,8 @@ func TestNetworkPolicyReconciler_deleteNetworkPolicyByName(t *testing.T) {
 	objs := []client.Object{}
 	r := createFakeNetworkPolicyReconciler(objs)
 
-	// listNetworkPolciyCRIDs returns an error
-	errList := errors.New("list error")
-	patch := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "listNetworkPolciyCRIDs", func(_ *NetworkPolicyReconciler) (sets.Set[string], error) {
-		return nil, errList
-	})
-	err := r.deleteNetworkPolicyByName("dummy-ns", "dummy-name")
-	assert.Equal(t, err, errList)
-
-	// listNetworkPolciyCRIDs returns items, and deletion fails
-	patch.Reset()
-	patch.ApplyPrivateMethod(reflect.TypeOf(r), "listNetworkPolciyCRIDs", func(_ *NetworkPolicyReconciler) (sets.Set[string], error) {
-		return sets.New[string]("uid1"), nil
-	})
-	patch.ApplyMethod(reflect.TypeOf(r.Service), "ListNetworkPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
+	// deletion fails
+	patch := gomonkey.ApplyMethod(reflect.TypeOf(r.Service), "ListNetworkPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
 		return []*model.SecurityPolicy{
 			{
 				Id:   pointy.String("sp-id-1"),
@@ -488,7 +476,7 @@ func TestNetworkPolicyReconciler_deleteNetworkPolicyByName(t *testing.T) {
 		return nil
 	})
 
-	err = r.deleteNetworkPolicyByName("dummy-ns", "dummy-name")
+	err := r.deleteNetworkPolicyByName("dummy-ns", "dummy-name")
 	assert.Error(t, err)
 	patch.Reset()
 }

pkg/controllers/pod/pod_controller.go

@@ -223,25 +223,15 @@ func podIsDeleted(pod *v1.Pod) bool {
 }
 
 func (r *PodReconciler) deleteSubnetPortByPodName(ctx context.Context, ns string, name string) error {
-	// When deleting SubnetPort by Name and Namespace, skip the SubnetPort belonging to the existed SubnetPort CR
+	// NamespacedName is the unique identity as only one worker can deal with the NamespacedName key at a time
 	nsxSubnetPorts := r.SubnetPortService.ListSubnetPortByPodName(ns, name)
 
-	crSubnetPortIDsSet, err := r.SubnetPortService.ListSubnetPortIDsFromCRs(ctx)
-	if err != nil {
-		log.Error(err, "failed to list SubnetPort CRs")
-		return err
-	}
-
 	for _, nsxSubnetPort := range nsxSubnetPorts {
-		if crSubnetPortIDsSet.Has(*nsxSubnetPort.Id) {
-			log.Info("skipping deletion, Pod CR still exists in K8s", "ID", *nsxSubnetPort.Id)
-			continue
-		}
 		if err := r.SubnetPortService.DeleteSubnetPort(nsxSubnetPort); err != nil {
 			return err
 		}
 	}
-	log.Info("successfully deleted nsxSubnetPort for Pod", "namespace", ns, "name", name)
+	log.Info("Successfully deleted nsxSubnetPort for Pod", "Namespace", ns, "Name", name)
 	return nil
 }
 

pkg/controllers/pod/pod_controller_test.go

@@ -489,7 +489,8 @@ func TestSubnetPortReconciler_GetSubnetPathForPod(t *testing.T) {
 func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 	subnetportId1 := "subnetport-1"
 	subnetportId2 := "subnetport-2"
-	podName := "pod-1"
+	podName1 := "pod-1"
+	podName2 := "pod-2"
 	namespaceScope := "nsx-op/namespace"
 	ns := "ns"
 	nameScope := "nsx-op/pod_name"
@@ -502,7 +503,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 			},
 			{
 				Scope: &nameScope,
-				Tag:   &podName,
+				Tag:   &podName1,
 			},
 		},
 	}
@@ -515,7 +516,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 			},
 			{
 				Scope: &nameScope,
-				Tag:   &podName,
+				Tag:   &podName2,
 			},
 		},
 	}
@@ -542,6 +543,6 @@ func TestSubnetPortReconciler_deleteSubnetPortByPodName(t *testing.T) {
 			return nil
 		})
 	defer patchesDeleteSubnetPort.Reset()
-	err := r.deleteSubnetPortByPodName(context.TODO(), ns, podName)
+	err := r.deleteSubnetPortByPodName(context.TODO(), ns, podName2)
 	assert.Nil(t, err)
 }

pkg/controllers/securitypolicy/securitypolicy_controller.go

@@ -369,19 +369,9 @@ func (r *SecurityPolicyReconciler) CollectGarbage(ctx context.Context) {
 }
 
 func (r *SecurityPolicyReconciler) deleteSecurityPolicyByName(ns, name string) error {
-	CRPolicySet, err := r.listSecurityPolciyCRIDs()
-	if err != nil {
-		return err
-	}
-
 	nsxSecurityPolicies := r.Service.ListSecurityPolicyByName(ns, name)
 	for _, item := range nsxSecurityPolicies {
 		uid := nsxutil.FindTag(item.Tags, servicecommon.TagValueScopeSecurityPolicyUID)
-		if CRPolicySet.Has(uid) {
-			log.Info("Skipping deletion, SecurityPolicy CR still exists in K8s", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
-			continue
-		}
-
 		log.Info("Deleting SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
 		if err := r.Service.DeleteSecurityPolicy(types.UID(uid), false, false, servicecommon.ResourceTypeSecurityPolicy); err != nil {
 			log.Error(err, "Failed to delete SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)

pkg/controllers/securitypolicy/securitypolicy_controller_test.go

@@ -672,10 +672,6 @@ func TestSecurityPolicyReconciler_listSecurityPolciyCRIDsForVPC(t *testing.T) {
 }
 
 func TestSecurityPolicyReconciler_deleteSecuritypolicyByName(t *testing.T) {
-	mockCtl := gomock.NewController(t)
-	defer mockCtl.Finish()
-
-	k8sClient := mock_client.NewMockClient(mockCtl)
 	service := &securitypolicy.SecurityPolicyService{
 		Service: common.Service{
 			NSXClient: &nsx.Client{},
@@ -690,26 +686,13 @@ func TestSecurityPolicyReconciler_deleteSecuritypolicyByName(t *testing.T) {
 		},
 	}
 	r := &SecurityPolicyReconciler{
-		Client:   k8sClient,
 		Scheme:   nil,
 		Service:  service,
 		Recorder: fakeRecorder{},
 	}
 
-	// listSecurityPolciyCRIDs returns an error
-	errList := errors.New("list error")
-	patch := gomonkey.ApplyPrivateMethod(reflect.TypeOf(r), "listSecurityPolciyCRIDs", func(_ *SecurityPolicyReconciler) (sets.Set[string], error) {
-		return nil, errList
-	})
-	err := r.deleteSecurityPolicyByName("dummy-ns", "dummy-name")
-	assert.Equal(t, err, errList)
-
-	// listSecurityPolciyCRIDs returns items, and deletion fails
-	patch.Reset()
-	patch.ApplyPrivateMethod(reflect.TypeOf(r), "listSecurityPolciyCRIDs", func(_ *SecurityPolicyReconciler) (sets.Set[string], error) {
-		return sets.New[string]("uid1"), nil
-	})
-	patch.ApplyMethod(reflect.TypeOf(service), "ListSecurityPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
+	// deletion fails
+	patch := gomonkey.ApplyMethod(reflect.TypeOf(service), "ListSecurityPolicyByName", func(_ *securitypolicy.SecurityPolicyService, _ string, _ string) []*model.SecurityPolicy {
 		return []*model.SecurityPolicy{
 			{
 				Id:   pointy.String("sp-id-1"),
@@ -732,7 +715,7 @@ func TestSecurityPolicyReconciler_deleteSecuritypolicyByName(t *testing.T) {
 		return nil
 	})
 
-	err = r.deleteSecurityPolicyByName("dummy-ns", "dummy-name")
+	err := r.deleteSecurityPolicyByName("dummy-ns", "dummy-name")
 	assert.Error(t, err)
 	patch.Reset()
 }

pkg/controllers/staticroute/staticroute_controller.go

@@ -63,25 +63,15 @@ func (r *StaticRouteReconciler) listStaticRouteCRIDs() (sets.Set[string], error)
 }
 
 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)
+		log.Info("Deleting StaticRoute", "Namespace", ns, "Name", name, "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)
+			log.Error(err, "failed to delete StaticRoute", "nsxStaticRouteId", *item.Id)
 			return err
 		}
-		log.Info("successfully deleted StaticRoute", "StaticRouteUID", uid, "nsxStaticRouteId", *item.Id)
+		log.Info("successfully deleted StaticRoute", "Namespace", ns, "Name", name, "nsxStaticRouteId", *item.Id)
 	}
 	return nil
 }

pkg/controllers/staticroute/staticroute_controller_test.go

@@ -387,7 +387,6 @@ 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{
@@ -404,24 +403,12 @@ func TestStaticRouteReconciler_deleteStaticRouteByName(t *testing.T) {
 	}
 
 	r := &StaticRouteReconciler{
-		Client:  k8sClient,
 		Scheme:  nil,
 		Service: service,
 	}
 
-	// listStaticRouteCRIDs returns an error
-	errList := errors.New("list error")
+	// deletion fails
 	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 {
@@ -446,7 +433,7 @@ func TestStaticRouteReconciler_deleteStaticRouteByName(t *testing.T) {
 		return nil
 	})
 
-	err = r.deleteStaticRouteByName("dummy-name", "dummy-ns")
+	err := r.deleteStaticRouteByName("dummy-name", "dummy-ns")
 	assert.Error(t, err)
 	patch.Reset()
 }

pkg/controllers/subnet/subnet_controller.go

@@ -156,14 +156,12 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}
 	// Create or update the subnet in NSX
 	if _, err := r.SubnetService.CreateOrUpdateSubnet(subnetCR, vpcInfoList[0], tags); err != nil {
-		if err != nil {
-			if errors.As(err, &nsxutil.ExceedTagsError{}) {
-				r.StatusUpdater.UpdateFail(ctx, subnetCR, err, "Tags limit exceeded", setSubnetReadyStatusFalse)
-				return ResultNormal, nil
-			}
-			r.StatusUpdater.UpdateFail(ctx, subnetCR, err, "Failed to create/update Subnet", setSubnetReadyStatusFalse)
-			return ResultRequeue, err
+		if errors.As(err, &nsxutil.ExceedTagsError{}) {
+			r.StatusUpdater.UpdateFail(ctx, subnetCR, err, "Tags limit exceeded", setSubnetReadyStatusFalse)
+			return ResultNormal, nil
 		}
+		r.StatusUpdater.UpdateFail(ctx, subnetCR, err, "Failed to create/update Subnet", setSubnetReadyStatusFalse)
+		return ResultRequeue, err
 	}
 	// Update status
 	if err := r.updateSubnetStatus(subnetCR); err != nil {
@@ -201,29 +199,9 @@ func (r *SubnetReconciler) deleteSubnets(nsxSubnets []*model.VpcSubnet) error {
 	return nil
 }
 
-func (r *SubnetReconciler) deleteStaleSubnets(nsxSubnets []*model.VpcSubnet) error {
-	crdSubnetIDs, err := r.listSubnetIDsFromCRs(context.Background())
-	if err != nil {
-		log.Error(err, "Failed to list Subnet CRs")
-		return err
-	}
-	crdSubnetIDsSet := sets.NewString(crdSubnetIDs...)
-	nsxSubnetsToDelete := make([]*model.VpcSubnet, 0, len(nsxSubnets))
-	for _, nsxSubnet := range nsxSubnets {
-		uid := nsxutil.FindTag(nsxSubnet.Tags, servicecommon.TagScopeSubnetCRUID)
-		if crdSubnetIDsSet.Has(uid) {
-			log.Info("Skipping deletion, Subnet CR still exists in K8s", "ID", *nsxSubnet.Id)
-			continue
-		}
-		nsxSubnetsToDelete = append(nsxSubnetsToDelete, nsxSubnet)
-	}
-	log.Info("Cleaning stale Subnets", "Count", len(nsxSubnetsToDelete))
-	return r.deleteSubnets(nsxSubnetsToDelete)
-}
-
 func (r *SubnetReconciler) deleteSubnetByName(name, ns string) error {
 	nsxSubnets := r.SubnetService.ListSubnetByName(ns, name)
-	return r.deleteStaleSubnets(nsxSubnets)
+	return r.deleteSubnets(nsxSubnets)
 }
 
 func (r *SubnetReconciler) updateSubnetStatus(obj *v1alpha1.Subnet) error {

pkg/controllers/subnetport/subnetport_controller.go

@@ -184,37 +184,25 @@ func addressBindingNamespaceVMIndexFunc(obj client.Object) []string {
 }
 
 func (r *SubnetPortReconciler) deleteSubnetPortByName(ctx context.Context, ns string, name string) error {
-	// When deleting SubnetPort by Name and Namespace, skip the SubnetPort belonging to the existed SubnetPort CR
+	// NamespacedName is the unique identity as only one worker can deal with the NamespacedName key at a time
 	nsxSubnetPorts := r.SubnetPortService.ListSubnetPortByName(ns, name)
 
-	crSubnetPortIDsSet, err := r.SubnetPortService.ListSubnetPortIDsFromCRs(ctx)
-	if err != nil {
-		log.Error(err, "failed to list SubnetPort CRs")
-		return err
-	}
-
-	var hasSubnetPort bool
 	var externalIpAddress *string
 	for _, nsxSubnetPort := range nsxSubnetPorts {
-		if crSubnetPortIDsSet.Has(*nsxSubnetPort.Id) {
-			log.Info("skipping deletion, SubnetPort CR still exists in K8s", "ID", *nsxSubnetPort.Id)
-			hasSubnetPort = true
-			continue
-		}
 		if nsxSubnetPort.ExternalAddressBinding != nil && nsxSubnetPort.ExternalAddressBinding.ExternalIpAddress != nil && *nsxSubnetPort.ExternalAddressBinding.ExternalIpAddress != "" {
 			externalIpAddress = nsxSubnetPort.ExternalAddressBinding.ExternalIpAddress
 		}
 		if err := r.SubnetPortService.DeleteSubnetPort(nsxSubnetPort); err != nil {
-			if !hasSubnetPort && externalIpAddress != nil {
+			if externalIpAddress != nil {
 				r.collectAddressBindingGarbage(ctx, &ns, externalIpAddress)
 			}
 			return err
 		}
 	}
-	if !hasSubnetPort && externalIpAddress != nil {
+	if externalIpAddress != nil {
 		r.collectAddressBindingGarbage(ctx, &ns, externalIpAddress)
 	}
-	log.Info("successfully deleted nsxSubnetPort", "namespace", ns, "name", name)
+	log.Info("Successfully deleted nsxSubnetPort", "Namespace", ns, "Name", name)
 	return nil
 }
 

pkg/controllers/subnetport/subnetport_controller_test.go

@@ -445,10 +445,11 @@ func TestSubnetPortReconciler_vmMapFunc(t *testing.T) {
 }
 
 func TestSubnetPortReconciler_deleteSubnetPortByName(t *testing.T) {
-	subnetportId1 := "subnetport-1"
-	subnetportId2 := "subnetport-2"
+	subnetportId1 := "subnetport-id-1"
+	subnetportId2 := "subnetport-id-2"
 	namespaceScope := "nsx-op/vm_namespace"
-	subnetportName := "subnetport"
+	subnetportName1 := "subnetport-1"
+	subnetportName2 := "subnetport-2"
 	ns := "ns"
 	nameScope := "nsx-op/subnetport_name"
 	sp1 := &model.VpcSubnetPort{
@@ -460,7 +461,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByName(t *testing.T) {
 			},
 			{
 				Scope: &nameScope,
-				Tag:   &subnetportName,
+				Tag:   &subnetportName1,
 			},
 		},
 	}
@@ -473,7 +474,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByName(t *testing.T) {
 			},
 			{
 				Scope: &nameScope,
-				Tag:   &subnetportName,
+				Tag:   &subnetportName2,
 			},
 		},
 	}
@@ -500,7 +501,7 @@ func TestSubnetPortReconciler_deleteSubnetPortByName(t *testing.T) {
 			return nil
 		})
 	defer patchesDeleteSubnetPort.Reset()
-	err := r.deleteSubnetPortByName(context.TODO(), ns, subnetportName)
+	err := r.deleteSubnetPortByName(context.TODO(), ns, subnetportName2)
 	assert.Nil(t, err)
 }