Skip to content

Conversation

@ahardin-rh
Copy link
Contributor

@ahardin-rh ahardin-rh added this to the Next Release milestone Oct 29, 2020
@ahardin-rh ahardin-rh self-assigned this Oct 29, 2020
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 29, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In OCP 4.x, the MTU is configured at install time only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's from Ansible for 3.11? This is inapplicable in 4.x.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'd keep the MTU discussion possibly, but omit the procedure and note that this must happen during cluster installation only.

And the MTU size varies between OpenShift SDN and OVN-Kubernetes; We now offer both as of 4.6 GA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably makes sense to include OVN-Kubernetes as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it uses Geneve / GENEVE instead of VXLAN as the tunnel protocol.

@ahardin-rh ahardin-rh force-pushed the network-optimization branch from 4212aa2 to 1d60140 Compare November 6, 2020 03:17
@ahardin-rh
Copy link
Contributor Author

@chaitanyaenr @jboxman I made some updates. PTAL and let me know if we're getting closer 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have excised SDN mostly from the docs, because reasons. OpenShift SDN is a legacy name. Lately, it's called the cluster network provider. (Such as OpenShift SDN cluster network provider.) So I'd say

and the cluster network MTU.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/0must/must/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all handled internally now; Previously in OCP 3.x it was somewhat manual.

So I'd omit entirely the second paragraph. No one can actually do that anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's possible to xref to OVN-Kubernetes as well.

xref:../../networking/ovn_kubernetes_network_provider/about-ovn-kubernetes.adoc#about-ovn-kubernetes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or these xrefs:

[id="nw-operator-configuration-parameters-for-openshift-sdn_{context}"]
== Configuration parameters for the OpenShift SDN default CNI network provider
[id="nw-operator-configuration-parameters-for-ovn-sdn_{context}"]
== Configuration parameters for the OVN-Kubernetes default CNI network provider

@jboxman
Copy link
Contributor

jboxman commented Nov 6, 2020

@ahardin-rh, added a few more comments.

@ahardin-rh ahardin-rh force-pushed the network-optimization branch from 1d60140 to ef5096f Compare November 9, 2020 22:15
@ahardin-rh
Copy link
Contributor Author

@jboxman Updated. Thanks for the insight!

@ahardin-rh
Copy link
Contributor Author

@anuragthehatter Are you able to provide QE review or point me in the right direction? Thanks!

@ahardin-rh
Copy link
Contributor Author

@mffiedler PTAL. Thank you!

@ahardin-rh ahardin-rh requested a review from mffiedler December 3, 2020 14:07
@mffiedler
Copy link

@anuragthehatter @rbbratta Please reveiw - especially from a 3.11 -> 4.z change perspective and the addition of OVN

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I believe its minus 100 bytes for OVN. We can also include it as reference for OVN as geneve/ovn is already being discussed in this doc. wdyt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anuragthehatter I applied your suggestion. Can you please review the latest change? Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahardin-rh Thank you for applying the change. I guess we can put line For OVN and Geneve, the MTU must be less than the NIC MTU by 100 bytes at a minimum. after we finish sdn discussion. I means post On a jumbo frame ethernet network, set this to `8950`.. Currently seems like we are sandwiching ovn details in btw continued SDN dicussion. Will leave up to you to decide. Thank you. Rest LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I moved the OVN discussion to the right spot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

@ahardin-rh ahardin-rh added the peer-review-done Signifies that the peer review team has reviewed this PR label Dec 9, 2020
@ahardin-rh ahardin-rh force-pushed the network-optimization branch from ef5096f to fc761e2 Compare December 9, 2020 22:25
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2020
@ahardin-rh ahardin-rh merged commit a41e510 into openshift:master Dec 11, 2020
@ahardin-rh
Copy link
Contributor Author

ahardin-rh commented Dec 11, 2020

/cherrypick enterprise-4.7

@ahardin-rh
Copy link
Contributor Author

ahardin-rh commented Dec 11, 2020

/cherrypick enterprise-4.6

@ahardin-rh
Copy link
Contributor Author

ahardin-rh commented Dec 11, 2020

/cherrypick enterprise-4.5

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Dec 11, 2020

@ahardin-rh: new pull request created: #28048

Details

In response to this:

/cherrypick enterprise-4.7

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.

@ahardin-rh
Copy link
Contributor Author

ahardin-rh commented Dec 11, 2020

/cherrypick enterprise-4.4

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Dec 11, 2020

@ahardin-rh: new pull request created: #28049

Details

In response to this:

/cherrypick enterprise-4.6

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.

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Dec 11, 2020

@ahardin-rh: new pull request created: #28050

Details

In response to this:

/cherrypick enterprise-4.5

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.

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Dec 11, 2020

@ahardin-rh: new pull request created: #28051

Details

In response to this:

/cherrypick enterprise-4.4

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.4 branch/enterprise-4.5 branch/enterprise-4.6 branch/enterprise-4.7 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants