Skip to content

Add IPI Support for IBM Power Virtual Servers Offering#5224

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

Add IPI Support for IBM Power Virtual Servers Offering#5224
clnperez wants to merge 2 commits intoopenshift:masterfrom
clnperez:upstream-firstpass

Conversation

@clnperez
Copy link
Copy Markdown
Contributor

@clnperez clnperez commented Sep 16, 2021

For more background on IPI on Power VS, refer to the enhancement
proposal here: openshift/enhancements#736

We have upstream support for Power VS in the API
(openshift/enhancements#736) and cluster api
provider (https://github.com/openshift/cluster-api-provider-powervs).
Other required work is actively ongoing in the community.

Also actively worked is support for creating networking and boot images
in a way that work with the OCP installer, so our present implemenation
requires that these items be pre-created and passed in via the
install-config. This is temporary and we will add support for IPI-creation
of all required components.

Since Power VS is an offering as part of IBM Cloud (but with seperate APIs for
provisioning), we are able to re-use some of the ibmcloud provider
funtionality. More info in the enhancement proposal on the need to have
a completley seperate provider.

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com
Co-authored-by: Pradipta Banerjee bpradipta@in.ibm.com
Co-authored-by: Manjunath Kumatagi mkumatag@in.ibm.com
Co-authored-by: Mike Turek mjturek@us.ibm.com
Co-authored-by: wolf cory.klokman@ibm.com
Co-authored-by: Karthik K.N. karthik.k.n@ibm.com

@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 Sep 16, 2021
Comment thread go.mod Outdated
Comment thread go.mod Outdated
Comment thread pkg/types/defaults/machinepools.go Outdated
Copy link
Copy Markdown
Contributor

@Prashanth684 Prashanth684 Sep 17, 2021

Choose a reason for hiding this comment

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

isn't the architecture specified explicitly in the control plane and compute through install config? PowerVS only makes sense for ppc64le so there should be a validation failure if the user chooses some arch other than ppc64le. but this should not be hardcoded based on platform name because we already have an arch field.

Copy link
Copy Markdown
Contributor Author

@clnperez clnperez Sep 20, 2021

Choose a reason for hiding this comment

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

yes, it is there. i didn't create a new arch field. this updates the one in the control plane section of the install-config. it doesn't seem to be an option in the survey questions. are you saying we just need to add it as a question then?

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.

yes that's true. but the idea was that the defaultarch would be detected based on payload: 729882a. so if you use an openshift-install binary corresponding to P and create an install config with that, the arch would be set to ppc64le for you irrespective of the platform you are installing on (because the payload was built on a P host). in that sense the "default" when building your own installer will be based on the host system on which you build the installer. the setting of this arch based on platform conflicts with that idea.

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.

so, there's going to be a specific installer binary that is an amd/x86 binary, but "corresponding to P"? (b/c clearly i'm not running a binary on my laptop to do an install that was built on a power box)

or -- there needs to be doc stating that if you're installing from a different arch than you're installing onto, you need to manually edit your install-config?

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.

exactly so for AWS on ARM : https://console.redhat.com/openshift/install/aws/arm we have an option to select an openshift-install binary which is an x86 binary but which is built for an ARM payload, hence the install-config will have the arch set to arm64 when created through this binary. we would need to have the same for PowerVS too. cc @yselkowitz .

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.

and if you locally build the installer and use that install binary, you would have to manually edit the architecture. So, the tl;dr is - arch is based on payload. If you use an openhsift-install binary from the payload you shouldn't worry about the arch. it will be set correctly. if you build the install binary locally, you will have to change the arch manually (i.e, if you are not building on a host corresponding to the arch of the platform)

Copy link
Copy Markdown
Contributor

@yselkowitz yselkowitz Sep 20, 2021

Choose a reason for hiding this comment

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

As of 4.9 all payloads regardless of architecture have a linux/amd64 installer in addition to linux/"native". Also, when you get the installer from the payload, it will default to the payload architecture, regardless of the installer binary runtime arch. If you build the installer yourself (e.g. for development purposes), you can mimic this by running e.g. DEFAULT_ARCH=ppc64le ./hack/build.sh

Comment thread pkg/types/powervs/defaults/platform.go Outdated
Comment thread pkg/types/clusterquota.go Outdated
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.

testing regions should probably go under the _test.go files

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.

ah, misleading comment there. testing in this context should have been development. that TODO needs to removed and these zones need to be triple-checked. i missed a few stray TODOs there we should clean up -- including the one about programmatic generation (since we don't have a way to query zones)

Comment thread pkg/destroy/quota/quota.go Outdated
Comment thread pkg/asset/manifests/ibmcloud/cloudproviderconfig_test.go Outdated
Comment thread pkg/asset/manifests/ibmcloud/cloudproviderconfig.go Outdated
Comment thread pkg/asset/machines/worker.go Outdated
Comment thread pkg/types/installconfig.go Outdated
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2021
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2021
@Prashanth684
Copy link
Copy Markdown
Contributor

/retest

@clnperez
Copy link
Copy Markdown
Contributor Author

looks like now we're hitting #523

@clnperez
Copy link
Copy Markdown
Contributor Author

/retest

mjturek pushed a commit to mjturek/installer that referenced this pull request Sep 23, 2021
We will need to provide a zone when creating a PowerVS service. Also
switches back to using Region from the platform section of the
install config as it is no longer in PowerVSMachineProviderConfig [0]

[0] https://github.com/openshift/cluster-api-provider-powervs/blob/main/pkg/apis/powervsprovider/v1alpha1/powervsmachineproviderconfig_types.go

Depends on openshift#5224
@clnperez
Copy link
Copy Markdown
Contributor Author

/retest
hoping those jq errors go away

mjturek pushed a commit to mjturek/installer that referenced this pull request Sep 27, 2021
We will need to provide a zone when creating a PowerVS service. Also
switches back to using Region from the platform section of the
install config as it is no longer in PowerVSMachineProviderConfig [0]

[0] https://github.com/openshift/cluster-api-provider-powervs/blob/main/pkg/apis/powervsprovider/v1alpha1/powervsmachineproviderconfig_types.go

Depends on openshift#5224
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2021
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2021
mjturek pushed a commit to mjturek/installer that referenced this pull request Feb 25, 2022
this is set at build time using a flag. see openshift#5224 (comment)

Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
clnperez added a commit to clnperez/installer that referenced this pull request Feb 26, 2022
For more background on IPI on Power VS, refer to the enhancement
proposal here: openshift/enhancements#736

Older discussions on some of the code here can be found in
openshift#5224.
An older unreviewed version of this PR can be found here:
openshift#5292

Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
clnperez added a commit to clnperez/installer that referenced this pull request Mar 9, 2022
For more background on IPI on Power VS, refer to the enhancement
proposal here: openshift/enhancements#736

Older discussions on some of the code here can be found in openshift#5224

Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants