-
Notifications
You must be signed in to change notification settings - Fork 517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PowerVSMachineProviderConfig #1159
Add PowerVSMachineProviderConfig #1159
Conversation
/cc @mkumatag @JoelSpeed |
// UserDataSecret contains a local reference to a secret that contains the | ||
// UserData to apply to the instance. | ||
// +optional | ||
UserDataSecret *corev1.LocalObjectReference `json:"userDataSecret,omitempty"` | ||
|
||
// CredentialsSecret is a reference to the secret with IBM Cloud credentials. | ||
// +optional | ||
CredentialsSecret *corev1.LocalObjectReference `json:"credentialsSecret,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are consistent with other providers (even those added in the last couple of releases), however, there is a convention that we shouldn't use external types like LocalObjectReference, and instead, should use our own reference type that we control (to prevent external parties adding extra fields to our API). Will double check whether we should change this given how recently we merged those other APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I missed some during previous reviews. We do not re-use these types because we do not control the schemas and re-use in different contexts can result in fields that are not honored when/if they are added, lack of control over validation, and lack of control over doc. A LocalSecretRef and LocalConfigMap ref are appropriate to add for this package.
Changing already messed up fields may be possible if
- the client serialization is identical
- we don't tighten validation on the replacement fields
- any fields dropped were fully unused
- coordinate with @sanchezl to do a read/write cycle of the type in question during an upgrade.
// Region indicates the Power VS ServiceInstance region. | ||
// +optional | ||
Region string `json:"region,omitempty"` | ||
|
||
// Zone indicates the Power VS ServiceInstance zone. | ||
// +optional | ||
Zone string `json:"zone,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these relatively self explanatory or is there additional information we can add here? For example, I notice these are both optional, what happens if you don't set them? Should they actually be required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Region and Zone indicated the Power VS Service region and zone respectivly(https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-creating-power-virtual-server#creating-service) , If these are omitted currently its extracted from Power VS cloud with the help of ServiceInstanceID or ServiceInstanceName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rightnow these values aren't used anywhere and we are deriving the region and zone from ServiceInstance
resource .
@Karthik-K-N do you think these values will be useful in future anywhere? I'm okay to add them in future when required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently its not being actively used, Just added it for future use cases. If can add later when we have clear idea of the requirement we can remove it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not used today, delete it, adding new fields later (so long as they are not required), is something we can do without making a breaking API change
// +optional | ||
Zone string `json:"zone,omitempty"` | ||
|
||
// ServiceInstance is the reference to the Power VS ServiceInstance on which the machine instance will be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing the concept of a ServiceInstance is well known to power vs users? It doesn't need further definition?
// KeyPairName is the name of the KeyPair to use for SSH | ||
// +kubebuilder:validation:=Required | ||
KeyPairName string `json:"keyPairName"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Machine API doesn't control anything about the software layer, so I'm not sure what you could be doing with this. Is it actually used at all?
This should be dropped I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ssh-key-pair name is a mandatory param to the create vm call in the powervs service, without this it actually fails. User need to create the key pair in advance and send the same to the api to use while the deploy(though it doesn't play a big role here in case of RHCOS, will ask powervs team to make this optional going forward so that we can get rid off the same)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be hidden from the end user so that we don't expose it to them? IMO it doesn't make sense to expose this to the user as it's not something that actually effects them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be hidden from the end user so that we don't expose it to them? IMO it doesn't make sense to expose this to the user as it's not something that actually effects them
How this cab be hidden? this is a required param to the api, otherwise we have seen error while deploying the instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was, could the Machine controller handle this for the user by creating a random keypair per machine (and deleting it again later), and then the user doesn't need to know about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installer is already creating one per cluster, these keys are global to the account and I'm wondering how the whole system will behave when we have many more machines and users(don't want to hit with any limits here, though not sure what's the limit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the name of that key available somewhere within the cluster?
I have noticed that OpenStack have an SSH key pair in their API, I'm trying to gather context on that. That said, repeating past mistakes is not something we do in OpenShift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clnperez are we pushing ssh-key name we use using during the installer somewhere?
This means that we end up adding dependency on some of the resources like Infrastructure or any other which contains the key name, this will add the dependency on the other non-mapi resources, not sure if we really should do that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, we want to make the API our users consume as simple as possible, making them set a field that is redundant (ie this has no effect on the outcome of the VM, but they must set it to something) makes for a poor UX, so i'm a strong believer we should find some way to hide this, or make it optional from the provider side
Do we know why it is required on the provider side? Perhaps if there's some use case that we can explain then we can find a reason this needs to be exposed to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the user in the Power VS present today deploy the vms and access them via ssh-key, may be that is the reason they use have this field as required but we can work the folks to make this optional at least for the OS type coreos(can't promise about the timeline but something can be proposed)
// PowerVSResource is a reference to a specific PowerVS resource by ID, Name or RegEx | ||
// Only one of ID, Name or RegEx may be specified. Specifying more than one will result in | ||
// a validation error. | ||
type PowerVSResource struct { | ||
// ID of resource | ||
// +optional | ||
ID *string `json:"id,omitempty"` | ||
|
||
// Name of resource | ||
// +optional | ||
Name *string `json:"name,omitempty"` | ||
|
||
// Regex to find resource | ||
// +optional | ||
RegEx *string `json:"regex,omitempty"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a discriminated union and should be marked up as so. You will need a discriminator field
// PowerVSResource is a reference to a specific PowerVS resource by ID, Name or RegEx | |
// Only one of ID, Name or RegEx may be specified. Specifying more than one will result in | |
// a validation error. | |
type PowerVSResource struct { | |
// ID of resource | |
// +optional | |
ID *string `json:"id,omitempty"` | |
// Name of resource | |
// +optional | |
Name *string `json:"name,omitempty"` | |
// Regex to find resource | |
// +optional | |
RegEx *string `json:"regex,omitempty"` | |
} | |
// PowerVSResource is a reference to a specific PowerVS resource by ID, Name or RegEx | |
// Only one of ID, Name or RegEx may be specified. Specifying more than one will result in | |
// a validation error. | |
// +union | |
type PowerVSResource struct { | |
// Type is the reference type. | |
// Valid values are ID, Name and RegEx | |
// +kubebuilder:validation:Enum:=ID;Name;RegEx | |
// +optional | |
Type PowerVSResourceType `json:"type,omitempty"` | |
// ID of resource | |
// +optional | |
ID *string `json:"id,omitempty"` | |
// Name of resource | |
// +optional | |
Name *string `json:"name,omitempty"` | |
// Regex to find resource | |
// +optional | |
RegEx *string `json:"regex,omitempty"` | |
} |
Please also think about the validations for each of the fields within this API. Tell the user through comments what the expected values are. Are there formats they need to adhere to, characters they are/aren't allowed to use, length limits etc
// PowerVSMachineProviderConfigList contains a list of PowerVSMachineProviderConfig | ||
// | ||
// Compatibility level 2: Stable within a major release for a minimum of 9 months or 3 minor releases (whichever is longer). | ||
// +openshift:compatibility-gen:level=2 | ||
type PowerVSMachineProviderConfigList struct { | ||
metav1.TypeMeta `json:",inline"` | ||
// +optional | ||
metav1.ListMeta `json:"metadata,omitempty"` | ||
Items []PowerVSMachineProviderConfig `json:"items"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this as this isn't a CRD
// InstanceID is the instance ID of the machine created in PowerVS | ||
// +optional | ||
InstanceID *string `json:"instanceId,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will this be set, what does it mean if it is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstanceID uniquely identifies a VM instance under a Power VS Service, Once the VM is created in Power VS Service the InstanceID is set and later it is used to update the VM or delete the VM.
If InstanceID is missing we need to make a call to PowerVS cloud with VM name to identify a instance, In our case its more time consuming than querying with ID.
// InstanceState is the state of the PowerVS instance for this machine | ||
// +optional | ||
InstanceState *string `json:"instanceState,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the possible values for this, what does it mean for the field to be missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstanceState can be ACTIVE, BUILD, SHUTOFF, REBOOT. There is no effect if this field is mission just that it provide additional information about state of machine
// Conditions is a set of conditions associated with the Machine to indicate | ||
// errors or other status | ||
Conditions []PowerVSMachineProviderCondition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a metav1.Condition instead and move Conditions to the top of the status.
What are the expected condition types here, they should be enumerated in the comment (Eg look at the ControlPlaneMachineSetStatus.Conditions comments (it has a number of additional kubebuilder tags that should be add for patch merge type)
|
||
// processors is the number of virtual processors in a virtual machine. | ||
// Defaults to 0.5. | ||
// +optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This number can't be in fraction if ProcessorType is selected as Dedicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to add more explanation to this to explain the default values and acceptable values based on the different types of processor.
I'm not sure if the string
type is best here, have you considered an IntOrString, this is how we would typically handle this kind of field, float values being a string, integer values being an int, it's more user friendly
9eb5e44
to
622012e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates on this, it's looking a whole lot better! Have left some additional feedback, once this is resolved I think we are good to go
machine/v1/types_powervsprovider.go
Outdated
// userDataSecret contains a local reference to a secret that contains the | ||
// UserData to apply to the instance. | ||
// +optional | ||
UserDataSecret *corev1.LocalObjectReference `json:"userDataSecret,omitempty"` | ||
|
||
// credentialsSecret is a reference to the secret with IBM Cloud credentials. | ||
// +optional | ||
CredentialsSecret *corev1.LocalObjectReference `json:"credentialsSecret,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would prefer to use a specific local type for these references, see this convention https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-specific-types-for-object-references-and-omit-ref-suffix for more details
machine/v1/types_powervsprovider.go
Outdated
// More detail about Power VS service instance | ||
// https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-creating-power-virtual-server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useful, but because it's disconnected, the end user won't see it, how about moving this in just above the required validation line?
// serviceInstance can be created via IBM Cloud catalog or CLI. | ||
// serviceInstance identifier (name or id) can be obtained from IBM Cloud UI or IBM Cloud cli. | ||
// +kubebuilder:validation:=Required | ||
ServiceInstance PowerVSResource `json:"serviceInstance"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a PowerVSResource, are all three types of resource reference supported? Name, ID and Regex? If not, you should mention here which are supported and which aren't
// image is to identify the rhcos image uploaded to IBM COS bucket which is used to create the instance. | ||
// image identifier (name or id) can be obtained from IBM Cloud UI or IBM Cloud cli. | ||
// +kubebuilder:validation:=Required | ||
Image PowerVSResource `json:"image"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a PowerVSResource, are all three types of resource reference supported? Name, ID and Regex? If not, you should mention here which are supported and which aren't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you preventing Regex from being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have planned to add a necessary checks in mapio webhooks
// network is the reference to the Network to use for this instance. | ||
// network identifier (name or id) can be obtained from IBM Cloud UI or IBM Cloud cli. | ||
// +kubebuilder:validation:=Required | ||
Network PowerVSResource `json:"network"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a PowerVSResource, are all three types of resource reference supported? Name, ID and Regex? If not, you should mention here which are supported and which aren't
|
||
// processors is the number of virtual processors in a virtual machine. | ||
// Defaults to 0.5. | ||
// +optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to add more explanation to this to explain the default values and acceptable values based on the different types of processor.
I'm not sure if the string
type is best here, have you considered an IntOrString, this is how we would typically handle this kind of field, float values being a string, integer values being an int, it's more user friendly
machine/v1/types_powervsprovider.go
Outdated
// The minimum memory is 32GB | ||
// Defaults to 32 GB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, you should have a period on the end. Might also suggest a change to this doc
// The minimum memory is 32GB | |
// Defaults to 32 GB. | |
// The minimum memory is 32GiB. | |
// When omitted, this means the user has no opinion and the platform is left to choose a reasonable | |
// default. The current default is 32. |
machine/v1/types_powervsprovider.go
Outdated
// +optional | ||
Processors string `json:"processors,omitempty"` | ||
|
||
// memoryGiB is the size of a virtual machine's memory, in GB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be GibiBytes right?
// memoryGiB is the size of a virtual machine's memory, in GB. | |
// memoryGiB is the size of a virtual machine's memory, in GiB. |
|
||
// conditions is a set of conditions associated with the Machine to indicate | ||
// errors or other status | ||
// +optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional hints
// +optional | |
// +patchMergeKey=type | |
// +patchStrategy=merge | |
// +listType=map | |
// +listMapKey=type | |
// +optional |
machine/v1/types_powervsprovider.go
Outdated
ServiceInstanceID *string `json:"serviceInstanceID,omitempty"` | ||
|
||
// instanceState is the state of the PowerVS instance for this machine | ||
// Possible instance states are ACTIVE, BUILD, SHUTOFF, REBOOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the states conform to Kube pascal case conventions?
// Possible instance states are ACTIVE, BUILD, SHUTOFF, REBOOT | |
// Possible instance states are Active, Build, ShutOff, Reboot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again these are state what we get from the APIs, do we really need to change the case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous comment, but yes, this should match kube conventions over the cloud provider as we are building a user interface for Kube users, which we want to be consistent within OpenShift
machine/v1/types_powervsprovider.go
Outdated
type PowerVSSecretReference struct { | ||
// Name of the secret. | ||
// +optional | ||
Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the protobuf tag on this
// +kubebuilder:validation:=Required | ||
Network PowerVSResource `json:"network"` | ||
|
||
// keyPairName is the name of the KeyPair to use for SSH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like some more explanation as to how this affects the end user
// keyPairName is the name of the KeyPair to use for SSH. | |
// keyPairName is the name of the KeyPair to use for SSH. | |
// The key pair will be exposed to the instance via the instance metadata service. | |
// On boot, the OS will copy the public keypair into the authorized keys for the core user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment by Joel from a slack thread:
So I went to do some digging, it turns out coreos will actually pull in the SSH keys, so you could include that in the API (we have it in openstack), but, I still don't like it, and I'd prefer it not to be required as it is a weird disconnect for users, Machines do not configure software, apart from it turns out, the ssh key
https://github.com/coreos/afterburn/blob/main/systemd/afterburn-sshkeys%40.service.in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PowerVS platform is not really in the list https://github.com/coreos/afterburn/blob/main/systemd/afterburn-sshkeys%40.service.in here, @Prashanth684 can we get the support added for PowerVS in that afterburn sshkeys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sshkeys are configured through ignition when provided through the install config. are we saying we want this support in afterburn in case it is not specified through install config ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sshkeys are configured through ignition when provided through the install config. are we saying we want this support in afterburn in case it is not specified through install config ?
It will be good to have that as well, any user can deploy a RHCOS vm via powervs directly without any ingition for any testing or developement activities.
8b9e834
to
9202e58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question on the memory sizes for the different instance types, and I still think we need to work out what to do about the SSH key.
If the SSH key is going to be exposed to a user, it needs to have an effect that they can understand, therefore we need to either have afterburn set it on the host, or it shouldn't be exposed to the end user IMO
machine/v1/types_powervsprovider.go
Outdated
// when SystemType is set to e880 maximum MemoryGiB value is 6950.4604 GiB. | ||
// when SystemType is set to e980 maximum MemoryGiB value is 14255.755 GiB. | ||
// when SystemType is set to s922 maximum MemoryGiB value is 14847.144 GiB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did these values come from? When I read the specifications it said s922 had a maximum of 4TiB of memory or 4096 GiB? Likewise e980 said 64TiB or 65536 GiB. e880 is marketed as 32TiB or 32786 GiB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took that data from here: https://cloud.ibm.com/docs/power-iaas?topic=power-iaas-pricing-virtual-server, Also I need to update the value for s922 to 877.306 GiB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the values in their documentation are actually already in GiB, in which case we should just copy those values across. That said, I'm not sure whether we want to go with these maximums or the specified maximums for the IBM server types? Have you seen their spec sheets before?
Eg from here
The Power S922 server supports a maximum of 32 DDR4 DIMM slots. Memory features supported are 8 GB, 16 GB, 32 GB, 64 GB, and 128 GB and run at different speeds of 2133, 2400, and 2666 Mbps, offering a maximum system memory of 4096 GB.
Note these also mean GiB but, sigh, they haven't used the correct term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, I got confused with the units, Here is the summary of values from both the sites
SystemType, FromPricingDoc , FromServerSpec
e880 , 7,463 GB , 16 TB
e980 , 15,307 GB,
s922, 942 GB , 4TB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried to create, for example, a s922 with over 942 GB? Does it work? Is there anyone from IBM who can recommend which of these we should stick to?
Reading through the pricing doc it infers these maximums are inferred, so I think maybe we go with the pricing maximums
dbd79e5
to
3ab5e63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @Karthik-K-N
/lgtm
good structure, excellent docs. I have a few minor questions to clear up before merge. Get me on slack once updated. |
/test verify-client-go |
a9834a5
to
90a6d32
Compare
/approve |
38b13db
to
6b92d4d
Compare
@Karthik-K-N: all tests passed! Full PR test history. Your PR dashboard. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, elmiko, JoelSpeed, Karthik-K-N The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/label px-approved |
1 similar comment
/label px-approved |
This PR contains the changes to add Power VS machine provider config to api/machine directory
Related to enhancement: openshift/enhancements#736