Refactor scalesets NIC config #3188

CecileRobertMichon · 2023-02-17T23:03:20Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it: When #2411 merged it added some duplicate/dead code in the VMSS spec for network interfaces. #3243 fixed some functional issues, this PR cleans up the code to remove dead code.

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 #3224

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

NONE

mweibel

that's much better than before, thanks. I wonder if we should add some more tests (unit tests might suffice?) to ensure we have IP Forwarding enabled and accelerated networking set to the correct value?

mweibel · 2023-03-07T08:11:17Z

azure/services/scalesets/scalesets.go

-						PrivateIPAddressVersion:         compute.IPVersionIPv4,
-						LoadBalancerBackendAddressPools: &backendAddressPools,
+	nicConfigs := []compute.VirtualMachineScaleSetNetworkConfiguration{}
+	for i, n := range vmssSpec.NetworkInterfaces {


Unless I'm mistaken: if len(vmssSpec.NetworkInterfaces) == 0 this would not add any network configuration. Wondering how to handle this best. To keep code here simple, should we maybe add networkInterfaces to the defaulter webhook if none are specified?

Correct, the defaulting in the webhook is already being done so there should never be a case where len(vmssSpec.NetworkInterfaces) == 0:

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/exp/api/v1beta1/azuremachinepool_default.go#L136

hm, true. Wonder why this didn't catch when we upgraded CAPZ.. But maybe there were other issues. Looks fine like this.

What happens when the amp.Spec.Template.SubnetName and amp.Spec.Template.AcceleratedNetworking is not defined in the spec, will we be setting empty subnetName and dereferencing an empty pointer in the defaulting webhook?

and dereferencing an empty pointer

where do you see dereferencing of a pointer in the webhook?

If accelerated networking is nil we use the value of the SKU to determine if it's supported https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/3188/files#diff-a6779b0b37d0dcdbe7dcd6f00150a22e12ac067a96269038452d27ba39349a90R603

For empty subnet name it won't be empty because it's defaulted to the Cluster subnet name if empty before it reaches this part of the code on every reconcile loop:

cluster-api-provider-azure/azure/scope/machinepool.go

Line 726 in 2d7ffd3

if m.AzureMachinePool.Spec.Template.NetworkInterfaces[0].SubnetName == "" {

and dereferencing an empty pointer

where do you see dereferencing of a pointer in the webhook?

Sorry my bad, didn't post the question right.

My other concerns are addressed too. Thanks for pinning the info!

/lgtm

azure/services/scalesets/scalesets.go

mweibel · 2023-03-07T08:16:58Z

azure/services/scalesets/scalesets.go

+	for i, n := range vmssSpec.NetworkInterfaces {
+		nicConfig := compute.VirtualMachineScaleSetNetworkConfiguration{}
+		nicConfig.VirtualMachineScaleSetNetworkConfigurationProperties = &compute.VirtualMachineScaleSetNetworkConfigurationProperties{}
+		nicConfig.Name = pointer.String(vmssSpec.Name + "-nic-" + strconv.Itoa(i))


Suggested change

nicConfig.Name = pointer.String(vmssSpec.Name + "-nic-" + strconv.Itoa(i))

nicConfig.Name = pointer.String(vmssSpec.Name + "-nic-" + strconv.Itoa(i))

nicConfig.EnableIPForwarding = pointer.Bool(true)

EnableIPForwarding is necessary for the CNI to work

Added to maintain what's there, however I'm still not convinced this should be hardcoded. For VMs it's configurable: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/services/networkinterfaces/spec.go#L211

Can you expand on "is necessary for the CNI to work"? Which CNI? What IP family?

From the comment here, it's only needed for Calico + Ipv6

I'm not the expert in CNIs but I believe it is needed for Calico with VXLAN (which is required when you use windows nodes and Calico CNI) - also in IPv4 mode. At least not having IP Forwarding enabled was the root cause of our issues when we ugpraded CAPZ.

Adding this flag as a property on the networkInterfaces object would be nice, however if it's not enabled it might be difficult to figure out whats wrong for a user.

mweibel · 2023-03-07T08:19:26Z

azure/services/scalesets/scalesets.go

+		ipconfigs := []compute.VirtualMachineScaleSetIPConfiguration{}
+		for j := 0; j < n.PrivateIPConfigs; j++ {
+			ipconfig := compute.VirtualMachineScaleSetIPConfiguration{
+				Name: pointer.String(fmt.Sprintf("ipConfig" + strconv.Itoa(j))),


I'm not sure about the semantics of the IPConfiguration API resource. Is it namespaced below the nic config and the name does therefore not matter or do we need to include e.g. vmssSpec.Name in the name to avoid name collisions?

Asking because previous code used the vmss name in the IP configuration too.

I had the same question and tested this, IPConfig name is scoped to the NIC so we don't need VMSS name in there to avoid collision.

I don't have to change this (it works as-is) but the current naming is a bit awkward since all the IP configs end up having the scale set name in it which is not really relevant (especially in the scenario where you have more than 1 NIC per VMSS). This is better aligned with Azure naming conventions.

Also verified this won't be a breaking change. Because we never update the network config on existing scale sets, this change will only apply to new NICs created going forward.

CecileRobertMichon · 2023-03-20T21:53:51Z

@mweibel thanks for the review, I removed the ipv6 change from this PR since it's not quite working yet (it's blocked on #3221), so this is ready for review now.

nawazkh · 2023-03-20T23:41:15Z

Started taking a look at this
/assign

CecileRobertMichon · 2023-03-21T00:42:53Z

/retest

CecileRobertMichon · 2023-03-21T17:16:54Z

/test pull-cluster-api-provider-azure-e2e

just to see if by any chance this repros #3227

CecileRobertMichon · 2023-03-21T18:40:01Z

just to see if by any chance this repros #3227

...and it just did

/retest

CecileRobertMichon · 2023-03-21T20:56:58Z

/retest

nawazkh · 2023-03-21T21:50:11Z

Using this PR, I was able to bring up a cluster with multiple IPConfigs and with Azure CNI. So everything looks good to me so far, just had a concern which I have mentioned above.

❯ clusterctl describe cluster azure-cni-v1-30372
No default config file available
NAME                                                                   READY  SEVERITY  REASON  SINCE  MESSAGE
Cluster/azure-cni-v1-30372                                             True                     26m
├─ClusterInfrastructure - AzureCluster/azure-cni-v1-30372              True                     29m
├─ControlPlane - KubeadmControlPlane/azure-cni-v1-30372-control-plane  True                     26m
│ └─Machine/azure-cni-v1-30372-control-plane-zpxs8                     True                     26m
└─Workers
  └─MachineDeployment/azure-cni-v1-30372-md-0                          True                     24m
    └─2 Machines...                                                    True                     24m    See azure-cni-v1-30372-md-0-7459f94f6b-7kmg8, azure-cni-v1-30372-md-0-7459f94f6b-c4rxg

k8s-ci-robot · 2023-03-21T22:27:46Z

LGTM label has been added.

Git tree hash: bc990f4f7d821f4e1a44e957c12c0b4b9e5bda7f

CecileRobertMichon · 2023-03-21T22:32:21Z

/assign @jackfrancis @mboersma

mweibel · 2023-03-22T08:41:13Z

LGTM

(can't resolve the conversations since I'm not part of kubernetes-sigs)

mboersma

/lgtm
/approve

You had me at "removes 128 lines of code."

k8s-ci-robot · 2023-03-23T17:38:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

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:

~~OWNERS~~ [mboersma]

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

nawazkh · 2023-03-23T20:37:19Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 17, 2023

k8s-ci-robot requested review from marosset and nojnhuh February 17, 2023 23:03

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2023

CecileRobertMichon force-pushed the ipv6-vmss branch 2 times, most recently from 7e26d20 to ccac24e Compare February 18, 2023 00:03

CecileRobertMichon mentioned this pull request Feb 18, 2023

Cannot create a VMSS dual-stack cluster #3163

Closed

CecileRobertMichon mentioned this pull request Mar 6, 2023

MachinePools/VMSS NetworkInterfaces wrong configuration #3224

Closed

mweibel suggested changes Mar 7, 2023

View reviewed changes

mweibel mentioned this pull request Mar 8, 2023

fix: networkInterfaces IPForwarding and AcceleratedNetworking #3243

Merged

3 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023

CecileRobertMichon force-pushed the ipv6-vmss branch from 1f2d87a to da2995c Compare March 20, 2023 21:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2023

CecileRobertMichon force-pushed the ipv6-vmss branch from 16d0dc2 to dbdf066 Compare March 20, 2023 21:40

CecileRobertMichon changed the title ~~[WIP] Enable ipv6 for vmss~~ Refactor scalesets NIC config Mar 20, 2023

Refactor scalesets NIC config

f76a955

CecileRobertMichon force-pushed the ipv6-vmss branch from dbdf066 to f76a955 Compare March 20, 2023 23:00

k8s-ci-robot assigned nawazkh Mar 20, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2023

k8s-ci-robot assigned jackfrancis and mboersma Mar 21, 2023

mboersma approved these changes Mar 23, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2023

k8s-ci-robot merged commit 4622873 into kubernetes-sigs:main Mar 23, 2023

k8s-ci-robot added this to the v1.9 milestone Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor scalesets NIC config #3188

Refactor scalesets NIC config #3188

CecileRobertMichon commented Feb 17, 2023 •

edited

Loading

mweibel left a comment

mweibel Mar 7, 2023

CecileRobertMichon Mar 20, 2023

mweibel Mar 21, 2023

nawazkh Mar 21, 2023

CecileRobertMichon Mar 21, 2023

nawazkh Mar 21, 2023

mweibel Mar 7, 2023

CecileRobertMichon Mar 20, 2023

CecileRobertMichon Mar 20, 2023

mweibel Mar 21, 2023 •

edited

Loading

mweibel Mar 7, 2023

CecileRobertMichon Mar 20, 2023

CecileRobertMichon commented Mar 20, 2023

nawazkh commented Mar 20, 2023

CecileRobertMichon commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

nawazkh commented Mar 21, 2023

k8s-ci-robot commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

mweibel commented Mar 22, 2023

mboersma left a comment

k8s-ci-robot commented Mar 23, 2023

nawazkh commented Mar 23, 2023

	nicConfig.Name = pointer.String(vmssSpec.Name + "-nic-" + strconv.Itoa(i))
	nicConfig.Name = pointer.String(vmssSpec.Name + "-nic-" + strconv.Itoa(i))
	nicConfig.EnableIPForwarding = pointer.Bool(true)

Refactor scalesets NIC config #3188

Refactor scalesets NIC config #3188

Conversation

CecileRobertMichon commented Feb 17, 2023 • edited Loading

mweibel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweibel Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Mar 20, 2023

nawazkh commented Mar 20, 2023

CecileRobertMichon commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

nawazkh commented Mar 21, 2023

k8s-ci-robot commented Mar 21, 2023

CecileRobertMichon commented Mar 21, 2023

mweibel commented Mar 22, 2023

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 23, 2023

nawazkh commented Mar 23, 2023

CecileRobertMichon commented Feb 17, 2023 •

edited

Loading

mweibel Mar 21, 2023 •

edited

Loading