Skip to content

SR-IOV Network Operator configuration#16679

Merged
jboxman merged 1 commit intoopenshift:masterfrom
jboxman-rh:pliurh-sriov-network-operator
Nov 8, 2019
Merged

SR-IOV Network Operator configuration#16679
jboxman merged 1 commit intoopenshift:masterfrom
jboxman-rh:pliurh-sriov-network-operator

Conversation

@jboxman
Copy link
Copy Markdown
Contributor

@jboxman jboxman commented Sep 17, 2019

So this ended up becoming the working PR for this.

This PR adds content for using the SR-IOV Network Operator.

@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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 17, 2019
@openshift-docs-preview-bot
Copy link
Copy Markdown

The preview will be available shortly at:

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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.

s/Following/following/

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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.

Do we have an estimated time on how long the installation would take? perhaps give a number here so that user has expectation on how long they shall wait.

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.

It mainly depends on how long the operator images can be downloaded from Internet, which it hard to predict in customer's environment.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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.

Shall we have a explanation for interfaces field, followed by this example?
for example, all the properties are for physical functions.
We have vendorID and deviceID mentioned in several places in the doc, but they may refer to PF or VF IDs depending on whether they are in a status field of SriovNetworkNodeState CR or in the spec field of SriovNetworkNodePolicy CR.

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.

The field under interfaces is quite straightforward. User shall be able to know what it is through the name.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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 explicitly mention VF in SR-IoV device, for example, use SR-IoV VF 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.

The device ID should be PF's not VF's.
s/device hex/SR-IOV PF device hex/

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.

Updated.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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.

Is there a simple command to query which SriovNetwork are in use by a particular pod?

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.

I don't think there is any.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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.

Is there any pre-requisite to meet before installing SR-IOV Operator?
For example, we only support SR-IOV deployment on baremetal environment, might want to call out this as a pre-requisite in a separate section. Other I can think of is when using SR-IOV in userspace mode (DPDK), user might want to configure hugepages which is not supported by SR-IOV Operator itself, worth a link to MCO doc for how to configure kernel args etc.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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.

Since we are giving an pod example here, might be good to also include the example of net-attach-def CR(sriov-rdma-mlnx) used here. This applies to dpdk example below as well.

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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 cpu in resources requests and limits because dpdk application always need to bind cpu.

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.

resources:
  limits:
    memory: "1Gi"
    cpu: "2"
  requests:
    memory: "1Gi"
    cpu: "2"

Copy link
Copy Markdown
Contributor Author

@jboxman jboxman Sep 20, 2019

Choose a reason for hiding this comment

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

I updated to this:

    resources:
      limits:
        memory: "1Gi"
        cpu: "2"
        hugepages-1Gi: "4Gi"
      requests:
        memory: "1Gi"
        cpu: "2"
        hugepages-1Gi: "4Gi"

Is that okay? Or should hugepages be removed?

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.

I updated to this:

    resources:
      limits:
        memory: "1Gi"
        cpu: "2"
        hugepages-1Gi: "4Gi"
      requests:
        memory: "1Gi"
        cpu: "2"
        hugepages-1Gi: "4Gi"

Is that okay? Or should hugepages be removed?

I think it's ok. DPDK application requires hugepages.

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.

Please add a note with link to kernel argument configurations (1G hugepage needs to be configured via kernel arguments, this config is supported via MCO). for example: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/nodes-nodes-kernel-arguments.adoc

Please also add a note for application using 2Mi hugepages (this doesn't require changing of kernel argument and can be configured via tuned Operator), link to the doc: https://github.com/openshift/openshift-docs/blob/enterprise-4.2/modules/configuring-huge-pages.adoc

Copy link
Copy Markdown
Contributor Author

@jboxman jboxman Oct 9, 2019

Choose a reason for hiding this comment

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

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Copy link
Copy Markdown

@zhaozhanqi zhaozhanqi Sep 23, 2019

Choose a reason for hiding this comment

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

I did not see isRdma option in the SriovNetworkNodePolicy CR in my env.

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.

isRdma is empty if not set : https://github.com/openshift/sriov-network-operator/blob/master/pkg/apis/sriovnetwork/v1/sriovnetworknodepolicy_types.go#L36
If you take a look at the device plugin configMap generated by operator, it shall be configured as False if not set.

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.

@zhaozhanqi is this still an issue in your test?

Copy link
Copy Markdown

@zhaozhanqi zhaozhanqi Sep 29, 2019

Choose a reason for hiding this comment

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

@zshi-redhat yes, as you said, it's the false by default.
but I think we should add this one to template, here is the default template for now:

apiVersion: sriovnetwork.openshift.io/v1
kind: SriovNetworkNodePolicy
metadata:
  name: policy-1
  namespace: sriov-network-operator
spec:
  resourceName: intelnics
  nodeSelector:
    feature.node.kubernetes.io/sriov-capable: 'true'
  priority: 99
  mtu: 9000
  numVfs: 6
  nicSelector:
    vendor: '8086'
    rootDevices:
      - '0000:01:00.1'
    pfNames:
      - eth1
  deviceType: vfio-pci

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
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 should be VF device IDs according to my testing, otherwise device plugin will expose PF as resource.

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.

And we may also want to list a mapping of VF IDs to supported NICs so that user knows what to configure in node policy custom resource.

Copy link
Copy Markdown
Contributor

@pliurh pliurh Sep 27, 2019

Choose a reason for hiding this comment

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

This should be VF device IDs according to my testing, otherwise device plugin will expose PF as resource.

It's a bug, the deviceID shall always be set with the PF's ID. The operator shall convert it to the VF device ID when generating the device plugin configuration, if the numVFs is set. cc @zhaozhanqi

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.

@zhaozhanqi please help to confirm if that's the case in your testing.

Copy link
Copy Markdown

@zhaozhanqi zhaozhanqi Sep 29, 2019

Choose a reason for hiding this comment

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

this should be PF deviceID since we do not know the vf device id yet before the nodenetworkpolicy is created.

      - Vfs:
        - deviceID: 154c
          driver: iavf
          mtu: 1500
          pciAddress: '0000:3b:02.0'
          vendor: '8086'
        - deviceID: 154c
          driver: iavf
          mtu: 1500
          pciAddress: '0000:3b:02.1'
          vendor: '8086'
        - deviceID: 154c
          driver: iavf
          mtu: 1500
          pciAddress: '0000:3b:02.2'
          vendor: '8086'
        - deviceID: 154c
          driver: iavf
          mtu: 1500
          pciAddress: '0000:3b:02.3'
          vendor: '8086'
        - deviceID: 154c
          driver: iavf
          mtu: 1500
          pciAddress: '0000:3b:02.4'
          vendor: '8086'
        - deviceID: 154c
          driver: iavf
          mtu: 1500
          pciAddress: '0000:3b:02.5'
          vendor: '8086'
      driver: i40e
      vendor: '8086'
      name: ens1f0
      mtu: 1500
      deviceID: 158b
      numVfs: 6
      pciAddress: '0000:3b:00.0'
      totalvfs: 64

Copy link
Copy Markdown

@zhaozhanqi zhaozhanqi Sep 29, 2019

Choose a reason for hiding this comment

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

The deviceID do not be set in the dpconfig if not set 'deviceID' in the policy. and the numVFs is also not be set
oc get configmap -n sriov-network-operator device-plugin-config -o yaml apiVersion: v1 data: config.json: '{"resourceList":[{"resourceName":"intelnetdevice","IsRdma":false,"selectors":{"vendors":["8086"],"drivers":["iavf","mlx5_core","i40evf","ixgbevf"],"pfNames":["ens1f0"]}}]}' kind: ConfigMap

@zshi-redhat
Copy link
Copy Markdown
Contributor

zshi-redhat commented Sep 28, 2019

@jboxman this PR may need another commit to fix opening comments. can you help on it?

Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
Comment thread modules/nw-multinetwork-sriov.adoc Outdated
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Oct 9, 2019

@zshi-redhat, I'm going to work through the comments here; I may open a new PR with the updated content after for a clean review.

@jboxman jboxman force-pushed the pliurh-sriov-network-operator branch from 46112d9 to 9e2c742 Compare October 9, 2019 22:25
@jboxman jboxman self-assigned this Oct 9, 2019
@jboxman jboxman force-pushed the pliurh-sriov-network-operator branch from 9e2c742 to b790a73 Compare October 9, 2019 22:43
@zhaozhanqi
Copy link
Copy Markdown

hi, @jboxman
I saw the 4.2 docs for sriov is still using old way to setup sriov. https://docs.openshift.com/container-platform/4.2/networking/multiple-networks/configuring-sr-iov.html
Why this PR did not been merged?

@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Oct 18, 2019

@zhaozhanqi, it doesn't conform to our contributing guide, so I've been rewriting it; the work for that will be complete in a few days. Thanks for your patience.

@jboxman jboxman changed the title WIP Pliurh sriov network operator WIP SR-IOV Network Operator configuration Oct 20, 2019
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Oct 22, 2019

@pliurh

What is a resourceName in this context?

apiVersion: sriovnetwork.openshift.io/v1
kind: SriovNetworkNodePolicy
spec:
  resourceName: <sriov_resource_name>

@jboxman jboxman force-pushed the pliurh-sriov-network-operator branch from b790a73 to 4a6c27e Compare October 25, 2019 17:36
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Oct 25, 2019

@pliurh @zshi-redhat @zhaozhanqi

I've edited and reorganized the content. Can you take a look? Thanks!

@jboxman jboxman changed the title WIP SR-IOV Network Operator configuration SR-IOV Network Operator configuration Oct 25, 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 Oct 25, 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.

Thanks @jboxman for the update!
some minor comments inline.

Comment thread modules/nw-sriov-configuring-device.adoc Outdated
Comment thread modules/nw-sriov-configuring-device.adoc Outdated
Comment thread modules/nw-sriov-network-attachment.adoc Outdated
Comment thread networking/multiple-networks/configuring-sr-iov.adoc Outdated
@pliurh
Copy link
Copy Markdown
Contributor

pliurh commented Oct 29, 2019

@pliurh

What is a resourceName in this context?

apiVersion: sriovnetwork.openshift.io/v1
kind: SriovNetworkNodePolicy
spec:
  resourceName: <sriov_resource_name>

The resourceName is a logic name for a group of sriov VFs. It can be referenced by the manifests of pod and net-att-def, if user want to allocate the VF to a pod.

@jboxman jboxman force-pushed the pliurh-sriov-network-operator branch from 4a6c27e to 9119712 Compare October 29, 2019 20:19
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Oct 29, 2019

@zshi-redhat thanks! I've made those changes.

@zhaozhanqi, can you confirm the content for this PR is okay? Thanks!

Comment thread modules/nw-sriov-network-attachment.adoc Outdated
Comment thread modules/nw-sriov-configuring-device.adoc Outdated
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, "For Intel NIC, this value shall not be larger than the TotalVFs. For Mellanox NIC, this value shall not be larger than 128."

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.

@pliurh, what is TotalVFs in this context? Is it a configuration parameter in the CR? Or some fixed value for Intel?

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.

TotalVFs is the maximum number VF can be configured. For Intel NICs, it’s a static value. For Mellanox NICs it is variable with max value 128 for the NICs we claim to support, so it can be changed by the sriov operator when user specify the numVFs.

Comment thread modules/nw-sriov-configuring-device.adoc Outdated
@jboxman jboxman force-pushed the pliurh-sriov-network-operator branch from 9119712 to ce83395 Compare November 7, 2019 20:48
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Nov 7, 2019

@zshi-redhat @pliurh @zhaozhanqi

I've made the requested changes; PTAL. Thanks!

@pliurh
Copy link
Copy Markdown
Contributor

pliurh commented Nov 8, 2019

LGTM

@zshi-redhat
Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Nov 8, 2019

I'm going to merge this, at long last! We can iterate on it more if necessary under cover of a new PR.

cc @aburdenthehand @lamek

@jboxman jboxman merged commit 3d05385 into openshift:master Nov 8, 2019
@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Nov 8, 2019

/cherry-pick enterprise-4.3

@openshift-cherrypick-robot
Copy link
Copy Markdown

@jboxman: #16679 failed to apply on top of branch "enterprise-4.3":

.git/rebase-apply/patch:455: trailing whitespace.
<6> Specify a value for the maximum transmission unit (MTU) of the virtual function. The value for MTU must be in the range from `1` to `9000`. If you do not need to specify the MTU, specify a value of `''`. 
.git/rebase-apply/patch:715: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	modules/nw-multinetwork-sriov.adoc
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): modules/nw-multinetwork-sriov.adoc deleted in Add content for SR-IOV Network Operator and modified in HEAD. Version HEAD of modules/nw-multinetwork-sriov.adoc left in tree.
Patch failed at 0001 Add content for SR-IOV Network Operator

Details

In response to this:

/cherry-pick enterprise-4.3

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.

@jboxman
Copy link
Copy Markdown
Contributor Author

jboxman commented Nov 8, 2019

/cherry-pick enterprise-4.2

@openshift-cherrypick-robot
Copy link
Copy Markdown

@jboxman: #16679 failed to apply on top of branch "enterprise-4.2":

.git/rebase-apply/patch:455: trailing whitespace.
<6> Specify a value for the maximum transmission unit (MTU) of the virtual function. The value for MTU must be in the range from `1` to `9000`. If you do not need to specify the MTU, specify a value of `''`. 
.git/rebase-apply/patch:715: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	modules/nw-multinetwork-sriov.adoc
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): modules/nw-multinetwork-sriov.adoc deleted in Add content for SR-IOV Network Operator and modified in HEAD. Version HEAD of modules/nw-multinetwork-sriov.adoc left in tree.
Patch failed at 0001 Add content for SR-IOV Network Operator

Details

In response to this:

/cherry-pick enterprise-4.2

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.2 branch/enterprise-4.3 lgtm Indicates that a PR is ready to be merged. 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.

7 participants