Hi Mark Thanks for the review. See comments below. Thanks Xavier
On Tue, Sep 19, 2023 at 5:29 PM Mark Michelson <mmich...@redhat.com> wrote: > Thanks for the patch set Xavier. > > With the exception of patch 13: > > Acked-by: Mark Michelson <mmich...@redhat.com> > > I'm acking the patch set, but the nature of these changes worries me. I > could easily see another patch series like this needed in the future > because it's too easy to write flaky tests. Here are some high-level > ideas I have based on changes I see in these patches. > > +1 > 1) From patches 12 and 15: Should we change ovn_start() so that by > default it does not start a backup northd process? I suspect the number > of tests that rely on a backup northd process are very small. > Changing this would impact all tests, including all which do not query ovn-northd through appctl, and hence do not suffer from this issue. However, I am not sure that starting northd-backup makes so much sense, even for those tests. So I agree. If nobody disagrees, I will change the default in ovn-start (i.e. default = w/o northd), and add an option --with-northd-backup for the tests which require the backup to be running) > > 2) From patch 5 and possibly patch 14: Should we add a new --wait option > to ovn-nbctl (e.g. "--wait=ovs") that waits for OVS to provide a > confirmation that flows from this ovn-nbctl call have been installed? > This one is more complex. Waiting for some flows to be present was an easy way to fix test cases for which wait_for_port_up was not sufficient (either because flows are added later, such as remote ports related ones, or because port_up itself is reported too early by ovn-controller).The proper fix might require changing ovn-controller so that it reports ports up correctly. > > 3) From patch 4: Would it be possible to provide a "blocking" version of > `ovn-appctl -t ovn-controller exit` ? In other words, could we make a > version that will not return control to the shell until the process has > exited? I guess we could do something like "if command is exit (or a new one such as exit_wait - so we can keep current behaviour), then get the pid through unixctl, send the exit, and check for pid termination with a timeout" Barring that, we could write a macro for ovn-macros.at that will > call `ovn-appctl -t ovn-controller exit` and then block until the > process has exited. We could then always call that macro when we want to > shut ovn-controller down. > I'll add the macro in a v2 - it's a small change. Nothing prevents up to add the blocking ovn-appctl exit afterwards (and change the macro) > > There is at least one more similar issue I noted: some tests fail because pinctrl is slow to start (and first packets arrive before it's started), and packets are lost. I earlier added in some tests WAIT_UNTIL checking ovn-controller.log that pinctl is connected, but this should be done for more tests. We could change the tests for this, or update ovn-controller so that it only starts looping when pinctrl is connected Thanks Xavier On 9/18/23 12:46, Xavier Simonart wrote: > > Xavier Simonart (15): > > tests: fixed multiple ovn-ic tests > > tests: fixed "Logical router policy packet marking" > > tests: fixed "options:requested-chassis for logical port" > > tests: tests fixed "Encaps tunnel cleanup ..." and "ovn-controller > > exit" > > tests: wait for all flows to be installed before sending packets > > tests: fixed multiple tests not properly waiting for packets to > > be received > > tests: fixed multiple tests missing ovn-nbctl "wait" > > tests: fixed "L2 Drop and Allow ACL w/ Stateful ACL" > > tests: skip test "MAC binding aging" if scapy not available. > > tests: move trim_zeros() to ovn-macros > > tests: fixed "send gratuitous ARP for NAT rules on HA distributed > > router" > > tests: fixed "SCTP Load balancer health checks"a > > tests: fixed "LSP incremental processing" > > tests: fixed "ipsec -- basic configuration" > > tests: do not start northd-backup for northd tests querying northd > > > > tests/ovn-ic.at | 14 +- > > tests/ovn-ipsec.at | 4 +- > > tests/ovn-macros.at | 4 + > > tests/ovn-northd.at | 27 ++- > > tests/ovn.at | 484 ++++++++++++++++---------------------------- > > 5 files changed, 207 insertions(+), 326 deletions(-) > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
