Thanks Ales and Eelco. I pushed this change to main.

On 4/21/23 06:46, Eelco Chaudron wrote:


On 21 Apr 2023, at 12:00, 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.

Reported-at: https://bugzilla.redhat.com/2183516
Signed-off-by: Ales Musil <[email protected]>
---
v2: Rebase on top of current main.
     Address comment from Eelco about not removing the sctp
     module if it was already loaded before run.

Thanks for changing! Two nits below, but it looks good to me.

Acked-by: Eelco Chaudron <[email protected]>

---
  tests/system-common-macros.at | 13 ++++++++++++
  tests/system-ovn-kmod.at      | 31 ++++++++++++++++-------------
  tests/test-l7.py              | 37 +++++++++++++----------------------
  3 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index c6ba7fb4f..e6f204cc1 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -330,6 +330,19 @@ m4_define([OVS_CHECK_CT_CLEAR],
  m4_define([OVS_CHECK_CT_ZERO_SNAT],
      [AT_SKIP_IF([! grep -q "Datapath supports ct_zero_snat" 
ovs-vswitchd.log])])

+# LOAD_MODULE([name])
+#
+# Tries to load specified kernel module and removes it after
+# if it wasn't loaded before this call.
+#
+m4_define([LOAD_MODULE],
+    [if ! lsmod | grep -q $1; then
+         on_exit 'modprobe -q -r $1'
+     fi
+     AT_CHECK([modprobe $1])

Maybe the modprobe needs to be inside the if statement? I guess it does not 
matter for the end result, so I’m fine either way.

+    ]
+)
+
  # OVN_TEST_IPV6_PREFIX_DELEGATION()
  m4_define([OVN_TEST_IPV6_PREFIX_DELEGATION],
  [
diff --git a/tests/system-ovn-kmod.at b/tests/system-ovn-kmod.at
index 981cc598c..29e0824d7 100644
--- a/tests/system-ovn-kmod.at
+++ b/tests/system-ovn-kmod.at
@@ -9,6 +9,9 @@ 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

Not sure how strict you guys are with comments ending with a dot. Maybe the 
committer can add this.

We aren't very strict with it in things like code comments. For documentation, we do stress clarity and proper grammar and mechanics. I added the dot at the end before pushing though :)


+LOAD_MODULE([sctp])
+
  CHECK_CONNTRACK()
  CHECK_CONNTRACK_NAT()
  ovn_start
@@ -127,8 +130,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 +145,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" 
\
-| 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 +169,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 +189,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 +201,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
+    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.40.0

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to