cnp3 / ipmininet

Mininet extension to make experimenting with IP networks easy
GNU General Public License v2.0
65 stars 51 forks source link

Implement automatic logdir creation for daemons #93

Closed butjar closed 4 years ago

butjar commented 4 years ago

This PR adds automatic daemon logdir creation on Node startup. NodeDescriptions are factored out from IPTopo.

This PR also includes some changes for the OpenR daemon:

butjar commented 4 years ago

Most delicate change in 565cc8c313406f3d891644e31e41bba41417b4a3, since IPTopo is touched.

butjar commented 4 years ago

QA: 1. Tests seem to work fine. Looks like the TC tests are a bit brittle:

root@mininet-vm:~/ipmininet# python3 setup.py test
running pytest
running egg_info
writing ipmininet.egg-info/PKG-INFO
writing dependency_links to ipmininet.egg-info/dependency_links.txt
writing requirements to ipmininet.egg-info/requires.txt
writing top-level names to ipmininet.egg-info/top_level.txt
reading manifest file 'ipmininet.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'ipmininet.egg-info/SOURCES.txt'
running build_ext
========================================== test session starts ==========================================
platform linux -- Python 3.6.9, pytest-3.3.2, py-1.5.2, pluggy-0.6.0
rootdir: /root/ipmininet, inifile:
collected 155 items

ipmininet/tests/test_address_alllocation.py .....                                                 [  3%]
ipmininet/tests/test_bgp.py ..............                                                        [ 12%]
ipmininet/tests/test_cli.py .............                                                         [ 20%]
ipmininet/tests/test_dns.py .........                                                             [ 26%]
ipmininet/tests/test_gre.py .                                                                     [ 27%]
ipmininet/tests/test_iptables.py .                                                                [ 27%]
ipmininet/tests/test_link.py .............                                                        [ 36%]
ipmininet/tests/test_misc.py ..........................................                           [ 63%]
ipmininet/tests/test_ospf.py ........                                                             [ 68%]
ipmininet/tests/test_ospf6.py .......                                                             [ 72%]
ipmininet/tests/test_physicalinterface.py ...                                                     [ 74%]
ipmininet/tests/test_radv.py ....                                                                 [ 77%]
ipmininet/tests/test_ripng.py .....                                                               [ 80%]
ipmininet/tests/test_srv6.py .......                                                              [ 85%]
ipmininet/tests/test_sshd.py .                                                                    [ 85%]
ipmininet/tests/test_static.py ......                                                             [ 89%]
ipmininet/tests/test_switch.py .........                                                          [ 95%]
ipmininet/tests/test_tc.py .F                                                                     [ 96%]
ipmininet/tests/test_topologydb.py .....                                                          [100%]

=============================================== FAILURES ================================================
_________________________________ test_tc_example[TCAdvancedNet-49-10] __________________________________

topo = <class 'ipmininet.examples.tc_advanced_network.TCAdvancedNet'>, delay = 49, bw = 10

    @require_root
    @pytest.mark.parametrize("topo,delay,bw", [
        (TCNet, 32, 100),
        (TCAdvancedNet, 49, 10),
    ])
    def test_tc_example(topo, delay, bw):
        """
        :param topo: The topology class
        :param delay: The delay between host h1 and h2 in ms
        :param bw: The bandwidth between h1 and h2 in Mbps
        """
        try:
            net = IPNet(topo=topo())
            net.start()

            # Check connectivity
            assert_connectivity(net, v6=False)
            assert_connectivity(net, v6=True)

            # Check delay
>           assert_delay(net["h1"], net["h2"], delay, v6=False)

ipmininet/tests/test_tc.py:92:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

src = <IPHost h1: h1-eth0:192.168.0.2 pid=18498> , dst = <IPHost h2: h2-eth0:192.168.2.2 pid=18500>
delay_target = 49, tolerance = 1.5, v6 = False

    def assert_delay(src: IPNode, dst: IPNode, delay_target: float, tolerance=1.5,
                     v6=False):
        executable = "ping{v6}".format(v6="6" if v6 else "")
        require_cmd(executable, help_str="{executable} is required to run "
                                         "tests".format(executable=executable))

        cmd = "{executable} -c 10 {dst}".format(executable=executable,
                                                dst=dst.intf().ip6 if v6
                                                else dst.intf().ip)
        out, err, exitcode = src.pexec(cmd)

        assert exitcode == 0, "Cannot ping between {src} and {dst}: " \
                              "{err}".format(src=src, dst=dst, err=err)

        delays = []
        for line in out.split("\n"):
            match = delay_regex.search(line)
            if match is not None:
                delay = float(match.group(1))
                if delay_target - tolerance <= delay <= delay_target + tolerance:
                    delays.append(delay)
>       assert len(delays) >= 5, \
            "Less than half of the pings between {src} and {dst}" \
            " had the desired latency".format(src=src, dst=dst)
E       AssertionError: Less than half of the pings between h1 and h2 had the desired latency
E       assert 0 >= 5
E        +  where 0 = len([])

ipmininet/tests/test_tc.py:42: AssertionError
----------------------------------------- Captured stderr call ------------------------------------------

*** Adding Routers:
r1 r2
*** Creating network
*** Adding hosts:
h1 h2
*** Adding switches:
s1 s2 s3
*** Adding links:
(100.00Mbit) (15ms delay) (h1, s1) (10.00Mbit) (5ms delay) (r1, s2) (1000.00Mbit) (2ms delay) (r2, s3) (15ms delay) (100.00Mbit) (s1, r1) (5ms delay) (10.00Mbit) (s2, r2) (7ms delay) (1000.00Mbit) (s3, h2)
*** Configuring hosts
h1 h2
*** Found 5 broadcast domains
*** Allocating IPv4 addresses
*** Allocating IPv6 addresses
*** Starting controller

*** Starting 3 switches
s1 s2 s3
*** Starting,  2 routers
r1 r2 *** Starting,  2 hosts
h1 h2
*** Setting default host routes
h1 via r1-eth0, h2 via r2-eth1,
*** Removing excess controllers/ofprotocols/ofdatapaths/pings/noxes
killall controller ofprotocol ofdatapath ping nox_corelt-nox_core ovs-openflowd ovs-controllerovs-testcontroller udpbwtest mnexec ivs ryu-manager 2> /dev/null
killall -9 controller ofprotocol ofdatapath ping nox_corelt-nox_core ovs-openflowd ovs-controllerovs-testcontroller udpbwtest mnexec ivs ryu-manager 2> /dev/null
pkill -9 -f "sudo mnexec"
*** Removing junk from /tmp
rm -f /tmp/vconn* /tmp/vlogs* /tmp/*.out /tmp/*.log
*** Removing old X11 tunnels
*** Removing excess kernel datapaths
ps ax | egrep -o 'dp[0-9]+' | sed 's/dp/nl:/'
***  Removing OVS datapaths
ovs-vsctl --timeout=1 list-br
ovs-vsctl --timeout=1 list-br
*** Removing all links of the pattern foo-ethX
ip link show | egrep -o '([-_.[:alnum:]]+-eth[[:digit:]]+)'
( ip link del s1-eth1;ip link del s2-eth1;ip link del s3-eth1;ip link del s1-eth2;ip link del s2-eth2;ip link del s3-eth2 ) 2> /dev/null
ip link show
*** Killing stale mininet node processes
pkill -9 -f mininet:
*** Shutting down stale tunnels
pkill -9 -f Tunnel=Ethernet
pkill -9 -f .ssh/mn
rm -f ~/.ssh/mn/*
*** Cleanup complete.
*** Cleaning up daemons:
pkill -SIGINT -f "^zebra"
pkill -SIGINT -f "^ospfd"
pkill -SIGINT -f "^ospf6d"
pkill -SIGINT -f "^bgpd"
pkill -SIGINT -f "^/usr/sbin/sshd -D -u0"
pkill -SIGINT -f "^radvd"
pkill -SIGINT -f "^pimd"
pkill -SIGINT -f "^ripngd"
pkill -SIGINT -f "^staticd"
pkill -SIGINT -f "^openr"
pkill -SIGINT -f "^named"
"^zebra""^ospfd""^ospf6d""^bgpd""^/usr/sbin/sshd -D -u0""^radvd""^pimd""^ripngd""^staticd""^openr""^named"
================================ 1 failed, 154 passed in 6232.95 seconds ================================
butjar commented 4 years ago

QA: 2. log_dir is created:

root@mininet-vm:~/ipmininet# tree /var/tmp/log/
/var/tmp/log/
├── r1
│   ├── openr.ERROR -> openr.mininet-vm.root.log.ERROR.20200925-140255.20236
│   ├── openr.INFO -> openr.mininet-vm.root.log.INFO.20200925-140253.20236
│   ├── openr.WARNING -> openr.mininet-vm.root.log.WARNING.20200925-140255.20236
│   ├── openr.mininet-vm.root.log.ERROR.20200925-140255.20236
│   ├── openr.mininet-vm.root.log.INFO.20200925-140253.20236
│   └── openr.mininet-vm.root.log.WARNING.20200925-140255.20236
├── r2
│   ├── openr.ERROR -> openr.mininet-vm.root.log.ERROR.20200925-140255.20261
│   ├── openr.INFO -> openr.mininet-vm.root.log.INFO.20200925-140254.20261
│   ├── openr.WARNING -> openr.mininet-vm.root.log.WARNING.20200925-140255.20261
│   ├── openr.mininet-vm.root.log.ERROR.20200925-140255.20261
│   ├── openr.mininet-vm.root.log.INFO.20200925-140254.20261
│   └── openr.mininet-vm.root.log.WARNING.20200925-140255.20261
└── r3
    ├── openr.ERROR -> openr.mininet-vm.root.log.ERROR.20200925-140255.20286
    ├── openr.INFO -> openr.mininet-vm.root.log.INFO.20200925-140254.20286
    ├── openr.WARNING -> openr.mininet-vm.root.log.WARNING.20200925-140255.20286
    ├── openr.mininet-vm.root.log.ERROR.20200925-140255.20286
    ├── openr.mininet-vm.root.log.INFO.20200925-140254.20286
    └── openr.mininet-vm.root.log.WARNING.20200925-140255.20286

3 directories, 18 files
butjar commented 4 years ago

QA: 3. OpenR router have a private /tmp dir:

...
*** Starting CLI:
mininet> r1 ls /tmp
aq_persistent_config_store.bin  hosts_r1                platform-pub-url
force_crash_server              openr_config_store_cmd  resolv_r1.conf
mininet> r2 ls /tmp
aq_persistent_config_store.bin  hosts_r2                platform-pub-url
force_crash_server              openr_config_store_cmd  resolv_r2.conf
mininet> r3 ls /tmp
aq_persistent_config_store.bin  hosts_r3                platform-pub-url
force_crash_server              openr_config_store_cmd  resolv_r3.conf
mininet>
butjar commented 4 years ago

QA: 4. Mininet output if log_dir is created:

...
*** Starting,  3 routers
r1 Openr: Creating log_dir for node r1
Openr: Created missing directories in log_dir path /var/tmp/log/r1
r2 Openr: Creating log_dir for node r2
Openr: Created missing directories in log_dir path /var/tmp/log/r2
r3 Openr: Creating log_dir for node r3
Openr: Created missing directories in log_dir path /var/tmp/log/r3
...
butjar commented 4 years ago

QA: 5. Mininet output if log_dir exist:

...
cn_a1 Openr: Creating log_dir for node cn_a1
Openr: Path for log_dir already exists. Using log_dir /var/tmp/log/cn_a1
cn_b1 Openr: Creating log_dir for node cn_b1
Openr: Path for log_dir already exists. Using log_dir /var/tmp/log/cn_b1
cn_c1 Openr: Creating log_dir for node cn_c1
Openr: Path for log_dir already exists. Using log_dir /var/tmp/log/cn_c1
dn_a Openr: Creating log_dir for node dn_a
Openr: Path for log_dir already exists. Using log_dir /var/tmp/log/dn_a
dn_b Openr: Creating log_dir for node dn_b
Openr: Path for log_dir already exists. Using log_dir /var/tmp/log/dn_b
dn_c Openr: Creating log_dir for node dn_c
Openr: Path for log_dir already exists. Using log_dir /var/tmp/log/dn_c
...
butjar commented 4 years ago

QA: 6. OpenR config updated: --log_dir, --redistribute_ifaces, --ifname_prefix, --iface_regex_include

root@mininet-vm:~/ipmininet# ps aux | grep openr
root     19945  0.0  0.5  80624 23144 pts/0    S+   14:02   0:00 python3 -m ipmininet.examples --topo=simple_openr_network
root     20236  0.8  1.6 1412744 67472 ?       SLsl 14:02   0:04 openr --alloc_prefix_len=128 --assume_drained=False --config_store_filepath=/tmp/aq_persistent_config_store.bin --decision_debounce_max_ms=250 --decision_debounce_min_ms=10 --decision_rep_port=60004 --domain=openr --dryrun=False --enable_subnet_validation=True --enable_fib_sync=False --enable_health_checker=False --enable_legacy_flooding=True --enable_lfa=False --enable_netlink_fib_handler=True --enable_netlink_system_handler=True --enable_perf_measurement=True --enable_prefix_alloc=False --enable_rtt_metric=True --enable_secure_thrift_server=False --enable_segment_routing=False --enable_spark=True --enable_v4=True --enable_watchdog=True --fib_handler_port=60100 --fib_rep_port=60009 --health_checker_ping_interval_s=3 --health_checker_rep_port=60012 --ifname_prefix=r1-eth --iface_regex_exclude= --iface_regex_include=r1-eth.* --ip_tos=192 --key_prefix_filters= --kvstore_flood_msg_per_sec=0 --kvstore_flood_msg_burst_size=0 --kvstore_flood_msg_per_sec=0 --kvstore_ttl_decrement_ms=1 --kvstore_zmq_hwm=65536 --link_flap_initial_backoff_ms=1000 --link_flap_max_backoff_ms=60000 --link_monitor_cmd_port=60006 --loopback_iface=lo --memory_limit_mb=300 --minloglevel=0 --node_name=r1 --override_loopback_addr=False --prefix_manager_cmd_port=60011 --prefixes= --redistribute_ifaces=lo,r1-eth0,r1-eth1,r1-eth2,r1-eth3 --seed_prefix= --set_leaf_node=False --set_loopback_address=False --spark_fastinit_keepalive_time_ms=100 --spark_hold_time_s=30 --spark_keepalive_time_s=3 --static_prefix_alloc=False --tls_acceptable_peers= --tls_ecc_curve_name=prime256v1 --tls_ticket_seed_path= --x509_ca_path= --x509_cert_path= --x509_key_path= --logbufsecs=0 --log_dir=/var/tmp/log/r1 --max_log_size=1 --v=1
root     20261  0.8  1.6 1412748 67528 ?       SLsl 14:02   0:03 openr --alloc_prefix_len=128 --assume_drained=False --config_store_filepath=/tmp/aq_persistent_config_store.bin --decision_debounce_max_ms=250 --decision_debounce_min_ms=10 --decision_rep_port=60004 --domain=openr --dryrun=False --enable_subnet_validation=True --enable_fib_sync=False --enable_health_checker=False --enable_legacy_flooding=True --enable_lfa=False --enable_netlink_fib_handler=True --enable_netlink_system_handler=True --enable_perf_measurement=True --enable_prefix_alloc=False --enable_rtt_metric=True --enable_secure_thrift_server=False --enable_segment_routing=False --enable_spark=True --enable_v4=True --enable_watchdog=True --fib_handler_port=60100 --fib_rep_port=60009 --health_checker_ping_interval_s=3 --health_checker_rep_port=60012 --ifname_prefix=r2-eth --iface_regex_exclude= --iface_regex_include=r2-eth.* --ip_tos=192 --key_prefix_filters= --kvstore_flood_msg_per_sec=0 --kvstore_flood_msg_burst_size=0 --kvstore_flood_msg_per_sec=0 --kvstore_ttl_decrement_ms=1 --kvstore_zmq_hwm=65536 --link_flap_initial_backoff_ms=1000 --link_flap_max_backoff_ms=60000 --link_monitor_cmd_port=60006 --loopback_iface=lo --memory_limit_mb=300 --minloglevel=0 --node_name=r2 --override_loopback_addr=False --prefix_manager_cmd_port=60011 --prefixes= --redistribute_ifaces=lo,r2-eth0,r2-eth1,r2-eth2 --seed_prefix= --set_leaf_node=False --set_loopback_address=False --spark_fastinit_keepalive_time_ms=100 --spark_hold_time_s=30 --spark_keepalive_time_s=3 --static_prefix_alloc=False --tls_acceptable_peers= --tls_ecc_curve_name=prime256v1 --tls_ticket_seed_path= --x509_ca_path= --x509_cert_path= --x509_key_path= --logbufsecs=0 --log_dir=/var/tmp/log/r2 --max_log_size=1 --v=1
root     20286  0.8  1.6 1412728 68020 ?       SLsl 14:02   0:03 openr --alloc_prefix_len=128 --assume_drained=False --config_store_filepath=/tmp/aq_persistent_config_store.bin --decision_debounce_max_ms=250 --decision_debounce_min_ms=10 --decision_rep_port=60004 --domain=openr --dryrun=False --enable_subnet_validation=True --enable_fib_sync=False --enable_health_checker=False --enable_legacy_flooding=True --enable_lfa=False --enable_netlink_fib_handler=True --enable_netlink_system_handler=True --enable_perf_measurement=True --enable_prefix_alloc=False --enable_rtt_metric=True --enable_secure_thrift_server=False --enable_segment_routing=False --enable_spark=True --enable_v4=True --enable_watchdog=True --fib_handler_port=60100 --fib_rep_port=60009 --health_checker_ping_interval_s=3 --health_checker_rep_port=60012 --ifname_prefix=r3-eth --iface_regex_exclude= --iface_regex_include=r3-eth.* --ip_tos=192 --key_prefix_filters= --kvstore_flood_msg_per_sec=0 --kvstore_flood_msg_burst_size=0 --kvstore_flood_msg_per_sec=0 --kvstore_ttl_decrement_ms=1 --kvstore_zmq_hwm=65536 --link_flap_initial_backoff_ms=1000 --link_flap_max_backoff_ms=60000 --link_monitor_cmd_port=60006 --loopback_iface=lo --memory_limit_mb=300 --minloglevel=0 --node_name=r3 --override_loopback_addr=False --prefix_manager_cmd_port=60011 --prefixes= --redistribute_ifaces=lo,r3-eth0,r3-eth1,r3-eth2 --seed_prefix= --set_leaf_node=False --set_loopback_address=False --spark_fastinit_keepalive_time_ms=100 --spark_hold_time_s=30 --spark_keepalive_time_s=3 --static_prefix_alloc=False --tls_acceptable_peers= --tls_ecc_curve_name=prime256v1 --tls_ticket_seed_path= --x509_ca_path= --x509_cert_path= --x509_key_path= --logbufsecs=0 --log_dir=/var/tmp/log/r3 --max_log_size=1 --v=1
root     20439  0.0  0.0  14856  1012 pts/1    S+   14:10   0:00 grep --color=auto openr
butjar commented 4 years ago

Hi @butjar,

Thanks for the continuous stream of updates for OpenR support. Given that this change is mostly contained to OpenR (which you seem to be using/testing), with one exception (which is in the domain of @jadinm), this seems good to merge. I'd however like to get @jadinm's input on the typing-related changes first.

Thanks, I appreciate that you accepted to integrate OpenR into ipmininet although it is pretty niche. Therefore, I see it a bit of my duty to keep it up-to-date.

Wrt. the PR itself, this is indeed a very useful feature for OpenR, and arguably should be extended to all daemons (I do not know why I did not think of doing so right from the start...)--however it sounds both too intrusive and too time-consuming to ask to extend this feature to all daemons as part of this PR. Providing the means to do it seems however reasonable, i.e., extract the node-logdir creation/management into IPNode itself to allow later refactoring to use.

I see your point, it's good to know that the feature seems generally useful. I'll check the code and see how abstract I can make it to expand it to all/more daemons. Maybe you can help me with testing it thereafter, since this is actually the most time consuming part :sweat_smile:.

butjar commented 4 years ago

Testing results after generalizing the logdir creation:

QA: 7. Logdir creation on openr network:

...
*** Starting,  6 routers
cn_a1 cn_a1: Creating logdir /var/tmp/log/cn_a1.
cn_a1: Logdir /var/tmp/log/cn_a1 successfully created.
cn_b1 cn_b1: Creating logdir /var/tmp/log/cn_b1.
cn_b1: Logdir /var/tmp/log/cn_b1 successfully created.
cn_c1 cn_c1: Creating logdir /var/tmp/log/cn_c1.
cn_c1: Logdir /var/tmp/log/cn_c1 successfully created.
dn_a dn_a: Creating logdir /var/tmp/log/dn_a.
dn_a: Logdir /var/tmp/log/dn_a successfully created.
dn_b dn_b: Creating logdir /var/tmp/log/dn_b.
dn_b: Logdir /var/tmp/log/dn_b successfully created.
dn_c dn_c: Creating logdir /var/tmp/log/dn_c.
dn_c: Logdir /var/tmp/log/dn_c successfully created.
*** Starting,  18 hosts
...
butjar commented 4 years ago

QA: 8. Logdir creation on openr network when creation fails (filename for directory exists):

...
*** Starting,  6 routers
cn_a1 cn_a1: Creating logdir /var/tmp/log/cn_a1.
cn_a1: Could not create logdir /var/tmp/log/cn_a1. Stderr:
mkdir: cannot create directory ‘/var/tmp/log/cn_a1’: File exists

cn_b1 cn_b1: Creating logdir /var/tmp/log/cn_b1.
cn_b1: Logdir /var/tmp/log/cn_b1 successfully created.
cn_c1 cn_c1: Creating logdir /var/tmp/log/cn_c1.
cn_c1: Logdir /var/tmp/log/cn_c1 successfully created.
dn_a dn_a: Creating logdir /var/tmp/log/dn_a.
dn_a: Logdir /var/tmp/log/dn_a successfully created.
dn_b dn_b: Creating logdir /var/tmp/log/dn_b.
dn_b: Logdir /var/tmp/log/dn_b successfully created.
dn_c dn_c: Creating logdir /var/tmp/log/dn_c.
dn_c: Logdir /var/tmp/log/dn_c successfully created.
*** Starting,  18 hosts
...
butjar commented 4 years ago

QA: 9. Logdir creation on SimpleBGPNetwork:

python3 -m ipmininet.examples --topo=simple_bgp_network
...
*** Starting,  4 routers
as1r1 as1r1: Creating logdir /tmp.
as1r1: Logdir /tmp successfully created.
as1r1: Creating logdir /tmp.
as1r1: Logdir /tmp successfully created.
as1r1: Creating logdir /tmp.
as1r1: Logdir /tmp successfully created.
as1r1: Creating logdir /tmp.
as1r1: Logdir /tmp successfully created.
as2r1 as2r1: Creating logdir /tmp.
as2r1: Logdir /tmp successfully created.
as2r1: Creating logdir /tmp.
as2r1: Logdir /tmp successfully created.
as2r1: Creating logdir /tmp.
as2r1: Logdir /tmp successfully created.
as2r1: Creating logdir /tmp.
as2r1: Logdir /tmp successfully created.
as2r2 as2r2: Creating logdir /tmp.
as2r2: Logdir /tmp successfully created.
as2r2: Creating logdir /tmp.
as2r2: Logdir /tmp successfully created.
as2r2: Creating logdir /tmp.
as2r2: Logdir /tmp successfully created.
as2r2: Creating logdir /tmp.
as2r2: Logdir /tmp successfully created.
as3r1 as3r1: Creating logdir /tmp.
as3r1: Logdir /tmp successfully created.
as3r1: Creating logdir /tmp.
as3r1: Logdir /tmp successfully created.
as3r1: Creating logdir /tmp.
as3r1: Logdir /tmp successfully created.
as3r1: Creating logdir /tmp.
as3r1: Logdir /tmp successfully created.
*** Starting,  4 hosts
...
butjar commented 4 years ago

Concerning the test, sometimes this test fails when it is tested on a virtualbox VM with more than one CPU. Is it the case for your setup ?

Exactly, I run some heavy iperf processes in my setup :joy:

If yes, that is an expected problem: the emulation of delays is a mess in this setup

No worries, the tests provide a lot of confidence for new implementations. Moreover, for a e2e test setup I find it pretty stable. Unit tests would be pretty awesome though (me saying not providing any test at all).

When you are ok with your PR, I can launch the tests on the jenkins server to check ;)

Give me a final thought on the addRouter cls param and we are good to go.

butjar commented 4 years ago

@jadinm When I was diggin in addRouter I started some refactoring in the IPSwitch.

Please have a look at:

butjar commented 4 years ago

Yes, the last three commits are out-of-scope for the PR

Delayed :heavy_check_mark:

I am not so convinced by the need to dupplicate the cls argument in the addRouter method I would instead use more examples to showcase its use (but this is also out-of-scope for this PR) You can create an issue to remind us of that

Removed :heavy_check_mark:

For the rest of your PR, it would be nice to add a test on the daemon log locations (see ipmininet/tests) and document that on the documentation (see docs/ and https://ipmininet.readthedocs.io/en/latest/)

Test is WIP. Can you point me to an appropriate section in the docs? I couldn't find any section about Node/Router configuration or something similar, where the logdir creation would fit seamlessly.

butjar commented 4 years ago

OpenR test suite added :heavy_check_mark: :

root@mininet-vm:/home/vagrant/ipmininet# sudo python3 -m pytest ipmininet/tests/test_openr.py
========================================= test session starts ==========================================
platform linux -- Python 3.6.9, pytest-3.3.2, py-1.5.2, pluggy-0.6.0
rootdir: /home/vagrant/ipmininet, inifile:
collected 3 items

ipmininet/tests/test_openr.py ...                                                                [100%]
butjar commented 4 years ago

Added documentation for OpenR and OpenrRouter class usage :heavy_check_mark:

butjar commented 4 years ago

@jadinm I think the PR might be ready to ship, can you check again please

jadinm commented 4 years ago

Thank you for your work :+1: (and sorry for the delay ^^')

I have just installed OpenR on the jenkins server, let's try it

jadinm commented 4 years ago

Jenkins, run tests.

butjar commented 4 years ago

Jenkins, run tests.

:robot: :hear_no_evil:

jadinm commented 4 years ago

Jenkins, run tests.

robot hear_no_evil

Ah ah x) That was a problem of configuration that is fixed now

The tests failed because you are using a construct that is only available from Python 3.6: f"{variable}" But, I don't have any problem upgrading the minimum version of ipmininet since Python 3.5 reached its end-of-life last month

butjar commented 4 years ago

Jenkins, run tests.

robot hear_no_evil

Ah ah x) That was a problem of configuration that is fixed now

The tests failed because you are using a construct that is only available from Python 3.6: f"{variable}" But, I don't have any problem upgrading the minimum version of ipmininet since Python 3.5 reached its end-of-life last month

Let's keep the 3.5 compatibility for now, and I will change the lines.

jadinm commented 4 years ago

Thanks

Could you fix the few style warnings of pep8: https://jenkins-mininet.info.ucl.ac.be/job/ipmininet-pr/pythonversion=ipmininet-py37,testname=doctest/118/pep8/New/ ? No need to tackle both "too long lines" though

Pylint has a few remarks as well: https://jenkins-mininet.info.ucl.ac.be/job/ipmininet-pr/118/pylint/New/ Though, both "Possible unbalanced tuple unpacking" are ok of course, as well as the "node_description.py" file

butjar commented 4 years ago

Jenkins, run tests.

butjar commented 4 years ago

@jadinm Sorry for the delay, I fixed almost all remarks. Can you check on jenkins and run the pipeline again?

https://jenkins-mininet.info.ucl.ac.be/job/ipmininet-pr/124/

...
20:56:37 + sudo /var/lib/jenkins/miniconda3/envs//bin/python -m ipmininet.clean
20:56:37 sudo: /var/lib/jenkins/miniconda3/envs//bin/python: command not found
20:56:37 [PostBuildScript] - [INFO] Build does not have any of the results [SUCCESS]. Did not execute build step #1.
jadinm commented 4 years ago

@butjar Oh, I forgot to tell you that all the arguments in __new__ methods were needed despite what pylint thinks

Thanks ! :+1:

Jenkins, run tests.

butjar commented 4 years ago

Jenkins, run tests.

jadinm commented 4 years ago

Cool :)

If you remove the unused import to pytest in test_openr.py, I think that's ready for merging

butjar commented 4 years ago

Cool :)

If you remove the unused import to pytest in test_openr.py, I think that's ready for merging

Sure, was not sure if it can be removed. I think I copied from other tests (that probably use the import).

butjar commented 4 years ago

Once again. Jenkins, run tests.

butjar commented 4 years ago

Btw. I didn't introduce all the Mypi remarks, right? Seems like there is quite some work ahead to get the static code analysis green, but will definitely be worth it. I stumbled over black these days. Might be probably an option to fix a lot of issues in one shot. This month I will be pretty busy, but maybe in a few week I can try to see if it can help to improve code quality.

jadinm commented 4 years ago

Btw. I didn't introduce all the Mypi remarks, right? Seems like there is quite some work ahead to get the static code analysis green, but will definitely be worth it. I stumbled over black these days. Might be probably an option to fix a lot of issues in one shot. This month I will be pretty busy, but maybe in a few week I can try to see if it can help to improve code quality.

Ah indeed ^^ I'll still merge this PR and leave the correction of mypy warnings for a later PR ;)

butjar commented 4 years ago

Btw. I didn't introduce all the Mypi remarks, right? Seems like there is quite some work ahead to get the static code analysis green, but will definitely be worth it. I stumbled over black these days. Might be probably an option to fix a lot of issues in one shot. This month I will be pretty busy, but maybe in a few week I can try to see if it can help to improve code quality.

Ah indeed ^^ I'll still merge this PR and leave the correction of mypy warnings for a later PR ;)

I agree, awesome 🤘. Thx for support.