On Fri, Apr 21, 2023 at 10:47 AM Eelco Chaudron <echau...@redhat.com> wrote:
> > > On 18 Apr 2023, at 8:50, Ales Musil wrote: > > > Fix the outdated parts of SCTP test and > > allow it to be run on CI, in order to do that > > we just need to load sctp kernel module. > > Hi Ales, > > Thanks for looking into this. Some comments/questions inline… > Hi Eelco, thank you for the review. > > > Reported-at: https://bugzilla.redhat.com/2183516 > > Signed-off-by: Ales Musil <amu...@redhat.com> > > --- > > tests/system-ovn-kmod.at | 32 ++++++++++++++++++-------------- > > tests/test-l7.py | 37 ++++++++++++++----------------------- > > 2 files changed, 32 insertions(+), 37 deletions(-) > > > > diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at > > index c1272d5ef..593ddd2b2 100644 > > --- a/tests/system-ovn-kmod.at > > +++ b/tests/system-ovn-kmod.at > > @@ -9,6 +9,10 @@ AT_SKIP_IF([test $HAVE_SCTP = no]) > > AT_SKIP_IF([test $HAVE_NC = no]) > > AT_KEYWORDS([ovnlb sctp]) > > > > +# Make sure the SCTP kernel module is loaded > > +check modprobe sctp > > +on_exit 'modprobe -q -r sctp' > > Should we do some checks to only unload this if it was not loaded before? > That might be a safer bet, I'll look into that. > > > + > > CHECK_CONNTRACK() > > CHECK_CONNTRACK_NAT() > > ovn_start > > @@ -127,8 +131,8 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | > FORMAT_CT(30.0.0.1) | > > sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | > > sed -e 's/vtag_orig=[[0-9]]*/vtag_orig=<cleared>/' | > > sed -e 's/vtag_reply=[[0-9]]*/vtag_reply=<cleared>/' | uniq], [0], [dnl > > > -sctp,orig=(src=172.16.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > -sctp,orig=(src=172.16.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > +sctp,orig=(src=172.16.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > +sctp,orig=(src=172.16.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > ]) > > > > dnl Test load-balancing that includes L4 ports in NAT. > > @@ -142,20 +146,19 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | > FORMAT_CT(30.0.0.2) | > > sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | > > sed -e 's/vtag_orig=[[0-9]]*/vtag_orig=<cleared>/' | > > sed -e 's/vtag_reply=[[0-9]]*/vtag_reply=<cleared>/' | uniq], [0], [dnl > > > -sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > -sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > +sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > +sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > ]) > > > > check_est_flows () { > > - n=$(ovs-ofctl dump-flows br-int table=14 | grep \ > > > -"priority=120,ct_state=+est+trk,sctp,metadata=0x2,nw_dst=30.0.0.2,tp_dst=8000" > \ > > I see that in the new output the “sctp” protocol match is no longer > present. Is this something that changed in OVN matching, and can be ignored > now? > Yeah, recently we have changed the way we generate established flows for load balancers and we don't match on protocol anymore. https://github.com/ovn-org/ovn/commit/ce46a1bacf69140e64689a066a507471ba72d80f > > > -| grep nat | sed -n 's/.*n_packets=\([[0-9]]\{1,\}\).*/\1/p') > > + n=$(ovs-ofctl dump-flows br-int table=15 | grep "+est" \ > > + | grep "ct_mark=$1" | sed -n > 's/.*n_packets=\([[0-9]]\{1,\}\).*/\1/p') > > > > echo "n_packets=$n" > > - test "$n" != 0 > > + test -n "$n" && test "$n" != "0" > > } > > > > -OVS_WAIT_UNTIL([check_est_flows], [check established flows]) > > +OVS_WAIT_UNTIL([check_est_flows 0x2], [check established flows]) > > > > > > ovn-nbctl set logical_router R2 options:lb_force_snat_ip="20.0.0.2" > > @@ -167,13 +170,14 @@ ovn-nbctl destroy load_balancer $uuid > > uuid=`ovn-nbctl create load_balancer protocol=sctp > vips:30.0.0.1="192.168.1.2,192.168.2.2"` > > ovn-nbctl set logical_router R2 load_balancer=$uuid > > > > +check ovs-appctl dpctl/flush-conntrack > > + > > # Config OVN load-balancer with another VIP (this time with ports). > > ovn-nbctl set load_balancer $uuid vips:'"30.0.0.2:8000"'='" > 192.168.1.2:12345,192.168.2.2:12345"' > > > > ovn-nbctl list load_balancer > > ovn-sbctl dump-flows R2 > > -OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-flows br-int table=41 | \ > > -grep 'nat(src=20.0.0.2)']) > > +OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-flows br-int table=43 | > grep 'nat(src=20.0.0.2)']) > > > > dnl Test load-balancing that includes L4 ports in NAT. > > for i in `seq 1 20`; do > > @@ -186,8 +190,8 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | > FORMAT_CT(30.0.0.2) | > > sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | > > sed -e 's/vtag_orig=[[0-9]]*/vtag_orig=<cleared>/' | > > sed -e 's/vtag_reply=[[0-9]]*/vtag_reply=<cleared>/' | uniq], [0], [dnl > > > -sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > -sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > +sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.1.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=10,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > > +sctp,orig=(src=172.16.1.2,dst=30.0.0.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=172.16.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=10,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > ]) > > > > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(20.0.0.2) | > > @@ -198,7 +202,7 @@ > sctp,orig=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),reply > > > sctp,orig=(src=172.16.1.2,dst=192.168.2.2,sport=<cleared>,dport=<cleared>),reply=(src=192.168.2.2,dst=20.0.0.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,protoinfo=(state=<cleared>,vtag_orig=<cleared>,vtag_reply=<cleared>) > > ]) > > > > -OVS_WAIT_UNTIL([check_est_flows], [check established flows]) > > +OVS_WAIT_UNTIL([check_est_flows 0xa], [check established flows]) > > > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > > > diff --git a/tests/test-l7.py b/tests/test-l7.py > > index 701c63f0d..6e592f0dc 100755 > > --- a/tests/test-l7.py > > +++ b/tests/test-l7.py > > @@ -74,29 +74,20 @@ def get_tftpd(): > > > > > > def get_sctp(): > > - try: > > - import socket > > - import sctp > > - except ImportError: > > - print("Failed to import for SCTP") > > - server = None > > - pass > > - else: > > - class OVSSCTPServer(object): > > - def __init__(self, listen, handler=None): > > - self.sock = sctp.sctpsocket_tcp(socket.AF_INET) > > - self.sock.bind(listen) > > - self.sock.listen() > > - > > - def serve_forever(self): > > - while True: > > - client, _ = self.sock.accept() > > - client.send(b"SCRAM\r\n") > > - client.close() > > - > > - server = [OVSSCTPServer, None, 12345] > > - > > - return server > > Nice, this removes the pysctp dependency. > > > + class OVSSCTPServer(object): > > + def __init__(self, listen, handler=None): > > + self.sock = socket.socket(socket.AF_INET, > socket.SOCK_STREAM, > > + socket.IPPROTO_SCTP) > > + self.sock.bind(listen) > > + self.sock.listen() > > + > > + def serve_forever(self): > > + while True: > > + client, _ = self.sock.accept() > > + client.sendall(b"SCRAM\r\n") > > + client.close() > > + > > + return [OVSSCTPServer, None, 12345] > > > > > > def main(): > > -- > > 2.39.2 > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
