Skip to content

Commit

Permalink
Reduce listPIP calls
Browse files Browse the repository at this point in the history
  • Loading branch information
@jwtty
jwtty authored and k8s-infra-cherrypick-robot committed Apr 22, 2022
1 parent 73cf6a7 commit eef341f
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 49 deletions.
68 changes: 38 additions & 30 deletions pkg/provider/azure_loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (az *Cloud) GetLoadBalancer(ctx context.Context, clusterName string, servic
// there is a chance that we could orphan public IP resources while we delete the load blanacer (kubernetes/kubernetes#80571).
// We need to make sure the existence of the load balancer depends on the load balancer resource and public IP resource on Azure.
existsPip := func() bool {
pipName, _, err := az.determinePublicIPName(clusterName, service)
pipName, _, err := az.determinePublicIPName(clusterName, service, nil)
if err != nil {
return false
}
Expand Down Expand Up @@ -94,7 +94,7 @@ func (az *Cloud) reconcileService(ctx context.Context, clusterName string, servi
return nil, err
}

lbStatus, _, err := az.getServiceLoadBalancerStatus(service, lb)
lbStatus, _, err := az.getServiceLoadBalancerStatus(service, lb, nil)
if err != nil {
klog.Errorf("getServiceLoadBalancerStatus(%s) failed: %v", serviceName, err)
return nil, err
Expand Down Expand Up @@ -640,6 +640,9 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string,
}
}

// reuse pip list to reduce api call
var pips *[]network.PublicIPAddress

// check if the service already has a load balancer
for i := range existingLBs {
existingLB := existingLBs[i]
Expand Down Expand Up @@ -676,7 +679,7 @@ func (az *Cloud) getServiceLoadBalancer(service *v1.Service, clusterName string,
continue
}
var fipConfig *network.FrontendIPConfiguration
status, fipConfig, err = az.getServiceLoadBalancerStatus(service, &existingLB)
status, fipConfig, err = az.getServiceLoadBalancerStatus(service, &existingLB, pips)
if err != nil {
return nil, nil, false, err
}
Expand Down Expand Up @@ -810,7 +813,7 @@ func (az *Cloud) selectLoadBalancer(clusterName string, service *v1.Service, exi
return selectedLB, existsLb, nil
}

func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.LoadBalancer) (status *v1.LoadBalancerStatus, fipConfig *network.FrontendIPConfiguration, err error) {
func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.LoadBalancer, pips *[]network.PublicIPAddress) (status *v1.LoadBalancerStatus, fipConfig *network.FrontendIPConfiguration, err error) {
if lb == nil {
klog.V(10).Info("getServiceLoadBalancerStatus: lb is nil")
return nil, nil, nil
Expand All @@ -822,7 +825,7 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L
isInternal := requiresInternalLoadBalancer(service)
serviceName := getServiceName(service)
for _, ipConfiguration := range *lb.FrontendIPConfigurations {
owns, isPrimaryService, err := az.serviceOwnsFrontendIP(ipConfiguration, service)
owns, isPrimaryService, err := az.serviceOwnsFrontendIP(ipConfiguration, service, pips)
if err != nil {
return nil, nil, fmt.Errorf("get(%s): lb(%s) - failed to filter frontend IP configs with error: %w", serviceName, to.String(lb.Name), err)
}
Expand Down Expand Up @@ -876,7 +879,7 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L
return nil, nil, nil
}

func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, bool, error) {
func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service, pips *[]network.PublicIPAddress) (string, bool, error) {
var shouldPIPExisted bool
if name, found := service.Annotations[consts.ServiceAnnotationPIPName]; found && name != "" {
shouldPIPExisted = true
Expand All @@ -894,7 +897,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service)

// For the services with loadBalancerIP set, an existing public IP is required, primary
// or secondary, or a public IP not found error would be reported.
pip, err := az.findMatchedPIPByLoadBalancerIP(service, loadBalancerIP, pipResourceGroup)
pip, err := az.findMatchedPIPByLoadBalancerIP(service, loadBalancerIP, pipResourceGroup, pips)
if err != nil {
return "", shouldPIPExisted, err
}
Expand All @@ -906,13 +909,15 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service)
return "", shouldPIPExisted, fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
}

func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {
pips, err := az.ListPIP(service, pipResourceGroup)
if err != nil {
return nil, err
func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string, pips *[]network.PublicIPAddress) (*network.PublicIPAddress, error) {
if pips == nil {
pipList, err := az.ListPIP(service, pipResourceGroup)
if err != nil {
return nil, err
}
pips = &pipList
}

for _, pip := range pips {
for _, pip := range *pips {
if pip.PublicIPAddressPropertiesFormat.IPAddress != nil &&
*pip.PublicIPAddressPropertiesFormat.IPAddress == loadBalancerIP {
return &pip, nil
Expand Down Expand Up @@ -1320,8 +1325,8 @@ func getDomainNameLabel(pip *network.PublicIPAddress) string {
return to.String(pip.PublicIPAddressPropertiesFormat.DNSSettings.DomainNameLabel)
}

func (az *Cloud) isFrontendIPChanged(clusterName string, config network.FrontendIPConfiguration, service *v1.Service, lbFrontendIPConfigName string) (bool, error) {
isServiceOwnsFrontendIP, isPrimaryService, err := az.serviceOwnsFrontendIP(config, service)
func (az *Cloud) isFrontendIPChanged(clusterName string, config network.FrontendIPConfiguration, service *v1.Service, lbFrontendIPConfigName string, pips *[]network.PublicIPAddress) (bool, error) {
isServiceOwnsFrontendIP, isPrimaryService, err := az.serviceOwnsFrontendIP(config, service, pips)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -1353,7 +1358,7 @@ func (az *Cloud) isFrontendIPChanged(clusterName string, config network.Frontend
}
return config.PrivateIPAllocationMethod != network.IPAllocationMethodStatic || !strings.EqualFold(loadBalancerIP, to.String(config.PrivateIPAddress)), nil
}
pipName, _, err := az.determinePublicIPName(clusterName, service)
pipName, _, err := az.determinePublicIPName(clusterName, service, pips)
if err != nil {
return false, err
}
Expand Down Expand Up @@ -1482,9 +1487,10 @@ func findMatchedOutboundRuleFIPConfig(fipConfigID *string, outboundRuleFIPConfig
func (az *Cloud) findFrontendIPConfigOfService(
fipConfigs *[]network.FrontendIPConfiguration,
service *v1.Service,
pips *[]network.PublicIPAddress,
) (*network.FrontendIPConfiguration, bool, error) {
for _, config := range *fipConfigs {
owns, isPrimaryService, err := az.serviceOwnsFrontendIP(config, service)
owns, isPrimaryService, err := az.serviceOwnsFrontendIP(config, service, pips)
if err != nil {
return nil, false, err
}
Expand Down Expand Up @@ -1742,11 +1748,13 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
newConfigs = *lb.FrontendIPConfigurations
}

// Save pip list so it can be reused in loop
var pips *[]network.PublicIPAddress
var ownedFIPConfig *network.FrontendIPConfiguration
if !wantLb {
for i := len(newConfigs) - 1; i >= 0; i-- {
config := newConfigs[i]
isServiceOwnsFrontendIP, _, err := az.serviceOwnsFrontendIP(config, service)
isServiceOwnsFrontendIP, _, err := az.serviceOwnsFrontendIP(config, service, pips)
if err != nil {
return nil, false, err
}
Expand Down Expand Up @@ -1783,13 +1791,13 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
)
for i := len(newConfigs) - 1; i >= 0; i-- {
config := newConfigs[i]
isServiceOwnsFrontendIP, _, _ := az.serviceOwnsFrontendIP(config, service)
isServiceOwnsFrontendIP, _, _ := az.serviceOwnsFrontendIP(config, service, pips)
if !isServiceOwnsFrontendIP {
klog.V(4).Infof("reconcileFrontendIPConfigs for service (%s): the frontend IP configuration %s does not belong to the service", serviceName, to.String(config.Name))
continue
}
klog.V(4).Infof("reconcileFrontendIPConfigs for service (%s): checking owned frontend IP cofiguration %s", serviceName, to.String(config.Name))
isFipChanged, err = az.isFrontendIPChanged(clusterName, config, service, defaultLBFrontendIPConfigName)
isFipChanged, err = az.isFrontendIPChanged(clusterName, config, service, defaultLBFrontendIPConfigName, pips)
if err != nil {
return nil, false, err
}
Expand All @@ -1802,7 +1810,7 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv
break
}

ownedFIPConfig, _, err = az.findFrontendIPConfigOfService(&newConfigs, service)
ownedFIPConfig, _, err = az.findFrontendIPConfigOfService(&newConfigs, service, pips)
if err != nil {
return nil, false, err
}
Expand Down Expand Up @@ -1850,7 +1858,7 @@ func (az *Cloud) reconcileFrontendIPConfigs(clusterName string, service *v1.Serv

fipConfigurationProperties = &configProperties
} else {
pipName, shouldPIPExisted, err := az.determinePublicIPName(clusterName, service)
pipName, shouldPIPExisted, err := az.determinePublicIPName(clusterName, service, pips)
if err != nil {
return nil, false, err
}
Expand Down Expand Up @@ -2843,8 +2851,15 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
shouldPIPExisted bool
)

pipResourceGroup := az.getPublicIPAddressResourceGroup(service)

pips, err := az.ListPIP(service, pipResourceGroup)
if err != nil {
return nil, err
}

if !isInternal && wantLb {
desiredPipName, shouldPIPExisted, err = az.determinePublicIPName(clusterName, service)
desiredPipName, shouldPIPExisted, err = az.determinePublicIPName(clusterName, service, &pips)
if err != nil {
return nil, err
}
Expand All @@ -2858,13 +2873,6 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
lb = &loadBalancer
}

pipResourceGroup := az.getPublicIPAddressResourceGroup(service)

pips, err := az.ListPIP(service, pipResourceGroup)
if err != nil {
return nil, err
}

discoveredDesiredPublicIP, pipsToBeDeleted, deletedDesiredPublicIP, pipsToBeUpdated, err := az.getPublicIPUpdates(clusterName, service, pips, wantLb, isInternal, desiredPipName, serviceName, serviceIPTagRequest, shouldPIPExisted)
if err != nil {
return nil, err
Expand Down
10 changes: 5 additions & 5 deletions pkg/provider/azure_loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1675,7 +1675,6 @@ func TestIsFrontendIPChanged(t *testing.T) {
}

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes()
mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
for _, existingPIP := range test.existingPIPs {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *existingPIP.Name, gomock.Any()).Return(existingPIP, nil).AnyTimes()
Expand All @@ -1687,7 +1686,7 @@ func TestIsFrontendIPChanged(t *testing.T) {
test.service.Spec.LoadBalancerIP = test.loadBalancerIP
test.service.Annotations[consts.ServiceAnnotationLoadBalancerInternalSubnet] = test.annotations
flag, rerr := az.isFrontendIPChanged("testCluster", test.config,
&test.service, test.lbFrontendIPConfigName)
&test.service, test.lbFrontendIPConfigName, &test.existingPIPs)
if rerr != nil {
fmt.Println(rerr.Error())
}
Expand Down Expand Up @@ -1735,13 +1734,14 @@ func TestDeterminePublicIPName(t *testing.T) {
expectedError: false,
},
}

for i, test := range testCases {
az := GetTestCloud(ctrl)
service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
service.Spec.LoadBalancerIP = test.loadBalancerIP

mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).AnyTimes()
mockPIPsClient.EXPECT().List(gomock.Any(), "rg").Return(test.existingPIPs, nil).MaxTimes(1)
mockPIPsClient.EXPECT().CreateOrUpdate(gomock.Any(), "rg", gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
for _, existingPIP := range test.existingPIPs {
mockPIPsClient.EXPECT().Get(gomock.Any(), "rg", *existingPIP.Name, gomock.Any()).Return(existingPIP, nil).AnyTimes()
Expand All @@ -1750,7 +1750,7 @@ func TestDeterminePublicIPName(t *testing.T) {
t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
}
}
ip, _, err := az.determinePublicIPName("testCluster", &service)
ip, _, err := az.determinePublicIPName("testCluster", &service, nil)
assert.Equal(t, test.expectedIP, ip, "TestCase[%d]: %s", i, test.desc)
assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
}
Expand Down Expand Up @@ -2802,7 +2802,7 @@ func TestGetServiceLoadBalancerStatus(t *testing.T) {
}

for i, test := range testCases {
status, _, err := az.getServiceLoadBalancerStatus(test.service, test.lb)
status, _, err := az.getServiceLoadBalancerStatus(test.service, test.lb, nil)
assert.Equal(t, test.expectedStatus, status, "TestCase[%d]: %s", i, test.desc)
assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/provider/azure_standard.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ func (az *Cloud) serviceOwnsRule(service *v1.Service, rule string) bool {
// This means the name of the config can be tracked by the service UID.
// 2. The secondary services must have their loadBalancer IP set if they want to share the same config as the primary
// service. Hence, it can be tracked by the loadBalancer IP.
func (az *Cloud) serviceOwnsFrontendIP(fip network.FrontendIPConfiguration, service *v1.Service) (bool, bool, error) {
func (az *Cloud) serviceOwnsFrontendIP(fip network.FrontendIPConfiguration, service *v1.Service, pips *[]network.PublicIPAddress) (bool, bool, error) {
var isPrimaryService bool
baseName := az.GetLoadBalancerName(context.TODO(), "", service)
if strings.HasPrefix(to.String(fip.Name), baseName) {
Expand All @@ -353,7 +353,7 @@ func (az *Cloud) serviceOwnsFrontendIP(fip network.FrontendIPConfiguration, serv
// for external secondary service the public IP address should be checked
if !requiresInternalLoadBalancer(service) {
pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
pip, err := az.findMatchedPIPByLoadBalancerIP(service, loadBalancerIP, pipResourceGroup)
pip, err := az.findMatchedPIPByLoadBalancerIP(service, loadBalancerIP, pipResourceGroup, pips)
if err != nil {
klog.Warningf("serviceOwnsFrontendIP: unexpected error when finding match public IP of the service %s with loadBalancerLP %s: %v", service.Name, loadBalancerIP, err)
return false, isPrimaryService, nil
Expand Down
7 changes: 1 addition & 6 deletions pkg/provider/azure_standard_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
cloudprovider "k8s.io/cloud-provider"

"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/interfaceclient/mockinterfaceclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/publicipclient/mockpublicipclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmasclient/mockvmasclient"
"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/vmclient/mockvmclient"
azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
Expand Down Expand Up @@ -1787,11 +1786,7 @@ func TestServiceOwnsFrontendIP(t *testing.T) {
}

for _, test := range testCases {
mockPIPClient := mockpublicipclient.NewMockInterface(ctrl)
cloud.PublicIPAddressesClient = mockPIPClient
mockPIPClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(test.existingPIPs, nil).MaxTimes(1)

isOwned, isPrimary, err := cloud.serviceOwnsFrontendIP(test.fip, test.service)
isOwned, isPrimary, err := cloud.serviceOwnsFrontendIP(test.fip, test.service, &test.existingPIPs)
assert.Equal(t, test.expectedErr, err, test.desc)
assert.Equal(t, test.isOwned, isOwned, test.desc)
assert.Equal(t, test.isPrimary, isPrimary, test.desc)
Expand Down
12 changes: 6 additions & 6 deletions pkg/provider/azure_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,7 @@ func TestReconcileSecurityGroupNewServiceAddsPort(t *testing.T) {
mockLBBackendPool.EXPECT().ReconcileBackendPools(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, false, nil).AnyTimes()
mockLBBackendPool.EXPECT().EnsureHostsInPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
lb, _ := az.reconcileLoadBalancer(testClusterName, &svc1, clusterResources.nodes, true)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc1, lb)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc1, lb, nil)

sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, &lbStatus.Ingress[0].IP, true /* wantLb */)
if err != nil {
Expand Down Expand Up @@ -1219,7 +1219,7 @@ func TestReconcileSecurityGroupNewInternalServiceAddsPort(t *testing.T) {
mockLBBackendPool.EXPECT().EnsureHostsInPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()

lb, _ := az.reconcileLoadBalancer(testClusterName, &svc1, clusterResources.nodes, true)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc1, lb)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc1, lb, nil)
sg, err := az.reconcileSecurityGroup(testClusterName, &svc1, &lbStatus.Ingress[0].IP, true /* wantLb */)
if err != nil {
t.Errorf("Unexpected error: %q", err)
Expand Down Expand Up @@ -1248,7 +1248,7 @@ func TestReconcileSecurityGroupRemoveService(t *testing.T) {
lb, _ := az.reconcileLoadBalancer(testClusterName, &service1, clusterResources.nodes, true)
_, _ = az.reconcileLoadBalancer(testClusterName, &service2, clusterResources.nodes, true)

lbStatus, _, _ := az.getServiceLoadBalancerStatus(&service1, lb)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&service1, lb, nil)

sg := getTestSecurityGroup(az, service1, service2)
validateSecurityGroup(t, sg, service1, service2)
Expand Down Expand Up @@ -1279,7 +1279,7 @@ func TestReconcileSecurityGroupRemoveServiceRemovesPort(t *testing.T) {
expectedLBs := make([]network.LoadBalancer, 0)
setMockLBs(az, ctrl, &expectedLBs, "service", 1, 1, true)
lb, _ := az.reconcileLoadBalancer(testClusterName, &svc, clusterResources.nodes, true)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc, lb)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc, lb, nil)

sg, err := az.reconcileSecurityGroup(testClusterName, &svcUpdated, &lbStatus.Ingress[0].IP, true /* wantLb */)
if err != nil {
Expand Down Expand Up @@ -1310,7 +1310,7 @@ func TestReconcileSecurityWithSourceRanges(t *testing.T) {
expectedLBs := make([]network.LoadBalancer, 0)
setMockLBs(az, ctrl, &expectedLBs, "service", 1, 1, false)
lb, _ := az.reconcileLoadBalancer(testClusterName, &svc, clusterResources.nodes, true)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc, lb)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc, lb, nil)

sg, err := az.reconcileSecurityGroup(testClusterName, &svc, &lbStatus.Ingress[0].IP, true /* wantLb */)
if err != nil {
Expand Down Expand Up @@ -1350,7 +1350,7 @@ func TestReconcileSecurityGroupEtagMismatch(t *testing.T) {
mockLBBackendPool.EXPECT().EnsureHostsInPool(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()

lb, _ := az.reconcileLoadBalancer(testClusterName, &svc1, clusterResources.nodes, true)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc1, lb)
lbStatus, _, _ := az.getServiceLoadBalancerStatus(&svc1, lb, nil)

newSG, err := az.reconcileSecurityGroup(testClusterName, &svc1, &lbStatus.Ingress[0].IP, true /* wantLb */)
assert.Nil(t, newSG)
Expand Down

0 comments on commit eef341f

Please sign in to comment.