diff mbox series

[ovs-dev,2/5] northd: Use the same UUID for SB representation of Chassis_Template_Var.

Message ID 20241018100930.826293-3-amusil@redhat.com
State Superseded
Headers show
Series Reuse UUID for SB rows if there is 1:1 mapping | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ales Musil Oct. 18, 2024, 10:09 a.m. UTC 
The NB Chassis_Template_Var has exactly one corresponding row in SB database.
Use the NB UUID for the SB representation which makes the processing easier
and the link between the rows is obvious at first glance.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
 northd/northd.c     | 30 +++++++++++++++---------------
 tests/ovn-northd.at |  2 ++
 2 files changed, 17 insertions(+), 15 deletions(-)

Comments

Mark Michelson Oct. 18, 2024, 8:18 p.m. UTC | #1 
Acked-by: Mark Michelson <mmichels@redhat.com>

On 10/18/24 06:09, Ales Musil wrote:
> The NB Chassis_Template_Var has exactly one corresponding row in SB database.
> Use the NB UUID for the SB representation which makes the processing easier
> and the link between the rows is obvious at first glance.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>   northd/northd.c     | 30 +++++++++++++++---------------
>   tests/ovn-northd.at |  2 ++
>   2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index fb34d0231..0dac661b0 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -18283,37 +18283,37 @@ sync_template_vars(
>       const struct nbrec_chassis_template_var_table *nbrec_ch_template_var_table,
>       const struct sbrec_chassis_template_var_table *sbrec_ch_template_var_table)
>   {
> -    struct shash nb_tvs = SHASH_INITIALIZER(&nb_tvs);
> -
>       const struct nbrec_chassis_template_var *nb_tv;
>       const struct sbrec_chassis_template_var *sb_tv;
>   
> -    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
> -            nb_tv, nbrec_ch_template_var_table) {
> -        shash_add(&nb_tvs, nb_tv->chassis, nb_tv);
> -    }
> -
>       SBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH_SAFE (
>               sb_tv, sbrec_ch_template_var_table) {
> -        nb_tv = shash_find_and_delete(&nb_tvs, sb_tv->chassis);
> +        nb_tv = nbrec_chassis_template_var_table_get_for_uuid(
> +            nbrec_ch_template_var_table, &sb_tv->header_.uuid);
>           if (!nb_tv) {
>               sbrec_chassis_template_var_delete(sb_tv);
>               continue;
>           }
> +
>           if (!smap_equal(&sb_tv->variables, &nb_tv->variables)) {
> -            sbrec_chassis_template_var_set_variables(sb_tv,
> -                                                     &nb_tv->variables);
> +            sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables);
>           }
>       }
>   
> -    struct shash_node *node;
> -    SHASH_FOR_EACH (node, &nb_tvs) {
> -        nb_tv = node->data;
> -        sb_tv = sbrec_chassis_template_var_insert(ovnsb_txn);
> +    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
> +            nb_tv, nbrec_ch_template_var_table) {
> +        const struct uuid *nb_uuid = &nb_tv->header_.uuid;
> +        sb_tv = sbrec_chassis_template_var_table_get_for_uuid(
> +            sbrec_ch_template_var_table, nb_uuid);
> +        if (sb_tv) {
> +            continue;
> +        }
> +
> +        sb_tv = sbrec_chassis_template_var_insert_persist_uuid(ovnsb_txn,
> +                                                               nb_uuid);
>           sbrec_chassis_template_var_set_chassis(sb_tv, nb_tv->chassis);
>           sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables);
>       }
> -    shash_destroy(&nb_tvs);
>   }
>   
>   static void
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index bcee2d231..ab65ddb44 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10138,7 +10138,9 @@ check ovn-nbctl set Chassis_Template_Var hv2 variables:tv=v2
>   AS_BOX([Ensure values are propagated to SB])
>   check ovn-nbctl --wait=sb sync
>   check_column "tv=v1" sb:Chassis_Template_Var variables chassis="hv1"
> +check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv1\")" sb:Chassis_Template_Var _uuid chassis="hv1"
>   check_column "tv=v2" sb:Chassis_Template_Var variables chassis="hv2"
> +check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv2\")" sb:Chassis_Template_Var _uuid chassis="hv2"
>   
>   AS_BOX([Ensure SB is reconciled])
>   check ovn-sbctl --all destroy Chassis_Template_Var
Numan Siddique Nov. 13, 2024, 4:36 p.m. UTC | #2 
On Fri, Oct 18, 2024 at 4:18 PM Mark Michelson <mmichels@redhat.com> wrote:
>
> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> On 10/18/24 06:09, Ales Musil wrote:
> > The NB Chassis_Template_Var has exactly one corresponding row in SB database.
> > Use the NB UUID for the SB representation which makes the processing easier
> > and the link between the rows is obvious at first glance.
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
> >   northd/northd.c     | 30 +++++++++++++++---------------
> >   tests/ovn-northd.at |  2 ++
> >   2 files changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index fb34d0231..0dac661b0 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -18283,37 +18283,37 @@ sync_template_vars(
> >       const struct nbrec_chassis_template_var_table *nbrec_ch_template_var_table,
> >       const struct sbrec_chassis_template_var_table *sbrec_ch_template_var_table)
> >   {
> > -    struct shash nb_tvs = SHASH_INITIALIZER(&nb_tvs);
> > -
> >       const struct nbrec_chassis_template_var *nb_tv;
> >       const struct sbrec_chassis_template_var *sb_tv;
> >
> > -    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
> > -            nb_tv, nbrec_ch_template_var_table) {
> > -        shash_add(&nb_tvs, nb_tv->chassis, nb_tv);
> > -    }
> > -
> >       SBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH_SAFE (
> >               sb_tv, sbrec_ch_template_var_table) {
> > -        nb_tv = shash_find_and_delete(&nb_tvs, sb_tv->chassis);
> > +        nb_tv = nbrec_chassis_template_var_table_get_for_uuid(
> > +            nbrec_ch_template_var_table, &sb_tv->header_.uuid);
> >           if (!nb_tv) {
> >               sbrec_chassis_template_var_delete(sb_tv);
> >               continue;
> >           }
> > +
There is a bug here.   If existing SB chassis_template_var row is out
of sync for the "chassis" column,
this patch doesn't fix it.

You can test it out by
----
ovn-nbctl create chassis_template_var chassis=ch-1

ovn-sbctl set chassis_template_var . chassis=ch-2

ovn-sbctl list chassis_template_var
----

I think you should call : sbrec_chassis_template_var_set_chassis() here.


Thanks
Numan

> >           if (!smap_equal(&sb_tv->variables, &nb_tv->variables)) {
> > -            sbrec_chassis_template_var_set_variables(sb_tv,
> > -                                                     &nb_tv->variables);
> > +            sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables);
> >           }
> >       }
> >
> > -    struct shash_node *node;
> > -    SHASH_FOR_EACH (node, &nb_tvs) {
> > -        nb_tv = node->data;
> > -        sb_tv = sbrec_chassis_template_var_insert(ovnsb_txn);
> > +    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
> > +            nb_tv, nbrec_ch_template_var_table) {
> > +        const struct uuid *nb_uuid = &nb_tv->header_.uuid;
> > +        sb_tv = sbrec_chassis_template_var_table_get_for_uuid(
> > +            sbrec_ch_template_var_table, nb_uuid);
> > +        if (sb_tv) {
> > +            continue;
> > +        }
> > +
> > +        sb_tv = sbrec_chassis_template_var_insert_persist_uuid(ovnsb_txn,
> > +                                                               nb_uuid);
> >           sbrec_chassis_template_var_set_chassis(sb_tv, nb_tv->chassis);
> >           sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables);
> >       }
> > -    shash_destroy(&nb_tvs);
> >   }
> >
> >   static void
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index bcee2d231..ab65ddb44 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10138,7 +10138,9 @@ check ovn-nbctl set Chassis_Template_Var hv2 variables:tv=v2
> >   AS_BOX([Ensure values are propagated to SB])
> >   check ovn-nbctl --wait=sb sync
> >   check_column "tv=v1" sb:Chassis_Template_Var variables chassis="hv1"
> > +check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv1\")" sb:Chassis_Template_Var _uuid chassis="hv1"
> >   check_column "tv=v2" sb:Chassis_Template_Var variables chassis="hv2"
> > +check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv2\")" sb:Chassis_Template_Var _uuid chassis="hv2"
> >
> >   AS_BOX([Ensure SB is reconciled])
> >   check ovn-sbctl --all destroy Chassis_Template_Var
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ales Musil Nov. 14, 2024, 7:03 a.m. UTC | #3 
On Wed, Nov 13, 2024 at 5:36 PM Numan Siddique <numans@ovn.org> wrote:

> On Fri, Oct 18, 2024 at 4:18 PM Mark Michelson <mmichels@redhat.com>
> wrote:
> >
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> > On 10/18/24 06:09, Ales Musil wrote:
> > > The NB Chassis_Template_Var has exactly one corresponding row in SB
> database.
> > > Use the NB UUID for the SB representation which makes the processing
> easier
> > > and the link between the rows is obvious at first glance.
> > >
> > > Signed-off-by: Ales Musil <amusil@redhat.com>
> > > ---
> > >   northd/northd.c     | 30 +++++++++++++++---------------
> > >   tests/ovn-northd.at |  2 ++
> > >   2 files changed, 17 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index fb34d0231..0dac661b0 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -18283,37 +18283,37 @@ sync_template_vars(
> > >       const struct nbrec_chassis_template_var_table
> *nbrec_ch_template_var_table,
> > >       const struct sbrec_chassis_template_var_table
> *sbrec_ch_template_var_table)
> > >   {
> > > -    struct shash nb_tvs = SHASH_INITIALIZER(&nb_tvs);
> > > -
> > >       const struct nbrec_chassis_template_var *nb_tv;
> > >       const struct sbrec_chassis_template_var *sb_tv;
> > >
> > > -    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
> > > -            nb_tv, nbrec_ch_template_var_table) {
> > > -        shash_add(&nb_tvs, nb_tv->chassis, nb_tv);
> > > -    }
> > > -
> > >       SBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH_SAFE (
> > >               sb_tv, sbrec_ch_template_var_table) {
> > > -        nb_tv = shash_find_and_delete(&nb_tvs, sb_tv->chassis);
> > > +        nb_tv = nbrec_chassis_template_var_table_get_for_uuid(
> > > +            nbrec_ch_template_var_table, &sb_tv->header_.uuid);
> > >           if (!nb_tv) {
> > >               sbrec_chassis_template_var_delete(sb_tv);
> > >               continue;
> > >           }
> > > +
> There is a bug here.   If existing SB chassis_template_var row is out
> of sync for the "chassis" column,
> this patch doesn't fix it.
>
> You can test it out by
> ----
> ovn-nbctl create chassis_template_var chassis=ch-1
>
> ovn-sbctl set chassis_template_var . chassis=ch-2
>
> ovn-sbctl list chassis_template_var
> ----
>
> I think you should call : sbrec_chassis_template_var_set_chassis() here.
>
>
> Thanks
> Numan
>


Hi Numan,

that is a good point. It is fixed in v2.

Thank you,
Ales


> > >           if (!smap_equal(&sb_tv->variables, &nb_tv->variables)) {
> > > -            sbrec_chassis_template_var_set_variables(sb_tv,
> > > -
>  &nb_tv->variables);
> > > +            sbrec_chassis_template_var_set_variables(sb_tv,
> &nb_tv->variables);
> > >           }
> > >       }
> > >
> > > -    struct shash_node *node;
> > > -    SHASH_FOR_EACH (node, &nb_tvs) {
> > > -        nb_tv = node->data;
> > > -        sb_tv = sbrec_chassis_template_var_insert(ovnsb_txn);
> > > +    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
> > > +            nb_tv, nbrec_ch_template_var_table) {
> > > +        const struct uuid *nb_uuid = &nb_tv->header_.uuid;
> > > +        sb_tv = sbrec_chassis_template_var_table_get_for_uuid(
> > > +            sbrec_ch_template_var_table, nb_uuid);
> > > +        if (sb_tv) {
> > > +            continue;
> > > +        }
> > > +
> > > +        sb_tv =
> sbrec_chassis_template_var_insert_persist_uuid(ovnsb_txn,
> > > +
>  nb_uuid);
> > >           sbrec_chassis_template_var_set_chassis(sb_tv,
> nb_tv->chassis);
> > >           sbrec_chassis_template_var_set_variables(sb_tv,
> &nb_tv->variables);
> > >       }
> > > -    shash_destroy(&nb_tvs);
> > >   }
> > >
> > >   static void
> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > index bcee2d231..ab65ddb44 100644
> > > --- a/tests/ovn-northd.at
> > > +++ b/tests/ovn-northd.at
> > > @@ -10138,7 +10138,9 @@ check ovn-nbctl set Chassis_Template_Var hv2
> variables:tv=v2
> > >   AS_BOX([Ensure values are propagated to SB])
> > >   check ovn-nbctl --wait=sb sync
> > >   check_column "tv=v1" sb:Chassis_Template_Var variables chassis="hv1"
> > > +check_column "$(fetch_column nb:Chassis_Template_Var _uuid
> chassis=\"hv1\")" sb:Chassis_Template_Var _uuid chassis="hv1"
> > >   check_column "tv=v2" sb:Chassis_Template_Var variables chassis="hv2"
> > > +check_column "$(fetch_column nb:Chassis_Template_Var _uuid
> chassis=\"hv2\")" sb:Chassis_Template_Var _uuid chassis="hv2"
> > >
> > >   AS_BOX([Ensure SB is reconciled])
> > >   check ovn-sbctl --all destroy Chassis_Template_Var
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index fb34d0231..0dac661b0 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -18283,37 +18283,37 @@  sync_template_vars(
     const struct nbrec_chassis_template_var_table *nbrec_ch_template_var_table,
     const struct sbrec_chassis_template_var_table *sbrec_ch_template_var_table)
 {
-    struct shash nb_tvs = SHASH_INITIALIZER(&nb_tvs);
-
     const struct nbrec_chassis_template_var *nb_tv;
     const struct sbrec_chassis_template_var *sb_tv;
 
-    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
-            nb_tv, nbrec_ch_template_var_table) {
-        shash_add(&nb_tvs, nb_tv->chassis, nb_tv);
-    }
-
     SBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH_SAFE (
             sb_tv, sbrec_ch_template_var_table) {
-        nb_tv = shash_find_and_delete(&nb_tvs, sb_tv->chassis);
+        nb_tv = nbrec_chassis_template_var_table_get_for_uuid(
+            nbrec_ch_template_var_table, &sb_tv->header_.uuid);
         if (!nb_tv) {
             sbrec_chassis_template_var_delete(sb_tv);
             continue;
         }
+
         if (!smap_equal(&sb_tv->variables, &nb_tv->variables)) {
-            sbrec_chassis_template_var_set_variables(sb_tv,
-                                                     &nb_tv->variables);
+            sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables);
         }
     }
 
-    struct shash_node *node;
-    SHASH_FOR_EACH (node, &nb_tvs) {
-        nb_tv = node->data;
-        sb_tv = sbrec_chassis_template_var_insert(ovnsb_txn);
+    NBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH (
+            nb_tv, nbrec_ch_template_var_table) {
+        const struct uuid *nb_uuid = &nb_tv->header_.uuid;
+        sb_tv = sbrec_chassis_template_var_table_get_for_uuid(
+            sbrec_ch_template_var_table, nb_uuid);
+        if (sb_tv) {
+            continue;
+        }
+
+        sb_tv = sbrec_chassis_template_var_insert_persist_uuid(ovnsb_txn,
+                                                               nb_uuid);
         sbrec_chassis_template_var_set_chassis(sb_tv, nb_tv->chassis);
         sbrec_chassis_template_var_set_variables(sb_tv, &nb_tv->variables);
     }
-    shash_destroy(&nb_tvs);
 }
 
 static void
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index bcee2d231..ab65ddb44 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -10138,7 +10138,9 @@  check ovn-nbctl set Chassis_Template_Var hv2 variables:tv=v2
 AS_BOX([Ensure values are propagated to SB])
 check ovn-nbctl --wait=sb sync
 check_column "tv=v1" sb:Chassis_Template_Var variables chassis="hv1"
+check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv1\")" sb:Chassis_Template_Var _uuid chassis="hv1"
 check_column "tv=v2" sb:Chassis_Template_Var variables chassis="hv2"
+check_column "$(fetch_column nb:Chassis_Template_Var _uuid chassis=\"hv2\")" sb:Chassis_Template_Var _uuid chassis="hv2"
 
 AS_BOX([Ensure SB is reconciled])
 check ovn-sbctl --all destroy Chassis_Template_Var