Use zone to filter PowerVS service instance #1853

dharaneeshvrd · 2024-06-19T13:21:00Z

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 #1839

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:

netlify · 2024-06-19T13:21:17Z

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

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

Changes looks good to me, But trying to understand when there were two service instance with same name , why didn't you hit this error here

default:
		errStr := fmt.Errorf("there exist more than one service instance ID with same name %s, Try setting serviceInstance.ID", name)

Also within the same zone we can't have the same service instance right?

dharaneeshvrd · 2024-06-21T09:44:28Z

Problem I faced is, let's say there is an existing powervs instance called capi-dhar-serviceInstance in osa21 zone.
When I try to deploy new cluster with same name capi-dhar in dal12 zone, instead of creating a new powervs instance in dal12, it tries to use the one in osa21 since the it's having same name.
It won't hit this, since there no two matching instances in this situation.

Also within the same zone we can't have the same service instance right?

But for this unfortunately it still allows duplicate within zone too.

Karthik-K-N · 2024-06-21T10:15:01Z

Problem I faced is, let's say there is an existing powervs instance called capi-dhar-serviceInstance in osa21 zone. When I try to deploy new cluster with same name capi-dhar in dal12 zone, instead of creating a new powervs instance in dal12, it tries to use the one in osa21 since the it's having same name. It won't hit this, since there no two matching instances in this situation.

Also within the same zone we can't have the same service instance right?

But for this unfortunately it still allows duplicate within zone too.

Got the point now, Thank you.

Karthik-K-N

/lgtm

mkumatag · 2024-06-21T10:37:24Z

But for this unfortunately it still allows duplicate within zone too.

This is exactly my doubt too, we can't foolproof 100% here in this case, we need to mention somewhere in the api spec saying that if multiple instance with same name present then it will pick first in the list and if user wants to use any specific one then use by ID

mkumatag · 2024-06-21T09:39:24Z

cloud/scope/powervs_image.go

+	Client              client.Client
+	Logger              logr.Logger
+	IBMPowerVSImage     *infrav1beta2.IBMPowerVSImage
+	ServiceInstanceZone *string


Suggested change

ServiceInstanceZone *string

Zone *string

mkumatag · 2024-06-21T10:38:53Z

cloud/scope/powervs_machine.go

@@ -178,9 +179,10 @@ func NewPowerVSMachineScope(params PowerVSMachineScopeParams) (scope *PowerVSMac
 		serviceInstanceName = fmt.Sprintf("%s-%s", params.IBMPowerVSCluster.GetName(), "serviceInstance")
 		if params.IBMPowerVSCluster.Spec.ServiceInstance != nil && params.IBMPowerVSCluster.Spec.ServiceInstance.Name != nil {
 			serviceInstanceName = *params.IBMPowerVSCluster.Spec.ServiceInstance.Name
+			zone = params.IBMPowerVSCluster.Spec.Zone


will it make any difference? why don't we just pass params.IBMPowerVSCluster.Spec.Zone directly in the below function?

Agree. changed.

dharaneeshvrd · 2024-06-21T12:24:10Z

Updated the ServiceInstance field's description, ptal!

mkumatag · 2024-06-21T12:46:52Z

api/v1beta2/ibmpowervscluster_types.go

@@ -65,6 +65,7 @@ type IBMPowerVSClusterSpec struct {
 	// when omitted system will dynamically create the service instance with name CLUSTER_NAME-serviceInstance.
 	// when ServiceInstance.ID is set, its expected that there exist a service instance in PowerVS workspace with id or else system will give error.
 	// when ServiceInstance.Name is set, system will first check for service instance with Name in PowerVS workspace, if not exist system will create new instance.
+	// if there are more than one service instance exist with the ServiceInstance.Name in given Zone, installation fails with an error. Use ServiceInstance.ID in those situations to use the specific service instance.


where is this failure logic written?

@mkumatag Here https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/pull/1853/files#diff-c1cb986cbf553e2fe5a2a8955848ea3044570c1d27c74efef8553f3c8289856cR143

mkumatag

/lgtm

k8s-ci-robot · 2024-06-21T12:57:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharaneeshvrd, 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 the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Jun 19, 2024

k8s-ci-robot requested review from Amulyam24 and Karthik-K-N June 19, 2024 13:21

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2024

dharaneeshvrd changed the title ~~Use zone to filter PowerVS service instance~~ [WIP] Use zone to filter PowerVS service instance Jun 20, 2024

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 Jun 20, 2024

dharaneeshvrd force-pushed the fix-powervs-instance-get-with-zone branch from 836af54 to 46c74bc Compare June 21, 2024 05:42

dharaneeshvrd changed the title ~~[WIP] Use zone to filter PowerVS service instance~~ Use zone to filter PowerVS service instance Jun 21, 2024

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

Karthik-K-N reviewed Jun 21, 2024

View reviewed changes

k8s-ci-robot assigned Karthik-K-N Jun 21, 2024

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

mkumatag requested changes Jun 21, 2024

View reviewed changes

dharaneeshvrd force-pushed the fix-powervs-instance-get-with-zone branch from 46c74bc to e2dd106 Compare June 21, 2024 12:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2024

Use zone to filter PowerVS service instance

e50f895

dharaneeshvrd force-pushed the fix-powervs-instance-get-with-zone branch from e2dd106 to e50f895 Compare June 21, 2024 12:23

mkumatag requested changes Jun 21, 2024

View reviewed changes

mkumatag approved these changes Jun 21, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag Jun 21, 2024

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

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

k8s-ci-robot merged commit 104ee47 into kubernetes-sigs:main Jun 21, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use zone to filter PowerVS service instance #1853

Use zone to filter PowerVS service instance #1853

dharaneeshvrd commented Jun 19, 2024

netlify bot commented Jun 19, 2024 •

edited

Loading

Karthik-K-N left a comment

dharaneeshvrd commented Jun 21, 2024 •

edited

Loading

Karthik-K-N commented Jun 21, 2024

Karthik-K-N left a comment

mkumatag commented Jun 21, 2024

mkumatag Jun 21, 2024

mkumatag Jun 21, 2024

dharaneeshvrd Jun 21, 2024

dharaneeshvrd commented Jun 21, 2024

mkumatag Jun 21, 2024

dharaneeshvrd Jun 21, 2024

mkumatag left a comment

k8s-ci-robot commented Jun 21, 2024

Use zone to filter PowerVS service instance #1853

Use zone to filter PowerVS service instance #1853

Conversation

dharaneeshvrd commented Jun 19, 2024

netlify bot commented Jun 19, 2024 • edited Loading

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

Karthik-K-N left a comment

Choose a reason for hiding this comment

dharaneeshvrd commented Jun 21, 2024 • edited Loading

Karthik-K-N commented Jun 21, 2024

Karthik-K-N left a comment

Choose a reason for hiding this comment

mkumatag commented Jun 21, 2024

mkumatag Jun 21, 2024

Choose a reason for hiding this comment

mkumatag Jun 21, 2024

Choose a reason for hiding this comment

dharaneeshvrd Jun 21, 2024

Choose a reason for hiding this comment

dharaneeshvrd commented Jun 21, 2024

mkumatag Jun 21, 2024

Choose a reason for hiding this comment

dharaneeshvrd Jun 21, 2024

Choose a reason for hiding this comment

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 21, 2024

netlify bot commented Jun 19, 2024 •

edited

Loading

dharaneeshvrd commented Jun 21, 2024 •

edited

Loading