VPC: Extend VPC Machine API's #1978

cjschaef · 2024-09-27T16:44:13Z

Extend the VPC Machine API's to include additional fields and types for more VPC Machine configuration support.

Related: #1977

What this PR does / why we need it:

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:

Extend VPCMachine APIs to allow for more configuration and supported features. These are intended to be used with the extended VPC Cluster Infrastructure support.

k8s-ci-robot · 2024-09-27T16:44: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-sigs/prow repository.

netlify · 2024-09-27T16:45:08Z

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

Name	Link
🔨 Latest commit	`d606bba`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/67000107b05c50000870d696
😎 Deploy Preview	https://deploy-preview-1978--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 · 2024-10-01T05:13:12Z

api/v1beta2/ibmvpcmachine_types.go

+	// CatalogOffering is the Catalog Offering OS image which would be installed on the instance.
+	// An OfferingCRN or VersionCRN is required, the PlanCRN is optional.
+	// +optional
+	CatalogOffering *IBMCloudCatalogOffering `json:"catalogOffering,omitempty"`


For my understanding, how would this respective image would be imported to VPC or the global catalog images are generally available to all the VPC?

Alos since this catalogOffering is specific to os image should we rename the member to indicate that?

Sure, I can rename it.
In the cases I have used, this documentation provides details on creating a Private Catalog: https://cloud.ibm.com/docs/account?topic=account-restrict-by-user&interface=ui
And details on creating a sharable VPC Custom Image: https://cloud.ibm.com/docs/vpc?topic=vpc-custom-image-cloud-private-catalog&interface=ui

Karthik-K-N · 2024-10-01T05:18:22Z

api/v1beta2/ibmvpcmachine_types.go

 	// Image is the OS image which would be install on the instance.
 	// ID will take higher precedence over Name if both specified.
 	Image *IBMVPCResourceReference `json:"image"`

+	// LoadBalancerPoolMembers is the set of IBM Cloud VPC Load Balancer Backend Pools the machine should be added to as a member.
+	// +optional
+	LoadBalancerPoolMembers []VPCLoadBalancerBackendPoolMember `json:"loadBalancerPoolMembers,omitempty"`


We used to attach the machine to all the loadbalancers defined in cluster.Loadbalancer, Do you plan to avoid that and register to only loadbalancers set here?

What will happen when this is omitted?

The current code has some hardcoded values and expectations on attaching Machines to LB's.
I am trying to avoid doing that, especially since it is currently hardcoded to one LB.
I would expect that all machines CAPI creates then would be added into LB pools then, if we rely on Cluster definitions, which I do not expect is valid. Is that a fair assumption, if we rely on the Cluster resource?

We can do this way as well, but my point is eventually the lb referred by VPCLoadBalancerBackendPoolMember.Loadbalancer is created by cluster reconciler, So why can't we directly use it.

Karthik-K-N · 2024-10-01T05:19:00Z

api/v1beta2/ibmvpcmachine_types.go

+
+	// DedicatedHost is the name of the underlying provisioned host in the VPC on which the instance will be created.
+	// +optional
+	DedicatedHost *VPCResource `json:"dedicatedHost,omitempty"`


For optional field for our better understanding lets mention what will happen when the field is omited.

Will update.

Karthik-K-N · 2024-10-01T05:21:27Z

api/v1beta2/ibmvpcmachine_types.go

+	// LoadBalancerPoolMembers is the status of IBM Cloud VPC Load Balancer Backend Pools the machine is a member.
+	// +optional
+	// +k8s:conversion-gen=false
+	LoadBalancerPoolMembers []VPCLoadBalancerBackendPoolMember `json:"loadBalancerPoolMembers,omitempty"`


Is it required here? Is it just to verify that the machine is already registered to LB?

Yes, that is the intention, to track and make sure the Machine is in the required pools.

Karthik-K-N · 2024-10-01T05:24:20Z

api/v1beta2/types.go

+
+	// Port defines the Port the Load Balancer Pool Member listens for traffic.
+	// +required
+	Port int64 `json:"port"`


Any reasons for defining Port as int64 and wieght as pointer?

Yes, Port is required for LB member creation, for which we do not expect to always use 6443.
And weight is not required, hence it is made optional.

mkumatag · 2024-09-30T06:27:16Z

api/v1beta2/ibmvpcmachine_types.go

@@ -36,10 +39,23 @@ type IBMVPCMachineSpec struct {
 	// Name of the instance.
 	Name string `json:"name,omitempty"`

+	// CatalogOffering is the Catalog Offering OS image which would be installed on the instance.
+	// An OfferingCRN or VersionCRN is required, the PlanCRN is optional.


I don't see lot of information how we are consuming this in the create instance API, it will be great if you can point all the details in this PR which talks about how user will be using this field in the create flow.

I provided what details are available (not many) in regard to using an offering in place of a Custom Image
This is what I had to piece together to create VSI's (instances) with an offering instead
#1978 (comment)

mkumatag · 2024-09-30T06:31:24Z

api/v1beta2/ibmvpcmachine_types.go

+
+	// DedicatedHost is the name of the underlying provisioned host in the VPC on which the instance will be created.
+	// +optional
+	DedicatedHost *VPCResource `json:"dedicatedHost,omitempty"`


is this related to placement_target field name in the create flow or something else?

Yes, that is the expected field. We will need to keep parity for allowing DedicateHostIdentities to be supported for Machines (VSI's).

well, this is not limited to Host right, I see following combinations -

dedicated host

dedicated host group

placement group

So instead of making this field just to host, lets make it more generic so that user can consume for any of the above options..

Correct me if I'm missing something here!

I refactored to support the three options for placement target now.

mkumatag · 2024-10-04T05:28:09Z

/ok-to-test

mkumatag · 2024-10-04T06:12:21Z

api/v1beta2/types.go

+type VPCMachinePlacementTarget struct {
+	// DedicatedHost defines the Dedicated Host to place a VPC Machine (Instance) on.
+	// +optional
+	DedicatedHost *VPCResource `json:"dedicatedHost,omitempty"`


I'm not sure if VPCResource is a right option here because I see following fields for the value used for this field:

// The unique identifier for this dedicated host. ID *string `json:"id,omitempty"` // The CRN for this dedicated host. CRN *string `json:"crn,omitempty"` // The URL for this dedicated host. Href *string `json:"href,omitempty"`

The expectation is that the user would provide an ID or a name of the existing resource, and a lookup would be performed for those resources (to retrieve the ID if not supplied) and use that in the API calls, for whichever of the 3 was provided. Each of these three options has a Name or ID field, which the user could provide, simplifying the user experience.

mkumatag · 2024-10-04T08:33:40Z

api/v1beta2/ibmvpcmachine_types.go

+	// CatalogOfferingImage is the Catalog Offering OS image which would be installed on the instance.
+	// An OfferingCRN or VersionCRN is required, the PlanCRN is optional.
+	// +optional
+	CatalogOfferingImage *IBMCloudCatalogOffering `json:"catalogOfferingImage,omitempty"`


https://cloud.ibm.com/docs/account?topic=account-restrict-by-user&interface=ui looking at this it is not just to do with image, I think its better to remove the Image from the fieldname.

cc @Karthik-K-N

I have remove Image from the field.

mkumatag · 2024-10-04T11:02:04Z

api/v1beta2/ibmvpcmachine_types.go

-	// InstanceStatus is the status of the GCP instance for this machine.
+	// Conditions deefines current service state of the IBMVPCMachine.
+	// +optional
+	// +k8s:conversion-gen=false


any reason for adding +k8s:conversion-gen=false marker?

The generator could not perform conversion on this field. I added this field instead.

However, I retried the manual conversion this type and it appears it made linting and everything happy this time.
Hopefully this actually works in the CI checks.

mkumatag · 2024-10-04T11:10:34Z

api/v1beta2/types.go

+	// SecurityGroups defines a set of IBM Cloud VPC Security Groups to attach to the network interface.
+	// +optional
+	// +k8s:conversion-gen=false
+	SecurityGroups []VPCResource `json:"securityGroups,omitempty"`


Adding his field throwing error in the apidiff CI job - https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-ibmcloud/1978/pull-cluster-api-provider-ibmcloud-apidiff/1842074223984513024

Per comment above, I retried the manual conversion, so hopefully that resolves this failure now.

Extend the VPC Machine API's to include additional fields and types for more VPC Machine configuration support. Related: kubernetes-sigs#1977

mkumatag

/lgtm

/hold

@Karthik-K-N unhold when you are okay

k8s-ci-robot · 2024-10-07T09:53:03Z

[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

Karthik-K-N

/lgtm

/hold cancel

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

k8s-ci-robot requested review from Karthik-K-N and Prajyot-Parab September 27, 2024 16:44

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

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

Karthik-K-N reviewed Oct 1, 2024

View reviewed changes

mkumatag requested changes Oct 1, 2024

View reviewed changes

cjschaef force-pushed the vpc_extended_machine_apis branch 2 times, most recently from 2f0692e to eac62b0 Compare October 2, 2024 14:41

cjschaef mentioned this pull request Oct 3, 2024

Extend VPC (x86_64) Machine reconciliation support #1977

Closed

3 tasks

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

mkumatag requested changes Oct 4, 2024

View reviewed changes

mkumatag reviewed Oct 4, 2024

View reviewed changes

VPC: Extend VPC Machine API's

d606bba

Extend the VPC Machine API's to include additional fields and types for more VPC Machine configuration support. Related: kubernetes-sigs#1977

cjschaef force-pushed the vpc_extended_machine_apis branch from eac62b0 to d606bba Compare October 4, 2024 14:51

mkumatag approved these changes Oct 7, 2024

View reviewed changes

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

k8s-ci-robot assigned mkumatag Oct 7, 2024

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

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

Karthik-K-N reviewed Oct 7, 2024

View reviewed changes

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

k8s-ci-robot assigned Karthik-K-N Oct 7, 2024

k8s-ci-robot merged commit 959867b into kubernetes-sigs:main Oct 7, 2024
13 checks passed

cjschaef deleted the vpc_extended_machine_apis branch October 7, 2024 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPC: Extend VPC Machine API's #1978

VPC: Extend VPC Machine API's #1978

cjschaef commented Sep 27, 2024

k8s-ci-robot commented Sep 27, 2024

netlify bot commented Sep 27, 2024 •

edited

Loading

Karthik-K-N Oct 1, 2024

Karthik-K-N Oct 1, 2024

cjschaef Oct 1, 2024

Karthik-K-N Oct 1, 2024

cjschaef Oct 1, 2024

Karthik-K-N Oct 7, 2024

Karthik-K-N Oct 1, 2024

cjschaef Oct 1, 2024

Karthik-K-N Oct 1, 2024

cjschaef Oct 1, 2024

Karthik-K-N Oct 1, 2024

cjschaef Oct 1, 2024

mkumatag Sep 30, 2024

cjschaef Oct 1, 2024

mkumatag Sep 30, 2024

cjschaef Oct 1, 2024

mkumatag Oct 2, 2024

cjschaef Oct 2, 2024

mkumatag commented Oct 4, 2024

mkumatag Oct 4, 2024

cjschaef Oct 4, 2024

mkumatag Oct 4, 2024

mkumatag Oct 4, 2024

cjschaef Oct 4, 2024

mkumatag Oct 4, 2024

cjschaef Oct 4, 2024

mkumatag Oct 4, 2024

cjschaef Oct 4, 2024

mkumatag left a comment

k8s-ci-robot commented Oct 7, 2024

Karthik-K-N left a comment

VPC: Extend VPC Machine API's #1978

VPC: Extend VPC Machine API's #1978

Conversation

cjschaef commented Sep 27, 2024

k8s-ci-robot commented Sep 27, 2024

netlify bot commented Sep 27, 2024 • edited Loading

✅ 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

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 commented Oct 4, 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

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 7, 2024

Karthik-K-N left a comment

Choose a reason for hiding this comment

netlify bot commented Sep 27, 2024 •

edited

Loading