-
Notifications
You must be signed in to change notification settings - Fork 35
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
add ovs forwarder deployment examples #2014
Conversation
- name: nsc | ||
env: | ||
- name: NSM_NETWORK_SERVICES | ||
value: kernel://icmp-responder/nsm-1?sriovToken=worker.domain/100G |
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.
We have only 10G
NICs on our packet integration clusters, so this client will never start on them (if it is not somehow additionally handled in OVS forwarder).
Do you need this for some Smart VF purpose or probably it should be fixed to 10G
?
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.
This is not legacy sriov NIC and is based on Mellanox ConnectX-5 NIC (100G) which has HW offload feature enabled.
The OVS forwarder does special handling for these VFs so that OVS flows are offloaded into hardware.
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.
@pperiyasamy We should be able to CI the example provided... the packet.net env where we have HW to run such examples only has 10G nics... is it possible to get the example to conform to what we can actually test? Feel free to talk in commentary for the example about how to alter it for MLX5 :)
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.
@edwarnicke currently the plan is to configure the MLX5 100G NIC as per this instruction and create pci.config
file and pass it as NSM_SRIOV_CONFIG_FILE
into ovs forwarder. I think once this configuration is automated (like its done for legacy sriov VFs with sdk-sriov pcifunction) by enhancing sdk-sriov, then this can be easily integrated with packet.net, WDYT ?
@@ -0,0 +1,125 @@ | |||
# Test Smart VF connection |
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.
What is the difference with the SriovKernel2Noop
use case?
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.
I mean instead of name :)
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.
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.
So actually the actions for the both cases SriovKernel2Noop
and SmartVF2SmartVF
are just the same:
- Request different VF resources in NSC, NSE deployments.
- Request networkservice with kernel mechanism and SR-IOV token in NSC deployment.
- Set service domain label in NSE deployment.
But if you are requesting 100G
(instead of 10G
) NIC here, there is a difference, so we actually need this new file.
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.
@Bolodya1997 I'm not sure if MLX provides 10G NIC which has hw offload capability, we've used only 100G Connectx-5 NIC for testing. anyway this NIC is totally different from legacy SRIOV NIC and also different drivers to be bound on PFs and VFs. we can still use same sriov.conf file, but this would go into another physicalFunctions
.
LGTM, but we actually are not able to run this on our packet integration cluster, because we lack NIC needed for it. So there possibly can be problems with validating these new examples after some future NSM updates. @edwarnicke @denis-tingaikin |
hostPID: true | ||
hostNetwork: true | ||
containers: | ||
- image: networkservicemeshci/cmd-forwarder-host-ovs:latest |
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.
Typically we bind to particular version of a container (and they get updated from downstream). Could that be done here?
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.
@edwarnicke do you want image name something like this ghcr.io/networkservicemesh/ci/cmd-forwarder-host-ovs:d7ba447
? can we upload docker image into ghcr.io registry just with github credentials ?
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.
@pperiyasamy Look at how we are publishing docker containers in cmd-template. Its appears you already have that, so all you'd need to do here is use your latest githash (currently 811ba56 I believe). After that update deployments should kick in and maintain it for you :)
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.
@edwarnicke ah! ok, that's awesome. Thanks. Let me fix CI for Dockerfile.use-host-ovs in cmd-forwarder-ovs
repo and then will update this PR.
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. The PR is also raised to address this so that update deployments would kick in automatically to change image version.
apps/forwarder-ovs/forwarder.yaml
Outdated
hostPID: true | ||
hostNetwork: true | ||
containers: | ||
- image: networkservicemeshci/cmd-forwarder-ovs:latest |
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.
Typically we bind to particular version of a container (and they get updated from downstream). Could that be done here?
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 as above.
2c899a9
to
6de3724
Compare
6de3724
to
a920e0b
Compare
a920e0b
to
dda2337
Compare
dda2337
to
1961194
Compare
1961194
to
987648c
Compare
@pperiyasamy Did we merge all required PRs? |
@denis-tingaikin we need to merge the following PRs. |
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.
Currently, we cannot merge this due we got a red build on kind related to the last merges.
Will merge this PR when we'll fix the problem.
987648c
to
b19c4b2
Compare
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Co-authored-by: Uzlov Danil <36223296+d-uzlov@users.noreply.github.com> Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
b19c4b2
to
d81cbb5
Compare
…ployments-k8s@main PR link: networkservicemesh/deployments-k8s#2014 Commit: 5590380 Author: Network Service Mesh Bot Date: 2021-09-24 06:41:24 -0500 Message: - Update go.mod and go.sum to latest version from networkservicemesh/de… Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
adds ovs forwarder deployment examples, would help to test cmd-forwarder-ovs for which the code development is on going.
Depends on cmd-forwarder-ovs PR.
Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech