VPC: Add subnet reconciliation #1970

cjschaef · 2024-09-18T21:03:24Z

Add support to reoncile VPC subnets for the new v2 VPC Infrastructure reoncile logic.

Related: #1896

What this PR does / why we need it:
Add new support for reconciling VPC subnets for VPC Infra

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 #

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:

Add support to new (v2) reconciliation of Subnets for VPC Infrastructure

k8s-ci-robot · 2024-09-18T21:03:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cjschaef
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-09-18T21:03:34Z

Hi @cjschaef. 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-09-18T21:03:40Z

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

Name	Link
🔨 Latest commit	`6689ccf`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/66fab794c67a7d0008972da5
😎 Deploy Preview	https://deploy-preview-1970--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

Overall LGTM

Karthik-K-N · 2024-09-19T04:44:07Z

cloud/scope/vpc_cluster.go

+
+	// Reconcile Control Plane subnets.
+	requeue := false
+	for _, subnet := range subnets {


If no inter dependency, In future for performance optimization we can think of reconciling data and control plane subnets concurrently.

That is likely possible, if subnet creation is found to take longer than desired I can revisit that. I know Custom Image and LB's are the larger time sinks (in terms of waiting for them to be Ready, and SG/SGR reconciliation is heavily complex and can take time as well.

Karthik-K-N · 2024-09-19T04:56:37Z

cloud/scope/vpc_cluster.go

+	return nil
+}
+
+// findOrCreatePublicGateway will attempt to find if there is an existing Public Gateway for a specific zone, for the cluster (in cluster's Resource Group and VPC), or create a new one. Only one Public Gateway is required in each zone, for any subnets in that zone.


For my better understanding, Do you plan to support multiple subnets in same zone, Do you have any use case in mind when we will need to do this.

Yes, the simple path here, is when using different subnets for Control Plane and Data Plane, distributed across the same zones (one CP subnet per zone and one DP subnet per zone).
Otherwise, if the user desires to include edge subnets, or distribute nodes within smaller subnets IP CIDR's, multiple subnets could be used per zone as well.

Karthik-K-N · 2024-09-19T05:23:05Z

/ok-to-test

dharaneeshvrd · 2024-09-19T14:45:47Z

cloud/scope/vpc_cluster.go

+	// If no Worker subnets were supplied, attempt to create one in each zone.
+	if s.IBMVPCCluster.Spec.Network.WorkerSubnets == nil || len(s.IBMVPCCluster.Spec.Network.WorkerSubnets) == 0 {
+		// If neither Control Plane nor Worker subnets were supplied, we rely on both Planes using the same subnet per zone, and we will re-reconcile those subnets below, for IBMVPCCluster Status updates.
+		if len(s.IBMVPCCluster.Spec.Network.ControlPlaneSubnets) != 0 {


You have mentioned as when both are not supplied rely on same subnets.
Don't you need to compare the len equals to 0 here instead of not equals to?

I could reword, but no, in this case if some Control Plane subnets were supplied, we will auto-generate some Worker subnets.
If neither got provided, then Worker subnets will use the Control Plane subnets that were auto-generated.

dharaneeshvrd · 2024-09-19T14:50:18Z

cloud/scope/vpc_cluster.go

+		// If we have the subnet name, we can easily check Network Stauts on the subnet's status.
+		if isControlPlane {
+			// If we find the subnet in Control Plane subnet status and that it is ready, we can return, with no requeue required.
+			if subnet, ok := s.NetworkStatus().ControlPlaneSubnets[*subnet.Name]; ok && subnet.Ready {


s.NetworkStatus() != nil is not required here?

Yeah, that is a good idea to be safe.

dharaneeshvrd · 2024-09-19T14:51:07Z

cloud/scope/vpc_cluster.go

+			}
+		} else {
+			// If we find the subnet in Worker subnet status and that it is ready, we can return, with no requeue required.
+			if subnet, ok := s.NetworkStatus().WorkerSubnets[*subnet.Name]; ok && subnet.Ready {


same as prev comment.

will update.

dharaneeshvrd · 2024-09-19T15:02:33Z

cloud/scope/vpc_cluster.go

+		// If we have the subnet name, we can easily check Network Stauts on the subnet's status.
+		if isControlPlane {
+			// If we find the subnet in Control Plane subnet status and that it is ready, we can return, with no requeue required.
+			if subnet, ok := s.NetworkStatus().ControlPlaneSubnets[*subnet.Name]; ok && subnet.Ready {


Even if we find it in status, don't we need to check the cloud that its still in desired state?
Please take care of this in all the similar blocks.

The discussion I had previously, was that with certain VPC resources, if the details are already in Status, checking the realtime status of said resource would not need to happen. For instance, handling cases where CAPI created a resource, and someone/thing deleted the resource, having CAPI attempt to re-reconcile that case, even though Status had a name and ID (which are no longer valid, like ID) would not be desirable.

As VPC and Subnets, do not typically change status, unlike Load Balancers, the expectation is that once these resources are Ready (or the equivalent VPC status value/constant) it should stay that way until deletion (either requested or done maliciously). Load Balancers, due to updating being a frequent occurrence, likely would not be able to rely on previous status (instead checking status in realtime).

If this expectation has changed, and CAPI is expected to recreate all VPC resources (if they disappear, API fails to find them, etc.), I can change all resource logic to no rely on Status details (such as ID) if that changes in realtime.

@Karthik-K-N @mkumatag could you please share your opinion here?

We usually follow this pattern of fetching id from status and checking its actual status in cloud, Its to make sure that the observed resource status is correct and up to date with real world.

I think its better to follow this pattern as its helps in early detection of any potential problems.

Should reconciliation completely stop (panic) if a resource cannot be found then?
Or should resources be recreated and update Status with new ID's, etc.?

As of now reconciler will throw error. For reference. Its upto the user to resolve the error.

I will attempt to refactor, with the expectation that anything in Status that cannot be found in IBM Cloud, etc. results in an continuous error. Other resources will be updated in the same fashion in other PR's then.

dharaneeshvrd · 2024-09-19T15:09:39Z

cloud/scope/vpc_cluster.go

+}
+
+// checkSubnetStatus will check the status of a IBM Cloud Subnet and update the Network Status.
+func (s *VPCClusterScope) checkSubnetStatus(subnetDetails *vpcv1.Subnet, isControlPlane bool) (bool, error) {


I feel we need to rename this if you are going to update the status also as part of this func.

I will change to update.

dharaneeshvrd · 2024-09-19T15:20:28Z

cloud/scope/vpc_cluster.go

+	}
+
+	// Add a tag to the subnet for the cluster.
+	err = s.TagResource(s.IBMVPCCluster.Name, *subnetDetails.CRN)


Hope tagging subnet is tested? We have nt tried before.

Yes it does. I don't perform exhaustive testing, due to time constraints, but I test all the reconciliation logic after each set of changes for the standard path. So tagging works on subnets (based on the API I don't think GlobalTagging cares what a resource is, as long as it has a CRN), even though the UI leaves a lot to be desired in terms of tagging support.

# ibmcloud resource search "tags:\"us-east-capi-subnet-1-v2vzf\"" | grep "Name\|Resource Type" Name: us-east-capi-subnet-1-v2vzf-pgateway-us-east-1 Resource Type: public-gateway Name: us-east-capi-subnet-1-v2vzf-subnet-compute-us-east-1 Resource Type: subnet Name: us-east-capi-subnet-1-v2vzf-subnet-control-plane-us-east-1 Resource Type: subnet Name: us-east-capi-subnet-1-v2vzf-pgateway-us-east-2 Resource Type: public-gateway Name: us-east-capi-subnet-1-v2vzf-subnet-control-plane-us-east-2 Resource Type: subnet Name: us-east-capi-subnet-1-v2vzf-subnet-compute-us-east-2 Resource Type: subnet Name: us-east-capi-subnet-1-v2vzf-pgateway-us-east-3 Resource Type: public-gateway Name: us-east-capi-subnet-1-v2vzf-subnet-compute-us-east-3 Resource Type: subnet Name: us-east-capi-subnet-1-v2vzf-subnet-control-plane-us-east-3 Resource Type: subnet Name: us-east-capi-subnet-1-v2vzf-rhcos Resource Type: image Name: us-east-capi-subnet-1-v2vzf-vpc Resource Type: vpc

cjschaef · 2024-09-19T17:25:30Z

I don't see this error is local linting, and other instances don't appear to fail

controllers/ibmvpccluster_controller.go:268:176: printf: non-constant format string in call to sigs.k8s.io/cluster-api/util/conditions.MarkFalse (govet)
		conditions.MarkFalse(clusterScope.IBMVPCCluster, infrav1beta2.VPCSubnetReadyCondition, infrav1beta2.VPCSubnetReconciliationFailedReason, capiv1beta1.ConditionSeverityError, err.Error())
		                                                                                                                                                                             ^

Going to assume this is a test flake and get the test to rebuild.

cjschaef · 2024-09-19T18:12:17Z

ah, think I found the error and have fixed it now.

Karthik-K-N · 2024-09-20T05:10:47Z

cloud/scope/vpc_cluster.go

+	}
+
+	options := &vpcv1.CreateSubnetOptions{}
+	options.SetSubnetPrototype(&vpcv1.SubnetPrototype{


Currently in existing VPC flow we create subnet with these properties https://github.com/Karthik-K-N/cluster-api-provider-ibmcloud/blob/83e571fe6e85a6edd94d46b762a1844f9f7dd195/cloud/scope/cluster.go#L219-L236. Do you think its not required to set CIDR block or is it not required when you set TotalIpv4AddressCount?

It seems like the two are mutually exclusive (not completely sure though), so you are able to define one or the other (total IPs or a CIDR).
Given the history we have, I opted to keep using the total IP's for now, but once we consider allowing this to be configurable, we could provide both as options (total IP's or CIDR's).

dharaneeshvrd · 2024-09-25T04:46:22Z

cloud/scope/vpc_cluster.go

+			if _, ok := subnetMap[*subnet.Name]; ok {
+				subnetName = subnet.Name
+			}
+		} else if subnetID != nil && subnet.ID != nil {


Why are you comparing subnetID != nil here?
Are n't you defining this few lines above?

Yes, I'll clean that up.

dharaneeshvrd · 2024-09-25T04:53:28Z

cloud/scope/vpc_cluster.go

+	if subnetDetails == nil || subnetDetails.ID == nil || subnetDetails.CRN == nil {
+		return fmt.Errorf("error failed creating subnet: %s", *subnet.Name)
+	}
+
+	// Initially populate subnet's status.
+	resourceStatus := &infrav1beta2.ResourceStatus{
+		ID:    *subnetDetails.ID,
+		Name:  subnetDetails.Name,
+		Ready: false,
+	}
+	if isControlPlane {
+		s.SetResourceStatus(infrav1beta2.ResourceTypeControlPlaneSubnet, resourceStatus)
+	} else {
+		s.SetResourceStatus(infrav1beta2.ResourceTypeWorkerSubnet, resourceStatus)
+	}


can we use updateSubnetStatus() here?

I think it's better to default during the initial population and update on followup loops.

And to be honest, it doesn't matter anymore, we will have a followup loop after subnet(s) creation, so we will callback to updateSubnetStatus on the next iteration.

No, what I meant is that I don't see a code difference here from line 986 to 1000 comparing to updateSubnetStatus() func.
So why can't we just call that func instead of these dup lines?

The code diff is Ready: false is hardcoded here, compared with the function lookup.

Won't that func sets the ready as false if you pass the subnet details from here? Since it won't be available immediately after creation.
IMHO we can just use that func here instead of building and setting the status from here. It looks duplicate to me.
@Karthik-K-N can you please see whether my opinion making sense here?

Suggested change

if subnetDetails == nil || subnetDetails.ID == nil || subnetDetails.CRN == nil {

return fmt.Errorf("error failed creating subnet: %s", *subnet.Name)

}

// Initially populate subnet's status.

resourceStatus := &infrav1beta2.ResourceStatus{

ID: *subnetDetails.ID,

Name: subnetDetails.Name,

Ready: false,

}

if isControlPlane {

s.SetResourceStatus(infrav1beta2.ResourceTypeControlPlaneSubnet, resourceStatus)

} else {

s.SetResourceStatus(infrav1beta2.ResourceTypeWorkerSubnet, resourceStatus)

}

if subnetDetails == nil || subnetDetails.ID == nil || subnetDetails.CRN == nil {

return fmt.Errorf("error failed creating subnet: %s", *subnet.Name)

}

return s.updateSubnetStatus(subnetDetails, isControlPlane)

I am not sure whats will be the value for these subnetDetails, for the newly created subnet
but if either subnetDetails.Status is nil or subnetDetails.Status not vpcv1.SubnetStatusAvailableConst then anyhow we are setting ready as false. so we can reuse I believe.

subnetDetails.Status != nil && *subnetDetails.Status == string(vpcv1.SubnetStatusAvailableConst)

If create succeeds, it would populate and give subnetDetails properly or else it will throw an error.
Not sure in what case it would return err as nil and subnetDetails also as nil.

dharaneeshvrd · 2024-09-25T05:03:35Z

cloud/scope/vpc_cluster.go

+	}
+
+	// Otherwise, if these is an ID or name, attempt to lookup the subnet and update status as necessary.
+	if subnet.ID != nil {


If you found an existing subnet with id or name, wondering how are you planning to differentiate this during delete phase? Just asking for my understanding!
If it via tags, hope all the resources involved are having CRN like you mentioned, to attach the tags.

At this moment, I don't have solid plans for deletion logic. Tagging is going to likely be involved/required to properly handle all of the edge cases involved with bring your own 'resource' (vpc, subnet, image, etc.).
Yes, I have already gone through and tested the tagging required for the resource CAPI is creating, but deletion has not been a focus, given time constraints.

Karthik-K-N

Overall LGTM

mkumatag · 2024-09-30T04:22:07Z

cloud/scope/vpc_cluster.go

+		return nil, err
+	}
+	if subnet == nil {
+		return nil, nil


wondering if we can return some custom error here for better consumption?

mkumatag · 2024-09-30T04:50:13Z

cloud/scope/vpc_cluster.go

+	var subnets []infrav1beta2.Subnet
+	var err error
+	// If no ControlPlane Subnets were supplied, we default to create one in each availability zone of the region.
+	if s.IBMVPCCluster.Spec.Network.ControlPlaneSubnets == nil || len(s.IBMVPCCluster.Spec.Network.ControlPlaneSubnets) == 0 {


nil check is not required for the ControlPlaneSubnets as it is a slice and len function will take care of that.

mkumatag · 2024-09-30T04:52:05Z

cloud/scope/vpc_cluster.go

+// For Subnets, we collect all of the required subnets, for each Plane, and reconcile them individually. Requeing if one is missing or just created. Reconciliation is attempted on all subnets each loop, to prevent single subnet creation per reconciliation loop.
+func (s *VPCClusterScope) ReconcileSubnets() (bool, error) {
+	var subnets []infrav1beta2.Subnet
+	var err error


I don't see pointing in declaring this err variable here, can be safely removed and move the declaration wherever needed.

I believe I need this declared here, so that when assigning subnets below, it is not redefined. The linting at least requires the declarations first, before assigning.

mkumatag · 2024-09-30T05:00:55Z

cloud/scope/vpc_cluster.go

+	}
+
+	// If no Worker subnets were supplied, attempt to create one in each zone.
+	if s.IBMVPCCluster.Spec.Network.WorkerSubnets == nil || len(s.IBMVPCCluster.Spec.Network.WorkerSubnets) == 0 {


same as above, remove the nil check

mkumatag · 2024-09-30T05:16:10Z

cloud/scope/vpc_cluster.go

+	} else {
+		// If no ID or name was provided, that is an error to be raised. One or the other must be specified when subnets are supplied.
+		return false, fmt.Errorf("error subnet has no defined id or name, one is required")
+	}


this can be checked in the beginning and return early failure

Okay, I think that is a safe expectation. updated

mkumatag · 2024-09-30T05:17:17Z

cloud/scope/vpc_cluster.go

+		} else if subnetDetails != nil {
+			// Update status if subnet was found.
+			return s.updateSubnetStatus(subnetDetails, isControlPlane)


updateSubnetStatus called multiple times, lets optimise later.

There might be a slight improvement possible, but the complexity of lookup calls, dependent on which fields/values are defined, may make the logic more complex to reduce the duplication of updateSubnetStatus.
I could review once VPC support changes are completed.

mkumatag · 2024-09-30T05:24:04Z

cloud/scope/vpc_cluster.go

+	}
+
+	// TODO(cjschaef): Move to webhook validation.
+	if subnet.Zone == nil {


check and return early

mkumatag · 2024-09-30T05:25:12Z

cloud/scope/vpc_cluster.go

+	var ipCount int64 = 256
+	// We currnetly only support IPv4
+	ipVersion := "ipv4"


can this be moved to constant section in the debugging of the file?

Sure, I moved to consts.

Add support to reoncile VPC subnets for the new v2 VPC Infrastructure reoncile logic. Related: kubernetes-sigs#1896

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Sep 18, 2024

k8s-ci-robot requested review from mkumatag and Prajyot-Parab September 18, 2024 21:03

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 18, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 18, 2024

cjschaef mentioned this pull request Sep 18, 2024

Extend VPC (x86_64) Infrastructure reconciliation support #1896

Open

Karthik-K-N reviewed Sep 19, 2024

View reviewed changes

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 Sep 19, 2024

dharaneeshvrd suggested changes Sep 19, 2024

View reviewed changes

cjschaef force-pushed the vpc_reconcile_subnets branch from 43a3472 to 5828b2a Compare September 19, 2024 16:40

cjschaef force-pushed the vpc_reconcile_subnets branch 2 times, most recently from 8fb2ad3 to f0f141c Compare September 19, 2024 18:10

Karthik-K-N reviewed Sep 20, 2024

View reviewed changes

cjschaef force-pushed the vpc_reconcile_subnets branch from f0f141c to baa81c8 Compare September 24, 2024 17:53

dharaneeshvrd reviewed Sep 25, 2024

View reviewed changes

cjschaef force-pushed the vpc_reconcile_subnets branch from baa81c8 to f670ee5 Compare September 25, 2024 14:23

Karthik-K-N reviewed Sep 27, 2024

View reviewed changes

mkumatag requested changes Sep 30, 2024

View reviewed changes

VPC: Add subnet reconciliation

6689ccf

Add support to reoncile VPC subnets for the new v2 VPC Infrastructure reoncile logic. Related: kubernetes-sigs#1896

cjschaef force-pushed the vpc_reconcile_subnets branch from f670ee5 to 6689ccf Compare September 30, 2024 14:37

VPC: Add subnet reconciliation #1970

Are you sure you want to change the base?

VPC: Add subnet reconciliation #1970

Conversation

cjschaef commented Sep 18, 2024

k8s-ci-robot commented Sep 18, 2024

k8s-ci-robot commented Sep 18, 2024

netlify bot commented Sep 18, 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

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

Karthik-K-N commented Sep 19, 2024

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

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

cjschaef commented Sep 19, 2024

cjschaef commented Sep 19, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karthik-K-N Sep 30, 2024 • edited Loading

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

Karthik-K-N 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

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

netlify bot commented Sep 18, 2024 •

edited

Loading

Karthik-K-N Sep 30, 2024 •

edited

Loading