Acked-by: Mark Michelson <mmich...@redhat.com>

I have some python-related nits below, but I don't think any of them is strong enough to prevent this from being merged.

On 11/4/22 18:11, Dumitru Ceara wrote:
In a sandbox run:

$ ./ovn-lb-benchmark.sh <N> <VIPS> <BACKENDS> <USE_TEMPLATES>

to simulate an ovn-k8s-like topology with N nodes, VIPS NodePort services
applied to all nodes.  Each service has BACKENDS backends.

If USE_TEMPLATES is "yes" then the configuration will be optimized to use
Chassis_Template_Vars.  Otherwise it will create N LBs per service, one
for every node.

Signed-off-by: Dumitru Ceara <dce...@redhat.com>
---
  tutorial/automake.mk                 |    4 +
  tutorial/ovn-gen-lb-template-vars.py |  116 ++++++++++++++++++++++++++++++++++
  tutorial/ovn-lb-benchmark.sh         |  110 ++++++++++++++++++++++++++++++++
  3 files changed, 229 insertions(+), 1 deletion(-)
  create mode 100755 tutorial/ovn-gen-lb-template-vars.py
  create mode 100755 tutorial/ovn-lb-benchmark.sh

diff --git a/tutorial/automake.mk b/tutorial/automake.mk
index 046962c000..171da8de66 100644
--- a/tutorial/automake.mk
+++ b/tutorial/automake.mk
@@ -1,6 +1,8 @@
  EXTRA_DIST += \
        tutorial/ovs-sandbox \
-       tutorial/ovn-setup.sh
+       tutorial/ovn-setup.sh \
+       tutorial/ovn-lb-benchmark.sh \
+       tutorial/ovn-gen-lb-template-vars.py
  sandbox: all
        cd $(srcdir)/tutorial && MAKE=$(MAKE) HAVE_OPENSSL=$(HAVE_OPENSSL) \
                ./ovs-sandbox -b $(abs_builddir) --ovs-src $(ovs_srcdir) 
--ovs-build $(ovs_builddir) $(SANDBOXFLAGS)
diff --git a/tutorial/ovn-gen-lb-template-vars.py 
b/tutorial/ovn-gen-lb-template-vars.py
new file mode 100755
index 0000000000..fe7e6b93f6
--- /dev/null
+++ b/tutorial/ovn-gen-lb-template-vars.py
@@ -0,0 +1,116 @@
+import getopt
+import os
+import re
+import sys
+import uuid
+
+import ovs.db.idl
+import ovs.db.schema
+import ovs.db.types
+import ovs.ovsuuid
+import ovs.poller
+import ovs.stream
+import ovs.util
+import ovs.vlog
+from ovs.db import data
+from ovs.db import error
+from ovs.db.idl import _row_to_uuid as row_to_uuid
+from ovs.fatal_signal import signal_alarm

The following imports are not used:
* os
* re
* uuid
* ovs.db.schema
* ovs.db.types
* ovs.ovsuuid
* from ovs.db import data
* from ovs.db.idl import _row_to_uuid as row_to_uuid

+
+vlog = ovs.vlog.Vlog("template-lb-stress")
+vlog.set_levels_from_string("console:info")
+vlog.init(None)
+
+SCHEMA = '../ovn-nb.ovsschema'
+
+
+def add_chassis_template_vars(idl, n, n_vips, n_backends):
+    for i in range(1, n + 1):
+        print(f'ADDING LBs for node {i}')
+        txn = ovs.db.idl.Transaction(idl)

Would it make sense to create the transaction outside this for loop and then commit just one big transaction at the end?

+        tv = txn.insert(idl.tables["Chassis_Template_Var"])
+        tv.chassis = f'chassis-{i}'
+        tv.setkey('variables', 'vip', f'42.42.42.{i}')
+
+        for j in range(1, n_vips + 1):
+            backends = ''
+            for k in range(0, n_backends):
+                j1 = j // 250
+                j2 = j % 250

The j1 and j2 calculations can be moved out of this loop since they are constant across all values of k.

If you do that, then you can rewrite this inner loop as a list comprehension:

backends = [f'42.{k}.{j1}.{j2}:{j}' for k in range(n_backends)]

(note that when using range(), you can omit the '0' as the first parameter and just put the upper bound)

Then when you need to stringify backends, use

','.join(backends)



+                backends = f'42.{k}.{j1}.{j2}:{j},{backends}'
+            tv.setkey('variables', f'backends{j}', backends)
+        status = txn.commit_block()
+        sys.stdout.write(
+            f'commit status = 
{ovs.db.idl.Transaction.status_to_string(status)}\n'
+        )
+
+
+def run(remote, n, n_vips, n_backends):
+    schema_helper = ovs.db.idl.SchemaHelper(SCHEMA)
+    schema_helper.register_all()
+    idl = ovs.db.idl.Idl(remote, schema_helper, leader_only=False)
+
+    seqno = 0
+
+    error, stream = ovs.stream.Stream.open_block(
+        ovs.stream.Stream.open(remote), 2000
+    )
+    if error:
+        sys.stderr.write(f'failed to connect to \"{remote}\"')
+        sys.exit(1)
+
+    if not stream:
+        sys.stderr.write(f'failed to connect to \"{remote}\"')
+        sys.exit(1)
+    rpc = ovs.jsonrpc.Connection(stream)
+
+    while idl.change_seqno == seqno and not idl.run():
+        rpc.run()
+
+        poller = ovs.poller.Poller()
+        idl.wait(poller)
+        rpc.wait(poller)
+        poller.block()
+
+    add_chassis_template_vars(idl, n, n_vips, n_backends)
+
+
+def main(argv):
+    try:
+        options, args = getopt.gnu_getopt(
+            argv[1:], 'n:v:b:r:', ['vips', 'backends', 'remote']
+        )
+    except getopt.GetoptError as geo:
+        sys.stderr.write(f'{ovs.util.PROGRAM_NAME}: {geo.msg}\n')
+        sys.exit(1)
+
+    n = None
+    vips = None
+    backends = None
+    remote = None
+    for key, value in options:
+        if key == '-n':
+            n = int(value)
+        elif key in ['-v', '--vips']:
+            vips = int(value)
+        elif key in ['-b', '--backends']:
+            backends = int(value)
+        elif key in ['-r', '--remote']:
+            remote = value
+        else:
+            sys.stderr.write(f'{ovs.util.PROGRAM_NAME}: unknown input args')
+            sys.exit(1)
+
+    if not n or not vips or not backends:
+        sys.stderr.write(f'{ovs.util.PROGRAM_NAME}: invalid input args')
+        sys.exit(1)

I'm of the opinion that argparse is streets ahead of getopt. argparse allows for:
* automatic type-checking of inputs
* automatic usage and help text generation
* automatic detection of unknown/missing options

I also think it's easier to read and write, and less error-prone than getopt.

As I said at the top of the email, none of the findings here are actionable enough to require an update to the series. However, don't be surprised if I end up submitting a change later that migrates from getopt to argparse :)

+
+    run(remote, n, vips, backends)
+
+
+if __name__ == '__main__':
+    try:
+        main(sys.argv)
+    except error.Error as e:
+        sys.stderr.write(f'{e}\n')
+        sys.exit(1)
diff --git a/tutorial/ovn-lb-benchmark.sh b/tutorial/ovn-lb-benchmark.sh
new file mode 100755
index 0000000000..6e8129ab97
--- /dev/null
+++ b/tutorial/ovn-lb-benchmark.sh
@@ -0,0 +1,110 @@
+#!/bin/bash
+
+nrtr=$1
+nlb=$2
+nbackends=$3
+use_template=$4
+
+echo "ROUTERS        : $nrtr"
+echo "LBS            : $nlb"
+echo "BACKENDS PER LB: $nbackends"
+echo "USE TEMPLATE   : ${use_template}"
+
+export OVN_NB_DAEMON=$(ovn-nbctl --detach)
+export OVN_SB_DAEMON=$(ovn-sbctl --detach)
+trap "killall -9 ovn-nbctl; killall -9 ovn-sbctl" EXIT
+
+lbg=$(ovn-nbctl create load_balancer_group name=lbg)
+
+ovn-nbctl ls-add join
+ovn-nbctl lr-add cluster
+ovn-nbctl lrp-add cluster rcj 00:00:00:00:00:01 10.0.0.1/8
+ovn-nbctl lsp-add join sjc \
+    -- lsp-set-type sjc router \
+    -- lsp-set-addresses sjc router \
+    --  lsp-set-options sjc router-port=rcj
+
+for i in $(seq $nrtr); do
+    ch="chassis-$i"
+    gwr=lr-$i
+    echo "ROUTER $gwr"
+    gwr2join=lr2j-$i
+    join2gwr=j2lr-$i
+
+    ovn-nbctl lr-add $gwr \
+        -- set logical_router $gwr load_balancer_group=$lbg \
+        -- set logical_router $gwr options:chassis=$ch
+    ovn-nbctl lrp-add $gwr $gwr2join 00:00:00:00:00:01 10.0.0.1/8
+    ovn-nbctl lsp-add join $join2gwr \
+        -- lsp-set-type $join2gwr router \
+        -- lsp-set-addresses $join2gwr router \
+        -- lsp-set-options $join2gwr router-port=$gwr2join
+
+    s=ls-$i
+    echo "SWITCH $s"
+    s2cluster=s2c-$s
+    cluster2s=c2s-$s
+    ovn-nbctl ls-add $s \
+        -- set logical_switch $s load_balancer_group=$lbg
+    ovn-nbctl lsp-add $s $s2cluster \
+        -- lsp-set-type $s2cluster router \
+        -- lsp-set-addresses $s2cluster router \
+        -- lsp-set-options $s2cluster router-port=$cluster2s
+    ovn-nbctl lrp-add cluster $cluster2s 00:00:00:00:00:01 10.0.0.1/8
+    ovn-nbctl lrp-set-gateway-chassis $cluster2s $ch 1
+
+    lsp=lsp-$i
+    echo "LSP $lsp"
+    ovn-nbctl lsp-add $s $lsp
+done
+
+# Bind a port from the first LS locally.
+ovs-vsctl add-port br-int lsp-1 \
+    -- set interface lsp-1 external_ids:iface-id=lsp-1
+
+# Add NodePort-like LBs using templates.
+function add_template_lbs() {
+    for l in $(seq $nlb); do
+        lb=lb-$l
+        echo "LB $lb"
+        ovn-nbctl --template lb-add $lb "^vip:$l" "^backends$l" tcp
+        lb_uuid=$(ovn-nbctl --columns _uuid --bare find load_balancer name=$lb)
+        ovn-nbctl add load_balancer_group $lbg load_balancer $lb_uuid
+    done
+
+    # Generate LBs chassis_template_vars.
+    python ovn-gen-lb-template-vars.py -n $nrtr -v $nlb -b $nbackends \
+        -r unix:$PWD/sandbox/nb1.ovsdb
+}
+
+# Add NodePort-like LBs without using templates.
+function add_non_template_lbs() {
+    for i in $(seq $nrtr); do
+        echo ITERATION $i
+        gwr=lr-$i
+        s=ls-$i
+        for l in $(seq $nlb); do
+            lb=lb-$l-$i
+            echo LB $lb
+            backends=""
+            for k in $(seq $nbackends); do
+                l1=$(expr $l / 250)
+                l2=$(expr $l % 250)
+                backends="42.$k.$l1.$l2:$l,$backends"
+            done
+            lb_uuid=$(ovn-nbctl create load_balancer name=$lb \
+                      protocol=tcp vips:"\"42.42.42.$i:$l\""="\"$backends\"")
+            ovn-nbctl add logical_switch $s load_balancer ${lb_uuid}
+            ovn-nbctl add logical_router $gwr load_balancer ${lb_uuid}
+        done
+    done
+}
+
+if [ "${use_template}" = "yes" ]; then
+    add_template_lbs
+else
+    add_non_template_lbs
+fi
+
+ovs-appctl -t $PWD/sandbox/nb1 ovsdb-server/compact
+ovs-appctl -t $PWD/sandbox/sb1 ovsdb-server/compact


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to