Skip to content

Power VS IPI Support: TF Only#5270

Closed
clnperez wants to merge 2 commits intoopenshift:masterfrom
clnperez:upstream-tf-only
Closed

Power VS IPI Support: TF Only#5270
clnperez wants to merge 2 commits intoopenshift:masterfrom
clnperez:upstream-tf-only

Conversation

@clnperez
Copy link
Copy Markdown
Contributor

@clnperez clnperez commented Oct 6, 2021

For background see #5224

Instead of one massive PR, splitting the Power VS work into multiple PRs. This one contains the Terraform changes for supporting Power VS. There are currently three resources that aren't created via TF:

  • The Power VS DHCP Network (available to test 10/18)
  • The IBM VPC Network (available now)
  • Direct Link (available now) to connect the Power VS network to the VPC Network

The two that are available aren't added here yet as it would make dev trickier. We plan to add these as soon as the functionality is available.

Everything is tested and is functional.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 6, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jhixson74 after the PR has been reviewed.
You can assign the PR to them by writing /assign @jhixson74 in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci Bot requested review from e-tienne and rna-afk October 6, 2021 18:37
@openshift-ci openshift-ci Bot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 6, 2021
@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Oct 7, 2021

/verify-owners

@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Oct 8, 2021

@mjturek should show up in the Openshift org soon.

/retest-required

Comment thread pkg/rhcos/powervs_regions.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should come from the regions that RHCOS is pushed to which is parsed from the stream metadata.similar to how AMI regions are done through the generate script.

But the question here is - are these the regions where the bootimage will be imported to ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so we'll probably have to come up with a kind of hybrid approach then, or move this somewhere outside the rhcos purview, and combine what's there with the stream metadata. the regions at the top level are the Power VS regions -- not the IBM Cloud DC/COS regions the bootimage will be imported to.

Copy link
Copy Markdown
Contributor Author

@clnperez clnperez Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the VPCRegions are tied to Regions here, so that we pick a close VPC Region to the Power VS Region. But again, we should probably create a different regions file since this doesn't always relate to the images. It does, though, when we're choosing which bucket to import from. It doesn't when we're creating the LBs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it makes sense that the user provides the PowerVS zones (us-east-1) and not care about the region where the image is located. so we have to have a mechanism to map these zones -> regions where the cos bucket is.

Comment thread pkg/rhcos/powervs_regions.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we just have a single list for zones? instead of listing regions and zones under them ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, sure. Then we'd have to create a sub-list of Zones later using the name of the Region as a pattern. Sounds like we're re-thinking this file as a whole though so maybe not get too far into the weeds quite yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a brief explanation of why this is needed would be nice.

@openshift-ci openshift-ci Bot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 12, 2021
Prashanth684 added a commit to Prashanth684/machine-config-operator that referenced this pull request Oct 21, 2021
Enable Power VS(Virtual Servers) as a platform. PowerVS is an extension of IBMCloud which supports deploying ppc64le VMs.
Details here: openshift/enhancements#736

The hostname for the VMs are set through afterburn which reads the metadata provided through the config drive which is the primary mechanism through which ignition and hostname are provided.
(coreos/afterburn#592)

Cross referencing some other PRs for completeness:
openshift/installer#5270
openshift/installer#5224 (closed, WIP to split into multiple PRs)

ova images for PowerVS are already generated today:
coreos/coreos-assembler#2200
coreos/coreos-assembler#2361
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021
Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 1, 2021

@clnperez: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ovirt 92ba212 link false /test e2e-ovirt
ci/prow/e2e-aws-workers-rhel8 92ba212 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-aws-workers-rhel7 92ba212 link false /test e2e-aws-workers-rhel7
ci/prow/e2e-crc 92ba212 link false /test e2e-crc
ci/prow/okd-e2e-aws-upgrade 92ba212 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-openstack 92ba212 link false /test e2e-openstack
ci/prow/openstack-manifests 92ba212 link true /test openstack-manifests
ci/prow/e2e-aws-upgrade 92ba212 link true /test e2e-aws-upgrade
ci/prow/e2e-aws 92ba212 link true /test e2e-aws

Full PR test history. Your PR dashboard.

Details

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.

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 1, 2021

@clnperez: PR needs rebase.

Details

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.

@clnperez
Copy link
Copy Markdown
Contributor Author

clnperez commented Jan 7, 2022

this is very outdated. closing, and will create new ones when the installer team has bandwidth to review.

@clnperez clnperez closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants