VPC: Create v2 path for new Infrastructure implementation #1858

cjschaef · 2024-06-26T16:36:20Z

Create a v2 path that will be used for the new Infrastructure implementation for VPC Clusters. All new functionality will be placed in these new v2 paths, based on the new NetworkSpec field, to prevent breaking existing implementation.

What this PR does / why we need it: adds new path to fully implement VPC Cluster Infrastructure support w/o breaking current functionality

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:

adding new VPC Infrastructure pathways

k8s-ci-robot · 2024-06-26T16:36:29Z

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-06-26T16:36:37Z

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

Name	Link
🔨 Latest commit	`147250c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/669171a0c69b6f00087887a8
😎 Deploy Preview	https://deploy-preview-1858--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.

cjschaef · 2024-06-26T18:27:26Z

/ok-to-test

k8s-ci-robot · 2024-06-26T18:27:41Z

@cjschaef: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

Prajyot-Parab · 2024-06-27T08:01:34Z

/ok-to-test

Karthik-K-N · 2024-06-28T06:08:03Z

api/v1beta2/conditions_consts.go

+
+	// CreateVPCInfrastructureAnnotation is the name of an annotation that indicates if
+	// VPC infrastructure should be created as part of cluster creation.
+	CreateVPCInfrastructureAnnotation = "vpc.cluster.x-k8s.io/create-infra"


@cjschaef what will be used to differentiate v2 vs v1 code path. I think in VPC we create infra by default so this annotation might not be required. Are you planning to use this as a differentiator?

Yes, I plan on using that and flagging off the NetworkSpec field in the Cluster resource for the v2 path.

ok, If I understand correctly, in V2 path you are expecting NetworkSpec to be mandatory?

That is correct. NetworkSpec is part of the new Infrastructure deployment support, and is required for that support.

Got it thanks for the clarification,
May be we can get started with using that annotation if we can't find any alternative suitable name which matches for what we are doing but eventually we need to remove it as it may confuse the users because now also by default we create the resource.

I could remove the annotation altogether, if we wish to start the so-called "v2" VPC support without requiring an annotation (rely only on using NetworkSpec to flag the new (v2) VPC support, creating all of the required Infrastructure.

I had expected that annotation was required in order to get CAPI to perform Infrastructure Creation/Management. But if we expect for our new (v2) support, CAPI will always be attempting to create/manage Infrastructure resources, I can drop that annotation completely.

Another question, can CAPI be run within a cluster with only the Machine Controller running (not the Cluster/Infra Controller), so as to not attempt to reprovision Infrastructure post cluster creation/install? I had expected this annotation would help with that situation as well, to prevent additional churn by CAPI by continuing to monitor (or deleting) Infrastructure post cluster create.

In PowerVS v1 flow we were not creating any infra rather using existing infra and expecting user to provide them. We introduced this annotation in for v2 to make sure we need to create infra and not expecting anything from user.

This is not true for VPC, in vpc v1 flow as well we create required resources so this annotation does imply anything. But for you to differentiate V2 path as you suggested its better to go with NetworkSpec. If user set NetworkSpec you can assume its V2 flow.

Another question, can CAPI be run within a cluster with only the Machine Controller running (not the Cluster/Infra Controller), so as to not attempt to reprovision Infrastructure post cluster creation/install? I had expected this annotation would help with that situation as well, to prevent additional churn by CAPI by continuing to monitor (or deleting) Infrastructure post cluster create.

No this annotation is nothing to do with that, But to achieve what you are expecting capi provides annotation named [cluster.x-k8s.io/managed-by](https://github.com/Karthik-K-N/cluster-api/blob/e2bf2b7d108ecd51bf7b23bf50e2432e82d8f2fe/api/v1beta1/common_types.go#L154-L160) and our controllers are configured to use them here. But the controllers will be running but will ignore the objects with this annotations.

Okay, thanks for the details, I will drop the annotation. and rely only on the NetworkSpec.

cjschaef · 2024-07-08T16:10:28Z

Re-pushing to try to get CLA check to pass (no idea why it failed all of a sudden).

Karthik-K-N · 2024-07-09T04:55:30Z

api/v1beta2/ibmvpccluster_types.go

+	// This can be different than the Resource Group containing the remaining cluster resources.
+	ResourceGroup *string `json:"resourceGroup,omitempty"`
+
+	// TODO(cjschaef): Complete spec definition (SecurityGroups, VPC)


Are you planning to add any more definition as a part of this PR or in subsequent PRs.

Yes, I plan to add additional details in more functional PR's.

Karthik-K-N · 2024-07-09T05:00:50Z

cloud/scope/vpc_cluster.go

+}
+
+// NewVPCClusterScope creates a new VPCClusterScope from the supplied parameters.
+func NewVPCClusterScope(params VPCClusterScopeParams) (*VPCClusterScope, error) {


I am still not clear on how this will be invoked and embedded into existing flow, Do you mind adding that snippet of code to this PR. Also if possible we can move api definition to seperate PR if possible.

It gets utilized here, in this new v2 path, which allows me to modify the Cluster resource without affecting the existing VPC (legacy) support:
https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/pull/1858/files#diff-647a58f1c11f771d3ec32c60c28c4b8b29442f9915129e16eeddb59120dcc7d8R140-R146

I can attempt to break up this PR, and maybe see if adding anything more for the new VPCClusterScope use that doesn't balloon the PR too much.

API only PR
#1875

It gets utilized here, in this new v2 path, which allows me to modify the Cluster resource without affecting the existing VPC (legacy) support:
I got that but where do you call reconcilerv2?, Something like you need to add a check to see whether vpcclusterspec.NetworkSpec is set or not, If not set go with v1 flow and if set go with new v2 flow?

If this is the case, now if you are expecting user to go with v2 flow say for IPI case but user misses to set networkspec now the controller by default with v1 flow which is not you are expecting and we cannot make networkspec mandatory also, So we dont have any way to add a validation for v2 flow. Its up-to user intelligence and documentation to go with v2 flow..

Yes, that is my expectation, the burden is on the user to provide details in order to utilize the v2 flow.
The architecture deployment for the v2 flow is going to be drastically different than the existing deployment.
In order to achieve and configure this, the user will be on the hook to configure the Cluster resource properly.

I got that but where do you call reconcilerv2?, Something like you need to add a check to see whether vpcclusterspec.NetworkSpec is set or not, If not set go with v1 flow and if set go with new v2 flow?

That is right here
https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/pull/1858/files#diff-647a58f1c11f771d3ec32c60c28c4b8b29442f9915129e16eeddb59120dcc7d8R74-R77

Otherwise, it continues with the legacy (v1) flow

got it thanks

Karthik-K-N

overall LGTM, may be we can merge the api pr then merge this,

Also I see lots of code duplication can be avoided like in NewClusterScope, May be once we have clear picture of v2 flow, we can combine whereever possible.

Thanks.

cjschaef · 2024-07-12T13:33:36Z

Rebased off API updates: #1875

Yes, I imagine some refactoring could be done to handle duplicate code, but the intention is to avoid interacting/modifying the existing VPC flow to simplify testing and support (I am not familiar with the expectations with the current VPC flow). Depending on whether we expect to deprecate and remove the current flow, in favor of the extended VPC support (v2).

mkumatag · 2024-07-12T14:48:15Z

controllers/ibmvpccluster_controller.go

@@ -71,6 +71,11 @@ func (r *IBMVPCClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, err
 	}

+	// Determine whether the Cluster is designed for extended Infrastructure support, implemented in a separate path.
+	if ibmCluster.Spec.Network != nil {


this code block can above line 64 to avoid one extra Get call..

I think this needs to be here, after the ibmCluster variable is defined and then populated via r.Get from the IBMVPCCluster resource on line 65-66.

yep, my bad..

mkumatag · 2024-07-12T14:51:10Z

cloud/scope/vpc_cluster.go

+	}
+
+	if params.IBMVPCCluster.Spec.Network == nil || params.IBMVPCCluster.Spec.Region == "" {
+		return nil, fmt.Errorf("error failed to generate vpc client as NetworkSpec info is nil")


if statement is checking for Region but it is not reported in the error, this may mislead the user..

updated error info

Create a v2 path that will be used for the new Infrastructure implementation for VPC Clusters. All new functionality will be placed in these new v2 paths, based on the new Network field, to prevent breaking existing implementation.

cjschaef · 2024-07-12T18:10:39Z

Repushing to get CLA to pass.

mkumatag

/lgtm

k8s-ci-robot · 2024-07-15T12:10:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjschaef, mkumatag

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

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 added area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 26, 2024

k8s-ci-robot requested review from Karthik-K-N and mkumatag June 26, 2024 16:36

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 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 Jun 27, 2024

cjschaef force-pushed the vpc_v2_branching branch from df3d98d to 4ae8910 Compare June 27, 2024 13:39

Karthik-K-N reviewed Jun 28, 2024

View reviewed changes

cjschaef force-pushed the vpc_v2_branching branch from 4ae8910 to 9cdaddb Compare July 3, 2024 11:48

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 5, 2024

cjschaef force-pushed the vpc_v2_branching branch from 9cdaddb to 37f9952 Compare July 8, 2024 16:11

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 8, 2024

Karthik-K-N reviewed Jul 9, 2024

View reviewed changes

cjschaef mentioned this pull request Jul 9, 2024

VPC: Extend VPC API's to support v2 Infra #1875

Merged

Karthik-K-N reviewed Jul 11, 2024

View reviewed changes

cjschaef force-pushed the vpc_v2_branching branch from 37f9952 to ee96c35 Compare July 12, 2024 13:30

mkumatag requested changes Jul 12, 2024

View reviewed changes

cjschaef force-pushed the vpc_v2_branching branch from ee96c35 to e3a02d5 Compare July 12, 2024 16:07

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2024

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 12, 2024

cjschaef force-pushed the vpc_v2_branching branch from e3a02d5 to 147250c Compare July 12, 2024 18:10

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 12, 2024

mkumatag approved these changes Jul 15, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag Jul 15, 2024

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

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

k8s-ci-robot merged commit 5b01057 into kubernetes-sigs:main Jul 15, 2024
13 checks passed

VPC: Create v2 path for new Infrastructure implementation #1858

VPC: Create v2 path for new Infrastructure implementation #1858

Conversation

cjschaef commented Jun 26, 2024

k8s-ci-robot commented Jun 26, 2024

netlify bot commented Jun 26, 2024 • edited Loading

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

cjschaef commented Jun 26, 2024

k8s-ci-robot commented Jun 26, 2024

Prajyot-Parab commented Jun 27, 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

Karthik-K-N Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjschaef commented Jul 8, 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

Karthik-K-N left a comment

Choose a reason for hiding this comment

cjschaef commented Jul 12, 2024

Choose a reason for hiding this comment

cjschaef Jul 12, 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

cjschaef commented Jul 12, 2024

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 15, 2024

netlify bot commented Jun 26, 2024 •

edited

Loading

Karthik-K-N Jul 3, 2024 •

edited

Loading

cjschaef Jul 12, 2024 •

edited

Loading