Skip to content

WIP Add doc for the SR-IOV Network Operator#16393

Closed
pliurh wants to merge 3 commits intoopenshift:masterfrom
pliurh:sriov-network-operator
Closed

WIP Add doc for the SR-IOV Network Operator#16393
pliurh wants to merge 3 commits intoopenshift:masterfrom
pliurh:sriov-network-operator

Conversation

@pliurh
Copy link
Copy Markdown
Contributor

@pliurh pliurh commented Aug 27, 2019

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2019
Copy link
Copy Markdown
Contributor

@zshi-redhat zshi-redhat left a comment

Choose a reason for hiding this comment

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

It might be worth having a few examples of Pod spec for using SR-IOV in different mode, kernel mode, userspace mode and RDMA.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Resources, which are generated and updated by the operator automatically.

One CR is created for each worker node, and shares the same name as the node. In
the interface list, you can the information of the network devices.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing word at you can <> the information

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
<1> The name of the CR.
<2> The namespace of the CR, this must be in the same namespace of operator.
<3> The resource name of SR-IOV device plugin. Prefix `openshift.io/` will be
add when it's referred in pod annotation. It's allowed to create multiple CRs of `SriovNetworkNodePolicy` for one resource name.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add -> added
pod annotation -> pod spec? or net-attach-def ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
The SR-IOV Network Operator also introduces a CRD `SriovNetwork` for creating
the Custom Resource of `NetworkAttachmentDefinition` of SR-IOV CNI plugin. When
you create a CR of `SriovNetwork`, the operator will created a CR of
`NetworkAttachmentDefinition` according.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

accordingly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated

[NOTE]
=====
You shall not modify or delete a Custom `SriovNetwork`, when it has been used by
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Custom 'SriovNetwork' -> 'SriovNetwork' Custom Resource.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
=====

The following is an example of a typical Custom Resource for
`SriovNetworkNodePolicy`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

'SriovNetworkNodePolicy' -> 'SriovNetwork'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
"rangeStart": "10.56.217.171",
"rangeEnd": "10.56.217.181",
"routes": [{
"dst": "10.0.0.0/8"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

overwrite default route is not possible with current ipam plugin, perhaps add an example of non-default route address.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Range from 0 to 99.
<6> The MTU of the virtual functions. Range from 1 to 9000. Leave it blank if
you don't need to change the MTU.
<7> The number of the virtual functions for each SR-IOV network device.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for each SR-IOV network device => will be created for each SR-IOV physical network device

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
<6> The MTU of the virtual functions. Range from 1 to 9000. Leave it blank if
you don't need to change the MTU.
<7> The number of the virtual functions for each SR-IOV network device.
<8> The vendor hex code of SR-IoV device. Allowed value "8086", "15b3".
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

might be good to add note that

  1. it's not necessary to fill all nicSelector filters in every policy
  2. some nicSelector filters may conflict with each other if not configured properly, such as pfNames and rootDevices.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
<11> The PCI addresses of SR-IoV physical function.
<12> The driver type of the virtual functions. Allowed value "netdevice",
"vfio-pci". Defaults to "netdevice".
<13> The RDMA mode. Defaults to false.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may be worth more explanation or notes on

  1. which types of NIC are supported for RDMA mode in current release,
  2. supported RDMA mode is RoCE.
  3. consequence of configuring this option, it should not affect other non-RDMA capable network devices be discovered and used with the same resource name, this just enables SR-IOV device plugin to attach additional RDMA device spec (/dev/infiniband etc) when RDMA device is allocated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don' think we shall suggest user to use the same resource name for both RDMA and non-RDMA devices even it is doable, shall we? Since normally the RDMA devices shall be in a separated network.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When RDMA flag is configured to true, it doesn't prevent user from using RDMA enabled VF as a network device. A device can be used in either mode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note added.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
* Initialize the supported SR-IOV NIC models on nodes.
* Provision SR-IOV network device plugin on nodes.
* Provision SR-IOV CNI plugin executable on nodes.
* Manage configuration of SR-IOV device plugin.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SR-IOV device plugin -> SR-IOV network device plugin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread modules/nw-multinetwork-sriov.adoc
@zshi-redhat
Copy link
Copy Markdown
Contributor

This PR is created against openshift-docs:master, shall we do that in 4.2 branch? I didn't see the files in this PR included in modules directory on master branch.

@jboxman
Copy link
Copy Markdown
Contributor

jboxman commented Aug 28, 2019

Hi @zshi-redhat! For 4.2 features, a PR against master is the correct approach. Then we'll cherry-pick the (squashed) commit into the 4.2 branch.

@pliurh thanks for opening this PR!

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 29, 2019
@jboxman
Copy link
Copy Markdown
Contributor

jboxman commented Aug 29, 2019

@vikram-redhat, what's the process for this? It's contributed content for a 4.2 feature. It's related to Multus, but SR-IOV is more complex so the feature includes an Operator. For docs this epic was considered out of scope.

@vikram-redhat
Copy link
Copy Markdown
Contributor

@jboxman if it passes QE and peer review (from you), then it will be acceptable.

Confirm with @aheslin.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
kind: Namespace
metadata:
name: openshift-sriov
name: openshift-sriov-network-operator
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This namespace name depends on openshift/sriov-network-operator#70

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

change namespace back to 'sriov-network-operator' for 4.2. https://bugzilla.redhat.com/show_bug.cgi?id=1746222

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
<11> The PCI addresses of SR-IoV physical function.
<12> The driver type of the virtual functions. Allowed value "netdevice",
"vfio-pci". Defaults to "netdevice".
<13> The RDMA mode. Defaults to false.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When RDMA flag is configured to true, it doesn't prevent user from using RDMA enabled VF as a network device. A device can be used in either mode.

* The SR-IOV CNI plug-in plumbs VF interfaces allocated from the SR-IOV device
plug-in directly into a Pod.

You can use the {product-title} console to install SR-IOV, by deploying,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SR-IOV Operator will be the only interface user needs to interact with, perhaps move above two paragraphs about sriov network device plugin and sriov cni after introducing all the operator features. For example, maybe have detailed introduction of each component that managed by SR-IOV operator after listing the operator features.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

if a NetworkAttachmentDefinition CR of SRIOV CNI plugin is referred in the Pod
annotation.
=====
+
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps adding pod examples for both dpdk and rdma mode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added.

@pliurh pliurh changed the title [WIP]Add doc for the SR-IOV Network Operator Add doc for the SR-IOV Network Operator Sep 9, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2019
@jboxman
Copy link
Copy Markdown
Contributor

jboxman commented Sep 12, 2019

@pliurh

If you enable Allow edits from maintainers. I can edit this PR directly:

https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

(If it's enabled already, I can't tell. GitHub doesn't say.)

@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Sep 13, 2019

@pliurh

If you enable Allow edits from maintainers. I can edit this PR directly:

https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

(If it's enabled already, I can't tell. GitHub doesn't say.)

It's enabled already.

@jboxman
Copy link
Copy Markdown
Contributor

jboxman commented Sep 13, 2019

Perfect. Then I'll push any updates back to the PR for your review.

@jboxman jboxman self-assigned this Sep 13, 2019
@jboxman
Copy link
Copy Markdown
Contributor

jboxman commented Sep 17, 2019

Ended up with #16679 which is not what I intended; I may ultimately create a clean PR with the proposed changes. GitHub makes it inconvenient to push to someone else's PR.

@jboxman jboxman changed the title Add doc for the SR-IOV Network Operator WIP Add doc for the SR-IOV Network Operator Sep 20, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2019
@vikram-redhat
Copy link
Copy Markdown
Contributor

@jboxman can this be closed?

@openshift-ci-robot
Copy link
Copy Markdown

@pliurh: PR needs rebase.

Details

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-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2020
@jboxman
Copy link
Copy Markdown
Contributor

jboxman commented Jan 6, 2020

Merged in #16679.

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

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants