-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update docs for OVS Hardware Offload #755
Update docs for OVS Hardware Offload #755
Conversation
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Pull Request Test Coverage Report for Build 10383349369Details
💛 - Coveralls |
doc/ovs-hw-offload.md
Outdated
``` | ||
|
||
### Create NetworkAttachmentDefinition CRD with OVS CNI config | ||
_Note: `spec.bridge.ovs: {}` will create OVS bridges with default configuration that is suitable for HW-offloading with OVS-kernel dataplane. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add that it will create a bridge for every device (PF) selected by the policy ?
i know its mentioned below as an observation of the node state but i think we need to put this info upfront.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense. I mentioned this explicitly in the section where I describe the policy.
9602148
to
026bcec
Compare
@adrianchiris Thx for the review. I addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx @ykulazhenkov !
``` | ||
|
||
### Create NetworkAttachmentDefinition CRD with OVS CNI config | ||
_Note: `spec.bridge.ovs: {}` - means use default settings (suitable for HW-offloading with OVS-kernel dataplane) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardware offload is enabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for k8s, the k8s plugin will enable HW-offloading for OVS. For Openshift it is required to create SriovNetworkPoolConfig
if vars.ClusterType == constants.ClusterTypeOpenshift { |
026bcec
to
4c4ae14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work 2 small comments and we should wait for #750 to be merged
doc/ovs-hw-offload.md
Outdated
@@ -9,63 +9,148 @@ host net-device. The VF representor plays the same role as TAP devices | |||
in Para-Virtual (PV) setup. A packet sent through the VF representor on the host | |||
arrives to the VF, and a packet sent through the VF is received by its representor. | |||
|
|||
OVS Hardware Offloading requires NIC to be configured in `switchdev` mode. | |||
|
|||
The operator can automate the creation and configuration of OVS bridges when the "manageSoftwareBridges" function is activated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function -> featureGate ?
doc/ovs-hw-offload.md
Outdated
``` | ||
|
||
### Create NetworkAttachmentDefinition CRD with OVS CNI config | ||
_Note: `spec.bridge.ovs: {}` - means use default settings (suitable for HW-offloading with OVS-kernel dataplane) | ||
The fields defined in the [Bridge type](https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/4563178259d2f6f61451d167d3e4affdb8f2fc3a/api/v1/sriovnetworknodepolicy_types.go#L84) can be used to configure advanced bridge and interface level options._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want the link to point to master if there are changes in the API in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Yury Kulazhenkov <[email protected]>
4c4ae14
to
3d6c50d
Compare
Thx for the review @SchSeba. I addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
let wait for #750 to be merged first before we merge this one
#750 was merged. merging this one as well. |
Update docs for OVS Hardware Offload to add examples for "softwareBridgeManagement" feature.
fixes #749