Bugzilla – Bug 600
lower the default routing priority of Ipv4GlobalRouting; move to helper
Last modified: 2009-06-26 22:23:47 EDT
In: void GlobalRouteManagerImpl::SelectRouterNodes () ... // XXX make the below priority value an attribute ipv4ListRouting->AddRoutingProtocol (globalRouting, 3); A user pointed out that the above default magic number buried deep in this code makes it difficult to run global routing and then selectively override with static routes, since global routes will be consulted at higher priority than static routes. At the very least, the default priority should probably be a negative integer, but there probably should be more done to this set of functions. GlobalRouteManager API predates the existence of helper APIs, and many of these functions should really be at the helper API level.
Created attachment 479 [details] start of routing helper API This is really just the start of the new API. I stumbled upon a couple of issues worth being mentionned: 1) olsr needs to be told when the ip stack is fully setup with its ip interfaces to iterate them and do things. i.e., we need to call olsr::RoutingProtocol::Start at some point. A temporary hack here is to call Simulator::ScheduleNow which I have done. 2) Ipv4L3Protocol::DoDispose must not invoke DoDispose on m_routingProtocol 3) I have not done global routing yet
Created attachment 480 [details] handle global routing I have tested this patch to ensure --regression still passes.
As the said user, I'd like to add that current way of handling routing information is not resembling how real-world routers do it. In practice, the routing table is supposed to be populated like so: Longest match Administrative costs of routing protocol ("Priority") Metrics within a routing protocol (Cisco's reference: http://www.cisco.com/en/US/tech/tk365/technologies_tech_note09186a0080094823.shtml - especially ctrl+f for "prefix lengths" chapter) Anyway, problem with ns-3 right now seems to be that priority is all that matters. So, if I have 0.0.0.0/0 (default) via static routing, and it has higher priority than global, global routes are NEVER considered. Or more concrete example, if I have 10.1.2.128/25, nexthop 1.2.3.4 configured via static routing and 10.1.2.0/24, nexthop 5.6.7.8 learned via global, depending on priorities packets probably don't up where they were intended. E.g. if I have topology 10.1.2.0/24 Router-----LAN ----Router | / | Router | / | / Router | | Host And I want to have destinations for other half of the LAN use different path than for the other half, yet wish to use global routing, ns-3 doesn't cut it. Anyway: Proposing adjustment to ipv4-list-routing-impl.cc, RouteOutput, where instead of immediately returning route, it would check all routing protocols and then compare prefix lengths and only then priorities. Unfortunately, it seems that even inside routing protocols the first match is returned, so if, in the above example, I use completely static routing and first configure route 10.1.2.0/24 via other path and 10.1.2.0/25 via other, the first one is matched. At least I think so, since on line 83 of ipv4-static-routing-impl the new route is simply push_back():ed to the end of the list... What should be changed here is that either (again) search through the entire table first and return route longest matching prefix, or, when configuring new routes, first check if there's is a route to same destination with shorter prefix and insert() it just before. Ok, do note that I'm still very unfamiliar with ns-3's code and my C++ is rusty, so I might have gotten some things completely wrong... However, if I'm right this will definitely be a problem at latest when stuff like injecting summary routes occur. I would very much like to have access to something resembling a real routing table for each node, instead of one that sort of gets constructed (by querying each protocol in list) every time you forward a packet.
Hi, I agree with your comments that most systems don't do it this way. Personally, my goal is to obtain a different implementation of Ipv4RoutingProtocol that is like linux policy routing, and let quagga run on top of it. For this release cycle, I was mainly concerned about an API and structure that could permit the existing implementation (which leans towards on-demand routing support) and in the future, do more work more on the implementations (new and existing). I believe that what you are seeking could be accomplished in the current framework by a different variant of Ipv4ListRouting that set itself as the callback for each protocol, then applied policy to select among the routes found. I agree that within the static routing module itself, the routes could be better organized.
(In reply to comment #2) > Created an attachment (id=480) [details] > handle global routing > > I have tested this patch to ensure --regression still passes. > I do not see any global routing bits-- did you attach the wrong patch?
Created attachment 482 [details] the actual patch
(In reply to comment #6) > Created an attachment (id=482) [details] > the actual patch > A few comments; in general, I agree with this approach. - does not change the policy of adding globalRouting at priority 3; I suggest it goes to -3. Should these magic numbers be exposed to users and settable? - need to consider future IPv6 addition when we pick names for these objects-- will GlobalRouting just populate Ipv4 and Ipv6 by default? We have an object Ipv4GlobalRoutingHelper that is fronting for an underlying GlobalRouter. - needs more documentation - does not seem to support particularly well a deployment of global routing that only works on a subset of nodes (i.e., the InternetStack is needed on all nodes but global routing only on some nodes)-- this part of the API seems to have been lost - suggest some kind of deprecated log on the use of the old API for one release cycle (since it is likely to be a common script breakage)
(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=482) [details] [details] > > the actual patch > > > > A few comments; in general, I agree with this approach. > > - does not change the policy of adding globalRouting at priority 3; I suggest > it goes to -3. Should these magic numbers be exposed to users and settable? No: this is just a conveniant default. If you want something else, you should just provide/configure your own RoutingHelper. > - need to consider future IPv6 addition when we pick names for these objects-- > will GlobalRouting just populate Ipv4 and Ipv6 by default? We have an object > Ipv4GlobalRoutingHelper that is fronting for an underlying GlobalRouter. I am not sure I understand what you are asking. > - needs more documentation > - does not seem to support particularly well a deployment of global routing > that only works on a subset of nodes (i.e., the InternetStack is needed on all > nodes but global routing only on some nodes)-- this part of the API seems to > have been lost I am not sure I understand this comment. Users are free to use their own routing helpers when they want to: InternetStackHelper stack; stack.SetRoutingHelper (mything); stack.Install (mysubset); stack.SetRoutingHelper (myotherthing); stack.Install (myothersubset); > - suggest some kind of deprecated log on the use of the old API for one release > cycle (since it is likely to be a common script breakage) Given the amount of API breakage of this release, I would just go for more without the compatibility layer, but this is your call.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Created an attachment (id=482) [details] [details] [details] > > > the actual patch > > > > > > > A few comments; in general, I agree with this approach. > > > > - does not change the policy of adding globalRouting at priority 3; I suggest > > it goes to -3. Should these magic numbers be exposed to users and settable? > > No: this is just a conveniant default. If you want something else, you should > just provide/configure your own RoutingHelper. I still think it probably makes sense to set to a negative number by default; I don't care too strongly about exposing this constant so how about we just set it at -3? > > > - need to consider future IPv6 addition when we pick names for these objects-- > > will GlobalRouting just populate Ipv4 and Ipv6 by default? We have an object > > Ipv4GlobalRoutingHelper that is fronting for an underlying GlobalRouter. > > I am not sure I understand what you are asking. > I was thinking about how IPv6 will be supported. Does GlobalRouteManager extend to cover Ipv6 or is there a separate implementation? > > - needs more documentation > > - does not seem to support particularly well a deployment of global routing > > that only works on a subset of nodes (i.e., the InternetStack is needed on all > > nodes but global routing only on some nodes)-- this part of the API seems to > > have been lost > > I am not sure I understand this comment. Users are free to use their own > routing helpers when they want to: > > InternetStackHelper stack; > stack.SetRoutingHelper (mything); > stack.Install (mysubset); > stack.SetRoutingHelper (myotherthing); > stack.Install (myothersubset); OK, I will think about this-- maybe we need an example of a partial coverage and see how this API works out. > > > - suggest some kind of deprecated log on the use of the old API for one release > > cycle (since it is likely to be a common script breakage) > > Given the amount of API breakage of this release, I would just go for more > without the compatibility layer, but this is your call. > Yes, but you will notice that the previous API breakage was confined mostly to the helper and internet-stack layers-- the examples were hardly touched. This touches any example that uses global routing.
(In reply to comment #9) > > No: this is just a conveniant default. If you want something else, you should > > just provide/configure your own RoutingHelper. > > I still think it probably makes sense to set to a negative number by default; I > don't care too strongly about exposing this constant so how about we just set > it at -3? If you think that the default value should be changed, I am sll for it. > > > - need to consider future IPv6 addition when we pick names for these objects-- > > > will GlobalRouting just populate Ipv4 and Ipv6 by default? We have an object > > > Ipv4GlobalRoutingHelper that is fronting for an underlying GlobalRouter. > > > > I am not sure I understand what you are asking. > > > > I was thinking about how IPv6 will be supported. Does GlobalRouteManager > extend to cover Ipv6 or is there a separate implementation? I don't know about this: I have to confess that I don't understand much about the global router. > > I am not sure I understand this comment. Users are free to use their own > > routing helpers when they want to: > > > > InternetStackHelper stack; > > stack.SetRoutingHelper (mything); > > stack.Install (mysubset); > > stack.SetRoutingHelper (myotherthing); > > stack.Install (myothersubset); > > OK, I will think about this-- maybe we need an example of a partial coverage > and see how this API works out. I think that there was one such example in simple-point-to-point-example.cc > > > - suggest some kind of deprecated log on the use of the old API for one release > > > cycle (since it is likely to be a common script breakage) > > > > Given the amount of API breakage of this release, I would just go for more > > without the compatibility layer, but this is your call. > > > > Yes, but you will notice that the previous API breakage was confined mostly to > the helper and internet-stack layers-- the examples were hardly touched. This > touches any example that uses global routing. The main reason is that global routing was never converted to the helper API...
Created attachment 486 [details] additional patch work I didn't really improve on this much, but removed some dead code, and changed the policy (which will cause 1 regression trace to change). I am not sure that NS_DEPRECATED works. I also found that some examples need to be fixed (e.g. mixed-wireless.cc, mixed-wireless.py). This patch applies on top of the previous patch by Mathieu.
(In reply to comment #11) > Created an attachment (id=486) [details] > additional patch work > > I didn't really improve on this much, but removed some dead code, and changed > the policy (which will cause 1 regression trace to change). I am not sure that > NS_DEPRECATED works. I also found that some examples need to be fixed (e.g. > mixed-wireless.cc, mixed-wireless.py). > > This patch applies on top of the previous patch by Mathieu. > This patch is changing the regression test output. for the dynamic global routing. I guess this is because you changed the priority of the global router by default. Are you ok wtih this ?
Created attachment 491 [details] a complete patch with InternetStackHelper documentation
Created attachment 494 [details] updated patch with start of doxygen
changeset: a84f60b6cd12
(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=486) [details] [details] > > additional patch work > > > > I didn't really improve on this much, but removed some dead code, and changed > > the policy (which will cause 1 regression trace to change). I am not sure that > > NS_DEPRECATED works. I also found that some examples need to be fixed (e.g. > > mixed-wireless.cc, mixed-wireless.py). > > > > This patch applies on top of the previous patch by Mathieu. > > > > This patch is changing the regression test output. for the dynamic global > routing. I guess this is because you changed the priority of the global router > by default. Are you ok wtih this ? I will double check this output today. Yes, I am OK with it.
> > This patch is changing the regression test output. for the dynamic global > > routing. I guess this is because you changed the priority of the global router > > by default. Are you ok wtih this ? > > I will double check this output today. Yes, I am OK with it. > The new output is correct-- I changed the example program's comments to reflect this change.