Add enhancement - IBM Cloud provider for Power Virtual Server platform#736
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thank you SO MUCH @jaypoulz for kicking this off! Not at all important for technical discussions, but we've been asked to use |
jaypoulz
changed the title
jaypoulz
changed the title
jaypoulz
changed the title
clnperez
left a comment
clnperez
left a comment
There was a problem hiding this comment.
Coming along nicely! Added some comments for review.
jaypoulz
force-pushed
the
powervs
branch
11 times, most recently
from
3b88092 to
6bbde74
Compare
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
/approve
This looks good from my team's perspective.
I can see the installer team has LGTM'd, are there any other teams that we need to get to review this before we merge?
What's the story behind storage for PowerVS? Is this being considered as part of this?
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| #### Existing APIs in the Upstream | ||
| There is a working [implementation of a machine API](https://github.com/openshift-powervs/cluster-api-provider-powervs) |
There was a problem hiding this comment.
This doesn't need updating in the enhancement, but something I wanted to note is that we are trying to align the naming of our providers now to machine-api-provider-..., it would be good if we could work to rename this repo before we get it into the product so that it conforms with the other providers for MAPI (this change has been worked on over the last few months)
There was a problem hiding this comment.
@Karthik-K-N is on it and already started working on this.
If it is about csi storage then not part of first phase, we are exploring this for next phase. |
Missing storage (PVs / PVCs) is explicitly noted in the "Persistent Storage" chapter, I find it sufficient. I did not check if there are enough details about storage for VM's root disks and/or registry. |
There was a problem hiding this comment.
A couple of comments and generally, since powervs is already in the mco repo, what types of changes are you expecting in the MCO for this cloud provider? I'd presume it wouldn't be too disruptive? cc: @jaypoulz
@kikisdeliveryservice this is the PR for PowerVS support in MCO (openshift/machine-config-operator#2801). The only thing worthy of mention here is that the hostname for the node is configured through the afterburn service which reads the hostname from the metadata in the config drive. |
|
/approve cancel Just to prevent merge until the comments from @kikisdeliveryservice are resolved |
|
Replaced references from CoreOS to RHCOS where applicable. |
|
Working with the team to add detail around how ingress interfaces with Direct Link as per request by Network-Edge team |
|
From an MCO perspective this seems reasonable especially given that openshift/machine-config-operator#2801 and coreos/afterburn#592 have already merged. LGTM |
|
Gave this a read through, everything looks sane to me. |
| or on-premises environments. Both public and private network is supported for instances (VMs). For public networks, only | ||
| limited ports are opened for inbound access and this is not configurable. | ||
|
|
||
| Basic Walkthrough: |
There was a problem hiding this comment.
I really appreciate that you included the level of detail in this walkthrough. It's very helpful to understand who is doing what, and how.
|
LGTM, but if possible, could you improve the traffic diagram so it doesn't have Direct Link or properly acknowledges the role of Direct Link? https://github.com/openshift/enhancements/pull/736/files#diff-bfbf78d36c375706902a827b5aba7f0244f3a25c0edfb52e934332faf63fedd6 |
The IBM Cloud APIs have the ability to provision power hardware using their Power Virtual Server offering. You can find out more at https://www.ibm.com/cloud/power-virtual-server.
|
Updated formatting and added a note in traffic image about direct link being a transparent GRE tunnel to highlight that thecluster operators don't operate with it directly. Should be good to merge now:
/unhold |
|
@jaypoulz: all tests passed! 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. |
|
As far as I can tell, all of the feedback has now been addressed and we have had acks from all of the relevant teams, re-adding my approval |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, JoelSpeed 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 |
|
/lgtm |
20 participants
The IBM Cloud provides access to power hardware through their Power Virtual Server offering (Power VS). You can find out more at https://www.ibm.com/cloud/power-virtual-server.