| Message ID | 20250710153345.104954-7-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Multiple unit/system tests fixes. | 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 | fail | github build: failed |
The commit message makes good sense. I think it would be useful to put a comment in the test that explains the need for the extra sync. Otherwise someone with good intentions might try to "fix" the code by removing what appears to be unnecessary syncs. On 7/10/25 11:33 AM, Xavier Simonart via dev wrote: > When a container port is added to a ls, with a new tag allocation being > requested (e.g. ovn-nbctl lsp-add ls1 c1 parent1 0), this is done by > northd in two iterations: the sb pb is created, and then the tag is allocated. > Hence, there is no guarantee that the tag is allocated in sb when > ovn-nbctl --wait=sb returns. > Before [1], this was not causing any issue, as the second transaction > (writing the tag in the sb db) was initiated before ovn-nbctl --wait=sb > returned. But this is not the case anymore. > > [1] e1495fbc86bb: "northd: Remove delay in sb_cfg propagation.". > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > tests/ovn.at | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tests/ovn.at b/tests/ovn.at > index 108de260e..01221a264 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -10247,6 +10247,7 @@ AT_CHECK([ovn-nbctl ls-add ls1]) > > AS_BOX([requested tag for parent1]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c0 parent1 3]) > +check ovn-nbctl --wait=sb sync > c0_tag=$(ovn-nbctl lsp-get-tag c0) > echo c0_tag=$c0_tag > check test "$c0_tag" = 3 > @@ -10255,6 +10256,7 @@ check_row_count Port_Binding 1 logical_port=c0 tag=$c0_tag > > AS_BOX([tag allocation 1 for parent1]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c1 parent1 0]) > +check ovn-nbctl --wait=sb sync > c1_tag=$(ovn-nbctl lsp-get-tag c1) > echo c1_tag=$c1_tag > check test "$c1_tag" != "$c0_tag" > @@ -10262,6 +10264,7 @@ check_row_count Port_Binding 1 logical_port=c1 tag=$c1_tag > > AS_BOX([tag allocation 2 for parent1]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c2 parent1 0]) > +check ovn-nbctl --wait=sb sync > c2_tag=$(ovn-nbctl lsp-get-tag c2) > echo c2_tag=$c2_tag > check test "$c2_tag" != "$c0_tag" > @@ -10270,6 +10273,7 @@ check_row_count Port_Binding 1 logical_port=c2 tag=$c2_tag > > AS_BOX([tag allocation 3 for parent1]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c3 parent1 0]) > +check ovn-nbctl --wait=sb sync > c3_tag=$(ovn-nbctl lsp-get-tag c3) > echo c3_tag=$c3_tag > check test "$c3_tag" != "$c0_tag" > @@ -10279,12 +10283,14 @@ check_row_count Port_Binding 1 logical_port=c3 tag=$c3_tag > > AS_BOX([tag allocation 1 for parent2]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c4 parent2 0]) > +check ovn-nbctl --wait=sb sync > c4_tag=$(ovn-nbctl lsp-get-tag c4) > echo c4_tag=$c4_tag > check_row_count Port_Binding 1 logical_port=c4 tag=$c4_tag > > AS_BOX([tag allocation 2 for parent2]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c5 parent2 0]) > +check ovn-nbctl --wait=sb sync > c5_tag=$(ovn-nbctl lsp-get-tag c5) > echo c5_tag=$c5_tag > check test "$c5_tag" != "$c4_tag" > @@ -10293,6 +10299,7 @@ check_row_count Port_Binding 1 logical_port=c5 tag=$c5_tag > AS_BOX([delete and add tag allocation for parent1]) > AT_CHECK([ovn-nbctl --wait=sb lsp-del c1]) > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c6 parent1 0]) > +check ovn-nbctl --wait=sb sync > c6_tag=$(ovn-nbctl lsp-get-tag c6) > echo c6_tag=$c6_tag > check_row_count Port_Binding 1 logical_port=c6 tag=$c6_tag
Hi Mark Thanks for the review and comments. I'll send v2. Thanks Xavier On Mon, Jul 21, 2025 at 10:50 PM Mark Michelson <mmichels@redhat.com> wrote: > The commit message makes good sense. I think it would be useful to put a > comment in the test that explains the need for the extra sync. Otherwise > someone with good intentions might try to "fix" the code by removing > what appears to be unnecessary syncs. > > On 7/10/25 11:33 AM, Xavier Simonart via dev wrote: > > When a container port is added to a ls, with a new tag allocation being > > requested (e.g. ovn-nbctl lsp-add ls1 c1 parent1 0), this is done by > > northd in two iterations: the sb pb is created, and then the tag is > allocated. > > Hence, there is no guarantee that the tag is allocated in sb when > > ovn-nbctl --wait=sb returns. > > Before [1], this was not causing any issue, as the second transaction > > (writing the tag in the sb db) was initiated before ovn-nbctl --wait=sb > > returned. But this is not the case anymore. > > > > [1] e1495fbc86bb: "northd: Remove delay in sb_cfg propagation.". > > > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > > tests/ovn.at | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 108de260e..01221a264 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -10247,6 +10247,7 @@ AT_CHECK([ovn-nbctl ls-add ls1]) > > > > AS_BOX([requested tag for parent1]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c0 parent1 3]) > > +check ovn-nbctl --wait=sb sync > > c0_tag=$(ovn-nbctl lsp-get-tag c0) > > echo c0_tag=$c0_tag > > check test "$c0_tag" = 3 > > @@ -10255,6 +10256,7 @@ check_row_count Port_Binding 1 logical_port=c0 > tag=$c0_tag > > > > AS_BOX([tag allocation 1 for parent1]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c1 parent1 0]) > > +check ovn-nbctl --wait=sb sync > > c1_tag=$(ovn-nbctl lsp-get-tag c1) > > echo c1_tag=$c1_tag > > check test "$c1_tag" != "$c0_tag" > > @@ -10262,6 +10264,7 @@ check_row_count Port_Binding 1 logical_port=c1 > tag=$c1_tag > > > > AS_BOX([tag allocation 2 for parent1]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c2 parent1 0]) > > +check ovn-nbctl --wait=sb sync > > c2_tag=$(ovn-nbctl lsp-get-tag c2) > > echo c2_tag=$c2_tag > > check test "$c2_tag" != "$c0_tag" > > @@ -10270,6 +10273,7 @@ check_row_count Port_Binding 1 logical_port=c2 > tag=$c2_tag > > > > AS_BOX([tag allocation 3 for parent1]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c3 parent1 0]) > > +check ovn-nbctl --wait=sb sync > > c3_tag=$(ovn-nbctl lsp-get-tag c3) > > echo c3_tag=$c3_tag > > check test "$c3_tag" != "$c0_tag" > > @@ -10279,12 +10283,14 @@ check_row_count Port_Binding 1 logical_port=c3 > tag=$c3_tag > > > > AS_BOX([tag allocation 1 for parent2]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c4 parent2 0]) > > +check ovn-nbctl --wait=sb sync > > c4_tag=$(ovn-nbctl lsp-get-tag c4) > > echo c4_tag=$c4_tag > > check_row_count Port_Binding 1 logical_port=c4 tag=$c4_tag > > > > AS_BOX([tag allocation 2 for parent2]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c5 parent2 0]) > > +check ovn-nbctl --wait=sb sync > > c5_tag=$(ovn-nbctl lsp-get-tag c5) > > echo c5_tag=$c5_tag > > check test "$c5_tag" != "$c4_tag" > > @@ -10293,6 +10299,7 @@ check_row_count Port_Binding 1 logical_port=c5 > tag=$c5_tag > > AS_BOX([delete and add tag allocation for parent1]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-del c1]) > > AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c6 parent1 0]) > > +check ovn-nbctl --wait=sb sync > > c6_tag=$(ovn-nbctl lsp-get-tag c6) > > echo c6_tag=$c6_tag > > check_row_count Port_Binding 1 logical_port=c6 tag=$c6_tag > >
diff --git a/tests/ovn.at b/tests/ovn.at index 108de260e..01221a264 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -10247,6 +10247,7 @@ AT_CHECK([ovn-nbctl ls-add ls1]) AS_BOX([requested tag for parent1]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c0 parent1 3]) +check ovn-nbctl --wait=sb sync c0_tag=$(ovn-nbctl lsp-get-tag c0) echo c0_tag=$c0_tag check test "$c0_tag" = 3 @@ -10255,6 +10256,7 @@ check_row_count Port_Binding 1 logical_port=c0 tag=$c0_tag AS_BOX([tag allocation 1 for parent1]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c1 parent1 0]) +check ovn-nbctl --wait=sb sync c1_tag=$(ovn-nbctl lsp-get-tag c1) echo c1_tag=$c1_tag check test "$c1_tag" != "$c0_tag" @@ -10262,6 +10264,7 @@ check_row_count Port_Binding 1 logical_port=c1 tag=$c1_tag AS_BOX([tag allocation 2 for parent1]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c2 parent1 0]) +check ovn-nbctl --wait=sb sync c2_tag=$(ovn-nbctl lsp-get-tag c2) echo c2_tag=$c2_tag check test "$c2_tag" != "$c0_tag" @@ -10270,6 +10273,7 @@ check_row_count Port_Binding 1 logical_port=c2 tag=$c2_tag AS_BOX([tag allocation 3 for parent1]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c3 parent1 0]) +check ovn-nbctl --wait=sb sync c3_tag=$(ovn-nbctl lsp-get-tag c3) echo c3_tag=$c3_tag check test "$c3_tag" != "$c0_tag" @@ -10279,12 +10283,14 @@ check_row_count Port_Binding 1 logical_port=c3 tag=$c3_tag AS_BOX([tag allocation 1 for parent2]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c4 parent2 0]) +check ovn-nbctl --wait=sb sync c4_tag=$(ovn-nbctl lsp-get-tag c4) echo c4_tag=$c4_tag check_row_count Port_Binding 1 logical_port=c4 tag=$c4_tag AS_BOX([tag allocation 2 for parent2]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c5 parent2 0]) +check ovn-nbctl --wait=sb sync c5_tag=$(ovn-nbctl lsp-get-tag c5) echo c5_tag=$c5_tag check test "$c5_tag" != "$c4_tag" @@ -10293,6 +10299,7 @@ check_row_count Port_Binding 1 logical_port=c5 tag=$c5_tag AS_BOX([delete and add tag allocation for parent1]) AT_CHECK([ovn-nbctl --wait=sb lsp-del c1]) AT_CHECK([ovn-nbctl --wait=sb lsp-add ls1 c6 parent1 0]) +check ovn-nbctl --wait=sb sync c6_tag=$(ovn-nbctl lsp-get-tag c6) echo c6_tag=$c6_tag check_row_count Port_Binding 1 logical_port=c6 tag=$c6_tag
When a container port is added to a ls, with a new tag allocation being requested (e.g. ovn-nbctl lsp-add ls1 c1 parent1 0), this is done by northd in two iterations: the sb pb is created, and then the tag is allocated. Hence, there is no guarantee that the tag is allocated in sb when ovn-nbctl --wait=sb returns. Before [1], this was not causing any issue, as the second transaction (writing the tag in the sb db) was initiated before ovn-nbctl --wait=sb returned. But this is not the case anymore. [1] e1495fbc86bb: "northd: Remove delay in sb_cfg propagation.". Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- tests/ovn.at | 7 +++++++ 1 file changed, 7 insertions(+)