Use multiple zones in case of multiple subnets #1793

Shilpa-Gokul · 2024-05-23T11:22:00Z

What this PR does / why we need it:
Currently when there are multiple subnets specified, it uses the first available vpcZone for all subnets by default and as a result, there is a clash in the CIDR. Hence added logic to use the subsequent vpcZones for the subnets.

If there are multiple subnets provided than the available vpcZones (ex: when 4 subnets are provided but only 3 vpcZones are available in the region), in this case we return error as below

The IBMPowerVSCluster "capi-test-cluster-sg" is invalid: : Invalid value: "spec.vpcSubnets": eu-es vpc region supports only 3 subnets but 4 subnets were provided
shilpagokul@Shilpas-MacBook-Pro templates %

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

Special notes for your reviewer:

/area provider/ibmcloud

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

Release note:

k8s-ci-robot · 2024-05-23T11:22:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Shilpa-Gokul
Once this PR has been reviewed and has the lgtm label, please assign mkumatag for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-05-23T11:22:09Z

Hi @Shilpa-Gokul. 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-sigs/prow repository.

netlify · 2024-05-23T11:22:16Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`ec9c2de`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/673216caca8e7f0008f386ef
😎 Deploy Preview	https://deploy-preview-1793--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Karthik-K-N

How about handling this in webhooks, Having a default webhook which adds unique zone to subnets if it does not present.
Also I think we can error out if the user specifies more subnets than zone.

@dharaneeshvrd will there be any uses cases of user setting more subnets? like vpc region has 2 zones but user wants to create 4 subnets?

dharaneeshvrd · 2024-05-24T05:33:43Z

will there be any uses cases of user setting more subnets? like vpc region has 2 zones but user wants to create 4 subnets?

its possible, we can just round robin the zones once subnets created for all zones.
i.e. consider the region has 2 zones, 10.241.0.0/24 is already used for subnet 1 for subnet 3, it automatically picks 10.241.1.0/24.

cloud/scope/powervs_cluster.go

Shilpa-Gokul · 2024-07-25T09:33:33Z

Tested the following scenarios

Case 1: When user provides more subnets than the available vpczones and provides no zone information with each subnet (subnets are created in the available vpc zones in a round robin method)

vpcSubnets:
  - name: capi-test-cluster-sg-vpcsubnet
  - name: capi-test-cluster-sg-vpcsubnet-1
  - name: capi-test-cluster-sg-vpcsubnet-2
  - name: capi-test-cluster-sg-vpcsubnet-3
  - name: capi-test-cluster-sg-vpcsubnet-4
  - name: capi-test-cluster-sg-vpcsubnet-5
  - name: capi-test-cluster-sg-vpcsubnet-6
  zone: mad02

Case 2: When user provides 2 subnets in the same zone to be created

 vpcSubnets:
  - name: capi-test-cluster-sg-vpcsubnet-1
    zone: eu-es-2
  - name: capi-test-cluster-sg-vpcsubnet-2
    zone: eu-es-2

Case 3: User provides 1 subnet and the zone to be created in

 vpcSubnets:
  - name: capi-test-cluster-sg-vpcsubnet-1
    zone: eu-es-2

Case 4: When user provides already created subnet

vpc:
  name: capi-test-cluster-sg-vpc-new
  region: eu-es
vpcSubnets:
- name: sn-20240723-01
zone: mad02

Log- [manager] I0723 12:15:38.121366 1 powervs_cluster.go:1103] "VPC subnet ID is set, fetching details" controller="ibmpowervscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="IBMPowerVSCluster" IBMPowerVSCluster="default/capi-test-cluster-sg" namespace="default" name="capi-test-cluster-sg" reconcileID="19a61637-59a3-4ea0-896f-6bb3a4616902" cluster="default/capi-test-cluster-sg" id="02w7-e123549e-4414-480c-b018-7017492889b3"

Case 5: When user provides subnets with no zone information

vpcSubnets:
  - name: capi-test-cluster-sg-vpcsubnet-1
  - name: capi-test-cluster-sg-vpcsubnet-2
  zone: mad02

@Karthik-K-N @Amulyam24 Please check this and let me know if I have to test anything else

Karthik-K-N · 2024-10-29T05:38:45Z

Thanks for the detailed explaination, For usecase1 can we use webhooks to validate?

Shilpa-Gokul · 2024-10-29T15:33:46Z

Thanks for the detailed explaination, For usecase1 can we use webhooks to validate?

Added the validation in the webhook itself, below is the error response

The IBMPowerVSCluster "capi-test-cluster-sg" is invalid: : Invalid value: "spec.vpcSubnets": eu-es vpc region supports only 3 subnets but 4 subnets were provided
shilpagokul@Shilpas-MacBook-Pro templates %

Shilpa-Gokul · 2024-11-07T04:38:16Z

@Karthik-K-N @Prajyot-Parab Addressed all changes suggested, could you please review this PR? Also the checks are in pending state for so many days, retesting is also not working. could you please help on that as well?

Karthik-K-N · 2024-11-07T04:45:22Z

/ok-to-test

api/v1beta2/ibmpowervscluster_webhook.go

cloud/scope/powervs_cluster.go

Karthik-K-N · 2024-11-12T16:21:34Z

LGTM
/assign @dharaneeshvrd

dharaneeshvrd · 2024-11-13T07:27:08Z

@Shilpa-Gokul what happens with this case?
Case 2: When user provides 2 subnets in the same zone to be created
I think we probably need to add a validation to not to allow dup zones, it would cause collision with ip range with current code I guess.

Prajyot-Parab · 2024-11-13T09:20:10Z

cloud/scope/powervs_cluster.go

+	// vpcSubnetIPAddressCount is the total IP Addresses for the subnet. Until support for custom
+	// address prefixes is added, we use the ip count for subnet creation.


Suggested change

// vpcSubnetIPAddressCount is the total IP Addresses for the subnet. Until support for custom

// address prefixes is added, we use the ip count for subnet creation.

// vpcSubnetIPAddressCount is the total IP Addresses for the subnet.

// Support for custom address prefixes will be added at a later time. Currently, we use the ip count for subnet creation.

Prajyot-Parab · 2024-11-13T09:23:39Z

cloud/scope/powervs_cluster.go


 	// create subnet
 	vpcID := s.GetVPCID()
 	if vpcID == nil {
 		return nil, fmt.Errorf("VPC ID is empty")
 	}
-	cidrBlock, err := s.IBMVPCClient.GetSubnetAddrPrefix(*vpcID, zone)


You also need to remove the function definitions as well, if they are not being used anywhere else.

Prajyot-Parab · 2024-11-13T09:30:11Z

cloud/scope/powervs_cluster.go

@@ -1172,7 +1181,10 @@ func (s *PowerVSClusterScope) ReconcileVPCSubnets() (bool, error) {
 		}
 		s.Info("Created VPC subnet", "subnetID", subnetID)
 		s.SetVPCSubnetStatus(*subnet.Name, infrav1beta2.ResourceReference{ID: subnetID, ControllerCreated: ptr.To(true)})
-		return true, nil
+		// Requeue only when all subnets' creation are triggered


Suggested change

// Requeue only when all subnets' creation are triggered

// Requeue only when the creation of all subnets has been triggered.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 23, 2024

k8s-ci-robot requested review from Karthik-K-N and Prajyot-Parab May 23, 2024 11:22

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 23, 2024

Karthik-K-N reviewed May 24, 2024

View reviewed changes

Shilpa-Gokul force-pushed the multiple-subnet branch 3 times, most recently from ed9c04c to 666b32f Compare May 24, 2024 05:54

Shilpa-Gokul force-pushed the multiple-subnet branch from 666b32f to b56cac6 Compare June 3, 2024 09:07

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

Shilpa-Gokul force-pushed the multiple-subnet branch from b56cac6 to 2d53312 Compare June 4, 2024 09:04

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

Shilpa-Gokul force-pushed the multiple-subnet branch 3 times, most recently from 0cdecca to 8fac823 Compare June 4, 2024 09:27

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

mkumatag added this to the Next milestone Jul 23, 2024

Shilpa-Gokul force-pushed the multiple-subnet branch from 8fac823 to 292d3fa Compare July 24, 2024 13:52

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

Karthik-K-N reviewed Jul 25, 2024

View reviewed changes

cloud/scope/powervs_cluster.go Outdated Show resolved Hide resolved

Shilpa-Gokul force-pushed the multiple-subnet branch from 42195bc to 836bbc9 Compare July 25, 2024 08:03

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

Shilpa-Gokul force-pushed the multiple-subnet branch 2 times, most recently from 5229869 to 0e9a02c Compare October 29, 2024 05:12

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 Oct 29, 2024

Shilpa-Gokul force-pushed the multiple-subnet branch 2 times, most recently from bd249cc to c17611e Compare October 29, 2024 15:43

Shilpa-Gokul requested a review from Karthik-K-N November 4, 2024 05:18

Shilpa-Gokul changed the title ~~[WIP]: Use multiple zones in case of multiple subnets~~ Use multiple zones in case of multiple subnets Nov 4, 2024

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 4, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 7, 2024

Shilpa-Gokul added 3 commits November 7, 2024 14:18

Use multiple zones for multiple subnets

0d4ce9f

Fix conflicts

2c6f136

Use TotalIpAddress instead of cidrBlock in creating vpc subnet

3d1680d

Shilpa-Gokul force-pushed the multiple-subnet branch 3 times, most recently from 9b870ce to 76b57c8 Compare November 7, 2024 09:51

Karthik-K-N reviewed Nov 11, 2024

View reviewed changes

api/v1beta2/ibmpowervscluster_webhook.go Outdated Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_webhook.go Outdated Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_webhook.go Show resolved Hide resolved

cloud/scope/powervs_cluster.go Outdated Show resolved Hide resolved

Shilpa-Gokul force-pushed the multiple-subnet branch from 76b57c8 to d1a773e Compare November 11, 2024 10:33

Add validation in webhook

ec9c2de

Shilpa-Gokul force-pushed the multiple-subnet branch from d1a773e to ec9c2de Compare November 11, 2024 14:38

Shilpa-Gokul requested a review from Karthik-K-N November 12, 2024 05:34

k8s-ci-robot assigned dharaneeshvrd Nov 12, 2024

Prajyot-Parab suggested changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use multiple zones in case of multiple subnets #1793

Use multiple zones in case of multiple subnets #1793

Shilpa-Gokul commented May 23, 2024 •

edited

Loading

k8s-ci-robot commented May 23, 2024

k8s-ci-robot commented May 23, 2024

netlify bot commented May 23, 2024 •

edited

Loading

Karthik-K-N left a comment

dharaneeshvrd commented May 24, 2024 •

edited

Loading

Shilpa-Gokul commented Jul 25, 2024

Karthik-K-N commented Oct 29, 2024

Shilpa-Gokul commented Oct 29, 2024

Shilpa-Gokul commented Nov 7, 2024

Karthik-K-N commented Nov 7, 2024

Karthik-K-N commented Nov 12, 2024

dharaneeshvrd commented Nov 13, 2024 •

edited

Loading

Prajyot-Parab Nov 13, 2024

Prajyot-Parab Nov 13, 2024

Prajyot-Parab Nov 13, 2024

		// vpcSubnetIPAddressCount is the total IP Addresses for the subnet. Until support for custom
		// address prefixes is added, we use the ip count for subnet creation.

	// Requeue only when all subnets' creation are triggered
	// Requeue only when the creation of all subnets has been triggered.

Use multiple zones in case of multiple subnets #1793

Are you sure you want to change the base?

Use multiple zones in case of multiple subnets #1793

Conversation

Shilpa-Gokul commented May 23, 2024 • edited Loading

k8s-ci-robot commented May 23, 2024

k8s-ci-robot commented May 23, 2024

netlify bot commented May 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Karthik-K-N left a comment

Choose a reason for hiding this comment

dharaneeshvrd commented May 24, 2024 • edited Loading

Shilpa-Gokul commented Jul 25, 2024

Karthik-K-N commented Oct 29, 2024

Shilpa-Gokul commented Oct 29, 2024

Shilpa-Gokul commented Nov 7, 2024

Karthik-K-N commented Nov 7, 2024

Karthik-K-N commented Nov 12, 2024

dharaneeshvrd commented Nov 13, 2024 • edited Loading

Prajyot-Parab Nov 13, 2024

Choose a reason for hiding this comment

Prajyot-Parab Nov 13, 2024

Choose a reason for hiding this comment

Prajyot-Parab Nov 13, 2024

Choose a reason for hiding this comment

Shilpa-Gokul commented May 23, 2024 •

edited

Loading

netlify bot commented May 23, 2024 •

edited

Loading

dharaneeshvrd commented May 24, 2024 •

edited

Loading

dharaneeshvrd commented Nov 13, 2024 •

edited

Loading