Add support to use different subnets in different node pools #1411

fiunchinho · 2021-06-04T11:27:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
It allows to define several different subnets for nodes, not just one. Then we can specify the subnet to use when creating AzureMachines or AzureMachinePools.

Which issue(s) this PR fixes:
Fixes #664
Fixes #1362

Special notes for your reviewer:
I kept the role field in the subnets. I think it's still useful to see that a subnet is for control-plane or for the nodes.

I've added a new SubnetName field , but if it's empty, capz will try to find the right subnet to use. This makes the code work with some already existing scenarios.

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:

Add support to use different subnets in different node pools

k8s-ci-robot · 2021-06-04T11:28:00Z

Hi @fiunchinho. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

api/v1alpha4/azuremachine_types.go

fiunchinho · 2021-06-29T15:19:30Z

api/v1alpha4/azurecluster_default.go

+	var nodeSubnetFound bool
+	var nodeSubnetCounter int
+	for i := range c.Spec.NetworkSpec.Subnets {
+		if c.Spec.NetworkSpec.Subnets[i].Role == SubnetNode {


To keep the same spirit as before, we only require users to specify subnet name and role when specifying subnets. The rest of the fields will be defaulted. That's why we need DefaultNodeSubnetCIDRPattern, so we can generate different cidrs for different subnets.

fiunchinho · 2021-06-29T15:20:55Z

api/v1alpha4/azuremachine_types.go

+
+	// SubnetName selects the Subnet where the VM will be placed
+	// +optional
+	SubnetName string `json:"subnetName,omitempty"`


This field is optional so that the code keeps working with existing manifests. In the future we may think about changing it to be required?

should we add validation to make it required when there is more than one subnet specified? otherwise it could be ambiguous

Just to clarify: is your concern addressed by this? https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/1411/files#diff-b6931ac61f37a13f66fc7836e039bf714a0553d5db4f313cc8ef31ca5f5647b9R542-R563

fiunchinho · 2021-07-19T15:04:58Z

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

fiunchinho · 2021-07-19T15:35:57Z

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

fiunchinho · 2021-07-20T13:56:04Z

@shysank @CecileRobertMichon I believe I addressed all the comments, please take a look, thank you!!

CecileRobertMichon

Could you please add some docs about this in https://capz.sigs.k8s.io/topics/custom-vnet.html ?

shysank · 2021-07-20T21:23:02Z

azure/scope/machine.go

+		}
+	}
+
+	// If Nat Gateway is not enabled, then the NIC needs to reference the LB to get outbound traffic.


Yes, make sense. Thanks for the detailed explanation. Is it possible to abstract the details in machineScope.Subnet(), and perhaps add a method Subnet.IsNatGatewayEnabled() so that we can do something like m.Subnet().IsNatGatewayEnabled()

azure/scope/cluster.go

shysank · 2021-07-20T21:35:09Z

azure/services/natgateways/natgateways.go

+			ID:   azure.NatGatewayID(s.Scope.SubscriptionID(), s.Scope.ResourceGroup(), natGatewaySpec.Name),
+			Name: natGatewaySpec.Name,
+			NatGatewayIP: infrav1.PublicIPSpec{
+				Name: *(*natGatewayToCreate.NatGatewayPropertiesFormat.PublicIPAddresses)[0].ID,


We'll either need to create a new field ID for public ip spec, or not set it. There was an issue reported for this: #1525

I changed it so that it uses the name instead of the ID.

shysank · 2021-07-20T21:54:41Z

azure/scope/machine.go

+// SetSubnetName defaults the AzureMachine subnet name defined when empty to the name of the subnet with role 'node' where there is only one of them.
+// Note: this logic exists only for purposes of ensuring backwards compatibility for old clusters created without the `subnetName` field being set, and should be removed in the future.


IIUC, this also handles setting subnet name for control plane nodes. Unless, we are planning to implement multiple subnets for cp nodes, this is cannot be removed, I think.

The plan would be to make the subnetName field to be mandatory. Once that happens, this logic is not needed anymore. The same applies for both control-plane and node subnets.

I will tweak the comment a bit, because it only refers to node subnets, and as you mentioned, it also applies to the control-plane subnet.

fiunchinho · 2021-07-21T12:00:03Z

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

k8s-ci-robot · 2021-07-21T14:32:57Z

@fiunchinho: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`0f4a838`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fiunchinho · 2021-07-21T16:26:27Z

Could you please add some docs about this in https://capz.sigs.k8s.io/topics/custom-vnet.html ?

I added some docs, PTAL.

shysank · 2021-07-21T18:03:08Z

/lgtm Awesome work @fiunchinho! Thanks for being patient with the reviews!

CecileRobertMichon · 2021-07-21T21:53:27Z

/label tide/merge-method-squash

CecileRobertMichon

/lgtm
/approve

thanks for leaving the code cleaner than you found it 🧹

k8s-ci-robot · 2021-07-21T22:37:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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~~ [CecileRobertMichon]

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 requested review from alexeldeib and cpanato June 4, 2021 11:27

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 4, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 4, 2021

CecileRobertMichon reviewed Jun 4, 2021

View reviewed changes

api/v1alpha4/azuremachine_types.go Outdated Show resolved Hide resolved

api/v1alpha4/azuremachine_types.go Outdated Show resolved Hide resolved

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

fiunchinho force-pushed the multiple-subnets branch from eeaaabf to f2b6595 Compare June 16, 2021 11:25

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

fiunchinho force-pushed the multiple-subnets branch 2 times, most recently from be95c69 to 24246df Compare June 16, 2021 14:25

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 16, 2021

fiunchinho force-pushed the multiple-subnets branch from 24246df to eb1eb0b Compare June 17, 2021 16:40

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2021

fiunchinho force-pushed the multiple-subnets branch 2 times, most recently from 673484f to c0ca96f Compare June 29, 2021 15:12

fiunchinho commented Jun 29, 2021

View reviewed changes

fiunchinho force-pushed the multiple-subnets branch 2 times, most recently from 3a9c884 to 130bf20 Compare July 19, 2021 14:23

fiunchinho force-pushed the multiple-subnets branch from 130bf20 to c1426fe Compare July 19, 2021 15:46

CecileRobertMichon reviewed Jul 20, 2021

View reviewed changes

shysank reviewed Jul 20, 2021

View reviewed changes

fiunchinho force-pushed the multiple-subnets branch 2 times, most recently from d54fdd9 to b0db456 Compare July 21, 2021 10:45

💎 Add support to use different subnets in different node pools

0f4a838

fiunchinho force-pushed the multiple-subnets branch from b0db456 to 0f4a838 Compare July 21, 2021 14:30

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 21, 2021

CecileRobertMichon reviewed Jul 21, 2021

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Jul 21, 2021

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021

k8s-ci-robot merged commit e1faf08 into kubernetes-sigs:master Jul 21, 2021

k8s-ci-robot added this to the v0.5.1 milestone Jul 21, 2021

fiunchinho deleted the multiple-subnets branch July 22, 2021 15:27

This was referenced Aug 9, 2021

fix: don't create outbound LB if using NatGateway #1589

Merged

Don't create outbound LB if nat-gateway is enabled #1587

Closed

CecileRobertMichon mentioned this pull request Feb 2, 2023

Add subnetName support to ammp #3132

Merged

3 tasks

fiunchinho mentioned this pull request Oct 9, 2023

REQUEST: New membership for fiunchinho kubernetes/org#4510

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to use different subnets in different node pools #1411

Add support to use different subnets in different node pools #1411

fiunchinho commented Jun 4, 2021 •

edited

Loading

k8s-ci-robot commented Jun 4, 2021

fiunchinho Jun 29, 2021

fiunchinho Jun 29, 2021

CecileRobertMichon Jul 6, 2021

fiunchinho Jul 19, 2021 •

edited

Loading

fiunchinho commented Jul 19, 2021

fiunchinho commented Jul 19, 2021

fiunchinho commented Jul 20, 2021

CecileRobertMichon left a comment

shysank Jul 20, 2021 •

edited

Loading

shysank Jul 20, 2021

fiunchinho Jul 21, 2021

shysank Jul 20, 2021

fiunchinho Jul 21, 2021 •

edited

Loading

fiunchinho commented Jul 21, 2021

k8s-ci-robot commented Jul 21, 2021

fiunchinho commented Jul 21, 2021

shysank commented Jul 21, 2021

CecileRobertMichon commented Jul 21, 2021

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 21, 2021

		// SetSubnetName defaults the AzureMachine subnet name defined when empty to the name of the subnet with role 'node' where there is only one of them.
		// Note: this logic exists only for purposes of ensuring backwards compatibility for old clusters created without the `subnetName` field being set, and should be removed in the future.

Add support to use different subnets in different node pools #1411

Add support to use different subnets in different node pools #1411

Conversation

fiunchinho commented Jun 4, 2021 • edited Loading

k8s-ci-robot commented Jun 4, 2021

fiunchinho Jun 29, 2021

Choose a reason for hiding this comment

fiunchinho Jun 29, 2021

Choose a reason for hiding this comment

CecileRobertMichon Jul 6, 2021

Choose a reason for hiding this comment

fiunchinho Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

fiunchinho commented Jul 19, 2021

fiunchinho commented Jul 19, 2021

fiunchinho commented Jul 20, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

shysank Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

shysank Jul 20, 2021

Choose a reason for hiding this comment

fiunchinho Jul 21, 2021

Choose a reason for hiding this comment

shysank Jul 20, 2021

Choose a reason for hiding this comment

fiunchinho Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

fiunchinho commented Jul 21, 2021

k8s-ci-robot commented Jul 21, 2021

fiunchinho commented Jul 21, 2021

shysank commented Jul 21, 2021

CecileRobertMichon commented Jul 21, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 21, 2021

fiunchinho commented Jun 4, 2021 •

edited

Loading

fiunchinho Jul 19, 2021 •

edited

Loading

shysank Jul 20, 2021 •

edited

Loading

fiunchinho Jul 21, 2021 •

edited

Loading