Implementation for #200 #215
Conversation
|
Hi @torwen1. Thanks for your PR. I'm waiting for a ocp-power-automation member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
var.tfvars
Outdated
| @@ -47,7 +47,8 @@ cluster_id = "" # It will use random generated id with | |||
|
|
|||
| #network_type = "SRIOV" | |||
| #scg_id = "df21cec9-c244-4d3d-b927-df1518672e87" | |||
|
|
|||
| #sriov_vnic_failover_adapter = 2 | |||
There was a problem hiding this comment.
Better keep the default value of 1 here.
There was a problem hiding this comment.
Good point. Will update the var.tfvars accordingly.
| @@ -122,6 +122,18 @@ variable "network_type" { | |||
| description = "Specify the name of the network adapter type to use for creating hosts" | |||
| } | |||
|
|
|||
| variable "sriov_vnic_failover_adapter" { | |||
| # Eg: 1 = VNIC without failover; 2 = VNIC failover with 2 SR-IOV LPs | |||
| default = 1 | |||
There was a problem hiding this comment.
I read about it in API docs. Would recommend a validation block for vnic_required_vfs cannot be greater than 6
There was a problem hiding this comment.
Also good point to have a validation, so that a user cannot enter a higher number. Have to check what is possible with validators, but I would prefer a stop of the deployment with a message about the restriction.
There was a problem hiding this comment.
Just a question, because I'm not (yet) familiar with the process in GitHub: Will a change like this be a new PR, or is there a different process like squash commits?
There was a problem hiding this comment.
Also good point to have a validation, so that a user cannot enter a higher number. Have to check what is possible with validators, but I would prefer a stop of the deployment with a message about the restriction.
You could use Terraform Custom Validation Rules but I am okay with just a mention in the documentation.
There was a problem hiding this comment.
Just a question, because I'm not (yet) familiar with the process in GitHub: Will a change like this be a new PR, or is there a different process like squash commits?
Since this is a small change you could make the changes and ammend to the same commit and then force push to your branch.
There was a problem hiding this comment.
Ok. Is done. Added and tested validation rule, updated documentation and changed the variable name from sriov_vnic_failover_adapter to sriov_vnic_failover_vfs (because we don't define adapters here, we define virtual functions and the users might get mislead with the original name).
|
/ok-to-test |
torwen1
force-pushed
the
Implemenation-for-#200.2
branch
2 times, most recently
from
02f3018
to
6ec2db0
Compare
This is a suggestion for the implementation of ocp-power-automation#200 I already implemented exactly this code at a customer. It introduces two new variables to var.tfvars to control how many logical ports shall be used for VNIC failover and also to define the capacity of the logical ports. Signed-off-by: Torsten Wendland <twendlan@de.ibm.com> PS: Added validation of number of VFs, updated documentation with min and max, changed variable name from sriov_vnic_failover_adapter to sriov_vnic_failover_vfs (because we don't define amount of adapters, we are defining amount of virtual functions; adapters might mislead the user)
torwen1
force-pushed
the
Implemenation-for-#200.2
branch
from
6ec2db0
to
5b714fb
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prajyot-Parab, torwen1, yussufsh 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 |
This is a suggestion for the implementation of #200
I already implemented exactly this code at a customer. It introduces two new variables to var.tfvars to control how many logical ports shall be used for VNIC failover and also to define the capacity of the logical ports.
PS: This time I have everything in one commit.
Signed-off-by: Torsten Wendland twendlan@de.ibm.com