| Message ID | 20230428120038.2208429-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] tests: Fixed "nested containers" test | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
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 <xsimonar@redhat.com> > --- > 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.
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 <mmichels@redhat.com> 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 <xsimonar@redhat.com> > > --- > > 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. > >
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
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 <xsimonar@redhat.com> --- tests/ovn.at | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)