Ns-3 BRITE Integration

The Boston university Representative Internet Topology gEnerator (BRITE) is a topology generation framework built with flexibility and extensibility in mind. By integrating this topology framework with ns-3, users will have the power to quickly and efficiently create large Internet topologies, while taking advantage of ns-3's simulation capabilities.

More documentation for the code is found in the source, as well as src/brite/doc/brite.rst

[Tommaso Pecorella, 2011-06-02 14:30:14]

Nice work.
I have some comments, mostly in the file.

As a general comment I've seen that the user "could" use some informations in
the nodes and edge structures but it's not that easy to do it.

Plus: in the structures the node kind is stored as string. Wouldn't be better to
define an enum ?

Cheers,

Tom

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst
File src/brite/doc/brite.rst (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode29
src/brite/doc/brite.rst:29: according to this configuration file. Please see the
BRITE documenation or the
documentation

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode54
src/brite/doc/brite.rst:54: same file name for the seed file and new seed file
in ns-3
The user might get confused by this statement, as it is not evident that ns-3
needs a "patched" BRITE to run.

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode90
src/brite/doc/brite.rst:90: $ ./waf configure
--with-brite=/your/path/to/brite/source --enable-examples
--enable-examples is not necessary to build ns-3 with BRITE integration (but it
is to run the example). I'd not put it in the documentation.

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
File src/brite/examples/brite-generic-example.cc (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:240: bth.GetBriteTopology
()->OtterOutput (const_cast<char *> (otterOutput.c_str ()));
The above two functions are inherited by the BRITE library, not documented here.
personally I'd avoid mixing the two.

A better solution could be to provide stub functions for the useful functions.

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/wscript
File src/brite/examples/wscript (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/wscript#newcode4
src/brite/examples/wscript:4: obj =
bld.create_ns3_program('brite-generic-example', ['brite', 'internet',
'point-to-point', 'nix-vector-routing', 'applications', 'visualizer'])
I don't think 'visualizer' is necessary.

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
File src/brite/helper/brite-topology-helper.cc (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:83: nodeInfo.type = "RT_BACKBONE ";
spaces before the "
Also: document the meaning of the constants (see the comment in the header file)

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:109: nodeInfo.type = "AS_BACKBONE ";
Same as above

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:171: edgeInfo.type = "E_RT_BACKBONE ";
same as above

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.cc:191: edgeInfo.type = "E_AS_BACKBONE ";
same as above

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
File src/brite/helper/brite-topology-helper.h (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:57: std::string newseedFile);
document the 3rd parameter

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:68: struct BriteNodeInfo
document the various struct variables

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:86: struct BriteEdgeInfo
document the various struct variables

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:102: * struct, and each instance is
stored in a vector.
"stored in a container". The user should not be interested if it's a vector or a
list.

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:132: brite::Topology* GetBriteTopology
(void);
The above function is "dangerous", as it returns the pointer to a private
variable (not used after the initialization, tho).
My question is: is this really necessary? Check the comments in the example as
well.

[Tom Henderson, 2011-06-07 18:24:53]

A few additional comments:
- there is no test suite, so I would not +1 to merge until this is remedied

- BRITE has router-level and AS-level topology generators, which you are turning
to router-level topologies.  I am not sure about just dumping in the AS*.conf
files as examples without mentioning this distinction somewhere in the
documentation (i.e. that ns-3 doesn't support AS-level topology building yet).

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst
File src/brite/doc/brite.rst (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode42
src/brite/doc/brite.rst:42: An example, brite-generic-example, is provided to
show the user how to do this.
It is not really clear from the above description which type of ns-3 links are
created, and that if there are brite attributes such as delay and bandwidth,
then these attributes will be set accordingly on the ns-3 links.

You have more details on this aspect embedded in comments in your example; this
should be highlighted more here in the documentation.

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode46
src/brite/doc/brite.rst:46: in case you wish to randomize subsequent runs.
Finally, it saves the most
Why not just dynamically create a temporary seed_file from an ns-3
RandomVariable before invoking brite?

http://codereview.appspot.com/4517125/diff/1/src/brite/doc/brite.rst#newcode131
src/brite/doc/brite.rst:131: nix:         Enables nix-vector routing. Global
routing is used by default.
toggling this option and running against a set of topologies could be a nice way
to profile the run-time performance of nix-vector vs global-routing

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
File src/brite/examples/brite-generic-example.cc (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:81: /*
suggest a command line option rather than commented out block.  Suggest to
default to on, since you tell people in the manual about the --vis option but
not that by default the placement is random.

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:145: // assigned are going to be all
over the place on a specfic node's
specific

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:147: // which IPs correspond to
which node's netdevices later on.
I think it wouldn't be that hard to do a better job on this aspect, by assigning
addresses to links according to subnet boundaries.  Either perform address
assignment as you go about defining the links, or else extend the helper to do
this magically for the user.  For instance, you could have a method that allowed
you to assign prefixes intelligently to all links, and choose the mask size
(default /30).

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:171: Address remoteAddress
(InetSocketAddress (ipv4Sink, 1024));
these types of code blocks in our examples (lines 168-171) cry out for more
helper functionality (perhaps using the Object name system)

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/brite-generic...
src/brite/examples/brite-generic-example.cc:236: /*
suggest to add as command-line options to enable this output. Agree with Tommaso
that perhaps it may be better to wrap this with documented helper methods.

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/conf_files/AS...
File src/brite/examples/conf_files/ASBarabasi.conf (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/examples/conf_files/AS...
src/brite/examples/conf_files/ASBarabasi.conf:2: 
I would recommend that if you keep these conf files in ns-3 (copied over from
brite library), then you should prepend with a copyright/license statement from
brite.

Another option would be to avoid copying these over and to just ask the user to
open a conf file from the brite library itself.

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
File src/brite/helper/brite-topology-helper.h (right):

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:32: 
\defgroup brite

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:34: * \brief Interface with BRITE, the
Boston university Representative Internet
\ingroup brite

http://codereview.appspot.com/4517125/diff/1/src/brite/helper/brite-topology-...
src/brite/helper/brite-topology-helper.h:132: brite::Topology* GetBriteTopology
(void);
On 2011/06/02 14:30:14, Tommaso Pecorella wrote:
> The above function is "dangerous", as it returns the pointer to a private
> variable (not used after the initialization, tho).
> My question is: is this really necessary? Check the comments in the example as
> well.

If deemed necessary/useful to keep this, it should be clear in the documentation
what the ownership is of the raw pointer being passed.  We have a few instances
of these raw pointers in our public APIs but they should have suitable
documentation around them.