MULTIARCH-3667: Add support for CPMSO for Power VS #7226
Conversation
|
/hold Wait for testing and merging of openshift/machine-api-provider-powervs#41 PR |
openshift-ci
bot
added
the
do-not-merge/hold
Karthik-K-N
force-pushed
the
powervs-cpms-support
branch
2 times, most recently
from
3730b42 to
ec043c0
Compare
Karthik-K-N
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@Karthik-K-N: This pull request references MULTIARCH-3667 which is a valid jira issue. 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
|
/lgtm |
Karthik-K-N
force-pushed
the
powervs-cpms-support
branch
from
ec043c0 to
c6e4d35
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
|
@rna-afk could you please help in merging this PR |
|
/test ? |
|
@patrickdillon: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
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. |
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
/approve
Is there a powervs sme that can review/lgtm?
| for _, machine := range machines { | ||
| providerSpec, ok := machine.Spec.ProviderSpec.Value.Object.(*machinev1.PowerVSMachineProviderConfig) | ||
| if !ok { | ||
| return errors.New("Unable to set load balancers to control plane machine set") |
There was a problem hiding this comment.
nit: errors should start with a lowercase letter, but no need to change this unless you're making other changes
|
|
||
| providerSpec, ok := controlPlane.Spec.Template.OpenShiftMachineV1Beta1Machine.Spec.ProviderSpec.Value.Object.(*machinev1.PowerVSMachineProviderConfig) | ||
| if !ok { | ||
| return errors.New("Unable to set load balancers to control plane machine set") |
There was a problem hiding this comment.
nit: errors should start with a lowercase letter, but no need to change this unless you're making other changes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkumatag, patrickdillon 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
Thank you, Its already been reviewed and approved from Power VS team #7226 (review) |
|
/lgtm |
|
@Karthik-K-N: 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. |
|
@Karthik-K-N Looks like this is still inactive by default? Any reason not to be making these active? Also, do you intend to have upgraded clusters add CPMSs? On other platforms we have a generator in the CPMS repo itself that creates CPMSs based on the existing Machines in the cluster, in an inactive state |
|
We started with inactive state thinking to have enough testing done before making it active. @juliemathew If testing looks or if there is not anything concerning, Shall I make it active by default. Initially I thought its not required to have generator as at that time PowerVS was still in TP mode. @mkumatag do you think we need to cover this scenario as well? |
7 participants
Add support for CPMSO for Power VS platform, Initially it will be in Inactive state, Later based on the feedback we can by default make it active.
opesnhift/api changes PR: openshift/api#1415
openshfit/machine-api-provider-powervs changes: openshift/machine-api-provider-powervs#41