Re: [ovs-dev] [PATCH ovn] tests: Fixed "nested containers" test
Hi Mark Thanks for the review and sorry for the delay. Yes, you are right, this could have been easily modified as part of a test "clean up" ... I'll go for option 1, so that the test fails in case ovn is modified and a longer sync time is needed. Hence we'll get notified - it might be expected (in which case we'll fix the test) ... or not. Thanks Xavier On Mon, May 1, 2023 at 9:05 PM Mark Michelson wrote: > Hi Xavier, > > See below for my review > > On 4/28/23 08:00, Xavier Simonart wrote: > > When a port is removed from a logical switch, the ct-zone is > > flushed, then the ct-zone-id is removed from external_ids. > > This is done in two steps (ct-zone-id is removed when the ransaction > > flushing the ct_zone is complete). > > ovn-nbctl --wait=hv sync does not take this into account, and hence > checking > > external_ids right after lsp-del lsp; ovn-nbctl --wait=hv sync might > > fail. > > > > Signed-off-by: Xavier Simonart > > --- > > tests/ovn.at | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 7e804699a..7b8604caf 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -10218,14 +10218,14 @@ AT_CHECK([test ! -z $foo2_zoneid]) > > bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar2) > > AT_CHECK([test ! -z $bar2_zoneid]) > > > > -ovn-nbctl lsp-del bar2 > > +ovn-nbctl --wait=hv lsp-del bar2 > > ovn-nbctl --wait=hv sync > > I think you should do one of two things here: > > 1) Use two different --wait=hv calls, but add a comment explaining why > they are there. I could see someone trying to be "helpful" and removing > the sync command, since it looks redundant. > > 2) Instead of adding an additional "--wait=hv" to the lsp-del command, > you could change the below AT_CHECK to be an OVS_WAIT_UNTIL, so that as > long as it eventually succeeds, the test will pass. > > > > > bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar2) > > AT_CHECK([test -z $bar2_zoneid]) > > > > # Add back bar2 > > -ovn-nbctl lsp-add bar bar2 vm2 1 \ > > +ovn-nbctl --wait=hv lsp-add bar bar2 vm2 1 \ > > -- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3" > > wait_for_ports_up > > ovn-nbctl --wait=hv sync > > My comment above applies here, too. > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] tests: Fixed "nested containers" test
Hi Xavier, See below for my review On 4/28/23 08:00, Xavier Simonart wrote: When a port is removed from a logical switch, the ct-zone is flushed, then the ct-zone-id is removed from external_ids. This is done in two steps (ct-zone-id is removed when the ransaction flushing the ct_zone is complete). ovn-nbctl --wait=hv sync does not take this into account, and hence checking external_ids right after lsp-del lsp; ovn-nbctl --wait=hv sync might fail. Signed-off-by: Xavier Simonart --- tests/ovn.at | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ovn.at b/tests/ovn.at index 7e804699a..7b8604caf 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -10218,14 +10218,14 @@ AT_CHECK([test ! -z $foo2_zoneid]) bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2) AT_CHECK([test ! -z $bar2_zoneid]) -ovn-nbctl lsp-del bar2 +ovn-nbctl --wait=hv lsp-del bar2 ovn-nbctl --wait=hv sync I think you should do one of two things here: 1) Use two different --wait=hv calls, but add a comment explaining why they are there. I could see someone trying to be "helpful" and removing the sync command, since it looks redundant. 2) Instead of adding an additional "--wait=hv" to the lsp-del command, you could change the below AT_CHECK to be an OVS_WAIT_UNTIL, so that as long as it eventually succeeds, the test will pass. bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2) AT_CHECK([test -z $bar2_zoneid]) # Add back bar2 -ovn-nbctl lsp-add bar bar2 vm2 1 \ +ovn-nbctl --wait=hv lsp-add bar bar2 vm2 1 \ -- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3" wait_for_ports_up ovn-nbctl --wait=hv sync My comment above applies here, too. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] tests: Fixed "nested containers" test
When a port is removed from a logical switch, the ct-zone is flushed, then the ct-zone-id is removed from external_ids. This is done in two steps (ct-zone-id is removed when the ransaction flushing the ct_zone is complete). ovn-nbctl --wait=hv sync does not take this into account, and hence checking external_ids right after lsp-del lsp; ovn-nbctl --wait=hv sync might fail. Signed-off-by: Xavier Simonart --- tests/ovn.at | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ovn.at b/tests/ovn.at index 7e804699a..7b8604caf 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -10218,14 +10218,14 @@ AT_CHECK([test ! -z $foo2_zoneid]) bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2) AT_CHECK([test ! -z $bar2_zoneid]) -ovn-nbctl lsp-del bar2 +ovn-nbctl --wait=hv lsp-del bar2 ovn-nbctl --wait=hv sync bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2) AT_CHECK([test -z $bar2_zoneid]) # Add back bar2 -ovn-nbctl lsp-add bar bar2 vm2 1 \ +ovn-nbctl --wait=hv lsp-add bar bar2 vm2 1 \ -- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3" wait_for_ports_up ovn-nbctl --wait=hv sync -- 2.31.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
