Add Chennai region and zones and update SysTypes #15

arshadd-b · 2024-11-18T06:11:38Z

This PR adds regions and zones and updates SysTypes
Fixes issue #14

ppc64le-cloud-bot · 2024-11-18T06:11:46Z

Welcome @arshadd-b! It looks like this is your first PR to ppc64le-cloud/powervs-utils 🎉

mkumatag · 2024-11-18T06:15:16Z

please review and also Chennai DC and also go through this doc - https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-ibm-cloud-reg and see what else is missing.

Prajyot-Parab · 2024-11-18T12:06:32Z

@arshadd-b
Add Chennai DC

zone   -> che01
region -> che

region.go

dharaneeshvrd · 2024-11-19T05:05:41Z

region.go

@@ -11,13 +11,11 @@ func GetRegion(zone string) (region string, err error) {
 	case strings.HasPrefix(zone, "us-south"):
 		region = "us-south"
 	case strings.HasPrefix(zone, "dal"):
-		region = "dal"
+		region = "us-south"


Why do we need this change? Not sure it might cause some breakage to existing modules using this func. Please validate this across OpenShift and capi projects for its usage.

I had referred this mapping https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/blob/main/docs/book/src/reference/regions-zones-mapping.md
Could see that the zones us-south dal10 dal12 having same region us-south
same case with wdc

It is getting used just here , I don't think will create any issue
https://github.com/openshift/machine-api-provider-powervs/blob/e253fa30d99bb7ed00c1f27ca3d810fa682a1901/pkg/client/powervs_client.go#L162C36-L162C45

Ok I think @Karthik-K-N can comment here.

I think when we are setting provider id we set / so till now its dal/dal10, I think it will affect it.
@KeerthanaAP @Shilpa-Gokul whats the value you set for region when using dal10 zone? in IPI

We set the powervs region as "dal" while using "dal10" zone and I think we use "us-south" for VPCRegion.
https://github.com/openshift/installer/blob/release-4.15/pkg/types/powervs/powervs_regions.go#L24C3-L24C6

dharaneeshvrd · 2024-11-19T05:06:22Z

region.go

+		Zones: []string{
+			"che01",
+		},
+		SysTypes: []string{"s922", "e980"},


Hope you have validated the sys types available in Chennai! If not you can check via PowerVS workspace in cloud.

I have checked the machine types for this zone che01 I can see only these two machine types

Do we need to include Dedicated host types as well in sysTypes ?

arshadd-b · 2024-11-19T09:22:09Z

I have noticed new machine Types added in zones, I have added those as well
But in some cases like in Zone dal12 having machine types E980, S922and dal10 is having machine types E980, S1022, S922 . Machine Types are different for different zones.
How should we handle these cases since we have single entry of zones belonging to same region in map ?

For now we have kept the common machine Types in zones
https://github.com/ppc64le-cloud/powervs-utils/blob/main/region.go#L65

dharaneeshvrd · 2024-11-20T05:57:37Z

Individual zone based machine types would alter the current region map.
If we are sure that region map is not directly consumed and only util funcs are consumed, then we can alter the map and util funcs.

Karthik-K-N · 2024-11-21T06:53:54Z

I dont have much idea on the impact, @hamzy @mjturek could you please take a look.

mkumatag · 2024-11-21T07:28:02Z

we shouldn't have made public if not required!

Individual zone based machine types would alter the current region map.
If we are sure that region map is not directly consumed and only util funcs are consumed, then we can alter the map and util funcs.

arshadd-b · 2024-11-26T07:57:34Z

I have noticed new machine Types added in zones, I have added those as well But in some cases like in Zone dal12 having machine types E980, S922and dal10 is having machine types E980, S1022, S922 . Machine Types are different for different zones. How should we handle these cases since we have single entry of zones belonging to same region in map ?

For now we have kept the common machine Types in zones https://github.com/ppc64le-cloud/powervs-utils/blob/main/region.go#L65

Have created different issue for this #16

dharaneeshvrd · 2024-11-26T08:55:05Z

Overall LGTM!
@arshadd-b Please update the commit message with proper details!

arshadd-b · 2024-11-26T09:24:35Z

Overall LGTM! @arshadd-b Please update the commit message with proper details!

done

mkumatag · 2024-11-28T05:50:22Z

region.go

@@ -16,8 +16,6 @@ func GetRegion(zone string) (region string, err error) {
 		region = "sao"
 	case strings.HasPrefix(zone, "us-east"):
 		region = "us-east"
-	case strings.HasPrefix(zone, "tor"):


why is this removed? I still see an entry here for tor - https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-ibm-cloud-reg

@Prajyot-Parab @dharaneeshvrd @Karthik-K-N am I missing something here?

Its a dup entry, that's why she removed I guess.

Yeah two entries were there for tor, so I have removed one

mkumatag

/lgtm

ppc64le-cloud-bot · 2024-11-28T06:19:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arshadd-b, 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

ppc64le-cloud-bot requested review from bkhadars and mkumatag November 18, 2024 06:11

ppc64le-cloud-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 18, 2024

ppc64le-cloud-bot requested review from dharaneeshvrd and Karthik-K-N November 18, 2024 06:15

arshadd-b force-pushed the update-regions-and-zones branch from f8b6a3b to 76469e2 Compare November 18, 2024 06:16

Prajyot-Parab suggested changes Nov 18, 2024

View reviewed changes

region.go Outdated Show resolved Hide resolved

region.go Outdated Show resolved Hide resolved

arshadd-b force-pushed the update-regions-and-zones branch from 76469e2 to adc1ded Compare November 19, 2024 04:14

ppc64le-cloud-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2024

dharaneeshvrd reviewed Nov 19, 2024

View reviewed changes

arshadd-b force-pushed the update-regions-and-zones branch from adc1ded to b8b8a24 Compare November 19, 2024 08:22

arshadd-b requested a review from Prajyot-Parab November 19, 2024 09:29

mkumatag closed this Nov 21, 2024

mkumatag reopened this Nov 21, 2024

arshadd-b force-pushed the update-regions-and-zones branch from b8b8a24 to 3187642 Compare November 26, 2024 06:53

ppc64le-cloud-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 26, 2024

arshadd-b mentioned this pull request Nov 26, 2024

Add SysTypes According to Zones #16

Open

arshadd-b changed the title ~~update regions and zones~~ Add missing regions and zones and update SysTypes Nov 26, 2024

arshadd-b requested a review from dharaneeshvrd November 26, 2024 08:35

arshadd-b changed the title ~~Add missing regions and zones and update SysTypes~~ Add missing regions and zones and SysTypes Nov 26, 2024

Add Chennai region and zones and update sysTypes

52428b5

arshadd-b force-pushed the update-regions-and-zones branch from 3187642 to 52428b5 Compare November 26, 2024 09:20

arshadd-b changed the title ~~Add missing regions and zones and SysTypes~~ Add Chennai region and zones and update SysTypes Nov 26, 2024

mkumatag requested changes Nov 28, 2024

View reviewed changes

mkumatag approved these changes Nov 28, 2024

View reviewed changes

ppc64le-cloud-bot assigned mkumatag Nov 28, 2024

ppc64le-cloud-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2024

ppc64le-cloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2024

ppc64le-cloud-bot merged commit a2c048c into ppc64le-cloud:main Nov 28, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Chennai region and zones and update SysTypes #15

Add Chennai region and zones and update SysTypes #15

arshadd-b commented Nov 18, 2024 •

edited

Loading

ppc64le-cloud-bot commented Nov 18, 2024

mkumatag commented Nov 18, 2024

Prajyot-Parab commented Nov 18, 2024

dharaneeshvrd Nov 19, 2024

arshadd-b Nov 19, 2024

dharaneeshvrd Nov 20, 2024

Karthik-K-N Nov 21, 2024

KeerthanaAP Nov 21, 2024

dharaneeshvrd Nov 19, 2024

arshadd-b Nov 19, 2024

arshadd-b commented Nov 19, 2024 •

edited

Loading

dharaneeshvrd commented Nov 20, 2024

Karthik-K-N commented Nov 21, 2024

mkumatag commented Nov 21, 2024 •

edited

Loading

arshadd-b commented Nov 26, 2024

dharaneeshvrd commented Nov 26, 2024

arshadd-b commented Nov 26, 2024

mkumatag Nov 28, 2024

mkumatag Nov 28, 2024

dharaneeshvrd Nov 28, 2024

arshadd-b Nov 28, 2024

mkumatag left a comment

ppc64le-cloud-bot commented Nov 28, 2024

Add Chennai region and zones and update SysTypes #15

Add Chennai region and zones and update SysTypes #15

Conversation

arshadd-b commented Nov 18, 2024 • edited Loading

ppc64le-cloud-bot commented Nov 18, 2024

mkumatag commented Nov 18, 2024

Prajyot-Parab commented Nov 18, 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

arshadd-b commented Nov 19, 2024 • edited Loading

dharaneeshvrd commented Nov 20, 2024

Karthik-K-N commented Nov 21, 2024

mkumatag commented Nov 21, 2024 • edited Loading

arshadd-b commented Nov 26, 2024

dharaneeshvrd commented Nov 26, 2024

arshadd-b commented Nov 26, 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

mkumatag left a comment

Choose a reason for hiding this comment

ppc64le-cloud-bot commented Nov 28, 2024

arshadd-b commented Nov 18, 2024 •

edited

Loading

arshadd-b commented Nov 19, 2024 •

edited

Loading

mkumatag commented Nov 21, 2024 •

edited

Loading