CSD CodeReview for Group 3 : v5.0 see details in README file

see what has been changed in readme file, this commitment is for version 5.0.

BUG=0

[lihongzhouchn, 2016-09-30 22:47:51]

Please review my code, see details and what has been changed in README file. 

Kind regards,
LI Hongzhou

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode33
master/TB-startup.py:33: onSignal_forPing=0;
'onSignal_forPing'  and 'onSignal_forRouting' are two locks to avoid collision
between ping-test and changing latency


when ping-test is running,  'onSignal_forRouting' resets
latency cannot be changed during 'onSignal_forRouting' is reset.

when changing-latency is running,  'onSignal_forPing' resets
Ping-test cannot be run during 'onSignal_forPing' is reset.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode92
master/TB-startup.py:92: switch= [self.addSwitch('s%d'%i,cls=OVSBridge,stp=1)for
i in range(1,int(numSwitches)+1) ]
use STP to avoid packets loop in topo

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode100
master/TB-startup.py:100: def imi_routing():
this function is to implement the changing effect

it can randomly change the latency of each link every certain seconds.

[lihongzhouchn, 2016-09-30 22:48:39]

Please review my code, see details and what has been changed in README file. 

Kind regards,
LI Hongzhou

[lihongzhouchn, 2016-09-30 22:49:14]

Please review my code, see details and what has been changed in README file. 

Kind regards,
LI Hongzhou

[lihongzhouchn, 2016-09-30 22:52:49]

Please review my code, see details and what has been changed in README file. 

Kind regards,
LI Hongzhou

[lihongzhouchn, 2016-09-30 22:53:22]

Please review my code, see details and what has been changed in README file.

Kind regards,
LI Hongzhou

[lihongzhouchn, 2016-09-30 22:54:05]

Please review my code, see details and what has been changed in README file.

Kind regards,
LI Hongzhou

[Atiq, 2016-10-03 22:33:55]

I have put my review comments. Please have a look on this.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode91
master/TB-startup.py:91: host  = [self.addHost('h%d'%i)for i in
range(1,int(numHosts)+1 )]
I am just wandering what might happens when numHosts becomes zero ??

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode104
master/TB-startup.py:104: while True:
How are we coming out of this while loop ?

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode107
master/TB-startup.py:107: for link in net.links:
if no links present in the net ?

[theodoor, 2016-10-04 14:48:30]

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode30
master/TB-startup.py:30: level = [25,25,25,30,30,30,30,35,35,35,40,40];
Use better and longer defining description names for constants. Also constants
can be named differently according to style.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode33
master/TB-startup.py:33: onSignal_forPing=0;
This can all be included in the comments in the file itself. 

On 2016/09/30 22:47:51, lihongzhouchn wrote:
> 'onSignal_forPing'  and 'onSignal_forRouting' are two locks to avoid collision
> between ping-test and changing latency
> 
> 
> when ping-test is running,  'onSignal_forRouting' resets
> latency cannot be changed during 'onSignal_forRouting' is reset.
> 
> when changing-latency is running,  'onSignal_forPing' resets
> Ping-test cannot be run during 'onSignal_forPing' is reset.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode45
master/TB-startup.py:45: f=open('topo-discription','r');
I have the feeling that using plain text for describing the topology is not the
most convenient option. I don't see much extra added value compared to use
python functions and if necessary define new functions that accept for example
default python data structures for lists or arrays. We can still use multiple
files to define the structures but let the python interpreter handle the reading
of the files. 

There is also an JSON module in python
(https://docs.python.org/3.4/library/json.html#module-json) that returns python
dictionaries and lists if we want to use that.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode116
master/TB-startup.py:116: log_expactation=open('log_expaction.TestData','a');
I would suggest that we set all variables like file names as variables at the
beginning of the file and use csv format for easy plotting with excel.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode131
master/TB-startup.py:131: log_actual.write('h1-----h2 :
\n\n'+net.get('h1').cmd('ping -c1 %s'%net.get('h2').IP())+'\n@%s\n\n' %
time.strftime( ISOTIMEFORMAT, time.localtime() )         );
We can use csv-format (;) for easy plotting (e.g. with excel) or use another
format that is easy readable by another plotting program. Maybe there is some
kind of format that allows real-time plotting. For example xgraph, gnuplot or
kst can work for us? gnuplot is most used as far as I know.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode167
master/TB-startup.py:167: time.sleep(30);
All! used constants should be defined.

[kirill.sc, 2016-10-04 17:24:40]

See my comments, please add everyone to the list of reviewers.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (left):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#oldcode63
master/TB-startup.py:63: Links_s2s_2.append(    i[i.find(',')+1:]    );
These alignments are often unnecessary, but if you go for it, make sure they are
in fact aligned

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode33
master/TB-startup.py:33: onSignal_forPing=0;
Why do you have this dependency between these 2 threads in the first place? 
When you going to run cassandra (a substitute for ping) you wont be able to
stop/pause it to change latencies.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode34
master/TB-startup.py:34: onSignal_forRouting=1;
Dont mix CamelCase and underscrols 2 different styles

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode45
master/TB-startup.py:45: f=open('topo-discription','r');
Use with open notation

with open(fname) as f:
    content = f.readlines()

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode50
master/TB-startup.py:50: if line=="Hosts\n":
use rstrip() to remove line endings before doing comparison

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode112
master/TB-startup.py:112: log_expactation.write("between %s  and  %s  : 
delay=%d   jitter=%d    @ %s\n"%(str(link.intf1.node.name) ,
str(link.intf2.node.name) , level_choice , jitter_choice ,  time.strftime(
ISOTIMEFORMAT, time.localtime() )    ) );
Watch the width of your lines, I recommend to limit it between 80 and 120
characters, so that people can open multiple columns of code

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode140
master/TB-startup.py:140: #			print net.get('h1').cmd('ping -c1
%s'%net.get('h2').IP());
remove commented code.
You can easily check the number of hosts dynamically from net object

[lihongzhouchn, 2016-10-04 17:52:11]

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (left):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#oldcode63
master/TB-startup.py:63: Links_s2s_2.append(    i[i.find(',')+1:]    );
On 2016/10/04 17:24:40, kirill.sc wrote:
> These alignments are often unnecessary, but if you go for it, make sure they
are
> in fact aligned

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode30
master/TB-startup.py:30: level = [25,25,25,30,30,30,30,35,35,35,40,40];
On 2016/10/04 14:48:30, theodoor wrote:
> Use better and longer defining description names for constants. Also constants
> can be named differently according to style. 

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode33
master/TB-startup.py:33: onSignal_forPing=0;
On 2016/10/04 14:48:30, theodoor wrote:
> This can all be included in the comments in the file itself. 
> 
> On 2016/09/30 22:47:51, lihongzhouchn wrote:
> > 'onSignal_forPing'  and 'onSignal_forRouting' are two locks to avoid
collision
> > between ping-test and changing latency
> > 
> > 
> > when ping-test is running,  'onSignal_forRouting' resets
> > latency cannot be changed during 'onSignal_forRouting' is reset.
> > 
> > when changing-latency is running,  'onSignal_forPing' resets
> > Ping-test cannot be run during 'onSignal_forPing' is reset.
> 

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode33
master/TB-startup.py:33: onSignal_forPing=0;
On 2016/10/04 17:24:40, kirill.sc wrote:
> Why do you have this dependency between these 2 threads in the first place? 
> When you going to run cassandra (a substitute for ping) you wont be able to
> stop/pause it to change latencies. 


This is because I met a error when ping-test and changing of latency both
happen. 
I looked through dynamic mininet and found out that when ping-test(more
specificlly only 'ping' command) is going on, we cannot change any properties of
link. So I just added these to avoid this situation.
But when it turns to cassandra, everything is working fine. I have finished a
demo of configuring cassandra and ycsb, both of them are working fine.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode34
master/TB-startup.py:34: onSignal_forRouting=1;
On 2016/10/04 17:24:40, kirill.sc wrote:
> Dont mix CamelCase and underscrols 2 different styles

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode45
master/TB-startup.py:45: f=open('topo-discription','r');
On 2016/10/04 14:48:30, theodoor wrote:
> I have the feeling that using plain text for describing the topology is not
the
> most convenient option. I don't see much extra added value compared to use
> python functions and if necessary define new functions that accept for example
> default python data structures for lists or arrays. We can still use multiple
> files to define the structures but let the python interpreter handle the
reading
> of the files. 
> 
> There is also an JSON module in python
> (https://docs.python.org/3.4/library/json.html#module-json) that returns
python
> dictionaries and lists if we want to use that. 

I'm now trying to do that.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode104
master/TB-startup.py:104: while True:
On 2016/10/03 22:33:55, Atiq wrote:
> How are we coming out of this while loop ?

This funchtion is called in a daemon thread. When father process ends, this
thread will be automatically killed.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode107
master/TB-startup.py:107: for link in net.links:
On 2016/10/03 22:33:55, Atiq wrote:
> if no links present in the net ?

yeah, a good question as your first one. I haven't consider about exceptional
handling and boundary condition. I think I will deal with that in next few
versions.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode112
master/TB-startup.py:112: log_expactation.write("between %s  and  %s  : 
delay=%d   jitter=%d    @ %s\n"%(str(link.intf1.node.name) ,
str(link.intf2.node.name) , level_choice , jitter_choice ,  time.strftime(
ISOTIMEFORMAT, time.localtime() )    ) );
On 2016/10/04 17:24:40, kirill.sc wrote:
> Watch the width of your lines, I recommend to limit it between 80 and 120
> characters, so that people can open multiple columns of code

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode116
master/TB-startup.py:116: log_expactation=open('log_expaction.TestData','a');
On 2016/10/04 14:48:30, theodoor wrote:
> I would suggest that we set all variables like file names as variables at the
> beginning of the file and use csv format for easy plotting with excel. 

Maybe we can discuss about this tomorrow

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode131
master/TB-startup.py:131: log_actual.write('h1-----h2 :
\n\n'+net.get('h1').cmd('ping -c1 %s'%net.get('h2').IP())+'\n@%s\n\n' %
time.strftime( ISOTIMEFORMAT, time.localtime() )         );
On 2016/10/04 14:48:30, theodoor wrote:
> We can use csv-format (;) for easy plotting (e.g. with excel) or use another
> format that is easy readable by another plotting program. Maybe there is some
> kind of format that allows real-time plotting. For example xgraph, gnuplot or
> kst can work for us? gnuplot is most used as far as I know. 

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode140
master/TB-startup.py:140: #			print net.get('h1').cmd('ping -c1
%s'%net.get('h2').IP());
On 2016/10/04 17:24:40, kirill.sc wrote:
> remove commented code.
> You can easily check the number of hosts dynamically from net object

Acknowledged.

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode167
master/TB-startup.py:167: time.sleep(30);
On 2016/10/04 14:48:30, theodoor wrote:
> All! used constants should be defined. 

Acknowledged.

[armen, 2016-10-06 10:59:15]

New comment!

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode164
master/TB-startup.py:164: link.intf1.reconfig(delay=30, jitter=jitter_setting,
distribution="pareto") # normal, pareto, paretonormal
The delay between the links could be lower, like 15-20.

[armen, 2016-10-06 11:12:57]

New comments

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode103
master/TB-startup.py:103: #	sys.stdout = open("imiRouting","w",buffering=0);
What does this function do?

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode148
master/TB-startup.py:148: time.sleep(0.5);
During the runtime of the code, the sleep time of 0.5ms is not implemented.

[lihongzhouchn, 2016-10-06 16:47:10]

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py
File master/TB-startup.py (right):

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode103
master/TB-startup.py:103: #	sys.stdout = open("imiRouting","w",buffering=0);
On 2016/10/06 11:12:56, armen wrote:
> What does this function do?

for nothing........I have commented this line

https://codereview.appspot.com/307380043/diff/1/master/TB-startup.py#newcode148
master/TB-startup.py:148: time.sleep(0.5);
On 2016/10/06 11:12:56, armen wrote:
> During the runtime of the code, the sleep time of 0.5ms is not implemented.

I don't understand what do you mean