kubernetes-sigs / cluster-api-provider-ibmcloud

Cluster API Provider for IBM Cloud
https://cluster-api-ibmcloud.sigs.k8s.io
Apache License 2.0
60 stars 76 forks source link

Use zone to filter PowerVS service instance #1853

Closed dharaneeshvrd closed 2 weeks ago

dharaneeshvrd commented 2 weeks ago

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

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

netlify[bot] commented 2 weeks ago

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

Name Link
Latest commit e50f895fc08b39a7e6dba27398d3a3ff7b21a3da
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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dharaneeshvrd commented 2 weeks ago

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 commented 2 weeks ago

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.

mkumatag commented 2 weeks ago

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

dharaneeshvrd commented 2 weeks ago

Updated the ServiceInstance field's description, ptal!

k8s-ci-robot commented 2 weeks ago

[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](https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/blob/main/OWNERS)~~ [mkumatag] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment