VPC: Create basic types for VPC #1670

cjschaef · 2024-03-18T20:01:38Z

Create a basic VPC resource type that VPC definitions will use.

What this PR does / why we need it: VPC updates will need references to generic VPC resources. This could be shared with PowerVS, if they wish to adopt them.

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: Replaces #1668

/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-03-18T20:01:47Z

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.

netlify · 2024-03-18T20:01:56Z

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

Name	Link
🔨 Latest commit	`dc4e223`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/660dc42d54c9c000082eab2f
😎 Deploy Preview	https://deploy-preview-1670--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.

mkumatag · 2024-03-22T08:18:01Z

/cc @Karthik-K-N @Prajyot-Parab

Prajyot-Parab · 2024-03-22T09:07:50Z

/ok-to-test

Prajyot-Parab · 2024-03-22T09:08:14Z

lgtm
@Karthik-K-N ptal

k8s-ci-robot · 2024-03-25T17:51:55Z

[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

Prajyot-Parab · 2024-03-26T07:55:43Z

api/v1beta2/types.go

+	Name string `json:"name"`
+
+	// id is the ID of the COS instance.
+	// +optional
+	ID *string `json:"id,omitempty"`


do we really need both Name and ID field?

We usually have both Name and ID fields for resources, just incase want to speed up the look up when ID is known.

I can remove, I simply copied the COSInstance from PowerVS code, as is.
If you have found you don't need name, I will remove it here as well.

Looks like the current PowerVS has no ID's, I kind of favor Id's over names and currently expect CAPI will not be creating COS Instances or resources....so I will see about using Id's over names ATM.
Will update.

I dropped Name for now.

We cannot drop Name as thats the only way user can specify the Name of COSInstance.

Also It wont be good to add VPCResource to COSInstance as both are two different entities in IBM Cloud. May be if we would rename VPCResource to something else to more appropriate as its just represents ID and Name in general, then only we can do.

My expectation at this time, is that the controller will not be creating COS resources, but it may be expected to create a VPC Custom Image, using a qcow2 file in a COS bucket, hence the need for this COS struct. I could rely only on a COS bucket CRN, but if this resource can be re-used, I expected Name/ID would be more beneficial, fields set depending on the requirements (PowerVS vs. VPC, etc.)

However, this is my best assumption on the workflow, as limitations may require changes and I am unsure if PowerVS creates COS resources (bucket maybe?) for the VSI Image. So now I wonder if VPC should deviate from the flow PowerVS uses.
It is early on in development, so I can mirror the flow PowerVS uses, for consistency, which would mean I follow similar field requirements (Name's)?

Perhaps you have an opinion on whether VPC should follow similar processes/expectations as PowerVS, when it makes sense. I am not opposed to relying on the controller to create COS resources if necessary, but I had expected the controller to rely on an existing COS Instance and Bucket and ideally the qcow2 Object holding the RHCOS image, and not be responsible for creating it.

I think this is what causing confusion, There are two things

In PowerVS we create cos instance and bucket to upload the ignition data as a object and pass the object URL as userdata while creating machine.

We have ibmpowervs_image_controller which takes care of importing the RHCOS image from respective COS bucket to where redhat will push the image into the PowerVS service instance.

The question I guess I still have is, do the COS instance and bucket need to be created as a prerequisite? I hadn't seen anything in the CAPI code that creates the instance and the bucket (only the Object/File), but I certainly could have missed that.

If this is the case, and I would follow the same flow as PowerVS (create the COS instance and bucket outside CAPI), that will influence what values I want, although I have seen I am limited by the IBM COS SDK, as far as values as well.

We do create COS instance and bucket as a part of IBMPowerVSCluster reconciler

cluster-api-provider-ibmcloud/controllers/ibmpowervscluster_controller.go

Lines 200 to 203 in 2a0c281

if err := clusterScope.ReconcileCOSInstance(); err != nil {

conditions.MarkFalse(powerVSCluster, infrav1beta2.COSInstanceReadyCondition, infrav1beta2.COSInstanceReconciliationFailedReason, capiv1beta1.ConditionSeverityError, err.Error())

return reconcile.Result{}, err

}

Prajyot-Parab · 2024-03-26T07:56:05Z

api/v1beta2/types.go

+	BucketName *string `json:"bucketName,omitempty"`
+
+	// bucketID is the IBM Cloud COS bucket ID.
+	// +optional
+	BucketID *string `json:"bucketID,omitempty"`


same comment as cos (do we really need both Name and ID field?)

Same as above, I can remove if you have found both to be unnecessary.

Same update as above, will try only relying on ID's

I dropped BucketName for now.

Karthik-K-N · 2024-03-27T03:18:31Z

api/v1beta2/types.go

+	Name string `json:"name"`
+
+	// id is the ID of the COS instance.
+	// +optional
+	ID *string `json:"id,omitempty"`


We usually have both Name and ID fields for resources, just incase want to speed up the look up when ID is known.

Karthik-K-N · 2024-03-27T03:20:24Z

api/v1beta2/types.go

+	Name *string `json:"name,omitempty"`
+
+	// type is the type of VPC resource.
+	Type *ResourceType `json:"type,omitempty"`


Would like to understand how this will be used. any example?

A VPCResource can be any kind of VPC resource (VSI, LB, SG, SGR, etc.).
Since I would only have a name or ID, it is helpful to have additional details as to what type of resource it is, IMHO.

I dropped Type for now.

same comments as above, if you have a plan on how to use that ID in controller that might help us in understanding this better.

Karthik-K-N · 2024-03-29T03:26:45Z

api/v1beta2/types.go

+	// bucketID is the IBM Cloud COS bucket ID.
+	// +optional
+	BucketID *string `json:"bucketID,omitempty"`
+


So if we want to reuse the same struct for PowerVS as well then lets add BucketName and Name here.
So this allows us to create and use the COSinstance and Bucket with user provided name.

Just for clarification, In PowerVS we create these resources to upload the ignition data.

I plan to handle the process for VSI Custom Image using a VPC unique type instead, and since Ignition data management is expected to occur elsewhere, I don't think COS details like this are necessary. I will divert from the PowerVS path in respect to COS.

I will be removing this COSInstanceReference type as I don't expect to need it for VPC based on my findings.

cjschaef · 2024-03-29T16:38:53Z

api/v1beta2/types.go

+
+	// bucketID is the IBM Cloud COS bucket ID.
+	// +optional
+	BucketID *string `json:"bucketID,omitempty"`


I need to swap back to BucketName, I don't think the IBM COS SDK provides additional details I want to use this.
Regardless of whether I expect CAPI to create COS instances and buckets.

mkumatag

@cjschaef I think we are mixing 2 things here - COS bucket for the image import and the ignition data. I recommend you @Karthik-K-N @Prajyot-Parab and @cjschaef please setup a meeting offline to discuss on the spec considering the amount of time we are left with for the implementation.

Karthik-K-N · 2024-04-01T05:00:48Z

@cjschaef I think we are mixing 2 things here - COS bucket for the image import and the ignition data. I recommend you @Karthik-K-N @Prajyot-Parab and @cjschaef please setup a meeting offline to discuss on the spec considering the amount of time we are left with for the implementation.

Sure, We can do this if there are still confusions on the workflow.

cjschaef · 2024-04-01T15:05:57Z

@mkumatag @Karthik-K-N
Based on the findings I've had, reviewing the PowerVS code, expectations, and dependencies, my expectations for the VPC path, is not to rely on CAPI controllers to create COS resources (COS Instance, Bucket, or Object).

Instead, I plan to rely on the COS Instance ID, the Bucket Name, and likely the COS Object Name, with the alternative being I can rely on the Object URL instead, but that depends on IBM Cloud VPC API's, which I have to re-review.

~~If PowerVS does not wish or plan to share this common COS Instance type, I will not add any additional fields to accommodate PowerVS (e.g., COS Instance Name).~~

Based on my discoveries, I have decided to drop COS altogether for VPC related support. References to any necessary COS resources will be handled as needed, using ObjectURL's, and not referencing Instances, Buckets, etc.

Prajyot-Parab · 2024-04-03T18:47:22Z

@Karthik-K-N ptal, lets merge if all okay

cjschaef · 2024-04-03T20:57:07Z

api/v1beta2/types.go

@@ -396,3 +396,17 @@ type VPCEndpoint struct {
 	// +optional
 	LBID *string `json:"loadBalancerIPID,omitempty"`
 }
+
+// VPCResource represents a specific VPC resource.
+// +kubebuilder:validation:XValidation:rule="!has(self.id) && !has(self.name)",message="an id or name must be provided"


This validation is written inversely, I will correct it.

Create a basic VPC resource type that VPC definitions will use.

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

k8s-ci-robot requested review from relyt0925 and wentao-zh March 18, 2024 20:01

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

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 18, 2024

cjschaef force-pushed the vpc_basic_types branch from 8351ca8 to 03f81ed Compare March 19, 2024 15:08

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

cjschaef force-pushed the vpc_basic_types branch from 03f81ed to 7b12dfe Compare March 19, 2024 15:08

cjschaef marked this pull request as ready for review March 21, 2024 14:47

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

k8s-ci-robot requested a review from jichenjc March 21, 2024 14:47

k8s-ci-robot requested review from Karthik-K-N and Prajyot-Parab March 22, 2024 08:18

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

cjschaef force-pushed the vpc_basic_types branch from 7b12dfe to f3e9c57 Compare March 25, 2024 17:51

cjschaef mentioned this pull request Mar 25, 2024

VPC: VPCCluster definition extension #1683

Open

Prajyot-Parab suggested changes Mar 26, 2024

View reviewed changes

Karthik-K-N reviewed Mar 27, 2024

View reviewed changes

cjschaef force-pushed the vpc_basic_types branch from f3e9c57 to 7afea68 Compare March 27, 2024 20:29

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 27, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 27, 2024

k8s-ci-robot requested a review from Karthik-K-N March 28, 2024 05:47

Karthik-K-N reviewed Mar 29, 2024

View reviewed changes

cjschaef commented Mar 29, 2024

View reviewed changes

mkumatag requested changes Mar 30, 2024

View reviewed changes

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

cjschaef force-pushed the vpc_basic_types branch from 7afea68 to 1e11e0a Compare April 1, 2024 17:30

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

cjschaef commented Apr 3, 2024

View reviewed changes

VPC: Create basic types for VPC

dc4e223

Create a basic VPC resource type that VPC definitions will use.

cjschaef force-pushed the vpc_basic_types branch from 1e11e0a to dc4e223 Compare April 3, 2024 21:03

mkumatag added this to the Next milestone May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPC: Create basic types for VPC #1670

VPC: Create basic types for VPC #1670

cjschaef commented Mar 18, 2024 •

edited

k8s-ci-robot commented Mar 18, 2024

netlify bot commented Mar 18, 2024 •

edited

mkumatag commented Mar 22, 2024

Prajyot-Parab commented Mar 22, 2024

Prajyot-Parab commented Mar 22, 2024

k8s-ci-robot commented Mar 25, 2024

Prajyot-Parab Mar 26, 2024

Karthik-K-N Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

Karthik-K-N Mar 28, 2024

cjschaef Mar 28, 2024

Karthik-K-N Mar 29, 2024

cjschaef Mar 29, 2024

Karthik-K-N Apr 1, 2024

Prajyot-Parab Mar 26, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

Karthik-K-N Mar 27, 2024

Karthik-K-N Mar 27, 2024

cjschaef Mar 27, 2024

cjschaef Mar 27, 2024

Prajyot-Parab Mar 28, 2024

Karthik-K-N Mar 29, 2024

cjschaef Apr 1, 2024

cjschaef Mar 29, 2024

mkumatag left a comment

Karthik-K-N commented Apr 1, 2024

cjschaef commented Apr 1, 2024 •

edited

Prajyot-Parab commented Apr 3, 2024

cjschaef Apr 3, 2024

cjschaef Apr 3, 2024

	if err := clusterScope.ReconcileCOSInstance(); err != nil {
	conditions.MarkFalse(powerVSCluster, infrav1beta2.COSInstanceReadyCondition, infrav1beta2.COSInstanceReconciliationFailedReason, capiv1beta1.ConditionSeverityError, err.Error())
	return reconcile.Result{}, err
	}

VPC: Create basic types for VPC #1670

Are you sure you want to change the base?

VPC: Create basic types for VPC #1670

Conversation

cjschaef commented Mar 18, 2024 • edited

k8s-ci-robot commented Mar 18, 2024

netlify bot commented Mar 18, 2024 • edited

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

mkumatag commented Mar 22, 2024

Prajyot-Parab commented Mar 22, 2024

Prajyot-Parab commented Mar 22, 2024

k8s-ci-robot commented Mar 25, 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

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

mkumatag left a comment

Choose a reason for hiding this comment

Karthik-K-N commented Apr 1, 2024

cjschaef commented Apr 1, 2024 • edited

Prajyot-Parab commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjschaef commented Mar 18, 2024 •

edited

netlify bot commented Mar 18, 2024 •

edited

cjschaef commented Apr 1, 2024 •

edited