Add support of standard LB to Azure vmss #62707
Conversation
k8s-ci-robot
added
release-note
feiskyer
force-pushed
the
vmss-standard-lb
branch
from
66b3ede to
ec5bb51
Compare
| if az.useStandardLoadBalancer() { | ||
| return "" | ||
| } | ||
|
|
There was a problem hiding this comment.
This will affect EnsureHostsInPool for availabilitySet if standard loadbalancer is on?
There was a problem hiding this comment.
@karataliu Yep, so availabilitySet logic is also updated here
|
/retest |
1 similar comment
|
/retest |
| standardNodes := []*v1.Node{} | ||
|
|
||
| for _, curNode := range nodes { | ||
| curScaleSetName, err := extractScaleSetNameByExternalID(curNode.Spec.ExternalID) |
There was a problem hiding this comment.
ExternalID is deprecated and being removed #61877, can we turn to providerId.
|
|
||
| if instanceIDs.Len() == 0 { | ||
| // This may happen when scaling a vmss capacity to 0. | ||
| return fmt.Errorf("no nodes running are belonging to vmss %q", ssName) |
There was a problem hiding this comment.
If there're 2 scalesets, do we allow one to be with 0 instances? What about just skip in this case.
There was a problem hiding this comment.
hmm, makes sense, let's just allow 0 instances.
| curScaleSetName, err := extractScaleSetNameByExternalID(curNode.Spec.ExternalID) | ||
| if err != nil { | ||
| glog.V(4).Infof("Node %q is not belonging to any scale sets, assuming it is belong to availability sets", curNode.Name) | ||
| standardNodes = append(standardNodes, curNode) |
There was a problem hiding this comment.
Call excludeMasterNodesFromStandardLB and verify master nodes?
Also what do we do if master node is a scaleset?
There was a problem hiding this comment.
Will add a check before extractScaleSetNameByExternalID, which would work for both cases
| // the same network interface couldn't be added to more than one load balancer of | ||
| // the same type. Omit those nodes (e.g. masters) so Azure ARM won't complain | ||
| // about this. | ||
| backendPool := *newBackendPools[0].ID |
There was a problem hiding this comment.
why not go through all newBackendPools here?
There was a problem hiding this comment.
We only need to check one because they are in same LB
There was a problem hiding this comment.
The pools is on path
VMSS -> Properties/NetworkProfile/networkInterfaceConfigurations/IPConfigurations/loadBalancerBackendAddressPools
Thus they can belong to different load balancers, right?
If master VMSS node is there and already with two LBs, the pool could be:
[
{ "id": "/*/Microsoft.Network/loadBalancers/master-internal/backendAddressPools/default" },
{ "id": "/*/Microsoft.Network/loadBalancers/master/backendAddressPools/default" }
]
If not checking the second one, it may later add this node to another public LB and the call would fail. Please correct me if there's something I missed.
There was a problem hiding this comment.
Good catch. There is also same issue with standard vm codes. Will fix togather in a new commit.
|
|
||
| for ssName, instanceIDs := range scalesets { | ||
| // Only add nodes belonging to specified vmSet to basic LB backends. | ||
| if !ss.useStandardLoadBalancer() && ssName != vmSetName { |
There was a problem hiding this comment.
Need to check vmSetName != "" like others? We'd better add a note what does it mean for vmSetName==""
There was a problem hiding this comment.
vmSetName won't be "" now, see updates here
There was a problem hiding this comment.
Also use !strings.EqualFold to align with Line438?
feiskyer
force-pushed
the
vmss-standard-lb
branch
from
ec5bb51 to
efb7ae6
Compare
|
@karataliu Rebased and addressed comments. PTAL |
feiskyer
force-pushed
the
vmss-standard-lb
branch
from
1831b61 to
c69cea4
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, karataliu 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 |
|
Automatic merge from submit-queue (batch tested with PRs 62857, 62707). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Add support of standard LB to Azure vmss.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #60485
Special notes for your reviewer:
Release note:
/sig azure