Vmware nsxt for ocp4 #2622
Vmware nsxt for ocp4 #2622
Conversation
This patch will add support for VMware's NSX-T network. It installs the required elements in NSX: logical switch, port connecting it to T1 Router, DHCP server and IP pool for the VMs running the Openshift cluster. Signed-off-by: Michal Kelner Mishali <mkelnermishal@vmware.com>
This patch will add support for VMware's NSX-T network. It installs the required elements in NSX: logical switch, port connecting it to T1 Router, DHCP server and IP pool for the VMs running the Openshift cluster. Signed-off-by: Michal Kelner Mishali <mkelnermishal@vmware.com>
…al/installer into vmware-nsxt-for-ocp4
openshift-ci-robot
added
do-not-merge/work-in-progress
|
Hi @mkelnermishal. Thanks for your PR. I'm waiting for a openshift 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. 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. |
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkelnermishal 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 |
| module "nsx_network" { | ||
| source = "./nsx_network" | ||
|
|
||
| base_domain = "${var.base_domain}" |
There was a problem hiding this comment.
indent does not seem right here
| max_retries = 10 | ||
| retry_min_delay = 500 | ||
| retry_max_delay = 5000 | ||
| retry_on_status_codes = [429] |
There was a problem hiding this comment.
It Might be worth retrying on 503 and 429
| // match the value of compute_count. | ||
| //compute_ips = ["10.0.0.30", "10.0.0.31", "10.0.0.32"] | ||
|
|
||
| // NSX part for NSX-T network. |
There was a problem hiding this comment.
While many deployments will have NSX-T not all deployments will be on NSX-T. We need a variable to switch NSX-T on/off. If off, the NSX-T resources won't be created.
|
|
||
| variable "transport_zone_id" { | ||
| type = "string" | ||
| } No newline at end of file |
There was a problem hiding this comment.
add a newline at the end of the file please
| description = "NSX manager password" | ||
| } | ||
|
|
||
| variable "ip_block_cidr" { |
There was a problem hiding this comment.
all these variables deserve a description imho
| ip_address = "${var.logical_switch_ip_address}" | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There should be a single blank line
| resource "nsxt_logical_dhcp_server" "logical_dhcp_server" { | ||
| display_name = "logical_dhcp_server" | ||
| dhcp_profile_id = "${nsxt_dhcp_server_profile.dhcp_profile.id}" | ||
| dhcp_server_ip = "${var.dhcp_server_ip}/24" |
There was a problem hiding this comment.
prefix should not be hardcoded. Can we have it as a variable?
| description = "ip pool" | ||
| logical_dhcp_server_id = "${nsxt_logical_dhcp_server.logical_dhcp_server.id}" | ||
| gateway_ip = "${var.gateway_ip}" | ||
| lease_time = 180 |
There was a problem hiding this comment.
I'm ok with hardcoding it, but why such a short lease?
There was a problem hiding this comment.
this was done for testing purposes, changing to a more reasonable value.
| ip_range { | ||
| start = "${var.ip_pool_start}" | ||
| end = "${var.ip_pool_end}" | ||
| } |
There was a problem hiding this comment.
I don't see where we configure default DHCP options. For OCP we will only need the gateway
| description = "ip_block_subnet" | ||
| block_id = "${nsxt_ip_block.ip_block.id}" | ||
| size = 16 | ||
| } |
There was a problem hiding this comment.
I'm not sure why you're cutting a /16 subnet from the block
|
Please open an GitHub issue that defines the problem so that there is a discussion. |
|
cc @jcpowermac just fyi |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
lifecycle/stale
|
@mkelnermishal: PR needs rebase. 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. |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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. |
6 participants
No description provided.