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.
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
[DualStack] Support NSG and clean LBs #3898
Conversation
✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lzhecheng 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 |
/retest |
a81bbfd
to
7217c25
Compare
669012f
to
ed6e470
Compare
/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz |
d814ca9
to
59de6d3
Compare
/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz |
/retest |
4192a01
to
98a237d
Compare
/retest |
e02069b
to
4ca9b1e
Compare
tests/e2e/network/node.go
Outdated
@@ -197,7 +197,7 @@ var _ = Describe("Azure node resources", Label(utils.TestSuiteLabelNode), func() | |||
|
|||
utils.Logf("getting all NICs of VMSS VMs") | |||
for _, vmssVM := range vmssVMs { | |||
utils.Logf("Checking %d VMSS VM %q", vmssVM.Name) | |||
utils.Logf("Checking %d VMSS VM %q", *vmssVM.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
pkg/provider/azure_vmssflex.go
Outdated
@@ -989,7 +989,9 @@ func (fs *FlexScaleSet) ensureBackendPoolDeletedFromNode(vmssFlexVMNameMap map[s | |||
nics[nicName] = nic | |||
} | |||
} | |||
nicUpdaterLock := "nicUpdaterLock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 atomic.Store() may be better than lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update in the other PR and rebase.
klog.V(2).Infof("IPv6 is not supported for private link service, skip reconcilePrivateLinkService for service(%s)", serviceName) | ||
return nil | ||
} | ||
if !isDualStack { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this if at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -135,10 +135,10 @@ func (bc *backendPoolTypeNodeIPConfig) CleanupVMSetFromBackendPoolByCondition(sl | |||
}) | |||
} | |||
if v4Enabled { | |||
findBackendpoolToBeDeleted(false) | |||
findBackendpoolToBeDeleted(IsIPv4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call findBackendpoolToBeDeleted directly?
pkg/provider/azure_standard.go
Outdated
@@ -296,8 +296,8 @@ func getBackendPoolName(clusterName string, isIPv6 bool) string { | |||
// getBackendPoolNames returns the IPv4 and IPv6 backend pool names. | |||
func getBackendPoolNames(clusterName string) map[bool]string { | |||
return map[bool]string{ | |||
false: getBackendPoolName(clusterName, false), | |||
true: getBackendPoolName(clusterName, true), | |||
IsIPv4: getBackendPoolName(clusterName, IsIPv4), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we define a enum type for IsIPv4 and IsIPv6? I'm not sure these two variables are mutually exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsIPv4 is actually NotIPv6, so they are mutually exclusive.
/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz |
/test pull-cloud-provider-azure-e2e-ccm-capz |
tests/e2e/network/node.go
Outdated
@@ -197,7 +197,7 @@ var _ = Describe("Azure node resources", Label(utils.TestSuiteLabelNode), func() | |||
|
|||
utils.Logf("getting all NICs of VMSS VMs") | |||
for _, vmssVM := range vmssVMs { | |||
utils.Logf("Checking %d VMSS VM %q", vmssVM.Name) | |||
utils.Logf("Checking VMSS VM %q", *vmssVM.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we safely refer to this variable securely by invoking to. String?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
/retest |
pkg/provider/azure_loadbalancer.go
Outdated
@@ -68,10 +68,10 @@ func (az *Cloud) existsPip(clusterName string, service *v1.Service) bool { | |||
return existingPip | |||
} | |||
|
|||
if v4Enabled && !existsPipSingleStack(false) { | |||
if v4Enabled && !existsPipSingleStack(IsIPv4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better name would be IPVersionIPv4. IsIPv4 sounds like a variable, not a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
pkg/provider/azure_loadbalancer.go
Outdated
// Before DualStack support, old logic takes the first ingress IP as non-additional one | ||
// and the second one as additional one. With DualStack support, the second IP may be | ||
// the IP of another IP family so the new logic returns two variables. | ||
func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.LoadBalancer) (status *v1.LoadBalancerStatus, lbIPsNoAdditionalPIPs []string, fipConfigs []*network.FrontendIPConfiguration, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is noAdditional means primary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means compared to ingress IPs in status
, lbIPsNoAdditionalPIPs
doesn't include those IPs from service.beta.kubernetes.io/azure-additional-public-ips
annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be better to replace to primary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, NoAdditional
seems too straightforward. Replaced.
pkg/provider/azure_utils.go
Outdated
@@ -34,6 +34,11 @@ import ( | |||
"sigs.k8s.io/cloud-provider-azure/pkg/consts" | |||
) | |||
|
|||
const ( | |||
IsIPv6 bool = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to consts.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
53d35e2
to
813fd2f
Compare
Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
* Support NSG and clean LBs for dualstack * Support related UTs for dualstack * Refactor Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: