Better logic to derive vpcRegion/Zone from vpcName/Subnets#6665
Conversation
miyamotoh
force-pushed
the
vpc-region-zone-from-name-subnets
branch
2 times, most recently
from
fe8c04e to
f935b39
Compare
There was a problem hiding this comment.
An error occurred, is there a reason to skip it here ?
There was a problem hiding this comment.
That check came from IBM Cloud code, and turns out it's rather misleading (giving the idea of potentially skipping), and in fact, it virtually never happens. So, I'm dropping the check in the next force-push. Thanks!!
miyamotoh
force-pushed
the
vpc-region-zone-from-name-subnets
branch
from
f935b39 to
67c16ea
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
All checks passed. @barbacbd PTAL. |
pkg/asset/cluster/tfvars.go
Outdated
There was a problem hiding this comment.
I am not familiar with powervs but looking at this I see some potentials for errors. What if there are other options for sub nets. Should we search for valid subnets and if you still want to use a random, randomly select from the list of valid subnets ?
There was a problem hiding this comment.
this is the way we've done it for a while. we (@mjturek and I) made a lot of time making sure it wasn't going to cause problems. however, we know it's not ideal to randomly select the vpc zone this way. but it's fine for now :)
There was a problem hiding this comment.
Not officially documented, but this knowledge of "valid zones" with the number 1 to 3 as suffix per region is gained from internal discussion with the IBM Cloud team. Therefore, this bit is basically allowing user to reuse his/her existing subnet, making sure it exists and deriving the zone from it, if specified, or we just pick one randomly from existing ones for the installer.
There was a problem hiding this comment.
thanks for the explanations !
clnperez
left a comment
clnperez
left a comment
There was a problem hiding this comment.
looks good and i can lgtm it when you've addressed the other review comments. thanks so much!
pkg/asset/cluster/tfvars.go
Outdated
There was a problem hiding this comment.
this is the way we've done it for a while. we (@mjturek and I) made a lot of time making sure it wasn't going to cause problems. however, we know it's not ideal to randomly select the vpc zone this way. but it's fine for now :)
miyamotoh
force-pushed
the
vpc-region-zone-from-name-subnets
branch
from
67c16ea to
3b46ef8
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rna-afk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest-required Remaining retests: 0 against base HEAD 40cf6fe and 2 for PR HEAD 3b46ef833d6e37a6aa72c8daa3bf2d5ad6b76eee in total |
|
/retest-required Remaining retests: 0 against base HEAD 6de3bb4 and 1 for PR HEAD 3b46ef833d6e37a6aa72c8daa3bf2d5ad6b76eee in total |
|
/retest-required Remaining retests: 0 against base HEAD 3c693eb and 0 for PR HEAD 3b46ef833d6e37a6aa72c8daa3bf2d5ad6b76eee in total |
|
/hold Revision 3b46ef833d6e37a6aa72c8daa3bf2d5ad6b76eee was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required Remaining retests: 0 against base HEAD 780b29a and 2 for PR HEAD 3b46ef833d6e37a6aa72c8daa3bf2d5ad6b76eee in total |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest-required |
|
/hold Revision 42e4531 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test e2e-azure-ovn |
2 similar comments
|
/test e2e-azure-ovn |
|
/test e2e-azure-ovn |
|
/override ci/prow/e2e-azure-ovn |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn DetailsIn response to this:
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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/skip |
|
/retest-required |
|
/hold Revision 42e4531 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/override ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn, ci/prow/e2e-gcp-ovn DetailsIn response to this:
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. |
|
@miyamotoh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/override ci/prow/e2e-vsphere-ovn |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-vsphere-ovn DetailsIn response to this:
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. |
7 participants
This change would allow users to specify which region to create a new or locate an existing VPC instance and its subnet. With this, users would no longer be tied to the installer-picked region for VPC resources that is hardcoded to be close to the PowerVS region, essentially allowing reuse of an existing VPC infrastructure the user always has, regardless of where the PowerVS instance is located.
The relevant params for VPC resources in question are; 1)
vpcRegion, 2)vpcName, and 3)vpcSubnets.Whether
publish: External(default) orpublish: Internal, whenvpcRegionis specified, the VPC instance the installer works with will be in that region. IfvpcNameis specified additionally, a VPC instance with that name has to be found in that region. If not, one will be created in that region.If
vpcSubnetsis specified additionally, the subnet must be found in and attached to the VPC specified byvpcName. CustomvpcSubnetswithoutvpcNamewill not be supported and thus error outcl.If
vpcNameis provided withoutvpcRegion, the VPC by the name will be searched in the nearby region of the PowerVS region, and would be accepted/reused if found, error out otherwise.The
validation[_test].gochanges would validate all these combos ofvpc*params.Signed-off-by: Hiro Miyamoto miyamotoh@us.ibm.com