Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support attaching node private IP to the load balancer backendpools #918

Merged
merged 1 commit into from Dec 9, 2021
Merged

feat: support attaching node private IP to the load balancer backendpools #918

merged 1 commit into from Dec 9, 2021

Conversation

nilo19
Copy link
Contributor

@nilo19 nilo19 commented Nov 29, 2021 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently we bind the VM/VMSS to the LB by attaching their NICs to the LB which needs to call the API of VM/VMSS. This PR introduces a new config loadBalancerBackendPoolConfigurationType and it can be set to nodeIPConfiguration (default) or nodeIP. If set to nodeIPConfiguration, everything will keep unchanged. If set to nodeIP, the cloud provider will call the LB API to attach the node private IPs to the LB instead of linking the NICs to the LB.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Introduce a new config `loadBalancerBackendPoolConfigurationType` and it can be set to `nodeIPConfiguration` (default) or `nodeIP`. If set to `nodeIPConfiguration`, everything will keep unchanged. If set to `vmIP`, the cloud provider will call the LB API to attach the node private IPs to the LB instead of linking the NICs to the LB.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 29, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2021
@coveralls
Copy link

coveralls commented Nov 29, 2021 •
edited

Coverage Status

Coverage decreased (-0.5%) to 79.986% when pulling 46091eb on nilo19:feat/vm-ip into a457bc0 on kubernetes-sigs:master.

@nilo19 nilo19 changed the title feat: support attaching node private IP to the load balancer backendpools [WIP] feat: support attaching node private IP to the load balancer backendpools Nov 29, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2021
@nilo19 nilo19 changed the title [WIP] feat: support attaching node private IP to the load balancer backendpools feat: support attaching node private IP to the load balancer backendpools Nov 29, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2021
feiskyer
feiskyer reviewed Nov 30, 2021
View reviewed changes
pkg/consts/consts.go
// LoadBalancerBackendPoolConfigurationTypeVMIP is the lb backend pool config type vm ip
LoadBalancerBackendPoolConfigurationTypeVMIP = "vmIP"
// LoadBalancerBackendPoolConfigurationTypePODIP is the lb backend pool config type pod ip
LoadBalancerBackendPoolConfigurationTypePODIP = "podIP"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is PodIP support added in this PR? If not, please add a TODO comment to say it is not supported yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

pkg/provider/azure.go
// are `vmNIC`, `vmIP` and `podIP`.
// `vmNIC`: vm network interfaces will be attached to the inbound backend pool of the load balancer (default);
// `vmIP`: vm private IPs will be attached to the inbound backend pool of the load balancer;
// `podIP`: pod IPs will be attached to the inbound backend pool of the load balancer (not supported yet).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the full name here and rename vm to node, e.g. nodeIPConfiguration, nodeIP, podIP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/provider/azure.go
@@ -496,6 +504,20 @@ func (az *Cloud) InitializeCloudFromConfig(config *Config, fromSecret, callFromC
}
}

if config.LoadBalancerBackendPoolConfigurationType == "" ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please omit the character size when comparing the values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/provider/azure.go
@@ -1037,6 +1059,15 @@ func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
}
}

func (az *Cloud) ListNodes(ctx context.Context) ([]v1.Node, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use the nodeInformer to cache all the Node IPs here? refer

cloud-provider-azure/pkg/provider/azure.go

Lines 971 to 1038 in be76f79

func (az *Cloud) updateNodeCaches(prevNode, newNode *v1.Node) {
az.nodeCachesLock.Lock()
defer az.nodeCachesLock.Unlock()
if prevNode != nil {
// Remove from nodeNames cache.
az.nodeNames.Delete(prevNode.ObjectMeta.Name)
// Remove from nodeZones cache.
prevZone, ok := prevNode.ObjectMeta.Labels[consts.LabelFailureDomainBetaZone]
if ok && az.isAvailabilityZone(prevZone) {
az.nodeZones[prevZone].Delete(prevNode.ObjectMeta.Name)
if az.nodeZones[prevZone].Len() == 0 {
az.nodeZones[prevZone] = nil
}
}
// Remove from nodeResourceGroups cache.
_, ok = prevNode.ObjectMeta.Labels[consts.ExternalResourceGroupLabel]
if ok {
delete(az.nodeResourceGroups, prevNode.ObjectMeta.Name)
}
// Remove from unmanagedNodes cache.
managed, ok := prevNode.ObjectMeta.Labels[consts.ManagedByAzureLabel]
if ok && strings.EqualFold(managed, consts.NotManagedByAzureLabelValue) {
az.unmanagedNodes.Delete(prevNode.ObjectMeta.Name)
az.excludeLoadBalancerNodes.Delete(prevNode.ObjectMeta.Name)
}
// Remove from excludeLoadBalancerNodes cache.
if _, hasExcludeBalancerLabel := prevNode.ObjectMeta.Labels[v1.LabelNodeExcludeBalancers]; hasExcludeBalancerLabel {
az.excludeLoadBalancerNodes.Delete(prevNode.ObjectMeta.Name)
}
}
if newNode != nil {
// Add to nodeNames cache.
az.nodeNames.Insert(newNode.ObjectMeta.Name)
// Add to nodeZones cache.
newZone, ok := newNode.ObjectMeta.Labels[consts.LabelFailureDomainBetaZone]
if ok && az.isAvailabilityZone(newZone) {
if az.nodeZones[newZone] == nil {
az.nodeZones[newZone] = sets.NewString()
}
az.nodeZones[newZone].Insert(newNode.ObjectMeta.Name)
}
// Add to nodeResourceGroups cache.
newRG, ok := newNode.ObjectMeta.Labels[consts.ExternalResourceGroupLabel]
if ok && len(newRG) > 0 {
az.nodeResourceGroups[newNode.ObjectMeta.Name] = strings.ToLower(newRG)
}
// Add to unmanagedNodes cache.
managed, ok := newNode.ObjectMeta.Labels[consts.ManagedByAzureLabel]
if ok && strings.EqualFold(managed, consts.NotManagedByAzureLabelValue) {
az.unmanagedNodes.Insert(newNode.ObjectMeta.Name)
az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)
}
// Add to excludeLoadBalancerNodes cache.
if _, hasExcludeBalancerLabel := newNode.ObjectMeta.Labels[v1.LabelNodeExcludeBalancers]; hasExcludeBalancerLabel {
az.excludeLoadBalancerNodes.Insert(newNode.ObjectMeta.Name)
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/provider/azure_backoff.go Outdated
agentPoolVMSetNamesSet.Insert(strings.ToLower(vmSetName))
}
}

for _, lb := range allLBs {
vmSetNameFromLBName := az.mapLoadBalancerNameToVMSet(to.String(lb.Name), clusterName)
if agentPoolVMSetNamesSet.Has(strings.ToLower(vmSetNameFromLBName)) {
if strings.EqualFold(to.String(lb.Name), clusterName) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this condition is this a bug even without this feature?

pkg/provider/azure_loadbalancer.go Outdated
_, vmSetName, err := az.VMSet.GetNodeNameByIPConfigurationID(ipConfigID)
if err != nil && !errors.Is(err, cloudprovider.InstanceNotFound) {
if strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeVMNIC) {
if bp.BackendAddressPoolPropertiesFormat != nil && bp.BackendIPConfigurations != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to get it easier to understand, could you extract the cleanup logic in L234 and L255 to separate functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/provider/azure_loadbalancer.go Outdated
return nil, fmt.Errorf("reconcileSharedLoadBalancer: failed to list managed LB: %w", err)
}
} else {
existingLBs, err = az.ListLB(service)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to merge this two LISTs together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merged

pkg/provider/azure_loadbalancer.go Outdated

if bp.BackendAddressPoolPropertiesFormat != nil {
if strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeVMNIC) &&
bp.BackendIPConfigurations != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to put the backend pool implementations behind the VMSet interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created a new interface LoadBalancerBackendPool

pkg/provider/azure_loadbalancer.go Outdated

useMultipleSLBs := strings.EqualFold(az.LoadBalancerSku, "standard") && az.EnableMultipleStandardLoadBalancers
if strings.EqualFold(az.LoadBalancerSku, "basic") || useMultipleSLBs {
vmSetName, err := az.VMSet.GetNodeVMSetName(node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you skip the standalone VM for this case? (and same for other places)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standalone VM will be skipped because az.VMSet.GetNodeVMSetName will return an empty string.

@nilo19 nilo19 changed the title feat: support attaching node private IP to the load balancer backendpools [WIP] feat: support attaching node private IP to the load balancer backendpools Dec 2, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 2, 2021
@nilo19 nilo19 changed the title [WIP] feat: support attaching node private IP to the load balancer backendpools feat: support attaching node private IP to the load balancer backendpools Dec 6, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2021
feiskyer
feiskyer reviewed Dec 8, 2021
View reviewed changes
pkg/provider/azure_loadbalancer.go Outdated
}

// remove the deleted LB from the list
existingLBs = append(existingLBs[:i], existingLBs[i+1:]...)
}

for _, primarySLB := range primarySLBs {
//var updatedNodeIPs bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

feiskyer
feiskyer reviewed Dec 8, 2021
View reviewed changes
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments. Let's get this merged and please add documents in a following PR.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2021
ZeroMagic
ZeroMagic approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@ZeroMagic ZeroMagic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, nilo19, ZeroMagic

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a53e3e3 into kubernetes-sigs:master Dec 9, 2021
@nilo19 nilo19 deleted the feat/vm-ip branch December 9, 2021 02:14
@nilo19 nilo19 mentioned this pull request Dec 20, 2021
Merged
This was referenced Dec 20, 2021
Merged
Merged
This was referenced Dec 30, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@ZeroMagic ZeroMagic ZeroMagic approved these changes

@andyzhangx andyzhangx Awaiting requested review from andyzhangx

Assignees

@feiskyer feiskyer

@ZeroMagic ZeroMagic

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nilo19 @coveralls @k8s-ci-robot @feiskyer @ZeroMagic