search

opensourcerouting / frr

Free Range Routing Protocol Suite
Other
37 stars 14 forks source link

Topotato: Added ExaBGP support #60

Closed eznix86 closed 1 year ago

eznix86 commented 2 years ago

Added support for ExaBGP

Usage:

from exabgp import ExaBGP

# the variable `routers` is fetched from `r1.network.routers`; variable used as parameter during instantiation
configuration = """
neighbor {{ routers.r2.ifaces[0].ip4[0].ip }} {
    router-id 127.0.0.0;
    local-address 127.0.0.1;
}
"""
# environment is not required
custom_environment = """ 
[exabgp.log]
all = false
configuration = true
daemon = true
destination = '/var/log/exabgp.log'
"""

peer = ExaBGP(r1, configuration, custom_environment)
yield from peer.start()

# control with cli
yield from peer.execute(f"neighbor { r2.ifaces[0].ip4[0].ip } announce route 2001:db8::1/128 next-hop self ")

yield from peer.stop()

Output for ExaBGPDemo file


test_exabgp_demo.py::ExaBGPDemo::startup PASSED (1.62)                                                                [ 14%]
test_exabgp_demo.py::ExaBGPDemo::prepare::#80:r2 ExaBGP (Start) PASSED (0.04)                                         [ 28%]
test_exabgp_demo.py::ExaBGPDemo::prepare::#91:r3 ExaBGP (Start) PASSED (0.04)                                         [ 42%]
test_exabgp_demo.py::ExaBGPDemo::bgp_check::#104:r1/bgpd/vtysh "show ip bgp summary json" PASSED (0.50)               [ 57%]
test_exabgp_demo.py::ExaBGPDemo::exabgp_announcement::#111:r3 ExaBGP (Execute) PASSED (0.26)                          [ 71%]
test_exabgp_demo.py::ExaBGPDemo::exabgp_announcement::#133:r1/bgpd/vtysh "show ip bgp json" PASSED (0.00)             [ 85%]
test_exabgp_demo.py::ExaBGPDemo::shutdown PASSED (2.25)                                                               [100%]

===================================================== 7 passed in 4.91s =====================================================
eqvinox commented 2 years ago

Looks good overall, but I wonder if it is possible to move the exabgp config into the FRRConfigs class? That would allow starting exabgp before tests, and work in --run-topology if the user requests only the topology…

eqvinox commented 2 years ago

Something is going wrong for me when I try on my laptop:

PASSED (0.75)
test_exabgp_demo.py::ExaBGPDemo::prepare::#80:r2 ExaBGP (Start) 
---------------------------------------------------------------------------------------- live log call -----------------------------------------------------------------------------------------
DEBUG    root:_exabgp.py:86 exabgp tempfiles generated at /tmp/exabgp-20220902-101537-fjiv7d0q
10:15:37 | 50     | welcome       | Thank you for using ExaBGP
10:15:37 | 50     | version       | 4.2.21  
10:15:37 | 50     | interpreter   | 3.10.6 (main, Aug 10 2022, 11:19:32) [GCC 12.1.0]
10:15:37 | 50     | os            | Linux r2 5.19.3+ #1 SMP PREEMPT_DYNAMIC Tue Aug 23 10:06:12 CEST 2022 x86_64
10:15:37 | 50     | installation  | /tmp/exabgp-20220902-101537-fjiv7d0q
10:15:37 | 50     | cli control   | named pipes for the cli are:
10:15:37 | 50     | cli control   | to send commands  /tmp/exabgp-20220902-101537-fjiv7d0q/run/exabgp.in
10:15:37 | 50     | cli control   | to read responses /tmp/exabgp-20220902-101537-fjiv7d0q/run/exabgp.out
10:15:37 | 50     | configuration | performing reload of exabgp 4.2.21
10:15:37 | 50     | configuration | > neighbor         | '10.101.0.1'
10:15:37 | 50     | configuration | . router-id        | '10.101.0.2'
10:15:37 | 50     | configuration | . local-address    | '10.101.0.2'
10:15:37 | 50     | configuration | . local-as         | '65001'
10:15:37 | 50     | configuration | . peer-as          | '65534'
10:15:37 | 50     | configuration | < neighbor         | 
10:15:37 | 50     | reactor       | new peer: neighbor 10.101.0.1 local-ip 10.101.0.2 local-as 65001 peer-as 65534 router-id 10.101.0.2 family-allowed in-open
10:15:37 | 50     | reactor       | loaded new configuration successfully
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration | parsed Neighbors, un-templated
10:15:37 | 50     | configuration | ------------------------------
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration | neighbor 10.101.0.1 {
10:15:37 | 50     | configuration |   description "";
10:15:37 | 50     | configuration |   router-id 10.101.0.2;
10:15:37 | 50     | configuration |   host-name r2;
10:15:37 | 50     | configuration |   domain-name r2;
10:15:37 | 50     | configuration |   local-address 10.101.0.2;
10:15:37 | 50     | configuration |   local-as 65001;
10:15:37 | 50     | configuration |   peer-as 65534;
10:15:37 | 50     | configuration |   hold-time 180;
10:15:37 | 50     | configuration |   rate-limit disable;
10:15:37 | 50     | configuration |   manual-eor false;
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration |   passive false;
10:15:37 | 50     | configuration |   group-updates true;
10:15:37 | 50     | configuration |   auto-flush true;
10:15:37 | 50     | configuration |   adj-rib-in true;
10:15:37 | 50     | configuration |   adj-rib-out true;
10:15:37 | 50     | configuration |   md5-base64 auto;
10:15:37 | 50     | configuration |   md5-ip "10.101.0.2";
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration |   capability {
10:15:37 | 50     | configuration |     asn4 enable;
10:15:37 | 50     | configuration |     route-refresh disable;
10:15:37 | 50     | configuration |     graceful-restart disable;
10:15:37 | 50     | configuration |     nexthop disable;
10:15:37 | 50     | configuration |     add-path disable;
10:15:37 | 50     | configuration |     multi-session disable;
10:15:37 | 50     | configuration |     operational disable;
10:15:37 | 50     | configuration |     aigp disable;
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration |   family {
10:15:37 | 50     | configuration |     ipv4 unicast;
10:15:37 | 50     | configuration |     ipv4 multicast;
10:15:37 | 50     | configuration |     ipv4 nlri-mpls;
10:15:37 | 50     | configuration |     ipv4 mpls-vpn;
10:15:37 | 50     | configuration |     ipv4 rtc;
10:15:37 | 50     | configuration |     ipv4 flow;
10:15:37 | 50     | configuration |     ipv4 flow-vpn;
10:15:37 | 50     | configuration |     ipv6 unicast;
10:15:37 | 50     | configuration |     ipv6 multicast;
10:15:37 | 50     | configuration |     ipv6 nlri-mpls;
10:15:37 | 50     | configuration |     ipv6 mpls-vpn;
10:15:37 | 50     | configuration |     ipv6 flow;
10:15:37 | 50     | configuration |     ipv6 flow-vpn;
10:15:37 | 50     | configuration |     l2vpn vpls;
10:15:37 | 50     | configuration |     l2vpn evpn;
10:15:37 | 50     | configuration |     bgp-ls bgp-ls;
10:15:37 | 50     | configuration |     bgp-ls bgp-ls-vpn;
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration |   nexthop {
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration |   add-path {
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration | }       
10:15:37 | 50     | configuration |         
PASSED (0.20)
test_exabgp_demo.py::ExaBGPDemo::prepare::#91:r3 ExaBGP (Start) 
---------------------------------------------------------------------------------------- live log call -----------------------------------------------------------------------------------------
DEBUG    root:_exabgp.py:86 exabgp tempfiles generated at /tmp/exabgp-20220902-101537-93ee5yfs
10:15:37 | 52     | welcome       | Thank you for using ExaBGP
10:15:37 | 52     | version       | 4.2.21  
10:15:37 | 52     | interpreter   | 3.10.6 (main, Aug 10 2022, 11:19:32) [GCC 12.1.0]
10:15:37 | 52     | os            | Linux r2 5.19.3+ #1 SMP PREEMPT_DYNAMIC Tue Aug 23 10:06:12 CEST 2022 x86_64
10:15:37 | 52     | installation  | /tmp/exabgp-20220902-101537-fjiv7d0q
10:15:37 | 52     | cli control   | named pipes for the cli are:
10:15:37 | 52     | cli control   | to send commands  /tmp/exabgp-20220902-101537-fjiv7d0q/run/exabgp.in
10:15:37 | 52     | cli control   | to read responses /tmp/exabgp-20220902-101537-fjiv7d0q/run/exabgp.out
10:15:37 | 52     | configuration | performing reload of exabgp 4.2.21
10:15:37 | 52     | configuration | > neighbor         | '10.101.0.1'
10:15:37 | 52     | configuration | . router-id        | '10.101.0.2'
10:15:37 | 52     | configuration | . local-address    | '10.101.0.2'
10:15:37 | 52     | configuration | . local-as         | '65001'
10:15:37 | 52     | configuration | . peer-as          | '65534'
10:15:37 | 52     | configuration | < neighbor         | 
10:15:37 | 52     | reactor       | new peer: neighbor 10.101.0.1 local-ip 10.101.0.2 local-as 65001 peer-as 65534 router-id 10.101.0.2 family-allowed in-open
10:15:37 | 52     | reactor       | loaded new configuration successfully
10:15:37 | 50     | welcome       | Thank you for using ExaBGP
10:15:37 | 50     | version       | 4.2.21  
10:15:37 | 50     | interpreter   | 3.10.6 (main, Aug 10 2022, 11:19:32) [GCC 12.1.0]
10:15:37 | 50     | os            | Linux r3 5.19.3+ #1 SMP PREEMPT_DYNAMIC Tue Aug 23 10:06:12 CEST 2022 x86_64
10:15:37 | 50     | installation  | /tmp/exabgp-20220902-101537-93ee5yfs
10:15:37 | 52     | process       | forked process api-internal-cli-6cebff56
10:15:37 | 50     | cli control   | named pipes for the cli are:
10:15:37 | 50     | cli control   | to send commands  /tmp/exabgp-20220902-101537-93ee5yfs/run/exabgp.in
10:15:37 | 50     | cli control   | to read responses /tmp/exabgp-20220902-101537-93ee5yfs/run/exabgp.out
10:15:37 | 50     | configuration | performing reload of exabgp 4.2.21
10:15:37 | 50     | configuration | > neighbor         | '10.101.0.1'
10:15:37 | 50     | configuration | . router-id        | '10.101.0.3'
10:15:37 | 50     | configuration | . local-address    | '10.101.0.3'
10:15:37 | 50     | configuration | . local-as         | '65002'
10:15:37 | 50     | configuration | . peer-as          | '65534'
10:15:37 | 50     | configuration | < neighbor         | 
10:15:37 | 50     | reactor       | new peer: neighbor 10.101.0.1 local-ip 10.101.0.3 local-as 65002 peer-as 65534 router-id 10.101.0.3 family-allowed in-open
10:15:37 | 50     | reactor       | loaded new configuration successfully
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration | parsed Neighbors, un-templated
10:15:37 | 50     | configuration | ------------------------------
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration | neighbor 10.101.0.1 {
10:15:37 | 50     | configuration |   description "";
10:15:37 | 50     | configuration |   router-id 10.101.0.3;
10:15:37 | 50     | configuration |   host-name r3;
10:15:37 | 50     | configuration |   domain-name r3;
10:15:37 | 50     | configuration |   local-address 10.101.0.3;
10:15:37 | 50     | configuration |   local-as 65002;
10:15:37 | 50     | configuration |   peer-as 65534;
10:15:37 | 50     | configuration |   hold-time 180;
10:15:37 | 50     | configuration |   rate-limit disable;
10:15:37 | 50     | configuration |   manual-eor false;
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration |   passive false;
10:15:37 | 50     | configuration |   group-updates true;
10:15:37 | 50     | configuration |   auto-flush true;
10:15:37 | 50     | configuration |   adj-rib-in true;
10:15:37 | 50     | configuration |   adj-rib-out true;
10:15:37 | 50     | configuration |   md5-base64 auto;
10:15:37 | 50     | configuration |   md5-ip "10.101.0.3";
10:15:37 | 50     | configuration |         
10:15:37 | 50     | configuration |   capability {
10:15:37 | 50     | configuration |     asn4 enable;
10:15:37 | 50     | configuration |     route-refresh disable;
10:15:37 | 50     | configuration |     graceful-restart disable;
10:15:37 | 50     | configuration |     nexthop disable;
10:15:37 | 50     | configuration |     add-path disable;
10:15:37 | 50     | configuration |     multi-session disable;
10:15:37 | 50     | configuration |     operational disable;
10:15:37 | 50     | configuration |     aigp disable;
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration |   family {
10:15:37 | 50     | configuration |     ipv4 unicast;
10:15:37 | 50     | configuration |     ipv4 multicast;
10:15:37 | 50     | configuration |     ipv4 nlri-mpls;
10:15:37 | 50     | configuration |     ipv4 mpls-vpn;
10:15:37 | 50     | configuration |     ipv4 rtc;
10:15:37 | 50     | configuration |     ipv4 flow;
10:15:37 | 50     | configuration |     ipv4 flow-vpn;
10:15:37 | 50     | configuration |     ipv6 unicast;
10:15:37 | 50     | configuration |     ipv6 multicast;
10:15:37 | 50     | configuration |     ipv6 nlri-mpls;
10:15:37 | 50     | configuration |     ipv6 mpls-vpn;
10:15:37 | 50     | configuration |     ipv6 flow;
10:15:37 | 50     | configuration |     ipv6 flow-vpn;
10:15:37 | 50     | configuration |     l2vpn vpls;
10:15:37 | 50     | configuration |     l2vpn evpn;
10:15:37 | 50     | configuration |     bgp-ls bgp-ls;
10:15:37 | 50     | configuration |     bgp-ls bgp-ls-vpn;
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration |   nexthop {
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration |   add-path {
10:15:37 | 50     | configuration |   }     
10:15:37 | 50     | configuration | }       
10:15:37 | 50     | configuration |         
PASSED (0.16)
test_exabgp_demo.py::ExaBGPDemo::bgp_check::#104:r1/bgpd/vtysh "show ip bgp summary json" 10:15:37 | 52     | welcome       | Thank you for using ExaBGP
10:15:37 | 52     | version       | 4.2.21  
10:15:37 | 52     | interpreter   | 3.10.6 (main, Aug 10 2022, 11:19:32) [GCC 12.1.0]
10:15:37 | 52     | os            | Linux r3 5.19.3+ #1 SMP PREEMPT_DYNAMIC Tue Aug 23 10:06:12 CEST 2022 x86_64
10:15:37 | 52     | installation  | /tmp/exabgp-20220902-101537-93ee5yfs
10:15:37 | 52     | cli control   | named pipes for the cli are:
10:15:37 | 52     | cli control   | to send commands  /tmp/exabgp-20220902-101537-93ee5yfs/run/exabgp.in
10:15:37 | 52     | cli control   | to read responses /tmp/exabgp-20220902-101537-93ee5yfs/run/exabgp.out
10:15:37 | 52     | configuration | performing reload of exabgp 4.2.21
10:15:37 | 52     | configuration | > neighbor         | '10.101.0.1'
10:15:37 | 52     | configuration | . router-id        | '10.101.0.3'
10:15:37 | 52     | configuration | . local-address    | '10.101.0.3'
10:15:37 | 52     | configuration | . local-as         | '65002'
10:15:37 | 52     | configuration | . peer-as          | '65534'
10:15:37 | 52     | configuration | < neighbor         | 
10:15:37 | 52     | reactor       | new peer: neighbor 10.101.0.1 local-ip 10.101.0.3 local-as 65002 peer-as 65534 router-id 10.101.0.3 family-allowed in-open
10:15:37 | 52     | reactor       | loaded new configuration successfully
10:15:37 | 52     | process       | forked process api-internal-cli-6d04c4d2
PASSED (0.20)
test_exabgp_demo.py::ExaBGPDemo::exabgp_announcement::#111:r3 ExaBGP (Execute) The BGP swiss army knife of networking

usage: exabgp [--help] [--version]
              [--root ROOT] [--env ENV]
              [[--full-ini | --diff-ini | --full-env | --diff-env] |
              [--fi | --di | --fe | --de]]
              [--debug] [--pdb] [--test]
              [--once] [--signal TIME]
              [--memory] [--profile PROFILE]
              [--validate]
              [--run HELPER]
              [--decode HEX_MESSAGE]...
              [<configuration>...]

positional arguments:
  configuration         peer and route configuration file

optional arguments:
  --help, -h            ExaBGP manual page
  --version, -v         shows ExaBGP version
  --root ROOT, -f ROOT
                        root folder where etc,bin,sbin are located
  --env ENV, -e ENV     environment configuration file
  --full-ini            display the configuration using the ini format
  --fi                  (shorthand for above)
  --diff-ini            display non-default configurations values using the ini
                        format
  --di                  (shorthand for above)
  --full-env            display the configuration using the env format
  --fe                  (shorthand for above)
  --diff-env            display non-default configurations values using the env
                        format
  --de                  (shorthand for above)
  --run HELPER          Do not run ExaBGP but one of its helper program
                        (options are: healthcheck and cli)

debugging:
  --debug, -d           start the python debugger on serious logging and on
                        SIGTERM (shortcut for exabgp.log.all=true
                        exabgp.log.level=DEBUG)
  --validate            validate the configuration file format only
  --signal TIME         issue a SIGUSR1 to reload the configuration after
                        <time> seconds, only useful for code debugging
  --once, -1            only perform one attempt to connect to peers (used for
                        debugging)
  --pdb, -p             fire the debugger on critical logging, SIGTERM, and
                        exceptions (shortcut for exabgp.pdb.enable=true)
  --memory, -s          display memory usage information on exit
  --profile PROFILE     enable profiling (shortcut for
                        exabgp.profile.enable=true exabgp.profile.file=PROFILE)
  --test, -t            perform a configuration validity check only
  --decode HEX_MESSAGE, -x HEX_MESSAGE
                        decode a raw route packet in hexadecimal string

ExaBGP will automatically look for its configuration file (in windows ini
format):
 * in the etc/exabgp folder located within the extracted tar.gz
 * in /etc/exabgp/exabgp.env

Individual configuration options can be set using environment variables, such
as:
 > env exabgp.daemon.daemonize=true ./sbin/exabgp
or:
 > env exabgp.daemon.daemonize=true ./sbin/exabgp
or:
 > export exabgp.daemon.daemonize=true; ./sbin/exabgp

Multiple environment values can be set, the order of preference being:
 1) command line environment value using dot separated notation
 2) exported value from the shell using dot separated notation
 3) command line environment value using underscore separated notation
 4) exported value from the shell using underscore separated notation
 5) the value in the ini configuration file
 6) the built-in defaults

For example :
 > env exabgp.profile.enable=true \
      exabgp.profile.file=~/profile.log  \
      exabgp.log.packets=true \
      exabgp.log.destination=host:127.0.0.1 \
      exabgp.daemon.user=wheel \
      exabgp.daemon.daemonize=true \
      exabgp.daemon.pid=/var/run/exabgp.pid \
 > ./bin/exabgp ./etc/bgp/configuration.txt

The program configuration can be controlled using signals:
 * SIGLARM : restart ExaBGP
 * SIGUSR1 : reload the configuration
 * SIGUSR2 : reload the configuration and the forked processes
 * SIGTERM : terminate ExaBGP
 * SIGHUP  : terminate ExaBGP (does NOT reload the configuration anymore)

configuration issue, invalid value for daemon.user : exabgp
FAILED
test_exabgp_demo.py::ExaBGPDemo::exabgp_announcement::#133:r1/bgpd/vtysh "show ip bgp json" # show ip bgp json
{
 "vrfId": 0,
 "vrfName": "default",
 "tableVersion": 0,
 "routerId": "10.255.0.1",
 "defaultLocPrf": 100,
 "localAS": 65534,
 "routes": {  }  } 

-----
FAILED

It's probably (by accident) loading some configuration from the system config, looks like it happens when starting the CLI. Will investigate some more.

eznix86 commented 2 years ago

Looks good overall, but I wonder if it is possible to move the exabgp config into the FRRConfigs class? That would allow starting exabgp before tests, and work in --run-topology if the user requests only the topology…

I see what you mean about moving the config, but if there is a config to be run on the topology, do you have a way in mind to represent an exabgp instance on a router ?

Should it be:

eqvinox commented 2 years ago

Should it be:

* `topo.router("r1").enable_exabgp()` ?

* `topo.exabgp("peer1")`  ?

Hmm. It's a question of usability/developer experience, and you have written at least a demo test with exabgp, so you have more context/information in determining which is the best approach :)

How about something like this?

class Configs(FRRConfigs):
    routers = ["dut"] # FRR
    exabgp_routers = ["exa1", "exa2"]
    # (above could be structured better maybe)
    # …
    exabgp = """
    # … exabgp config here …
    """

# …
class ExaBGPDemo(TestBase):
    # prepare is not needed anymore, exabgp already running
    @topotatofunc
    def test_bla(self, dut, exa1, exa2):
        # variant 1: - might be easier, but inconsistent with other assertions
        yield from exa1.exabgp.execute(… exabgp command…)
        # variant 2: - more consistent with other assertions, but less good code structure?
        yield from ExaBGPExecute(exa1, … exabgp command…)

But overall… I wonder… there are also good reasons to keep it as you currently have it, it's kinda easier to understand what is going on. Hmm.

eznix86 commented 2 years ago

Should it be:

* `topo.router("r1").enable_exabgp()` ?

* `topo.exabgp("peer1")`  ?

Hmm. It's a question of usability/developer experience, and you have written at least a demo test with exabgp, so you have more context/information in determining which is the best approach :)

How about something like this?

class Configs(FRRConfigs):
    routers = ["dut"] # FRR
    exabgp_routers = ["exa1", "exa2"]
    # (above could be structured better maybe)
    # …
    exabgp = """
    # … exabgp config here …
    """

# …
class ExaBGPDemo(TestBase):
    # prepare is not needed anymore, exabgp already running
    @topotatofunc
    def test_bla(self, dut, exa1, exa2):
        # variant 1: - might be easier, but inconsistent with other assertions
        yield from exa1.exabgp.execute(… exabgp command…)
        # variant 2: - more consistent with other assertions, but less good code structure?
        yield from ExaBGPExecute(exa1, … exabgp command…)

But overall… I wonder… there are also good reasons to keep it as you currently have it, it's kinda easier to understand what is going on. Hmm.

class Configs(FRRConfigs):
    routers = ["dut"] # FRR
    exabgp_routers = ["exa1", "exa2"]
    exabgp = """
    # … exabgp config with if else stuff
    """

The one class config is cleaner and for the execution part it also seems to be a better approach, maybe we can have a class but can encapsulate multiple functions. ex below execute, assertLog, restart, stop


class ExaBGPDemo(TestBase):
    # ressembles BackgroundCmd and make use of function hint? 
    @topotatofunc
    def test_bla(self, dut, exa1, exa2, exa3):
         peer1 = ExaBGP(exa1)
         peer2 = ExaBGP(exa2)
         peer3 = ExaBGP(exa3)
         yield from peer1.execute("....cli command")
         yield from peer1.execute("....cli command")
         compare = "...."
         yield from peer2.assertLog(compare)
         yield from peer3.assertLog(compare)

For assertLog, i see in topotests (check bgp_ecmp_topo1/peer1/exa-send.py ), that there is exa-receive.py and exa-send.py. Should we consider adding them into topotato so we may have a assertLogReceive() and assertLogAnnounce() ?

in the end we will end up with


class ExaBGPDemo(TestBase):
    # ressembles BackgroundCmd and make use of function hint? 
    @topotatofunc
    def test_bla(self, dut, exa1):
         peer1 = ExaBGP(exa1)
         yield from peer1.execute("....cli command")
         compare = "...."
         yield from peer1.assertLogReceive(compare)
         compare = "...."
         yield from peer1.assertLogAnnounce(compare)
eqvinox commented 2 years ago

exabgp-pipetool

eznix86 commented 2 years ago

Should it be:

* `topo.router("r1").enable_exabgp()` ?

* `topo.exabgp("peer1")`  ?

Hmm. It's a question of usability/developer experience, and you have written at least a demo test with exabgp, so you have more context/information in determining which is the best approach :) How about something like this?

class Configs(FRRConfigs):
    routers = ["dut"] # FRR
    exabgp_routers = ["exa1", "exa2"]
    # (above could be structured better maybe)
    # …
    exabgp = """
    # … exabgp config here …
    """

# …
class ExaBGPDemo(TestBase):
    # prepare is not needed anymore, exabgp already running
    @topotatofunc
    def test_bla(self, dut, exa1, exa2):
        # variant 1: - might be easier, but inconsistent with other assertions
        yield from exa1.exabgp.execute(… exabgp command…)
        # variant 2: - more consistent with other assertions, but less good code structure?
        yield from ExaBGPExecute(exa1, … exabgp command…)

But overall… I wonder… there are also good reasons to keep it as you currently have it, it's kinda easier to understand what is going on. Hmm.

class Configs(FRRConfigs):
    routers = ["dut"] # FRR
    exabgp_routers = ["exa1", "exa2"]
    exabgp = """
    # … exabgp config with if else stuff
    """

The one class config is cleaner and for the execution part it also seems to be a better approach, maybe we can have a class but can encapsulate multiple functions. ex below execute, assertLog, restart, stop

class ExaBGPDemo(TestBase):
    # ressembles BackgroundCmd and make use of function hint? 
    @topotatofunc
    def test_bla(self, dut, exa1, exa2, exa3):
         peer1 = ExaBGP(exa1)
         peer2 = ExaBGP(exa2)
         peer3 = ExaBGP(exa3)
         yield from peer1.execute("....cli command")
         yield from peer1.execute("....cli command")
         compare = "...."
         yield from peer2.assertLog(compare)
         yield from peer3.assertLog(compare)

For assertLog, i see in topotests (check bgp_ecmp_topo1/peer1/exa-send.py ), that there is exa-receive.py and exa-send.py. Should we consider adding them into topotato so we may have a assertLogReceive() and assertLogAnnounce() ?

in the end we will end up with

class ExaBGPDemo(TestBase):
    # ressembles BackgroundCmd and make use of function hint? 
    @topotatofunc
    def test_bla(self, dut, exa1):
         peer1 = ExaBGP(exa1)
         yield from peer1.execute("....cli command")
         compare = "...."
         yield from peer1.assertLogReceive(compare)
         compare = "...."
         yield from peer1.assertLogAnnounce(compare)

After consideration, I will stick with ExaBGPExecute(cmd), AssertExaBGPLog(compare, prefix), AssertExaBGPAnnouceLog(compare, prefix) due to TopotatoAssertion and TopotatoModifier structure

github-actions[bot] commented 1 year ago

This pull request has conflicts, please resolve those before we can evaluate the pull request.