OSDOCS#44070 - Docs for vLAN ID in localnet NAD #2 #97319
Conversation
openshift-ci
bot
added
the
size/XS
|
@jherrman The SME provided LGTM in the Jira ticket. This PR can now be merged. Thanks |
|
/label merge-review-required |
|
@jherrman: The label(s) 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-sigs/prow repository. |
|
/label sme-review-done |
openshift-ci
bot
added
the
sme-review-done
|
/label merge-review-needed |
openshift-ci
bot
added
the
merge-review-needed
| <4> The topological configuration for the network. The required value is `localnet`. | ||
| <5> Optional: The maximum transmission unit (MTU) value. If you do not set a value, the Cluster Network Operator (CNO) sets a default MTU value by calculating the difference among the underlay MTU of the primary network interface, the overlay MTU of the pod network, and byte capacity of any enabled features, such as IPsec. | ||
| <6> The value of the `namespace` and `name` fields in the `metadata` stanza of the `NetworkAttachmentDefinition` object. | ||
| <6> Optionally, for more fine-grained network management, you can configure a virtual LAN (VLAN) ID for the NAD. Pods that use this NAD will have have an interface whose traffic will be able to communicate only with other devices that use the same VLAN ID (`200` in this example). |
There was a problem hiding this comment.
"Pods that use this NAD will have have an interface..."
- My English grammar expertise is limited, so I might be wrong here, but I think that "have have" is wrong (should be "have to have"?).
- This is a bit confusing. Although this sentence is true, the context of this doc is VMs, not pods. Maybe it should say something like "Pods/VMs that use this NAD...".
There was a problem hiding this comment.
@yossisegev , no, you're absolutely right, "have have" is a typo here. I will also rewrite the second sentence to speak about VMs.
Thank you for the quick review!
jherrman
force-pushed
the
vlan-ID-for-local-topo-4.18_CNV-44070
branch
from
6ab0262 to
7e57de6
Compare
| <4> The topological configuration for the network. The required value is `localnet`. | ||
| <5> Optional: The maximum transmission unit (MTU) value. If you do not set a value, the Cluster Network Operator (CNO) sets a default MTU value by calculating the difference among the underlay MTU of the primary network interface, the overlay MTU of the pod network, and byte capacity of any enabled features, such as IPsec. | ||
| <6> The value of the `namespace` and `name` fields in the `metadata` stanza of the `NetworkAttachmentDefinition` object. | ||
| <6> Optionally, for more fine-grained network management, you can configure a virtual LAN (VLAN) ID for the NAD. VMs that use this NAD will have an interface whose traffic will be able to communicate only with other devices that use the same VLAN ID (`200` in this example). |
There was a problem hiding this comment.
VMs that use this NAD will have an interface whose traffic will be able to communicate only with other devices that use the same VLAN ID (
200in this example).
Avoid using future tense. This sentence is also quite long, and with the current CQA assessments checking for sentence length, it would be flagged.
Maybe something along the lines of:
VMs with this NAD can only communicate with other devices using the same VLAN ID (200 in this example).
There was a problem hiding this comment.
This exact wording might be somewhat misleading, I think, and AFAIK we should avoid the ambiguous gerund form in favour of "that does XY". Still, you're right that the meaning can be expressed accurately enough without the future tense, and without quite as many words.
Applied peer review
jherrman
force-pushed
the
vlan-ID-for-local-topo-4.18_CNV-44070
branch
from
b331237 to
4661987
Compare
|
@jherrman: 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-sigs/prow repository. I understand the commands that are listed here. |
|
/cherrypick enterprise-4.15 |
|
/cherrypick enterprise-4.16 |
|
@MirzWeiss: new pull request created: #97904 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-sigs/prow repository. |
|
@MirzWeiss: new pull request created: #97905 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-sigs/prow repository. |
|
@MirzWeiss: new pull request created: #97906 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-sigs/prow repository. |
|
/label CNV |
|
/label branch/enterprise-4.15 |
|
@MirzWeiss: The label(s) 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-sigs/prow repository. |
|
/remove-label merge-review-needed |
openshift-ci
bot
removed
the
merge-review-needed
7 participants
Version(s):
4.15 - 4.18
Issue:
https://issues.redhat.com/browse/CNV-44070
Link to docs preview:
https://97319--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/vm_networking/virt-connecting-vm-to-ovn-secondary-network.html#virt-creating-localnet-nad-cli_virt-connecting-vm-to-ovn-secondary-network
QE review:
Additional information:
This aims to be a replacement for #96699 , because the related changes likely are not relevant for 4.19 and later.