Merge pull request #184 from kyrtapz/fix_capacity_calc

OCPBUGS-63348: Fix capacity calculation

Author: openshift-merge-bot[bot]

pkg/cloudprovider/aws.go

@@ -14,8 +14,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws/endpoints"
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/ec2"
-	v1 "github.com/openshift/api/cloudnetwork/v1"
-	"github.com/openshift/cloud-network-config-controller/pkg/cloudprivateipconfig"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -181,7 +179,7 @@ func (a *AWS) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	}
 }
 
-func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	instance, err := a.getInstance(node)
 	if err != nil {
 		return nil, err
@@ -214,10 +212,8 @@ func (a *AWS) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConf
 	if v6Subnet != nil {
 		config.IFAddr.IPv6 = v6Subnet.String()
 	}
-	capV4, capV6, err := a.getCapacity(instanceV4Capacity, instanceV6Capacity, networkInterface, cloudPrivateIPConfigs)
-	if err != nil {
-		return nil, err
-	}
+
+	capV4, capV6 := a.getCapacity(instanceV4Capacity, instanceV6Capacity, networkInterface, cpicIPs)
 	config.Capacity = capacity{
 		IPv4: ptr.To(capV4),
 		IPv6: ptr.To(capV6),
@@ -304,32 +300,28 @@ func (a *AWS) getSubnet(networkInterface *ec2.InstanceNetworkInterface) (*net.IP
 
 // AWS uses a variable capacity per instance type, see:
 // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-eni.html#AvailableIpPerENI
-// Hence we need to retrieve that and then subtract the amount already assigned
-// by default.
-func (a *AWS) getCapacity(instanceV4Capacity, instanceV6Capacity int, networkInterface *ec2.InstanceNetworkInterface, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) (int, int, error) {
+// Capacity represents the number of IPs available for consumption.
+// We calculate this as: instance_limit - IPs_consumed_by_non_CPIC_sources
+// This way, CNCC managed IPs (regardless of their status) don't reduce capacity.
+func (a *AWS) getCapacity(instanceV4Capacity, instanceV6Capacity int, networkInterface *ec2.InstanceNetworkInterface, cpicIPs sets.Set[string]) (int, int) {
 	currentIPv4Usage, currentIPv6Usage := 0, 0
 	for _, assignedIPv6 := range networkInterface.Ipv6Addresses {
 		if assignedIP := net.ParseIP(*assignedIPv6.Ipv6Address); assignedIP != nil {
-			currentIPv6Usage++
+			if !cpicIPs.Has(assignedIP.String()) {
+				currentIPv6Usage++
+			}
 		}
 	}
+
 	for _, assignedIPv4 := range networkInterface.PrivateIpAddresses {
 		if assignedIP := net.ParseIP(*assignedIPv4.PrivateIpAddress); assignedIP != nil {
-			currentIPv4Usage++
-		}
-	}
-	for _, cloudPrivateIPConfig := range cloudPrivateIPConfigs {
-		_, ipFamily, err := cloudprivateipconfig.NameToIP(cloudPrivateIPConfig.Name)
-		if err != nil {
-			return 0, 0, err
-		}
-		if ipFamily == cloudprivateipconfig.IPv4 {
-			instanceV4Capacity++
-		} else if ipFamily == cloudprivateipconfig.IPv6 {
-			instanceV6Capacity++
+			if !cpicIPs.Has(assignedIP.String()) {
+				currentIPv4Usage++
+			}
 		}
 	}
-	return instanceV4Capacity - currentIPv4Usage, instanceV6Capacity - currentIPv6Usage, nil
+
+	return instanceV4Capacity - currentIPv4Usage, instanceV6Capacity - currentIPv6Usage
 }
 
 func (a *AWS) getNetworkInterfaces(instance *ec2.Instance) ([]*ec2.InstanceNetworkInterface, error) {
@@ -455,6 +447,9 @@ func sharedCredentialsFileFromDirectory(dir string) (string, error) {
 	return f.Name(), nil
 }
 
+func (a *AWS) CleanupNode(nodeName string) {
+}
+
 // newConfigForStaticCreds is copied verbatim from:
 // https://github.com/openshift/cluster-ingress-operator/blob/1600a0e349ef075fcb52ab65b33445e256358ab8/pkg/util/aws/shared_credentials_file.go#L52
 func newConfigForStaticCreds(accessKey string, accessSecret string) []byte {

pkg/cloudprovider/azure.go

@@ -21,9 +21,9 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
 	azureapi "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/msi-dataplane/pkg/dataplane"
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	configv1 "github.com/openshift/api/config/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
 )
@@ -322,7 +322,7 @@ func (a *Azure) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	return a.waitForCompletion(poller)
 }
 
-func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	instance, err := a.getInstance(node)
 	if err != nil {
 		return nil, err
@@ -352,7 +352,7 @@ func (a *Azure) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPCo
 	}
 	config.Capacity = capacity{
 		// IPv4 and IPv6 fields not used by Azure (uses IP-family-agnostic capacity)
-		IP: ptr.To(a.getCapacity(networkInterface, len(cloudPrivateIPConfigs))),
+		IP: ptr.To(a.getCapacity(networkInterface, cpicIPs)),
 	}
 	return []*NodeEgressIPConfiguration{config}, nil
 }
@@ -408,18 +408,17 @@ func (a *Azure) getSubnet(networkInterface armnetwork.Interface) (*net.IPNet, *n
 // We need to retrieve the amounts assigned to the node by default and subtract
 // that from the default 256 value. Note: there is also a "Private IP addresses
 // per virtual network" quota, but that's 65.536, so we can skip that.
-func (a *Azure) getCapacity(networkInterface armnetwork.Interface, cloudPrivateIPsCount int) int {
-	currentIPv4Usage, currentIPv6Usage := 0, 0
+func (a *Azure) getCapacity(networkInterface armnetwork.Interface, cpicIPs sets.Set[string]) int {
+	currentIPUsage := 0
 	for _, ipConfiguration := range networkInterface.Properties.IPConfigurations {
 		if assignedIP := net.ParseIP(ptr.Deref(ipConfiguration.Properties.PrivateIPAddress, "")); assignedIP != nil {
-			if utilnet.IsIPv4(assignedIP) {
-				currentIPv4Usage++
-			} else {
-				currentIPv6Usage++
+			if !cpicIPs.Has(assignedIP.String()) {
+				currentIPUsage++
 			}
 		}
 	}
-	return defaultAzurePrivateIPCapacity + cloudPrivateIPsCount - currentIPv4Usage - currentIPv6Usage
+
+	return defaultAzurePrivateIPCapacity - currentIPUsage
 }
 
 // This is what the node's providerID looks like on Azure
@@ -668,6 +667,12 @@ func (a *Azure) getNodeLock(nodeName string) *sync.Mutex {
 	return a.nodeLockMap[nodeName]
 }
 
+func (a *Azure) CleanupNode(nodeName string) {
+	a.nodeMapLock.Lock()
+	defer a.nodeMapLock.Unlock()
+	delete(a.nodeLockMap, nodeName)
+}
+
 func getNameFromResourceID(id string) string {
 	return id[strings.LastIndex(id, "/"):]
 }

pkg/cloudprovider/cloudprovider.go

@@ -13,8 +13,8 @@ import (
 	apifeatures "github.com/openshift/api/features"
 	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 var (
@@ -60,7 +60,13 @@ type CloudProviderIntf interface {
 	// for all instance types and IP families (GCP, Azure) or variable per
 	// instance and IP family (AWS), also: the interface is either keyed by name
 	// (GCP) or ID (Azure, AWS).
-	GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error)
+	// The cpicIPs parameter is a set of IP addresses that are
+	// managed by CloudPrivateIPConfigs and should be excluded from capacity calculations.
+	GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error)
+
+	// CleanupNode removes any internal state associated with the node.
+	// This should be called when a node is deleted.
+	CleanupNode(nodeName string)
 }
 
 // CloudProviderWithMoveIntf is additional interface that can be added to cloud
@@ -163,6 +169,7 @@ func NewCloudProviderClient(cfg CloudProviderConfig, platformStatus *configv1.Pl
 	case PlatformTypeGCP:
 		cloudProviderIntf = &GCP{
 			CloudProvider: cp,
+			nodeLockMap:   make(map[string]*sync.Mutex),
 		}
 	case PlatformTypeOpenStack:
 		cloudProviderIntf = &OpenStack{

pkg/cloudprovider/cloudprovider_fake.go

@@ -5,8 +5,8 @@ import (
 	"net"
 	"time"
 
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 type FakeCloudProvider struct {
@@ -63,9 +63,12 @@ func (f *FakeCloudProvider) waitForCompletion() error {
 	return nil
 }
 
-func (f *FakeCloudProvider) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (f *FakeCloudProvider) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	if f.mockErrorOnGetNodeEgressIPConfiguration {
 		return nil, fmt.Errorf("Get node egress IP configuration failed")
 	}
 	return nil, nil
 }
+
+func (f *FakeCloudProvider) CleanupNode(nodeName string) {
+}

pkg/cloudprovider/gcp.go

@@ -6,12 +6,12 @@ import (
 	"net"
 	"net/url"
 	"strings"
+	"sync"
 
-	v1 "github.com/openshift/api/cloudnetwork/v1"
 	google "google.golang.org/api/compute/v1"
 	"google.golang.org/api/option"
 	corev1 "k8s.io/api/core/v1"
-	utilnet "k8s.io/utils/net"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/ptr"
 )
 
@@ -31,7 +31,9 @@ const (
 // to the GCP cloud API
 type GCP struct {
 	CloudProvider
-	client *google.Service
+	client      *google.Service
+	nodeMapLock sync.Mutex
+	nodeLockMap map[string]*sync.Mutex
 }
 
 func (g *GCP) initCredentials() (err error) {
@@ -81,6 +83,10 @@ func (g *GCP) initCredentials() (err error) {
 // GCP can return 10.0.32.25/32 or 10.0.32.25 - we thus need to check for both
 // when validating that the IP provided doesn't already exist
 func (g *GCP) AssignPrivateIP(ip net.IP, node *corev1.Node) error {
+	nodeLock := g.getNodeLock(node.Name)
+	nodeLock.Lock()
+	defer nodeLock.Unlock()
+
 	project, zone, instance, err := g.getInstance(node)
 	if err != nil {
 		return err
@@ -114,6 +120,10 @@ func (g *GCP) AssignPrivateIP(ip net.IP, node *corev1.Node) error {
 // Important: GCP IP aliases can come in all forms, i.e: if you add 10.0.32.25
 // GCP can return 10.0.32.25/32 or 10.0.32.25
 func (g *GCP) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
+	nodeLock := g.getNodeLock(node.Name)
+	nodeLock.Lock()
+	defer nodeLock.Unlock()
+
 	project, zone, instance, err := g.getInstance(node)
 	if err != nil {
 		return err
@@ -155,7 +165,7 @@ func (g *GCP) ReleasePrivateIP(ip net.IP, node *corev1.Node) error {
 	return g.waitForCompletion(project, zone, operation.Name)
 }
 
-func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConfigs []*v1.CloudPrivateIPConfig) ([]*NodeEgressIPConfiguration, error) {
+func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cpicIPs sets.Set[string]) ([]*NodeEgressIPConfiguration, error) {
 	_, _, instance, err := g.getInstance(node)
 	if err != nil {
 		return nil, fmt.Errorf("error retrieving instance associated with node, err: %v", err)
@@ -184,7 +194,7 @@ func (g *GCP) GetNodeEgressIPConfiguration(node *corev1.Node, cloudPrivateIPConf
 		}
 		config.Capacity = capacity{
 			// IPv4 and IPv6 fields not used by GCP (uses IP-family-agnostic capacity)
-			IP: ptr.To(g.getCapacity(networkInterface, len(cloudPrivateIPConfigs))),
+			IP: ptr.To(g.getCapacity(networkInterface, cpicIPs)),
 		}
 		return []*NodeEgressIPConfiguration{config}, nil //nolint:staticcheck
 	}
@@ -235,25 +245,22 @@ func (g *GCP) getSubnet(networkInterface *google.NetworkInterface) (*net.IPNet,
 
 // Note: there is also a global "alias IP per VPC quota", but OpenShift clusters on
 // GCP seem to have that value defined to 15,000. So we can skip that.
-func (g *GCP) getCapacity(networkInterface *google.NetworkInterface, cloudPrivateIPsCount int) int {
-	currentIPv4Usage := 0
-	currentIPv6Usage := 0
+func (g *GCP) getCapacity(networkInterface *google.NetworkInterface, cpicIPs sets.Set[string]) int {
+	currentIPUsage := 0
 	for _, aliasIPRange := range networkInterface.AliasIpRanges {
+		var aliasIP net.IP
 		if assignedIP := net.ParseIP(aliasIPRange.IpCidrRange); assignedIP != nil {
-			if utilnet.IsIPv4(assignedIP) {
-				currentIPv4Usage++
-			} else {
-				currentIPv6Usage++
-			}
+			aliasIP = assignedIP
 		} else if _, assignedSubnet, err := net.ParseCIDR(aliasIPRange.IpCidrRange); err == nil {
-			if utilnet.IsIPv4CIDR(assignedSubnet) {
-				currentIPv4Usage++
-			} else {
-				currentIPv6Usage++
-			}
+			aliasIP = assignedSubnet.IP
+		}
+
+		if aliasIP != nil && !cpicIPs.Has(aliasIP.String()) {
+			currentIPUsage++
 		}
 	}
-	return defaultGCPPrivateIPCapacity + cloudPrivateIPsCount - currentIPv4Usage - currentIPv6Usage
+
+	return defaultGCPPrivateIPCapacity - currentIPUsage
 }
 
 // getInstance retrieves the GCP instance referred by the Node object.
@@ -317,3 +324,20 @@ func (g *GCP) parseSubnet(subnetURL string) (string, string, string, error) {
 	return subnetURLParts[len(subnetURLParts)-5], subnetURLParts[len(subnetURLParts)-3],
 		subnetURLParts[len(subnetURLParts)-1], nil
 }
+
+// getNodeLock retrieves node lock from nodeLockMap, If lock doesn't exist, then update map
+// with a new lock entry for the given node name.
+func (g *GCP) getNodeLock(nodeName string) *sync.Mutex {
+	g.nodeMapLock.Lock()
+	defer g.nodeMapLock.Unlock()
+	if _, ok := g.nodeLockMap[nodeName]; !ok {
+		g.nodeLockMap[nodeName] = &sync.Mutex{}
+	}
+	return g.nodeLockMap[nodeName]
+}
+
+func (g *GCP) CleanupNode(nodeName string) {
+	g.nodeMapLock.Lock()
+	defer g.nodeMapLock.Unlock()
+	delete(g.nodeLockMap, nodeName)
+}