API changes to support infra creation #1485

Karthik-K-N · 2023-11-09T13:09:56Z

What this PR does / why we need it:

Contains the API changes to infra creation as a part of Power VS cluster creation

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:

New fields are added to IBMPowerVSCluster resources to support infrastructure creation

Squash commits before merge

Partially fixes: #1450

netlify · 2023-11-09T13:10:02Z

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

Name	Link
🔨 Latest commit	`8f8ed21`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/65b0dacb603db10008b7b7e7
😎 Deploy Preview	https://deploy-preview-1485--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 · 2023-11-09T13:10:17Z

/cc @dharaneeshvrd @Amulyam24

mkumatag · 2023-11-09T13:51:43Z

api/v1beta2/ibmpowervscluster_types.go

+	ServiceInstance IBMPowerVSResourceReference `json:"serviceInstance"`
+
+	// The Name of Power VS zone where the cluster will be created
+	Zone string `json:"zone"`
+
+	// ResourceGroup name under which the resources will be created.
+	ResourceGroup string `json:"resourceGroup"`
+
+	// VPC contains information about IBM Cloud VPC resources
+	// +optional
+	VPC VPCResource `json:"vpc"`
+
+	// ControlPlaneLoadBalancer is optional configuration for customizing control plane behavior.
+	// +optional
+	ControlPlaneLoadBalancer *VPCLoadBalancerSpec `json:"controlPlaneLoadBalancer,omitempty"`


IMO everything should be optional and can have some default values if not supplied to have backward compatibility

agree, I think we can make use of webhooks to handle it.

we need a discussion about this, like what fields can be default set and what values.

mkumatag · 2023-11-09T13:52:52Z

api/v1beta2/ibmpowervscluster_types.go

 	// ServiceInstanceID is the id of the power cloud instance where the vsi instance will get deployed.
 	// +kubebuilder:validation:MinLength=1
+	// Deprecated: use ServiceInstance.ID instead


why just ID? there are other fields like name, regex

My intention was to refer ServiceInstanceID to ServiceInstance.ID, But yes we can use anything out of Name, ID, Regex, Update it accrodingly

mkumatag · 2023-11-09T13:53:34Z

api/v1beta2/ibmpowervscluster_types.go

 	Ready bool `json:"ready"`
+
+	// serviceInstance is the reference to the Power VS service on which the server instance(VM) will be created.
+	ServiceInstance IBMPowerVSResourceReference `json:"serviceInstance,omitempty"`


in status we can have just the id

mkumatag · 2023-11-09T13:53:50Z

api/v1beta2/ibmpowervscluster_types.go

+	ServiceInstance IBMPowerVSResourceReference `json:"serviceInstance,omitempty"`
+
+	// Network is the reference to the Network to use for this cluster.
+	Network IBMPowerVSResourceReference `json:"network,omitempty"`


same here, lets have only id here

mkumatag · 2023-11-09T13:54:30Z

api/v1beta2/ibmpowervscluster_types.go

+	Network IBMPowerVSResourceReference `json:"network,omitempty"`
+
+	// VPC holds the status of VPC resources
+	VPC VPCResource `json:"vpc,omitempty"`


again lets use only id in the status

mkumatag · 2023-11-09T13:56:44Z

api/v1beta2/ibmpowervscluster_types.go

+	Subnet Subnet `json:"subnet"`
+
+	// TransitGateway holds the resource details of TransitGateway
+	TransitGateway TransitGateway `json:"transitGateway"`


I believe TransitGateway itself is a different component and works like connector between the vpc and classic n/w, so wondering if we need to define outside the vpc data structures

mkumatag · 2023-11-09T13:57:10Z

api/v1beta2/ibmvpccluster_types.go

+type VPCLoadBalancerStatus struct {
+	// Name of the VPC load balancer.
+	// +optional
+	Name string `json:"name,omitempty"`


in the status we shouldn't be mentioning the name

Karthik-K-N · 2023-11-09T15:26:48Z

Addressed first set of review comments.

api/v1beta2/ibmpowervscluster_types.go

Karthik-K-N · 2023-11-13T12:13:04Z

Addressed all the review comments

api/v1beta2/ibmpowervscluster_types.go

mkumatag · 2023-11-14T06:02:58Z

api/v1beta2/ibmpowervscluster_types.go

+	Zone string `json:"zone"`
+
+	// resourceGroup name under which the resources will be created.
+	ResourceGroup string `json:"resourceGroup"`


lets have this as optional and use the default resource group when not supplied

mkumatag · 2023-11-14T06:07:02Z

api/v1beta2/ibmpowervscluster_types.go

+	ServiceInstance IBMPowerVSResourceReference `json:"serviceInstance"`
+
+	// zone is the name of Power VS zone where the cluster will be created
+	Zone string `json:"zone"`


we need have this as an optional to have a backward compatibility, lets also had a link which talks about all the possible values - https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-creating-power-virtual-server

What should be the default zone to use. Also I think we can validate this filed to have value when ServiceInstance info is not provided.

api/v1beta2/ibmpowervscluster_types.go

mkumatag · 2023-11-14T06:25:40Z

api/v1beta2/ibmvpcmachine_types.go

@@ -78,6 +78,9 @@ type IBMVPCResourceReference struct {
 	// +kubebuilder:validation:MinLength=1
 	// +optional
 	Name *string `json:"name,omitempty"`
+
+	// IBM Cloud VPC zone
+	Zone *string `json:"vpcZone,omitempty"`


As we have already a zone field in the IBMVPCMachineSpec, this entry may create confusion here

Will remove it from here and create new resource VPCResource type with zone.

api/v1beta2/ibmpowervscluster_types.go

Karthik-K-N · 2024-01-23T06:26:23Z

api/v1beta2/ibmpowervscluster_types.go

+
+	// vpcSubnet contains information about IBM Cloud VPC Subnet resources
+	// +optional
+	VPCSubnet *Subnet `json:"vpcSubnet,omitempty"`


Suggested change

VPCSubnet *Subnet `json:"vpcSubnet,omitempty"`

VPCSubnet []Subnet `json:"vpcSubnet,omitempty"`

Karthik-K-N · 2024-01-23T06:31:32Z

api/v1beta2/ibmvpccluster_types.go

+	// +kubebuilder:default=control-plane
+	// +kubebuilder:validation:Enum=control-plane
+	// +optional
+	Scheme *LoadBalancerScheme `json:"scheme,omitempty"`


change it to Public

Karthik-K-N · 2024-01-23T06:35:12Z

api/v1beta2/ibmpowervscluster_types.go

+	COSBucket *ResourceReference `json:"cosBucket,omitempty"`
+
+	// loadBalancers reference to IBM Cloud VPC Loadbalancer.
+	LoadBalancers []VPCLoadBalancerStatus `json:"loadBalancers,omitempty"`


Explore on using map instead of slice

Suggested change

LoadBalancers []VPCLoadBalancerStatus `json:"loadBalancers,omitempty"`

LoadBalancers map[string]VPCLoadBalancerStatus `json:"loadBalancers,omitempty"`

Karthik-K-N · 2024-01-23T06:39:07Z

api/v1beta2/ibmpowervscluster_types.go

+	// (https://coreos.github.io/ignition/) for bootstrapping (requires
+	// BootstrapFormatIgnition feature flag to be enabled).
+	// +optional
+	CosBucket *CosBucket `json:"cosBucket,omitempty"`


Suggested change

CosBucket *CosBucket `json:"cosBucket,omitempty"`

CosInstance *CosInstance `json:"cosInstance,omitempty"`

api/v1beta2/ibmpowervscluster_types.go

Karthik-K-N · 2024-01-23T06:44:46Z

Use annotations to support existing way of creating cluster

mkumatag

Lets start using this terminology PowerVS workspace instead of service instance

mkumatag · 2024-01-24T06:09:39Z

api/v1beta2/ibmpowervscluster_types.go

 	Network IBMPowerVSResourceReference `json:"network"`

 	// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
 	// +optional
 	ControlPlaneEndpoint capiv1beta1.APIEndpoint `json:"controlPlaneEndpoint"`
+
+	// serviceInstance is the reference to the Power VS service on which the server instance(VM) will be created.
+	// Power VS service is a container for all Power VS instances at a specific geographic region.


Suggested change

// Power VS service is a container for all Power VS instances at a specific geographic region.

// Power Virtual Server workspace is a container for all Power Virtual Server instances at a specific geographic region.

mkumatag · 2024-01-24T06:13:55Z

api/v1beta2/ibmpowervscluster_types.go

+	Zone *string `json:"zone,omitempty"`
+
+	// resourceGroup name under which the resources will be created.
+	// when omitted Default resource group will be used.


This is not a Default one by name, IBM cloud can set some other name as well for the default if I'm not wrong, lets modify the message accordingly.

mkumatag · 2024-01-24T06:19:06Z

api/v1beta2/ibmpowervscluster_types.go

+	// +optional
+	VPCSubnets []Subnet `json:"vpcSubnets,omitempty"`
+
+	// transitGateway contains information about IBM Cloud TransitGateway.


it will be great if you can add a link to the ibm cloud document for more information.

and the purpose of this entity here

mkumatag · 2024-01-24T06:20:34Z

api/v1beta2/ibmpowervscluster_types.go

+	// +optional
+	TransitGateway *TransitGateway `json:"transitGateway,omitempty"`
+
+	// loadBalancers is optional configuration for configuring loadbalancers to control plane or data plane nodes.


information needs to added to to mention why is this optional and what happens user doesn't supply

mkumatag · 2024-01-24T06:22:06Z

api/v1beta2/ibmpowervscluster_types.go

+	Name *string `json:"name,omitempty"`
+
+	// IBM Cloud VPC zone
+	Zone *string `json:"zone,omitempty"`


why do we need this? can we just create subnets in all the zones by default.?

This is reference used for VPC. zone indicates vpc zone and yes as of today only one subnet is created associated to this zone.

mkumatag · 2024-01-24T06:23:45Z

api/v1beta2/ibmpowervsmachine_types.go

+	// Version defines which version of Ignition will be used to generate bootstrap data.
+	//
+	// +optional
+	// +kubebuilder:default="2.3"


why it is set to 2.3 version?

Capi doc says only v2 is tested https://cluster-api.sigs.k8s.io/tasks/experimental-features/ignition#note

mkumatag · 2024-01-24T06:24:24Z

api/v1beta2/ibmpowervsmachine_types.go

+
+	// Ignition defined options related to the bootstrapping systems where Ignition is used.
+	// +optional
+	Ignition *Ignition `json:"ignition,omitempty"`


why a struct here? right now I see just a version, do we expect anything else in the future?

Took a reference from other provider(aws).

mkumatag · 2024-01-24T06:25:29Z

api/v1beta2/ibmvpccluster_types.go

+}
+
+// LoadBalancerScheme defines the scheme of a load balancer.
+type LoadBalancerScheme string


where is this used?

not used, will be removed.

mkumatag

/lgtm
/approve

/hold

for others to review

/cc @Prajyot-Parab

Prajyot-Parab

/unhold

k8s-ci-robot · 2024-01-24T17:19:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Karthik-K-N, mkumatag, Prajyot-Parab

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 the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Nov 9, 2023

k8s-ci-robot requested a review from mkumatag November 9, 2023 13:10

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 9, 2023

k8s-ci-robot requested a review from Prajyot-Parab November 9, 2023 13:10

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 9, 2023

k8s-ci-robot requested review from Amulyam24 and dharaneeshvrd November 9, 2023 13:10

Karthik-K-N marked this pull request as draft November 9, 2023 13:50

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2023

mkumatag requested changes Nov 9, 2023

View reviewed changes

Karthik-K-N force-pushed the create-infra branch from 43ebc19 to 00219a6 Compare November 9, 2023 15:24

Karthik-K-N force-pushed the create-infra branch from 00219a6 to 5f202fe Compare November 13, 2023 06:33

Karthik-K-N marked this pull request as ready for review November 13, 2023 06:35

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 Nov 13, 2023

k8s-ci-robot requested review from jichenjc and relyt0925 November 13, 2023 06:35

Karthik-K-N force-pushed the create-infra branch from 5f202fe to 270b2ce Compare November 13, 2023 07:00

dharaneeshvrd reviewed Nov 13, 2023

View reviewed changes

api/v1beta2/ibmpowervscluster_types.go Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_types.go Outdated Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_types.go Outdated Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_types.go Outdated Show resolved Hide resolved

Karthik-K-N force-pushed the create-infra branch from 270b2ce to f36b32e Compare November 13, 2023 12:12

dharaneeshvrd reviewed Nov 14, 2023

View reviewed changes

api/v1beta2/ibmpowervscluster_types.go Outdated Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_types.go Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_types.go Outdated Show resolved Hide resolved

api/v1beta2/ibmpowervscluster_types.go Outdated Show resolved Hide resolved

mkumatag requested changes Nov 14, 2023

View reviewed changes

Karthik-K-N force-pushed the create-infra branch from 91fb6bf to 6c7a301 Compare November 29, 2023 04:49

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

Karthik-K-N force-pushed the create-infra branch from 6c7a301 to d5d080b Compare November 29, 2023 04:55

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

Karthik-K-N force-pushed the create-infra branch from d5d080b to ff65733 Compare December 1, 2023 04:13

Karthik-K-N force-pushed the create-infra branch 2 times, most recently from e3a9a29 to 9443ac2 Compare January 17, 2024 09:01

Karthik-K-N commented Jan 23, 2024

View reviewed changes

api/v1beta2/ibmpowervscluster_types.go Show resolved Hide resolved

Karthik-K-N commented Jan 23, 2024

View reviewed changes

api/v1beta2/ibmpowervscluster_types.go Show resolved Hide resolved

Karthik-K-N force-pushed the create-infra branch from 9443ac2 to 49aa805 Compare January 23, 2024 09:32

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 23, 2024

Karthik-K-N force-pushed the create-infra branch from 49aa805 to a2d2a73 Compare January 24, 2024 05:58

mkumatag requested changes Jan 24, 2024

View reviewed changes

Karthik-K-N force-pushed the create-infra branch 2 times, most recently from 2309e2d to 1db1e21 Compare January 24, 2024 09:14

API changes to support infra creation

8f8ed21

Karthik-K-N force-pushed the create-infra branch from 1db1e21 to 8f8ed21 Compare January 24, 2024 09:39

mkumatag approved these changes Jan 24, 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 Jan 24, 2024

k8s-ci-robot assigned mkumatag Jan 24, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 24, 2024

Prajyot-Parab approved these changes Jan 24, 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 Jan 24, 2024

k8s-ci-robot merged commit d89b5ae into kubernetes-sigs:main Jan 24, 2024
13 checks passed

Karthik-K-N deleted the create-infra branch January 25, 2024 03:58

Karthik-K-N mentioned this pull request Feb 9, 2024

Feature: PowerVS cluster creation with dynamic resource creation #1608

Merged

3 tasks

Prajyot-Parab mentioned this pull request Feb 9, 2024

EPIC - Introduce new API Specs and infra creation flow #1611

Open

9 tasks

	VPCSubnet *Subnet `json:"vpcSubnet,omitempty"`
	VPCSubnet []Subnet `json:"vpcSubnet,omitempty"`

	LoadBalancers []VPCLoadBalancerStatus `json:"loadBalancers,omitempty"`
	LoadBalancers map[string]VPCLoadBalancerStatus `json:"loadBalancers,omitempty"`

	CosBucket *CosBucket `json:"cosBucket,omitempty"`
	CosInstance *CosInstance `json:"cosInstance,omitempty"`

	// Power VS service is a container for all Power VS instances at a specific geographic region.
	// Power Virtual Server workspace is a container for all Power Virtual Server instances at a specific geographic region.

API changes to support infra creation #1485

API changes to support infra creation #1485

Conversation

Karthik-K-N commented Nov 9, 2023 • edited Loading

netlify bot commented Nov 9, 2023 • edited Loading

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

Karthik-K-N commented Nov 9, 2023

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 commented Nov 9, 2023

Karthik-K-N commented Nov 13, 2023 • 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

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 Jan 23, 2024

mkumatag 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

mkumatag left a comment

Choose a reason for hiding this comment

Prajyot-Parab left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 24, 2024

Karthik-K-N commented Nov 9, 2023 •

edited

Loading

netlify bot commented Nov 9, 2023 •

edited

Loading

Karthik-K-N commented Nov 13, 2023 •

edited

Loading