Fetch Subnet totalip for each SubnetPort creation (#1380)

This PR fixes 2 issues for the current Subnet totalIP calculation

- We only fetch the Subnet totalIP for DHCP_DEACTIVATED Subnet with StaticIPAllocation enabled
once at the first SubnetPort is created on the Subnet, but when Subnet IPReservation is created or delete,
the Subnet totalIP will change. Thus we need to get the totalIP for such Subnet every time when a
SubnetPort is created.
- When shared Subnet is removed from cache, the Port count info of the Subnet should be deleted.

Signed-off-by: Yanjun Zhou <yanjun.zhou@broadcom.com>

Author: yanjunz97

cmd/main.go

@@ -206,7 +206,7 @@ func startServiceController(mgr manager.Manager, nsxClient *nsx.Client) {
 				os.Exit(1)
 			}
 		}
-		subnetIPReservationService, err := subnetipreservationservice.InitializeService(commonService)
+		subnetIPReservationService, err := subnetipreservationservice.InitializeService(commonService, subnetPortService)
 		if err != nil {
 			log.Error(err, "Failed to initialize SubnetIPReservation commonService", "controller", "SubnetIPReservation")
 			os.Exit(1)
@@ -230,7 +230,7 @@ func startServiceController(mgr manager.Manager, nsxClient *nsx.Client) {
 		reconcilerList = append(
 			reconcilerList,
 			networkinfocontroller.NewNetworkInfoReconciler(mgr, vpcService, ipblocksInfoService),
-			namespacecontroller.NewNamespaceReconciler(mgr, cf, vpcService, subnetService),
+			namespacecontroller.NewNamespaceReconciler(mgr, cf, vpcService, subnetService, subnetPortService),
 			subnet.NewSubnetReconciler(mgr, subnetService, subnetPortService, vpcService, subnetBindingService),
 			subnetSetReconcile,
 			node.NewNodeReconciler(mgr, nodeService),

pkg/clean/clean.go

@@ -115,6 +115,10 @@ func InitializeCleanupService(cf *config.NSXOperatorConfig, nsxClient *nsx.Clien
 	if err != nil {
 		return nil, err
 	}
+	subnetPortService, err := subnetport.InitializeSubnetPort(commonService, vpcService, ipAddressAllocationService)
+	if err != nil {
+		return nil, err
+	}
 
 	// initialize all the CR services
 	// Use Fluent Interface to escape error check hell
@@ -141,9 +145,9 @@ func InitializeCleanupService(cf *config.NSXOperatorConfig, nsxClient *nsx.Clien
 		}
 	}
 
-	wrapInitializeSubnetPort := func(service common.Service) cleanupFunc {
+	wrapInitializeSubnetPort := func(_ common.Service) cleanupFunc {
 		return func() (interface{}, error) {
-			return subnetport.InitializeSubnetPort(service, vpcService, ipAddressAllocationService)
+			return subnetPortService, nil
 		}
 	}
 	wrapInitializeIPAddressAllocation := func(service common.Service) cleanupFunc {
@@ -158,7 +162,7 @@ func InitializeCleanupService(cf *config.NSXOperatorConfig, nsxClient *nsx.Clien
 	}
 	wrapInitializeSubnetIPReservation := func(service common.Service) cleanupFunc {
 		return func() (interface{}, error) {
-			return subnetipreservation.InitializeService(service)
+			return subnetipreservation.InitializeService(service, subnetPortService)
 		}
 	}
 

pkg/controllers/namespace/namespace_controller.go

@@ -47,6 +47,7 @@ type NamespaceReconciler struct {
 	NSXConfig           *config.NSXOperatorConfig
 	VPCService          types.VPCServiceProvider
 	SubnetService       *subnet.SubnetService
+	SubnetPortService   types.SubnetPortServiceProvider
 	Recorder            record.EventRecorder
 	SubnetStatusUpdater common.StatusUpdater
 }
@@ -357,15 +358,16 @@ func (r *NamespaceReconciler) StartController(mgr ctrl.Manager, _ webhook.Server
 }
 
 func NewNamespaceReconciler(mgr ctrl.Manager, cf *config.NSXOperatorConfig, vpcService types.VPCServiceProvider,
-	subnetService *subnet.SubnetService) *NamespaceReconciler {
+	subnetService *subnet.SubnetService, subnetportService types.SubnetPortServiceProvider) *NamespaceReconciler {
 	nsReconciler := &NamespaceReconciler{
-		Client:        mgr.GetClient(),
-		APIReader:     mgr.GetAPIReader(),
-		Scheme:        mgr.GetScheme(),
-		NSXConfig:     cf,
-		VPCService:    vpcService,
-		SubnetService: subnetService,
-		Recorder:      mgr.GetEventRecorderFor("namespace-controller"),
+		Client:            mgr.GetClient(),
+		APIReader:         mgr.GetAPIReader(),
+		Scheme:            mgr.GetScheme(),
+		NSXConfig:         cf,
+		VPCService:        vpcService,
+		SubnetService:     subnetService,
+		SubnetPortService: subnetportService,
+		Recorder:          mgr.GetEventRecorderFor("namespace-controller"),
 	}
 	nsReconciler.SubnetStatusUpdater = common.NewStatusUpdater(nsReconciler.Client, nsReconciler.SubnetService.NSXConfig, nsReconciler.Recorder, MetricResTypeSubnet, "Subnet", "Subnet")
 	return nsReconciler

pkg/controllers/namespace/namespace_controller_test.go

@@ -35,6 +35,7 @@ import (
 	_ "github.com/vmware-tanzu/nsx-operator/pkg/nsx/ratelimiter"
 	servicetypes "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet"
+	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetport"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vpc"
 	"github.com/vmware-tanzu/nsx-operator/pkg/util"
 )
@@ -294,6 +295,11 @@ func TestNamespaceReconciler_StartController(t *testing.T) {
 			Client: fakeClient,
 		},
 	}
+	subnetportService := &subnetport.SubnetPortService{
+		Service: servicetypes.Service{
+			Client: fakeClient,
+		},
+	}
 	mockMgr := &MockManager{scheme: runtime.NewScheme(), client: fakeClient, apiReader: fakeAPIReader}
 	patches := gomonkey.ApplyFunc((*NamespaceReconciler).setupWithManager, func(r *NamespaceReconciler, mgr manager.Manager) error {
 		return nil
@@ -302,7 +308,7 @@ func TestNamespaceReconciler_StartController(t *testing.T) {
 		return
 	})
 	defer patches.Reset()
-	r := NewNamespaceReconciler(mockMgr, nil, vpcService, subnetService)
+	r := NewNamespaceReconciler(mockMgr, nil, vpcService, subnetService, subnetportService)
 	err := r.StartController(mockMgr, nil)
 	assert.Nil(t, err)
 }

pkg/controllers/namespace/subnet_share_controller.go

@@ -334,7 +334,13 @@ func (r *NamespaceReconciler) deleteUnusedSharedSubnets(ctx context.Context, ns
 
 				// Clear the cache to prevent stale data when the subnet is recreated on NSX
 				r.SubnetService.RemoveSubnetFromCache(associatedResource, "shared subnet CR deleted")
-
+				subnetPath, err := servicecommon.GetSubnetPathFromAssociatedResource(associatedResource)
+				if err != nil {
+					log.Error(err, "Invalid associatedResource", "associatedResource", associatedResource)
+					return err
+				} else {
+					r.SubnetPortService.DeletePortCount(subnetPath)
+				}
 				log.Info("Deleted Subnet CR for shared Subnet",
 					"Namespace", ns, "Name", subnet.Name, "AssociatedResource", associatedResource)
 				r.SubnetStatusUpdater.DeleteSuccess(client.ObjectKey{Namespace: ns, Name: subnet.Name}, subnet)

pkg/controllers/namespace/subnet_share_controller_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx"
 	servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet"
+	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetport"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/vpc"
 )
 
@@ -73,13 +74,22 @@ func createTestNamespaceReconciler(objs []client.Object) *NamespaceReconciler {
 		},
 	}
 
+	subnetPortService := &subnetport.SubnetPortService{
+		Service: servicecommon.Service{
+			Client:    fakeClient,
+			NSXClient: &nsx.Client{},
+			NSXConfig: nsxConfig,
+		},
+	}
+
 	nsReconciler := &NamespaceReconciler{
-		Client:        fakeClient,
-		APIReader:     fakeClient,
-		Scheme:        newScheme,
-		VPCService:    vpcService,
-		SubnetService: subnetService,
-		NSXConfig:     nsxConfig,
+		Client:            fakeClient,
+		APIReader:         fakeClient,
+		Scheme:            newScheme,
+		VPCService:        vpcService,
+		SubnetService:     subnetService,
+		SubnetPortService: subnetPortService,
+		NSXConfig:         nsxConfig,
 	}
 	nsReconciler.SubnetStatusUpdater = common.NewStatusUpdater(nsReconciler.Client, nsReconciler.SubnetService.NSXConfig, nil, "Subnet", "Subnet", "Subnet")
 	return nsReconciler
@@ -673,6 +683,7 @@ func TestProcessNewSharedSubnets(t *testing.T) {
 
 func TestDeleteUnusedSharedSubnets(t *testing.T) {
 	// Test cases
+	var subnetportService *subnetport.SubnetPortService
 	tests := []struct {
 		name                    string
 		existingSubnets         []client.Object
@@ -715,6 +726,7 @@ func TestDeleteUnusedSharedSubnets(t *testing.T) {
 					func(_ *NamespaceReconciler, _ context.Context, _ string, _ *v1alpha1.Subnet, _ string) (bool, error) {
 						return false, nil
 					})
+				patches.ApplyMethod(reflect.TypeOf(subnetportService), "DeletePortCount", func(_ servicecommon.SubnetPortServiceProvider, subnetPath string) {})
 				return patches
 			},
 		},
@@ -775,6 +787,7 @@ func TestDeleteUnusedSharedSubnets(t *testing.T) {
 						// Only subnet-1 has references
 						return subnet.Name == "subnet-1", nil
 					})
+				patches.ApplyMethod(reflect.TypeOf(subnetportService), "DeletePortCount", func(_ servicecommon.SubnetPortServiceProvider, subnetPath string) {})
 				return patches
 			},
 		},

pkg/controllers/subnet/subnet_poll.go

@@ -13,6 +13,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
+	servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 )
 
 var ticker = time.NewTicker(10 * time.Minute)
@@ -81,6 +82,12 @@ func (r *SubnetReconciler) pollAllSharedSubnets() {
 		if _, ok := r.SubnetService.SharedSubnetResourceMap[associatedResource]; !ok {
 			log.Debug("Remove Subnet from cache", "AssociatedResource", associatedResource)
 			r.SubnetService.RemoveSubnetFromCache(associatedResource, "no valid subnets")
+			subnetPath, err := servicecommon.GetSubnetPathFromAssociatedResource(associatedResource)
+			if err != nil {
+				log.Error(err, "Invalid associatedResource", "associatedResource", associatedResource)
+			} else {
+				r.SubnetPortService.DeletePortCount(subnetPath)
+			}
 		}
 	}
 }
@@ -197,6 +204,12 @@ func (r *SubnetReconciler) handleNSXSubnetError(ctx context.Context, err error,
 	errorType string) {
 	log.Error(err, fmt.Sprintf("Failed to get %s", errorType), "AssociatedResource", associatedResource)
 	r.SubnetService.RemoveSubnetFromCache(associatedResource, "NSXSubnetError")
+	subnetPath, err := servicecommon.GetSubnetPathFromAssociatedResource(associatedResource)
+	if err != nil {
+		log.Error(err, "Invalid associatedResource", "associatedResource", associatedResource)
+	} else {
+		r.SubnetPortService.DeletePortCount(subnetPath)
+	}
 	// Set subnet ready status to false for all valid subnets
 	for namespacedName := range validSubnets {
 		r.updateSharedSubnetWithError(ctx, namespacedName, err, errorType)

pkg/controllers/subnet/subnet_poll_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	subnetservice "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet"
+	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetport"
 )
 
 func TestHasSubnetSpecChanged(t *testing.T) {
@@ -689,12 +690,22 @@ func TestPollAllSharedSubnets(t *testing.T) {
 		sharedSubnetsMap        map[string][]types.NamespacedName
 		expectedUniqueResources int // Number of unique associated resources
 		expectedEnqueueCalls    int // Number of times enqueueSubnetForReconciliation should be called
+		initialCache            map[string]struct {
+			Subnet     *model.VpcSubnet
+			StatusList []model.VpcSubnetStatus
+		}
 	}{
 		{
 			name:                    "Empty shared subnets map",
 			sharedSubnetsMap:        map[string][]types.NamespacedName{},
 			expectedUniqueResources: 0,
 			expectedEnqueueCalls:    0,
+			initialCache: map[string]struct {
+				Subnet     *model.VpcSubnet
+				StatusList []model.VpcSubnetStatus
+			}{
+				"project1:vpc1:subnet1": {Subnet: &model.VpcSubnet{}},
+			},
 		},
 		{
 			name: "One shared subnet",
@@ -749,6 +760,7 @@ func TestPollAllSharedSubnets(t *testing.T) {
 			}
 
 			r := createFakeSubnetReconciler(subnetCRs)
+			r.SubnetService.NSXSubnetCache = tt.initialCache
 
 			// Set up the SharedSubnetResourceMap
 			for associatedResource, namespacedNames := range tt.sharedSubnetsMap {
@@ -802,6 +814,9 @@ func TestPollAllSharedSubnets(t *testing.T) {
 					enqueueSubnetCalls[namespacedName] = true
 				})
 
+			var subnetportService *subnetport.SubnetPortService
+			patches.ApplyMethod(reflect.TypeOf(subnetportService), "DeletePortCount", func(_ common.SubnetPortServiceProvider, subnetPath string) {})
+
 			// Call the actual function being tested
 			r.pollAllSharedSubnets()
 

pkg/mock/services_mock.go

@@ -164,6 +164,10 @@ func (m *MockSubnetPortServiceProvider) GetSubnetPathForSubnetPortFromStore(crUi
 	return args.String(0)
 }
 
+func (m *MockSubnetPortServiceProvider) ResetSubnetTotalIP(path string) {
+	return
+}
+
 type MockIPAddressAllocationProvider struct {
 	mock.Mock
 }

pkg/nsx/services/common/services.go

@@ -49,6 +49,7 @@ type SubnetPortServiceProvider interface {
 	IsEmptySubnet(path string) bool
 	DeletePortCount(path string)
 	GetSubnetPathForSubnetPortFromStore(crUid types.UID) string
+	ResetSubnetTotalIP(path string)
 }
 
 type NodeServiceReader interface {