Enhance Subnet totalip calculation for shared subnet (#1384)

User can create IPReservation and SubnetPort on shared Subnet from NSX side.
Thus we need to always check the shared Subnet ip pool for totalip and allocated ip changes.

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

Author: yanjunz97

pkg/controllers/common/utils.go

@@ -133,7 +133,7 @@ func GetSubnetFromSubnetSet(client k8sclient.Client, subnetSet *v1alpha1.SubnetS
 				continue
 			}
 		}
-		canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet)
+		canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet, servicecommon.IsSharedSubnet(subnetCR))
 		if err != nil {
 			log.Error(err, "Failed to check capacity of NSX Subnet", "Subnet", subnetName, "SubnetSet", subnetSet.Name, "Namespace", subnetSet.Namespace, "NSXSubnet", nsxSubnet.Id)
 			errList = append(errList, err)
@@ -198,7 +198,7 @@ func AllocateSubnetFromSubnetSet(client k8sclient.Client, apiReader k8sclient.Re
 	defer WUnlockSubnetSet(subnetSet.GetUID(), subnetSetLock)
 	subnetList := subnetService.GetSubnetsByIndex(servicecommon.TagScopeSubnetSetCRUID, string(subnetSet.GetUID()))
 	for _, nsxSubnet := range subnetList {
-		canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet)
+		canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet, false)
 		if err != nil {
 			return "", nil, nil, err
 		}
@@ -221,7 +221,7 @@ func AllocateSubnetFromSubnetSet(client k8sclient.Client, apiReader k8sclient.Re
 	if err != nil {
 		return "", nil, nil, err
 	}
-	canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet)
+	canAllocate, err := subnetPortService.AllocatePortFromSubnet(nsxSubnet, false)
 	if err != nil {
 		return "", nil, nil, err
 	}

pkg/controllers/common/utils_test.go

@@ -122,7 +122,7 @@ func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 						},
 					})
 				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("GetPortsOfSubnet", mock.Anything).Return([]*model.VpcSubnetPort{})
-				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("AllocatePortFromSubnet", mock.Anything).Return(true, nil)
+				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(true, nil)
 			},
 			expectedResult: expectedSubnetPath,
 			subnetSet:      subnetSet,
@@ -146,7 +146,7 @@ func TestAllocateSubnetFromSubnetSet(t *testing.T) {
 				ssp.(*pkg_mock.MockSubnetServiceProvider).On("GenerateSubnetNSTags", mock.Anything)
 				vsp.(*pkg_mock.MockVPCServiceProvider).On("ListVPCInfo", mock.Anything).Return([]servicecommon.VPCResourceInfo{{}})
 				ssp.(*pkg_mock.MockSubnetServiceProvider).On("CreateOrUpdateSubnet", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&model.VpcSubnet{Path: &expectedSubnetPath}, nil)
-				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("AllocatePortFromSubnet", mock.Anything).Return(true, nil)
+				spsp.(*pkg_mock.MockSubnetPortServiceProvider).On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(true, nil)
 			},
 			expectedResult: expectedSubnetPath,
 			subnetSet: &v1alpha1.SubnetSet{
@@ -735,7 +735,7 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				// We use mock.Anything here to avoid pointer address issues
 				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
-				mp.On("AllocatePortFromSubnet", mock.Anything).Return(true, nil)
+				mp.On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(true, nil)
 				return nil
 			},
 			expectedPath: path1,
@@ -754,8 +754,8 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
 				// First call returns false (full), second call returns true
-				mp.On("AllocatePortFromSubnet", mock.Anything).Return(false, nil).Once()
-				mp.On("AllocatePortFromSubnet", mock.Anything).Return(true, nil).Once()
+				mp.On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(false, nil).Once()
+				mp.On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(true, nil).Once()
 				return nil
 			},
 			expectedPath: path1,
@@ -780,7 +780,7 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil).Once()
 				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet2, nil).Once()
-				mp.On("AllocatePortFromSubnet", mock.Anything).Return(true, nil).Once()
+				mp.On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(true, nil).Once()
 				patches := gomonkey.ApplyFunc(GetVpcNetworkConfig, func(service servicecommon.VPCServiceProvider, ns string) (*v1alpha1.VPCNetworkConfiguration, error) {
 					return &v1alpha1.VPCNetworkConfiguration{
 						Spec: v1alpha1.VPCNetworkConfigurationSpec{
@@ -807,7 +807,7 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			},
 			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
-				mp.On("AllocatePortFromSubnet", mock.Anything).Return(false, nil)
+				mp.On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(false, nil)
 				return nil
 			},
 			expectedPath: "",
@@ -853,7 +853,7 @@ func TestGetSubnetFromSubnetSet(t *testing.T) {
 			},
 			mockSetup: func(ms *pkg_mock.MockSubnetServiceProvider, mp *pkg_mock.MockSubnetPortServiceProvider) *gomonkey.Patches {
 				ms.On("GetSubnetByCR", mock.Anything).Return(nsxSubnet1, nil)
-				mp.On("AllocatePortFromSubnet", mock.Anything).Return(false, errors.New("capacity-check-failed"))
+				mp.On("AllocatePortFromSubnet", mock.Anything, mock.Anything).Return(false, errors.New("capacity-check-failed"))
 				return nil
 			},
 			wantErr:     true,

pkg/controllers/subnetport/subnetport_controller.go

@@ -864,7 +864,7 @@ func (r *SubnetPortReconciler) CheckAndGetSubnetPathForSubnetPort(ctx context.Co
 			return
 		}
 		var canAllocate bool
-		canAllocate, err = r.SubnetPortService.AllocatePortFromSubnet(nsxSubnet)
+		canAllocate, err = r.SubnetPortService.AllocatePortFromSubnet(nsxSubnet, servicecommon.IsSharedSubnet(subnetCR))
 		if err != nil {
 			return
 		}

pkg/controllers/subnetport/subnetport_controller_test.go

@@ -1308,7 +1308,7 @@ func TestSubnetPortReconciler_CheckAndGetSubnetPathForSubnetPort(t *testing.T) {
 						}}
 					})
 				patches.ApplyFunc((*subnetport.SubnetPortService).AllocatePortFromSubnet,
-					func(s *subnetport.SubnetPortService, nsxSubnet *model.VpcSubnet) (bool, error) {
+					func(s *subnetport.SubnetPortService, nsxSubnet *model.VpcSubnet, sharedSubnet bool) (bool, error) {
 						return true, nil
 					})
 				return patches

pkg/mock/services_mock.go

@@ -146,8 +146,8 @@ func (m *MockSubnetPortServiceProvider) GetPortsOfSubnet(subnetPath string) (por
 	return args.Get(0).([]*model.VpcSubnetPort)
 }
 
-func (m *MockSubnetPortServiceProvider) AllocatePortFromSubnet(subnet *model.VpcSubnet) (bool, error) {
-	args := m.Called(subnet)
+func (m *MockSubnetPortServiceProvider) AllocatePortFromSubnet(subnet *model.VpcSubnet, sharedSubnet bool) (bool, error) {
+	args := m.Called(subnet, sharedSubnet)
 	return args.Bool(0), args.Error(1)
 }
 

pkg/nsx/services/common/services.go

@@ -44,7 +44,7 @@ type SubnetServiceProvider interface {
 
 type SubnetPortServiceProvider interface {
 	GetPortsOfSubnet(subnetPath string) (ports []*model.VpcSubnetPort)
-	AllocatePortFromSubnet(subnet *model.VpcSubnet) (bool, error)
+	AllocatePortFromSubnet(subnet *model.VpcSubnet, sharedSubnet bool) (bool, error)
 	ReleasePortInSubnet(path string)
 	IsEmptySubnet(path string) bool
 	DeletePortCount(path string)

pkg/nsx/services/subnetport/subnetport.go

@@ -409,7 +409,7 @@ func (service *SubnetPortService) ResetSubnetTotalIP(path string) {
 // AllocatePortFromSubnet checks the number of SubnetPorts on the Subnet.
 // If the Subnet has capacity for the new SubnetPorts, it will increase
 // the number of SubnetPort under creation and return true.
-func (service *SubnetPortService) AllocatePortFromSubnet(subnet *model.VpcSubnet) (bool, error) {
+func (service *SubnetPortService) AllocatePortFromSubnet(subnet *model.VpcSubnet, sharedSubnet bool) (bool, error) {
 	dhcpMode := "DHCP_DEACTIVATED"
 	subnetInfo, _ := servicecommon.ParseVPCResourcePath(*subnet.Path)
 	if subnet.SubnetDhcpConfig != nil && subnet.SubnetDhcpConfig.Mode != nil {
@@ -435,6 +435,7 @@ func (service *SubnetPortService) AllocatePortFromSubnet(subnet *model.VpcSubnet
 	info.lock.Lock()
 	defer info.lock.Unlock()
 
+	var allocatedIPNumber int
 	// For DHCP Server mode Subnet, get total IPs from DHCP IP Pool from NSX each time
 	// since user might update reservedIPRanges for the subnet and it impacts the DHCP Pool size
 	if dhcpMode == "DHCP_SERVER" {
@@ -446,45 +447,62 @@ func (service *SubnetPortService) AllocatePortFromSubnet(subnet *model.VpcSubnet
 		if len(dhcpServerStats.IpPoolStats) > 0 && dhcpServerStats.IpPoolStats[0].PoolSize != nil {
 			info.totalIP = int(*dhcpServerStats.IpPoolStats[0].PoolSize)
 		}
+		if sharedSubnet && len(dhcpServerStats.IpPoolStats) > 0 && dhcpServerStats.IpPoolStats[0].AllocatedNumber != nil {
+			allocatedIPNumber = int(*dhcpServerStats.IpPoolStats[0].AllocatedNumber)
+		}
 	}
-
-	if !ok || info.totalIP == 0 {
+	// When SubnetIPReservation is created/deleted, we will reset the info.totalIP and
+	// expect the totalIP is updated from NSX ip pool API
+	// For shared Subnet, user can create IPReservation and SubnetPort from NSX side.
+	// We call NSX ip pool API to for latest total ip and requested ip.
+	if (!ok || info.totalIP == 0 || sharedSubnet) && dhcpMode == "DHCP_DEACTIVATED" {
 		// For DHCP Deactivated mode Subnet, get total IPs from IP pool static-ipv4-default
-		if dhcpMode == "DHCP_DEACTIVATED" {
-			// only get Subnet total IPs from static IP Pool if staticIpAllocation enabled
-			if staticIpAllocationEnabled {
-				staticIPPool, err := service.NSXClient.IPPoolClient.Get(subnetInfo.OrgID, subnetInfo.ProjectID, subnetInfo.VPCID, subnetInfo.ID, "static-ipv4-default")
-				if err != nil {
-					log.Error(err, "Failed to get Subnet static IP Pool static-ipv4-default", "Subnet", *subnet.Path)
-					return false, err
-				}
-				if staticIPPool.PoolUsage.TotalIps != nil {
-					info.totalIP = int(*staticIPPool.PoolUsage.TotalIps)
-				}
+		// only get Subnet total IPs from static IP Pool if staticIpAllocation enabled
+		if staticIpAllocationEnabled {
+			staticIPPool, err := service.NSXClient.IPPoolClient.Get(subnetInfo.OrgID, subnetInfo.ProjectID, subnetInfo.VPCID, subnetInfo.ID, "static-ipv4-default")
+			if err != nil {
+				log.Error(err, "Failed to get Subnet static IP Pool static-ipv4-default", "Subnet", *subnet.Path)
+				return false, err
 			}
-		}
-		// For DHCP Relay mode Subnet, assume 4 reserved IPs
-		if dhcpMode == "DHCP_RELAY" {
-			var totalIP int
-			if subnet.Ipv4SubnetSize != nil {
-				totalIP = int(*subnet.Ipv4SubnetSize)
+			if staticIPPool.PoolUsage != nil && staticIPPool.PoolUsage.TotalIps != nil {
+				info.totalIP = int(*staticIPPool.PoolUsage.TotalIps)
 			}
-			if len(subnet.IpAddresses) > 0 {
-				// totalIP will be overrided if IpAddresses are specified.
-				totalIP, _ = util.CalculateIPFromCIDRs(subnet.IpAddresses)
+			if sharedSubnet && staticIPPool.PoolUsage != nil && staticIPPool.PoolUsage.RequestedIpAllocations != nil {
+				allocatedIPNumber = int(*staticIPPool.PoolUsage.RequestedIpAllocations)
 			}
-			// NSX reserves 4 ip addresses in each subnet for network address, gateway address,
-			// dhcp server address and broadcast address.
-			info.totalIP = totalIP - 4
 		}
 	}
+	if !ok && dhcpMode == "DHCP_RELAY" {
+		// For DHCP Relay mode Subnet, assume 4 reserved IPs
+		var totalIP int
+		if subnet.Ipv4SubnetSize != nil {
+			totalIP = int(*subnet.Ipv4SubnetSize)
+		}
+		if len(subnet.IpAddresses) > 0 {
+			// totalIP will be overrided if IpAddresses are specified.
+			totalIP, _ = util.CalculateIPFromCIDRs(subnet.IpAddresses)
+		}
+		// NSX reserves 4 ip addresses in each subnet for network address, gateway address,
+		// dhcp server address and broadcast address.
+		info.totalIP = totalIP - 4
+	}
 
 	if time.Since(info.exhaustedCheckTime) < IPReleaseTime {
 		return false, nil
 	}
+
+	existingPortCount := len(service.GetPortsOfSubnet(*subnet.Path))
+	// A shared Subnet can be used by other supervisors or other places where SubnetPort
+	// is created and not in operator cache.
+	// For DHCPServer Subnet, the allocated IP number wont change before the VM request IP
+	// from the DHCPServer.
+	// Thus we use the max number of port record in store and allocated number from API to
+	// reduce the possibility to create SubnetPort on a Subnet without available IP
+	if sharedSubnet {
+		existingPortCount = max(existingPortCount, allocatedIPNumber)
+	}
 	// Number of SubnetPorts on the Subnet includes the SubnetPorts under creation
 	// and the SubnetPorts already created
-	existingPortCount := len(service.GetPortsOfSubnet(*subnet.Path))
 	if info.dirtyCount+existingPortCount < info.totalIP {
 		info.dirtyCount += 1
 		log.Trace("Allocate Subnetport to Subnet", "Subnet", *subnet.Path, "dirtyPortCount", info.dirtyCount, "existingPortCount", existingPortCount)

pkg/nsx/services/subnetport/subnetport_test.go

@@ -931,7 +931,7 @@ func TestSubnetPortService_AllocateAndReleasePortFromSubnet(t *testing.T) {
 	subnetPortService := createSubnetPortService(t)
 	// Reset Subnet totalIP without SubnetPort does not influence the port count info
 	subnetPortService.ResetSubnetTotalIP(subnetPath)
-	ok, err := subnetPortService.AllocatePortFromSubnet(subnet1)
+	ok, err := subnetPortService.AllocatePortFromSubnet(subnet1, false)
 	assert.True(t, ok)
 	require.NoError(t, err)
 	empty := subnetPortService.IsEmptySubnet(subnetPath)
@@ -944,7 +944,7 @@ func TestSubnetPortService_AllocateAndReleasePortFromSubnet(t *testing.T) {
 	subnetPortService.updateExhaustedSubnet(subnetPath)
 	// Reset Subnet totalIP does not change other port count info
 	subnetPortService.ResetSubnetTotalIP(subnetPath)
-	ok, err = subnetPortService.AllocatePortFromSubnet(subnet1)
+	ok, err = subnetPortService.AllocatePortFromSubnet(subnet1, false)
 	assert.False(t, ok)
 	assert.Nil(t, err)
 }
@@ -1006,6 +1006,7 @@ func TestSubnetPortService_AllocatePortFromSubnet(t *testing.T) {
 	tests := []struct {
 		name          string
 		subnet        *model.VpcSubnet
+		sharedSubnet  bool
 		prepareFunc   func(service *SubnetPortService) *gomonkey.Patches
 		expectedValue bool
 		expectedErr   string
@@ -1089,6 +1090,20 @@ func TestSubnetPortService_AllocatePortFromSubnet(t *testing.T) {
 			},
 			expectedValue: true,
 		},
+		{
+			name:   "Allocate SubnetPort from static shared Subnet failed",
+			subnet: staticSubnet,
+			prepareFunc: func(service *SubnetPortService) *gomonkey.Patches {
+				patches := gomonkey.ApplyMethod(reflect.TypeOf(service.NSXClient.IPPoolClient), "Get", func(c *fakeIPPoolClient, orgIdParam string, projectIdParam string, vpcIdParam string, subnetIdParam string, poolIdParam string) (model.IpAddressPool, error) {
+					return model.IpAddressPool{
+						PoolUsage: &model.PolicyPoolUsage{TotalIps: common.Int64(10), RequestedIpAllocations: common.Int64(10)},
+					}, nil
+				})
+				return patches
+			},
+			expectedValue: false,
+			sharedSubnet:  true,
+		},
 		{
 			name:   "Allocate SubnetPort from dhcp server Subnet",
 			subnet: dhcpServerSubnet,
@@ -1119,15 +1134,15 @@ func TestSubnetPortService_AllocatePortFromSubnet(t *testing.T) {
 			if patches != nil {
 				defer patches.Reset()
 			}
-			canAllocate, err := subnetPortService.AllocatePortFromSubnet(tt.subnet)
+			canAllocate, err := subnetPortService.AllocatePortFromSubnet(tt.subnet, tt.sharedSubnet)
 			assert.Equal(t, tt.expectedValue, canAllocate)
 			if tt.expectedErr != "" {
 				assert.Contains(t, err.Error(), tt.expectedErr)
 			} else {
 				assert.Nil(t, err)
 			}
 			if tt.expectedValue {
-				canAllocate, err = subnetPortService.AllocatePortFromSubnet(tt.subnet)
+				canAllocate, err = subnetPortService.AllocatePortFromSubnet(tt.subnet, tt.sharedSubnet)
 				assert.Equal(t, tt.expectedValue, canAllocate)
 				if tt.expectedErr != "" {
 					assert.Contains(t, err.Error(), tt.expectedErr)