-
Notifications
You must be signed in to change notification settings - Fork 9
munet: expose network intf to tc #87
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
munet: expose network intf to tc #87
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 59.19% 60.16% +0.96%
==========================================
Files 19 19
Lines 5833 5879 +46
==========================================
+ Hits 3453 3537 +84
+ Misses 2380 2342 -38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
NAK: Need a solution that allows each connection of the network to be modified independently. An open suggestion is to configure values on the Node-side instead of Network-side. |
|
There are two paths forward that I can see regarding enabling the per-connection config of loss, latency, etc. for networks:
The updated networks schema would be as follows: Wherein the network-level config is the default for all interfaces managed by the network, and connection-level config is more specific. Note that the connection-level config was actually swapped out with network-level config within this PR (LoC 3198 in
For example, loss would be configured in a topology as follows: This technically reduces the set of offered functionalities (since munet would no longer support connections with unequal tx/rx metrics.) However, this might be more intuitive to a user who is not familiar with how the metrics are being applied under the hood (e.g. via Thoughts: Personally, I prefer option 1 since it maintains unequal tx/rx functionality that might have a valid use case in certain realistic topologies. A page in the docs can always be added to explain how it is applied under the hood. Do you have any insight/opinions @choppsv1 ? |
4d56297 to
4d8e520
Compare
|
I've pushed a new smaller code change. The code checks if the returned switch.connections config is empty and if so adds tc values from the root of switch config as the network side connection config. The add_native_link changes weren't needed as long as you don't include the switch "name" key from the root config in the interface config dict you pass in. Unfortunately I didn't force push the rebase first so the diff when you click the compare link in github is not very useful :( |
4d8e520 to
21e7978
Compare
tc network parameters can be configured per-network, with attached interfaces inheriting the configuration if they don't have node specific configuration under `connections`. Signed-off-by: Liam Brady <lbrady@labn.net> Signed-off-by: Christian Hopps <chopps@labn.net>
21e7978 to
bfee4d5
Compare
choppsv1
left a comment
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
tc network parameters are configured once per-network, with attached interfaces inheriting the configuration. This contrasts node config, wherein the same tc network parameters are exposed on a per-connection (i.e. per-interface) basis.