Adding experimental per-edge bandwidth limiting to Shadow by vaibhav-mattoo · Pull Request #3695 · shadow/shadow
There is a lot here and I haven't reviewed in detail yet, but sharing some initial thoughts.
I'm happy to see that the new parts are added as experimental options, are optional, and are off by default. And it is great that the tests and the documentation components were added.
Most of the logic is extended from existing files in multi-level nested code that might be difficult to follow and maintain long term. I wonder if there is some module-based design that would be more intuitive and allow us to break up the new functionality into much smaller units of code that would be easier to test using unit tests.
More generally, we have resisted adding too many Internet-level components to Shadow in order to avoid it snowballing into an Internet/routing simulator, which is not something that we would be able to do well and maintain. This patch gets us a little closer to that, though just a little. I think we would need to tease apart if this feature is generally useful or is more like a research prototype that maybe isn't worth full integration quite yet.
I wonder if this patch would maybe not be needed at all if we implemented a design something like the one discussed in #3701:
Sender Interface -> TokenBucket1 -> Router(CoDel) -> TokenBucket2 ->
(== Unlimited Core Link ==)
-> TokenBucket3 -> Router(CoDel) -> TokenBucket4 -> Receiver Interface
If you wanted to test out link-level congestion effects, you could configure token buckets 2 and 3 to have lower rate limits than 1 and 4. Can you comment on this? It would be great if we could handle two related issues with the same design, as it would simplify code design and maintenance on our side.