search

containernetworking / plugins

Some reference and example networking plugins, maintained by the CNI team.
Apache License 2.0
2.23k stars 788 forks source link

Dev/exclude subnets from traffic shaping2 #921

Closed oOraph closed 7 months ago

oOraph commented 1 year ago

bandwidth: possibility to exclude some subnets from traffic shapping

what changed:

we had to refactor the bandwidth plugin and switch from a classless qdisc (tbf) to a classful qdisc (htb).

subnets are to be provided in config or runtimeconfig just like other parameters

unit and integration tests were also adapted in consequence

unrelated changes:

test fixes: the most important tests were just silently skipped due to ginkgo Measure deprecation (the ones actually checking the effectiveness of the traffic control)

squeed commented 1 year ago

@oOraph thanks for this! And very good catch w.r.t. Ginkgo Measure.

Can you check the lint error?

oOraph commented 1 year ago

Can you check the lint error?

@squeed Done :)

oOraph commented 1 year ago

@s1061123 , about https://github.com/containernetworking/plugins/pull/921#pullrequestreview-1543806248 Done as you wished :). now you can either set unshappedSubnets or shappedSubnets to specify subnets to be excluded from/included in traffic shapping. Both parameters are mutually exclusive and will result in error when simultaneously defined

s1061123 commented 1 year ago

@oOraph Thank you to incorporate my comments! I will review code tomorrow again!

oOraph commented 1 year ago

Hi, almost code is reasonable to me. BTW, could you provide document for that, in https://github.com/containernetworking/cni.dev ?

done -> https://github.com/containernetworking/cni.dev/pull/130

oOraph commented 1 year ago

Hello @s1061123 @MikeZappa87 :). Do you still see anything wrong or missing with this pr ?

oOraph commented 1 year ago

/lgtm

Could you please address doc changes in another PR in doc repo, https://github.com/containernetworking/cni.dev?

Sure it's already done here: https://github.com/containernetworking/cni.dev/pull/130

oOraph commented 1 year ago

just rebased on upstream/main to fix lint issue

MikeZappa87 commented 11 months ago

This looks good. I did not run the plugin though.

nayihz commented 7 months ago

Hi all, I am curious why we need to create ifb devices and tc rules in host-side network namespace, rather than in container-side.

oOraph commented 7 months ago

Hi all, I am curious why we need to create ifb devices and tc rules in host-side network namespace, rather than in container-side.

Hello @nayihz :). If I understand correctly what you mean we need to apply tc rules on host side because the container side is the user side and we do not want them to arbitrarily edit/cancel them.

oOraph commented 7 months ago

@s1061123 @MikeZappa87 thanks for merging. Maybe we should also merge the doc wdyt ? https://github.com/containernetworking/cni.dev/pull/130