VPC: VPCCluster definition extension #1683

cjschaef · 2024-03-25T20:14:14Z

Extend the VPCCluster definition to include additional VPC resources and expand configuration options.

What this PR does / why we need it: Expands the supported resource creation/management for VPC clusters

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 # N/A

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:

Users will want to migrate to use the new definitions and configuration to take advantage of the extended support for VPCCluster infrastructure.

k8s-ci-robot · 2024-03-25T20:14:23Z

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/test-infra repository.

k8s-ci-robot · 2024-03-25T20:14:26Z

[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 prajyot-parab 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

netlify · 2024-03-25T20:14:31Z

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

Name	Link
🔨 Latest commit	`33d3af9`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/660485895c7efc000826ef94
😎 Deploy Preview	https://deploy-preview-1683--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-03-25T20:22:03Z

api/v1beta2/ibmvpccluster_types.go

+	ControlPlaneLoadBalancer *VPCLoadBalancerSpec `json:"controlPlaneLoadBalancer,omitempty"`
+
+	// cosInstance is the IBM COS instance to use for cluster resources.
+	COSInstance *COSInstance `json:"cosInstance,omitempty"`


If the shared basic types is added in time, this should be COSInstanceReference, from types.go.

I'm dropping this reference, as I don't plan to manage COS resources.

Instead, I will incorporate a new ImageSpec, which will have details about the expected VSI Custom Image, which could include a COS ObjectURL, to create a new Custom Image from, if necessary.

cjschaef · 2024-03-25T20:22:56Z

api/v1beta2/ibmvpccluster_types.go

+// SecurityGroup dummy.
+// TODO(cjschaef): Dummy SecurityGroup until it is defined in a common location.
+type SecurityGroup struct {
+	Name string `json:"name"`
+}
+
+// COSInstance dummy.
+// TODO(cjschaef): Dummy COSInstance until it is defined in a common location.
+type COSInstance struct {
+	Name string `json:"name"`
+}
+
+// VPCResource dummy.
+// TODO(cjschaef): Dummy VPCResource until it is defined in a common location.
+type VPCResource struct {
+	Name string `json:"name"`
+}


I'll remove these dummy types if the other PR's adding them are merged before.
#1670
#1669

Prajyot-Parab · 2024-03-26T05:13:16Z

/ok-to-test

Prajyot-Parab · 2024-03-26T08:00:05Z

/hold
let the other PRs be merged first, to avoid any confusion

mkumatag · 2024-03-27T00:57:48Z

/cc @Karthik-K-N

Karthik-K-N · 2024-03-27T03:26:07Z

api/v1beta2/ibmvpccluster_types.go

+// VPCResourceStatus identifies a resource by crn and type and whether it was created by the controller.
+type VPCResourceStatus struct {
+	// controllerCreated indicates whether the resource is created by the CAPI controller.
+	ControllerCreated bool `json:"controllerCreated,omitempty"`


Since most of VPC resources can be tagged, better to make use of tags instead of ControllerCreated. tags will be very easy comapared to managing state of this variable.

I may need to think on this one more. I could move to rely on tags, but I know in the past we could not, or simply did not, in terms of performing resource management, perhaps a limitation with VPC resources at the time, or the technology/component used to create such resources.
Perhaps CAPI relies on tags information, if possible, and outside layers and logic transform that information as necessary. I may not know more on how to handle this until I start getting deeper into interdependencies, expectancies, etc.

Do you have a suggestion for this, or simply added a Tags []string ...? I didn't see a PowerVS equivalent, and I think the Status results will be necessary for post-Infrastructure-provisioning processing.

Or are you expecting that CAPI (or other components/automation) would look for specific Tags on resources (say a VSI with CAPI-created tag or something like that)?

I dropped the bool for now.

Apologies some how I missed it. Yes what I am thinking is once the capi creates the resource in cloud say VPC, lets tag that resource with tag like CAPI-created or something, so it would help us in deciding on deleting that resource.

In PowerVS we cannot make use of tags as few services doesnot support tagging like DHCP, Network so on

Karthik-K-N · 2024-03-27T03:27:45Z

api/v1beta2/ibmvpccluster_types.go

+
+	// type defines the type of IBM Cloud resource.
+	// +required
+	Type ResourceType `json:"type"`


I think its usage will be like

VPC *VPCResourceStatus `json:"vpc,omitempty" ``` so do you think we need to again set VPC.Type =vpc

Yes I think that would be the simple case, I am concerned about more complex cases.

I will drop the type for now then, and attempt to add it back in if/when I need to when I start making changes to reconcile logic changes.

I dropped the Type for now.

Extend the VPCCluster definition to include additional VPC resources and expand configuration options.

k8s-ci-robot · 2024-04-01T18:35:04Z

PR needs rebase.

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.

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 Mar 25, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 25, 2024

k8s-ci-robot requested review from mkumatag and Prajyot-Parab March 25, 2024 20:14

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 25, 2024

cjschaef force-pushed the vpc_expanded_cluster_crd branch from dfd4d79 to 7fe0b99 Compare March 25, 2024 20:20

cjschaef commented Mar 25, 2024

View reviewed changes

cjschaef marked this pull request as ready for review March 25, 2024 20:23

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 Mar 25, 2024

k8s-ci-robot requested a review from Karthik-K-N March 25, 2024 20:23

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 Mar 26, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2024

Karthik-K-N reviewed Mar 27, 2024

View reviewed changes

VPC: VPCCluster definition extension

33d3af9

Extend the VPCCluster definition to include additional VPC resources and expand configuration options.

cjschaef force-pushed the vpc_expanded_cluster_crd branch from 7fe0b99 to 33d3af9 Compare March 27, 2024 20:46

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

mkumatag added this to the Next milestone May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPC: VPCCluster definition extension #1683

VPC: VPCCluster definition extension #1683

cjschaef commented Mar 25, 2024

k8s-ci-robot commented Mar 25, 2024

k8s-ci-robot commented Mar 25, 2024

netlify bot commented Mar 25, 2024 •

edited

cjschaef Mar 25, 2024

cjschaef Apr 1, 2024

cjschaef Mar 25, 2024

Prajyot-Parab commented Mar 26, 2024

Prajyot-Parab commented Mar 26, 2024 •

edited

mkumatag commented Mar 27, 2024

Karthik-K-N Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

Karthik-K-N Apr 2, 2024

Karthik-K-N Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

k8s-ci-robot commented Apr 1, 2024

VPC: VPCCluster definition extension #1683

Are you sure you want to change the base?

VPC: VPCCluster definition extension #1683

Conversation

cjschaef commented Mar 25, 2024

k8s-ci-robot commented Mar 25, 2024

k8s-ci-robot commented Mar 25, 2024

netlify bot commented Mar 25, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prajyot-Parab commented Mar 26, 2024

Prajyot-Parab commented Mar 26, 2024 • edited

mkumatag commented Mar 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

k8s-ci-robot commented Apr 1, 2024

netlify bot commented Mar 25, 2024 •

edited

Prajyot-Parab commented Mar 26, 2024 •

edited