MULTIARCH-3212: Add region mapping config for PowerVS cloud #1454

dharaneeshvrd · 2023-01-13T08:09:33Z

This is to keep the region mapping of PowerVS in common package, so all the components can access.

Region mapping is required to derive the location for VPC or COS from PowerVS region, since PowerVS follows different location notation, this mapping is required. With this mapping, user need to pass PowerVS region alone and other resource like VPC's region can be derived from this mapping.

As per @JoelSpeed's suggestion here raised this PR.

Fixes https://issues.redhat.com/browse/MULTIARCH-3212

dharaneeshvrd · 2023-01-13T08:12:11Z

/cc @JoelSpeed @clnperez @mkumatag @mjturek @Prashanth684

mkumatag

few minor changes and also mention the original location of code from where we are moving this code

mkumatag · 2023-01-18T05:19:40Z

pkg/config/cloud/powervs/powervs_regions.go

+	found := false
+	for r := range Regions {
+		if region == Regions[r].VPCRegion {
+			found = true
+			break
+		}
+	}
+	return found


Suggested change

found := false

for r := range Regions {

if region == Regions[r].VPCRegion {

found = true

break

}

}

return found

for r := range Regions {

if region == Regions[r].VPCRegion {

return true

}

}

return false

mkumatag · 2023-01-18T05:22:33Z

pkg/config/cloud/powervs/powervs_regions.go

+	keys := make([]string, len(Regions))
+	i := 0
+	for r := range Regions {
+		keys[i] = r
+		i++
+	}
+	return keys


Suggested change

keys := make([]string, len(Regions))

i := 0

for r := range Regions {

keys[i] = r

i++

}

return keys

keys := []string{}

for r := range Regions {

keys = append(keys, r)

}

return keys

dharaneeshvrd · 2023-01-18T09:32:35Z

Currently this mapping is placed inside installer repo here - https://github.com/openshift/installer/blob/master/pkg/types/powervs/powervs_regions.go
Once it moved here, we can remove from installer.

dharaneeshvrd · 2023-01-19T14:21:42Z

@mkumatag Addressed your comments.
@clnperez @Prashanth684
Could you please review this PR when you get a chance?

Prashanth684 · 2023-01-19T15:45:51Z

/lgtm

dharaneeshvrd · 2023-01-20T05:52:13Z

@soltysh @sttts Could you please take a look at this PR?

mkumatag · 2023-01-20T06:24:55Z

/assign @sttts

mjturek · 2023-01-24T15:54:15Z

/lgtm

clnperez · 2023-01-24T17:29:47Z

pkg/config/cloud/powervs/powervs_regions.go

+		VPCRegion:   "br-sao",
+		Zones:       []string{"sao01"},
+	},
+	"tor": {


can you delete this element?

Removed tor region.

mjturek · 2023-01-26T15:04:41Z

pkg/config/cloud/powervs/powervs_regions.go

+	Zones       []string
+}
+
+// Regions holds the regions for IBM Power VS, and descriptions used during the survey.


nit: "the survey" is an IPI term. I change this to:

"Regions provides a mapping between Power VS and IBM Cloud VPC regions."

mjturek · 2023-01-26T15:08:01Z

pkg/config/cloud/powervs/powervs_regions.go

+// VPCRegionForPowerVSRegion returns the VPC region for the specified PowerVS region.
+func VPCRegionForPowerVSRegion(region string) (string, error) {
+	if r, ok := Regions[region]; ok {
+		return r.VPCRegion, nil
+	}
+
+	return "", fmt.Errorf("VPC region corresponding to a PowerVS region %s not found ", region)
+}
+
+// RegionShortNames returns the list of region names
+func RegionShortNames() []string {
+	keys := []string{}
+	for r := range Regions {
+		keys = append(keys, r)
+	}
+	return keys
+}
+
+// ValidateVPCRegion validates that given VPC region is known/tested.
+func ValidateVPCRegion(region string) bool {
+	for r := range Regions {
+		if region == Regions[r].VPCRegion {
+			return true
+		}
+	}
+	return false
+}


I know these utility functions are useful for IPI, but do they belong in this repo? I'm personally fine with it but would like to hear from others.

Need to use func VPCRegionForPowerVSRegion() in image registry changes. Need to decide on remaining funcs. I am fine with it too on keeping them here.

This is to keep the region mapping of PowerVS in common package

openshift-ci · 2023-01-27T10:25:25Z

@dharaneeshvrd: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

mjturek · 2023-01-30T17:37:43Z

/lgtm

dharaneeshvrd · 2023-02-01T06:25:00Z

/assign @soltysh

dharaneeshvrd · 2023-02-01T06:53:08Z

@sttts @soltysh
Could you please take a look at this PR and provide your comments?

openshift-ci-robot · 2023-02-06T17:26:41Z

@dharaneeshvrd: This pull request references MULTIARCH-3212 which is a valid jira issue.

In response to this:

This is to keep the region mapping of PowerVS in common package, so all the components can access.

Region mapping is required to derive the location for VPC or COS from PowerVS region, since PowerVS follows different location notation, this mapping is required. With this mapping, user need to pass PowerVS region alone and other resource like VPC's region can be derived from this mapping.

As per @JoelSpeed's suggestion here raised this PR.

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/test-infra repository.

openshift-ci-robot · 2023-02-06T17:26:57Z

@dharaneeshvrd: This pull request references MULTIARCH-3212 which is a valid jira issue.

In response to this:

This is to keep the region mapping of PowerVS in common package, so all the components can access.

Region mapping is required to derive the location for VPC or COS from PowerVS region, since PowerVS follows different location notation, this mapping is required. With this mapping, user need to pass PowerVS region alone and other resource like VPC's region can be derived from this mapping.

As per @JoelSpeed's suggestion here raised this PR.

Fixes https://issues.redhat.com/browse/MULTIARCH-3212

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/test-infra repository.

soltysh · 2023-02-06T18:06:41Z

pkg/config/cloud/powervs/powervs_regions.go

+}
+
+// Regions provides a mapping between Power VS and IBM Cloud VPC regions.
+var Regions = map[string]Region{


This looks like configuration data, not something we'd want to keep in a repository.
/hold

@soltysh For some context this is a mapping between two different cloud services in IBM Cloud. Currently this is in the openshift-installer, but we found that it would need to be referenced by other projects as well. It was then suggested to put it here in a comment in this PR openshift/api#1383

If something like this doesn't belong here, could you suggest where it should go?

openshift-ci · 2023-02-06T18:11:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dharaneeshvrd, mjturek, Prashanth684
Once this PR has been reviewed and has the lgtm label, please ask for approval from soltysh by writing /assign @soltysh in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2023-05-08T09:00:32Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

clnperez · 2023-05-08T16:29:33Z

@mjturek i have forgotten where this discussion landed. should we close this?

dharaneeshvrd · 2023-05-11T04:26:52Z

I think We can close this one.
@clnperez As per RH team's suggestion we decided to keep the region mapping in IBM's repo here https://github.com/ppc64le-cloud/powervs-utils/blob/main/region.go since it does not have any other major functionality, we thought of keeping it here so we won't need to worry about dependency it would bring when we update a new region.
Yet to change the cluster-image-registry-operator to use region mapping from here.
@mjturek If need openshift/installer can also use this mapping I believe.

openshift-ci bot requested review from soltysh and sttts January 13, 2023 08:10

openshift-ci bot requested review from clnperez, JoelSpeed, mjturek, mkumatag and Prashanth684 January 13, 2023 08:12

mkumatag suggested changes Jan 18, 2023

View reviewed changes

mkumatag reviewed Jan 18, 2023

View reviewed changes

dharaneeshvrd force-pushed the powervs-regions branch from 2724b03 to 979bb31 Compare January 18, 2023 09:29

openshift-ci bot assigned Prashanth684 Jan 19, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2023

openshift-ci bot assigned sttts Jan 20, 2023

openshift-ci bot assigned mjturek Jan 24, 2023

clnperez suggested changes Jan 24, 2023

View reviewed changes

dharaneeshvrd force-pushed the powervs-regions branch from 979bb31 to 8316587 Compare January 25, 2023 06:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2023

mjturek reviewed Jan 26, 2023

View reviewed changes

Add region mapping config for PowerVS cloud

9178819

This is to keep the region mapping of PowerVS in common package

dharaneeshvrd force-pushed the powervs-regions branch from 8316587 to 9178819 Compare January 27, 2023 10:19

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2023

openshift-ci bot assigned soltysh Feb 1, 2023

dharaneeshvrd changed the title ~~Add region mapping config for PowerVS cloud~~ MULTIARCH-3212: Add region mapping config for PowerVS cloud Feb 6, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 6, 2023

soltysh requested changes Feb 6, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2023

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 8, 2023

dharaneeshvrd closed this May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MULTIARCH-3212: Add region mapping config for PowerVS cloud #1454

MULTIARCH-3212: Add region mapping config for PowerVS cloud #1454

dharaneeshvrd commented Jan 13, 2023 •

edited

dharaneeshvrd commented Jan 13, 2023

mkumatag left a comment

mkumatag Jan 18, 2023

mkumatag Jan 18, 2023

dharaneeshvrd commented Jan 18, 2023

dharaneeshvrd commented Jan 19, 2023

Prashanth684 commented Jan 19, 2023

dharaneeshvrd commented Jan 20, 2023

mkumatag commented Jan 20, 2023

mjturek commented Jan 24, 2023

clnperez Jan 24, 2023

dharaneeshvrd Jan 25, 2023

mjturek Jan 26, 2023

dharaneeshvrd Jan 27, 2023

mjturek Jan 26, 2023

dharaneeshvrd Jan 27, 2023

openshift-ci bot commented Jan 27, 2023

mjturek commented Jan 30, 2023

dharaneeshvrd commented Feb 1, 2023

dharaneeshvrd commented Feb 1, 2023

openshift-ci-robot commented Feb 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Feb 6, 2023 •

edited by openshift-ci bot

soltysh Feb 6, 2023

mjturek Feb 6, 2023

openshift-ci bot commented Feb 6, 2023

openshift-bot commented May 8, 2023

clnperez commented May 8, 2023

dharaneeshvrd commented May 11, 2023

MULTIARCH-3212: Add region mapping config for PowerVS cloud #1454

MULTIARCH-3212: Add region mapping config for PowerVS cloud #1454

Conversation

dharaneeshvrd commented Jan 13, 2023 • edited

dharaneeshvrd commented Jan 13, 2023

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

dharaneeshvrd commented Jan 18, 2023

dharaneeshvrd commented Jan 19, 2023

Prashanth684 commented Jan 19, 2023

dharaneeshvrd commented Jan 20, 2023

mkumatag commented Jan 20, 2023

mjturek commented Jan 24, 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

openshift-ci bot commented Jan 27, 2023

mjturek commented Jan 30, 2023

dharaneeshvrd commented Feb 1, 2023

dharaneeshvrd commented Feb 1, 2023

openshift-ci-robot commented Feb 6, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Feb 6, 2023 • edited by openshift-ci bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 6, 2023

openshift-bot commented May 8, 2023

clnperez commented May 8, 2023

dharaneeshvrd commented May 11, 2023

dharaneeshvrd commented Jan 13, 2023 •

edited

openshift-ci-robot commented Feb 6, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Feb 6, 2023 •

edited by openshift-ci bot