Some initial review comments; main suggestion is to add deeper tests. https://codereview.appspot.com/38960045/diff/10001/examples/routing/ripng-simple-network.cc File examples/routing/ripng-simple-network.cc (right): ...
11 years, 4 months ago
(2014-01-02 07:25:24 UTC)
#2
On 2014/01/02 07:25:24, Tom Henderson wrote: > Some initial review comments; main suggestion is to ...
11 years, 4 months ago
(2014-01-02 08:33:32 UTC)
#3
On 2014/01/02 07:25:24, Tom Henderson wrote:
> Some initial review comments; main suggestion is to add deeper tests.
>
> [...]
>
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/helper/ripng-...
> File src/internet/helper/ripng-helper.h (right):
>
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/helper/ripng-...
> src/internet/helper/ripng-helper.h:98: void SetDefaultRouter (Ptr<Node> node,
> uint32_t interface);
> I may have forgotten the details on the debate on SetDefaultRouter; why again
is
> this a method on the RipNgHelper rather than something at the IPv6 level?
Because in the IPv6 level helper the "SetDeafault" means to install the default
route in the nodes *other*
than the router, while in this case it means to install "::"/0 in the router's
RT.
I.e., the first case is for Ipv[4,6]StaticRouting, while the second is for
dynamic routing.
We can think to an unified helper function tho.
In order to avoid confusion, we could clarify the function by using a different
name: "InstallDefaultRoute".
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/test/ipv6-rip...
> src/internet/test/ipv6-ripng-test.cc:137: Ptr<SimpleNetDevice> fwDev1routerA,
> fwDev2routerA;
> This test makes me wonder whether some additional internet helpers are needed
to
> make writing of test topologies easier than this.
As a matter of fact, SimpleNetDevice does *not* have an helper. On the other
hand, since the device is not really intended for general use, we can live
without the helper.
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/test/ipv6-rip...
> src/internet/test/ipv6-ripng-test.cc:237: AddTestCase (new Ipv6RipngTest,
> TestCase::QUICK);
> more test cases suggested:
>
> 1) a test to demonstrate that the synchronization effects in Fall/Floyd paper:
> http://www.icir.org/floyd/papers/sync_94.pdf
> do not occur
Interesting, but quite hard. Maybe it could be an example rather than a test,
since its results have to be validated "visually".
> 2) split horizon and poison reverse validation (that operation is correct)
This involves a network topology change in the middle of the simulation. Could
be done (with some effort).
> 3) tests that explicitly test for convergence by specific times on sample
> topologies, such as the point-to-point grid for increasing grid sizes, or
other
> canonical topologies. As an aside, is there a good API for testing the
routing
> protocol for convergence (such as callbacks for route updates or routing table
> changes, and a way to fetch the existing table and compare it with known good
> table)?
That one falls also in the area of "nice, but it's an example"... because it's
kinda hard to check the condition.
We could write an API to gather and compare the RTs (we don't have one), but it
should be compared with a manually
written RT.
About RT changes, it's again possible, but we will discover that the condition
is true only after a long time.
> 4) perhaps a test to confirm that exceeding the hop limit of 15 causes the
> expected route failure
Can be done.
Cheers,
T.
On 2014/01/03 08:12:32, Tommaso Pecorella wrote: This changeset includes two "orthogonal" changes. 1) the changes ...
11 years, 4 months ago
(2014-01-03 08:25:25 UTC)
#5
On 2014/01/03 08:12:32, Tommaso Pecorella wrote:
This changeset includes two "orthogonal" changes.
1) the changes suggested by Tom (plus some code simplifications), and,
2) changes in ipv6/icmpv6 to actually send ICMPv6 errors when there's no route
to the destination.
Moreover, Ping6 has been modified to receive and print the no route (and
timeout) errors. The RIPng example now uses this modification to show the
routing topology evolution.
Interesting enough, I discovered that RFC 1058 (i.e., RIPv1), and RIPng RFC as a
consequence, are wrong. The topology in RFC 1058 Section 2.2, which the RIPng
example is using, does *not* show the effects of Split Horizon strategies.
I triple checked the RFC and protocol specifications, and I'm 100% sure about
this.
The only way to reproduce the "counting to infinity" effect is to *not* send the
destination unreachable RTE (i.e., update the route with metric 16 when a link
fails). But this is against the protocol specification.
Funny, isn't it ?
Moar tests in the future.
On 2014/01/02 08:33:32, Tommaso Pecorella wrote: > On 2014/01/02 07:25:24, Tom Henderson wrote: > > ...
11 years, 4 months ago
(2014-01-03 20:15:34 UTC)
#6
On 2014/01/02 08:33:32, Tommaso Pecorella wrote:
> On 2014/01/02 07:25:24, Tom Henderson wrote:
> > Some initial review comments; main suggestion is to add deeper tests.
> >
> > [...]
> >
> >
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/helper/ripng-...
> > File src/internet/helper/ripng-helper.h (right):
> >
> >
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/helper/ripng-...
> > src/internet/helper/ripng-helper.h:98: void SetDefaultRouter (Ptr<Node>
node,
> > uint32_t interface);
> > I may have forgotten the details on the debate on SetDefaultRouter; why
again
> is
> > this a method on the RipNgHelper rather than something at the IPv6 level?
>
> Because in the IPv6 level helper the "SetDeafault" means to install the
default
> route in the nodes *other*
> than the router, while in this case it means to install "::"/0 in the router's
> RT.
>
> I.e., the first case is for Ipv[4,6]StaticRouting, while the second is for
> dynamic routing.
> We can think to an unified helper function tho.
>
> In order to avoid confusion, we could clarify the function by using a
different
> name: "InstallDefaultRoute".
I'm not seeing the need to put default route API in a dynamic routing protocol
(when it exists in a static routing protocol already). What happens if you
remove this?
>
> >
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/test/ipv6-rip...
> > src/internet/test/ipv6-ripng-test.cc:137: Ptr<SimpleNetDevice>
fwDev1routerA,
> > fwDev2routerA;
> > This test makes me wonder whether some additional internet helpers are
needed
> to
> > make writing of test topologies easier than this.
>
> As a matter of fact, SimpleNetDevice does *not* have an helper. On the other
> hand, since the device is not really intended for general use, we can live
> without the helper.
This is somewhat related to our recent discussion of how we have to go through a
lot of configuration to generate traffic if we want to avoid dependencies on
application (and hence internet) module.
There probably needs to be a way to easily generate stock topologies for testing
things like routing protocols. We could either try to create things like
helpers for SimpleNetDevice topologies, or write the test to reuse something
like PointToPointGridHelper (which may require locating the test in src/test or
refactoring how our topology helpers are organized).
I'm not suggesting to expand the scope of your ripng patch to deal with it, but
just observing that in general you shouldn't have to jump through such hoops to
write tests.
>
> >
>
https://codereview.appspot.com/38960045/diff/10001/src/internet/test/ipv6-rip...
> > src/internet/test/ipv6-ripng-test.cc:237: AddTestCase (new Ipv6RipngTest,
> > TestCase::QUICK);
> > more test cases suggested:
> >
> > 1) a test to demonstrate that the synchronization effects in Fall/Floyd
paper:
> > http://www.icir.org/floyd/papers/sync_94.pdf
> > do not occur
>
> Interesting, but quite hard. Maybe it could be an example rather than a test,
> since its results have to be validated "visually".
Not necessarily, if there is a way to sample (via trace sources) the timing of
protocol events, and then look at how their distribution changes over time.
>
> > 2) split horizon and poison reverse validation (that operation is correct)
>
> This involves a network topology change in the middle of the simulation. Could
> be done (with some effort).
It seems to me that it is a major design feature of the protocol, thus deserving
of some testing.
>
> > 3) tests that explicitly test for convergence by specific times on sample
> > topologies, such as the point-to-point grid for increasing grid sizes, or
> other
> > canonical topologies. As an aside, is there a good API for testing the
> routing
> > protocol for convergence (such as callbacks for route updates or routing
table
> > changes, and a way to fetch the existing table and compare it with known
good
> > table)?
>
> That one falls also in the area of "nice, but it's an example"... because it's
> kinda hard to check the condition.
> We could write an API to gather and compare the RTs (we don't have one), but
it
> should be compared with a manually
> written RT.
Still might be nice to expose this outside of the "PrintRoutingTable" technique.
> About RT changes, it's again possible, but we will discover that the condition
> is true only after a long time.
>
> > 4) perhaps a test to confirm that exceeding the hop limit of 15 causes the
> > expected route failure
>
> Can be done.
>
> Cheers,
>
> T.
On 2014/01/03 20:16:01, Tom Henderson wrote: Hi, I'll reply to the comments one by one ...
11 years, 4 months ago
(2014-01-04 06:54:10 UTC)
#8
On 2014/01/03 20:16:01, Tom Henderson wrote:
Hi,
I'll reply to the comments one by one later. I'd just want to say "hold on on
the revision - I fund a major problem with Valgrind".
First things first, tho.
Default route:
1) Yes, it's necessary. The protocol specifically states about exporting a
default route.
2) No, we could live without it.
In a "real" world, you'll have an AS with RIPng as IGP, and a BGP running on
border routers.
The border routers will export a default route ("::/0") via RIPng to the IGP.
Each router will have a "local" method for its assigned subnets (either static
routing or RADVd) to declare itself as the default router for the LAN.
How the RIPng (IGP-level) default route is installed in the routers (i.e., if
you manually setup it in the border routers or it's setup in some other way)
it's outside of the RIPng specifications.
Overall, I think it's quite important to have it for a number of scenario.
Now the error and why I'm saying to hold on: Valgrind is extremely unhappy about
RIPng. After much swearing, I think I found the issues.
I'm using BindToNetDevice on an UDP socket on IPv6.
Guess what ?
void
UdpSocketImpl::BindToNetDevice (Ptr<NetDevice> netdevice)
{
NS_LOG_FUNCTION (netdevice);
Socket::BindToNetDevice (netdevice); // Includes sanity check
if (m_endPoint == 0)
{
if (Bind () == -1)
{
NS_ASSERT (m_endPoint == 0);
return;
}
NS_ASSERT (m_endPoint != 0);
}
m_endPoint->BindToNetDevice (netdevice);
return;
}
It's not supporting the v6 endpoint *and* it's opening a spurious v4 endpoint.
I *think* my Valgrind errors are from this, more testing is necessary.
If I'm right (and even if I'm wrong), the solution is:
1) find a workaround to fix this without touching UdpSocketImpl (maybe)
2) fix UdpSocketImpl, and TcpSocketBase.
I'll try to see if the second option is "easy".
Hence... dammit.
Cheers,
T.
On 2014/01/04 22:01:23, Tommaso Pecorella wrote: Hi again, after a LOT of invocations to the ...
11 years, 3 months ago
(2014-01-04 22:22:26 UTC)
#10
On 2014/01/04 22:01:23, Tommaso Pecorella wrote:
Hi again,
after a LOT of invocations to the Great Bug Eraser, I finally managed to find
why Valgding wasn't happy. But I can't remember which one of the changes I did
is the main one.
You'll fina a *lot* of changes, mainly related to:
1) Packet lost in transit (Hop Limit == 0), they must send back an ICMP error
2) ICMP are packets like any other. They deserve an ICMP error if dropped
3) *#@&%$!€ <- censored. GetIfIndex of a socket bound to a NetDevice returns the
*NetDevice* index, which is different from the IP-level interface index. We
should document this, as Loopback is always IP-level interface 0, but it's not
the node-level interface 0, if another NetDevice is installed before IP.
Point 3 caused a great pain, btw. Finally I manage to find the way, and it's all
but evident:
Add to the receiving socket the Ipv6PacketInfoTag. Then do this:
Ipv6PacketInfoTag interfaceInfo;
if (!packet->RemovePacketTag (interfaceInfo))
{
NS_ABORT_MSG ("No incoming interface on RIPng message, aborting.");
}
uint32_t incomingIf = interfaceInfo.GetRecvIf ();
Ptr<Node> node = this->GetObject<Node> ();
Ptr<NetDevice> dev = node->GetDevice (incomingIf);
uint32_t ipInterfaceIndex = m_ipv6->GetInterfaceForDevice (dev);
Easy, isn't it ? No. It's painful. However it's foolproof. You can add the
NetDevices in the order you want.
Anyway, enough whining.
Tests. I added a test about counting to infinity (don't remember if it was in
the previous post).
Other tests: the new code shows the effect of NoSplitHorizon: There's a count to
infinity, and it's evident from the example (if you activate the ping logs). The
other strategies (PoisonReverse and SplitHorizon) are virtually
indistinguishable.
If one looks at the pcaps, however, the difference is evident. PoisonReverse has
more overhead.
I still have to figure out a clean way to test 'em.
Last but not least, about probing the RTs. I agree that a function could be
useful. I'd avoid to work on it right now, if possible.
For RIPng, one can always resort to the management interface.
2.4.1 Request Messages
A Request is used to ask for a response containing all or part of a
router's routing table. Normally, Requests are sent as multicasts,
from the RIPng port, by routers which have just come up and are
seeking to fill in their routing tables as quickly as possible.
However, there may be situations (e.g., router monitoring) where the
routing table of only a single router is needed. In this case, the
Request should be sent directly to that router from a UDP port other
than the RIPng port. If such a Request is received, the router
responds directly to the requestor's address and port with a globally
valid source address since the requestor may not reside on the
directly attached network.
Of course the implementation (should) do this. Should because bugs are always
behind the door.
Cheers,
T.
On 2014/01/25 15:43:51, Tommaso Pecorella wrote: This is the last thing requested (I think): a ...
11 years, 3 months ago
(2014-01-25 15:49:22 UTC)
#12
On 2014/01/25 15:43:51, Tommaso Pecorella wrote:
This is the last thing requested (I think): a test for Split Horizoning
strategy. No need to say, it's working as intended.
Don't mind the "removed" lines in ipv6-l3-protocol.cc, it's an artifact from
another patch I have in the queue.
T.
Issue 38960045: RIPng (as is, RIP for IPv6)
(Closed)
Created 11 years, 4 months ago by Tommaso Pecorella
Modified 11 years ago
Reviewers: Tom Henderson
Base URL:
Comments: 22
