[ovs-dev,3/3] Revert "northd: Introduce ECMP_Nexthop table in SB db."

This reverts commit aeae21335a8bccbb9fe7746fcf4ed2d2a0e1c7a4.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 lib/ovn-util.h           |  2 --
 northd/en-northd.c       | 35 --------------------
 northd/en-northd.h       |  4 ---
 northd/inc-proc-northd.c |  7 +---
 northd/northd.c          | 70 ----------------------------------------
 northd/northd.h          | 10 ------
 ovn-sb.ovsschema         | 18 ++---------
 ovn-sb.xml               | 31 ------------------
 tests/ovn-northd.at      |  4 ---
 9 files changed, 3 insertions(+), 178 deletions(-)

Hi Lorenzo,

I have one complaint below, otherwise the series looks good to me.

On 8/22/24 10:30, Lorenzo Bianconi wrote:
> This reverts commit aeae21335a8bccbb9fe7746fcf4ed2d2a0e1c7a4.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   lib/ovn-util.h           |  2 --
>   northd/en-northd.c       | 35 --------------------
>   northd/en-northd.h       |  4 ---
>   northd/inc-proc-northd.c |  7 +---
>   northd/northd.c          | 70 ----------------------------------------
>   northd/northd.h          | 10 ------
>   ovn-sb.ovsschema         | 18 ++---------
>   ovn-sb.xml               | 31 ------------------
>   tests/ovn-northd.at      |  4 ---
>   9 files changed, 3 insertions(+), 178 deletions(-)
> 
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 622fec531..7b98b9b9a 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -38,8 +38,6 @@
>   #define STT_TUNNEL_OVERHEAD 18
>   #define VXLAN_TUNNEL_OVERHEAD 30
>   
> -#define ECMP_NEXTHOP_IDS_LEN 65535
> -
>   struct eth_addr;
>   struct nbrec_logical_router_port;
>   struct ovsrec_flow_sample_collector_set_table;
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 63f93bbf4..34f0a7df7 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -404,25 +404,6 @@ en_bfd_sync_run(struct engine_node *node, void *data)
>       engine_set_node_state(node, EN_UPDATED);
>   }
>   
> -void
> -en_ecmp_nexthop_run(struct engine_node *node, void *data)
> -{
> -    const struct engine_context *eng_ctx = engine_get_context();
> -    struct static_routes_data *static_routes_data =
> -        engine_get_input_data("static_routes", node);
> -    struct ecmp_nexthop_data *enh_data = data;
> -    const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table =
> -        EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node));
> -
> -    ecmp_nexthop_destroy(data);
> -    ecmp_nexthop_init(data);
> -    build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn,
> -                             &static_routes_data->parsed_routes,
> -                             &enh_data->nexthops,
> -                             sbrec_ecmp_nexthop_table);
> -    engine_set_node_state(node, EN_UPDATED);
> -}
> -
>   void
>   *en_northd_init(struct engine_node *node OVS_UNUSED,
>                   struct engine_arg *arg OVS_UNUSED)
> @@ -473,16 +454,6 @@ void
>       return data;
>   }
>   
> -void
> -*en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
> -                      struct engine_arg *arg OVS_UNUSED)
> -{
> -    struct ecmp_nexthop_data *data = xzalloc(sizeof *data);
> -
> -    ecmp_nexthop_init(data);
> -    return data;
> -}
> -
>   void
>   en_northd_cleanup(void *data)
>   {
> @@ -555,9 +526,3 @@ en_bfd_sync_cleanup(void *data)
>   {
>       bfd_destroy(data);
>   }
> -
> -void
> -en_ecmp_nexthop_cleanup(void *data)
> -{
> -    ecmp_nexthop_destroy(data);
> -}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 2666cc67e..631a7c17a 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -42,9 +42,5 @@ bool bfd_sync_northd_change_handler(struct engine_node *node,
>                                       void *data OVS_UNUSED);
>   void en_bfd_sync_run(struct engine_node *node, void *data);
>   void en_bfd_sync_cleanup(void *data OVS_UNUSED);
> -void en_ecmp_nexthop_run(struct engine_node *node, void *data);
> -void *en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
> -                           struct engine_arg *arg OVS_UNUSED);
> -void en_ecmp_nexthop_cleanup(void *data);
>   
>   #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 06c7ee2b8..1f79916a5 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -103,8 +103,7 @@ static unixctl_cb_func chassis_features_list;
>       SB_NODE(fdb, "fdb") \
>       SB_NODE(static_mac_binding, "static_mac_binding") \
>       SB_NODE(chassis_template_var, "chassis_template_var") \
> -    SB_NODE(logical_dp_group, "logical_dp_group") \
> -    SB_NODE(ecmp_nexthop, "ecmp_nexthop")
> +    SB_NODE(logical_dp_group, "logical_dp_group")
>   
>   enum sb_engine_node {
>   #define SB_NODE(NAME, NAME_STR) SB_##NAME,
> @@ -163,7 +162,6 @@ static ENGINE_NODE(route_policies, "route_policies");
>   static ENGINE_NODE(static_routes, "static_routes");
>   static ENGINE_NODE(bfd, "bfd");
>   static ENGINE_NODE(bfd_sync, "bfd_sync");
> -static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop");
>   
>   void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>                             struct ovsdb_idl_loop *sb)
> @@ -266,9 +264,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>       engine_add_input(&en_bfd_sync, &en_route_policies, NULL);
>       engine_add_input(&en_bfd_sync, &en_northd, bfd_sync_northd_change_handler);
>   
> -    engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL);
> -    engine_add_input(&en_ecmp_nexthop, &en_static_routes, NULL);
> -
>       engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
>       engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
>       engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 51ee5db1d..fb18fec90 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10670,64 +10670,6 @@ build_bfd_map(const struct nbrec_bfd_table *nbrec_bfd_table,
>       }
>   }
>   
> -void
> -build_ecmp_nexthop_table(
> -        struct ovsdb_idl_txn *ovnsb_txn,
> -        struct hmap *routes,
> -        struct simap *nexthops,
> -        const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table)
> -{
> -    if (!ovnsb_txn) {
> -        return;
> -    }
> -
> -    unsigned long *nexthop_ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
> -    const struct sbrec_ecmp_nexthop *sb_ecmp_nexthop;
> -    SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sb_ecmp_nexthop,
> -                                       sbrec_ecmp_nexthop_table) {
> -        simap_put(nexthops, sb_ecmp_nexthop->nexthop,
> -                  sb_ecmp_nexthop->id);
> -        bitmap_set1(nexthop_ids, sb_ecmp_nexthop->id);
> -    }
> -
> -    struct sset nb_nexthops_sset = SSET_INITIALIZER(&nb_nexthops_sset);
> -
> -    struct parsed_route *pr;
> -    HMAP_FOR_EACH (pr, key_node, routes) {
> -        if (!pr->ecmp_symmetric_reply) {
> -            continue;
> -        }
> -
> -        const struct nbrec_logical_router_static_route *r = pr->route;
> -        if (!simap_contains(nexthops, r->nexthop)) {
> -            int id = bitmap_scan(nexthop_ids, 0, 1, ECMP_NEXTHOP_IDS_LEN);
> -            if (id == ECMP_NEXTHOP_IDS_LEN) {
> -                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -                VLOG_WARN_RL(&rl, "nexthop id address space is exhausted");
> -                continue;
> -            }
> -            bitmap_set1(nexthop_ids, id);
> -            simap_put(nexthops, r->nexthop, id);
> -
> -            sb_ecmp_nexthop = sbrec_ecmp_nexthop_insert(ovnsb_txn);
> -            sbrec_ecmp_nexthop_set_nexthop(sb_ecmp_nexthop, r->nexthop);
> -            sbrec_ecmp_nexthop_set_id(sb_ecmp_nexthop, id);
> -        }
> -        sset_add(&nb_nexthops_sset, r->nexthop);
> -    }
> -
> -    SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH_SAFE (sb_ecmp_nexthop,
> -                                            sbrec_ecmp_nexthop_table) {
> -        if (!sset_contains(&nb_nexthops_sset, sb_ecmp_nexthop->nexthop)) {
> -            simap_find_and_delete(nexthops, sb_ecmp_nexthop->nexthop);
> -            sbrec_ecmp_nexthop_delete(sb_ecmp_nexthop);
> -        }
> -    }
> -
> -    sset_destroy(&nb_nexthops_sset);
> -    bitmap_free(nexthop_ids);
> -}
> -
>   /* Returns a string of the IP address of the router port 'op' that
>    * overlaps with 'ip_s".  If one is not found, returns NULL.
>    *
> @@ -18781,12 +18723,6 @@ bfd_init(struct bfd_data *data)
>       hmap_init(&data->bfd_connections);
>   }
>   
> -void
> -ecmp_nexthop_init(struct ecmp_nexthop_data *data)
> -{
> -    simap_init(&data->nexthops);
> -}
> -
>   void
>   northd_destroy(struct northd_data *data)
>   {
> @@ -18868,12 +18804,6 @@ static_routes_destroy(struct static_routes_data *data)
>       __bfd_destroy(&data->bfd_active_connections);
>   }
>   
> -void
> -ecmp_nexthop_destroy(struct ecmp_nexthop_data *data)
> -{
> -    simap_destroy(&data->nexthops);
> -}
> -
>   void
>   ovnnb_db_run(struct northd_input *input_data,
>                struct northd_data *data,
> diff --git a/northd/northd.h b/northd/northd.h
> index afd5f7113..9e326b746 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -195,10 +195,6 @@ struct bfd_data {
>       struct hmap bfd_connections;
>   };
>   
> -struct ecmp_nexthop_data {
> -    struct simap nexthops;
> -};
> -
>   struct lr_nat_table;
>   
>   struct lflow_input {
> @@ -742,12 +738,6 @@ void static_routes_destroy(struct static_routes_data *);
>   void bfd_init(struct bfd_data *);
>   void bfd_destroy(struct bfd_data *);
>   
> -void build_ecmp_nexthop_table(struct ovsdb_idl_txn *,
> -                              struct hmap *, struct simap *,
> -                              const struct sbrec_ecmp_nexthop_table *);
> -void ecmp_nexthop_init(struct ecmp_nexthop_data *);
> -void ecmp_nexthop_destroy(struct ecmp_nexthop_data *);
> -
>   struct lflow_table;
>   struct lr_stateful_tracked_data;
>   struct ls_stateful_tracked_data;
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 9d8e0ac46..ec39fdd81 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Southbound",
> -    "version": "20.36.0",
> -    "cksum": "1845967275 32154",
> +    "version": "20.35.0",
> +    "cksum": "2897301415 31493",

Even though the table that bumped the version is being removed, I think 
it makes more sense to bump the version to 23.37.0 instead of decreasing 
the version number here.

>       "tables": {
>           "SB_Global": {
>               "columns": {
> @@ -610,20 +610,6 @@
>                                             "refTable": "Datapath_Binding"}}}},
>               "indexes": [["logical_port", "ip"]],
>               "isRoot": true},
> -        "ECMP_Nexthop": {
> -            "columns": {
> -                "nexthop": {"type": "string"},
> -                "id": {"type": {"key": {"type": "integer",
> -                                        "minInteger": 0,
> -                                        "maxInteger": 65535}}},
> -                "external_ids": {
> -                    "type": {"key": "string", "value": "string",
> -                             "min": 0, "max": "unlimited"}},
> -                "options": {
> -                    "type": {"key": "string", "value": "string",
> -                             "min": 0, "max": "unlimited"}}},
> -            "indexes": [["nexthop"]],
> -            "isRoot": true},
>           "Chassis_Template_Var": {
>               "columns": {
>                   "chassis": {"type": "string"},
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index c11296d7c..746cc6308 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -5178,35 +5178,4 @@ tcp.flags = RST;
>         The set of variable values for a given chassis.
>       </column>
>     </table>
> -
> -  <table name="ECMP_Nexthop">
> -    <p>
> -      Each record in this table represents an active ECMP route committed by
> -      <code>ovn-northd</code> to <code>ovs</code> connection-tracking table.
> -      <code>ECMP_Nexthop</code> table is used by <code>ovn-controller</code>
> -      to track active ct entries and to flush stale ones.
> -    </p>
> -    <column name="nexthop">
> -      <p>
> -        Nexthop IP address for this ECMP route. Nexthop IP address should
> -        be the IP address of a connected router port or the IP address of
> -        an external device used as nexthop for the given destination.
> -      </p>
> -    </column>
> -
> -    <column name="id">
> -      <p>
> -        Nexthop unique identifier. Nexthop ID is used to track active
> -        ecmp-symmetric-reply connections and flush stale ones.
> -      </p>
> -    </column>
> -
> -    <column name="options">
> -      Reserved for future use.
> -    </column>
> -
> -    <column name="external_ids">
> -      See <em>External IDs</em> at the beginning of this document.
> -    </column>
> -  </table>
>   </database>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 93ccbce6b..fddf222b3 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -6800,7 +6800,6 @@ check ovn-nbctl lsp-set-addresses public-lr0 router
>   check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>   
>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.10
> -check_row_count ECMP_Nexthop 1
>   
>   ovn-sbctl dump-flows lr0 > lr0flows
>   
> @@ -6818,7 +6817,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | ovn_strip_lflows], [0], [dn
>   ])
>   
>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.20
> -check_row_count ECMP_Nexthop 2
>   
>   ovn-sbctl dump-flows lr0 > lr0flows
>   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> @@ -6848,7 +6846,6 @@ AT_CHECK([grep -e "lr_in_arp_resolve.*ecmp" lr0flows | ovn_strip_lflows], [0], [
>   
>   # add ecmp route with wrong nexthop
>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.1.20
> -check_row_count ECMP_Nexthop 2
>   
>   ovn-sbctl dump-flows lr0 > lr0flows
>   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> @@ -6868,7 +6865,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | sed 's/192\.168\.0\..0/192.
>   
>   check ovn-nbctl lr-route-del lr0
>   wait_row_count nb:Logical_Router_Static_Route 0
> -check_row_count ECMP_Nexthop 0
>   
>   check ovn-nbctl --wait=sb lr-route-add lr0 1.0.0.0/24 192.168.0.10
>   ovn-sbctl dump-flows lr0 > lr0flows

> Hi Lorenzo,
> 
> I have one complaint below, otherwise the series looks good to me.
> 

[...]

> > @@ -742,12 +738,6 @@ void static_routes_destroy(struct static_routes_data *);
> >   void bfd_init(struct bfd_data *);
> >   void bfd_destroy(struct bfd_data *);
> > -void build_ecmp_nexthop_table(struct ovsdb_idl_txn *,
> > -                              struct hmap *, struct simap *,
> > -                              const struct sbrec_ecmp_nexthop_table *);
> > -void ecmp_nexthop_init(struct ecmp_nexthop_data *);
> > -void ecmp_nexthop_destroy(struct ecmp_nexthop_data *);
> > -
> >   struct lflow_table;
> >   struct lr_stateful_tracked_data;
> >   struct ls_stateful_tracked_data;
> > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > index 9d8e0ac46..ec39fdd81 100644
> > --- a/ovn-sb.ovsschema
> > +++ b/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> >   {
> >       "name": "OVN_Southbound",
> > -    "version": "20.36.0",
> > -    "cksum": "1845967275 32154",
> > +    "version": "20.35.0",
> > +    "cksum": "2897301415 31493",
> 
> Even though the table that bumped the version is being removed, I think it
> makes more sense to bump the version to 23.37.0 instead of decreasing the
> version number here.

ack, I am fine with it. If we have a consensus for it I will post v2.

Regards,
Lorenzo

> 
> >       "tables": {
> >           "SB_Global": {
> >               "columns": {
> > @@ -610,20 +610,6 @@
> >                                             "refTable": "Datapath_Binding"}}}},
> >               "indexes": [["logical_port", "ip"]],
> >               "isRoot": true},
> > -        "ECMP_Nexthop": {
> > -            "columns": {
> > -                "nexthop": {"type": "string"},
> > -                "id": {"type": {"key": {"type": "integer",
> > -                                        "minInteger": 0,
> > -                                        "maxInteger": 65535}}},
> > -                "external_ids": {
> > -                    "type": {"key": "string", "value": "string",
> > -                             "min": 0, "max": "unlimited"}},
> > -                "options": {
> > -                    "type": {"key": "string", "value": "string",
> > -                             "min": 0, "max": "unlimited"}}},
> > -            "indexes": [["nexthop"]],
> > -            "isRoot": true},
> >           "Chassis_Template_Var": {
> >               "columns": {
> >                   "chassis": {"type": "string"},
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index c11296d7c..746cc6308 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -5178,35 +5178,4 @@ tcp.flags = RST;
> >         The set of variable values for a given chassis.
> >       </column>
> >     </table>
> > -
> > -  <table name="ECMP_Nexthop">
> > -    <p>
> > -      Each record in this table represents an active ECMP route committed by
> > -      <code>ovn-northd</code> to <code>ovs</code> connection-tracking table.
> > -      <code>ECMP_Nexthop</code> table is used by <code>ovn-controller</code>
> > -      to track active ct entries and to flush stale ones.
> > -    </p>
> > -    <column name="nexthop">
> > -      <p>
> > -        Nexthop IP address for this ECMP route. Nexthop IP address should
> > -        be the IP address of a connected router port or the IP address of
> > -        an external device used as nexthop for the given destination.
> > -      </p>
> > -    </column>
> > -
> > -    <column name="id">
> > -      <p>
> > -        Nexthop unique identifier. Nexthop ID is used to track active
> > -        ecmp-symmetric-reply connections and flush stale ones.
> > -      </p>
> > -    </column>
> > -
> > -    <column name="options">
> > -      Reserved for future use.
> > -    </column>
> > -
> > -    <column name="external_ids">
> > -      See <em>External IDs</em> at the beginning of this document.
> > -    </column>
> > -  </table>
> >   </database>
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 93ccbce6b..fddf222b3 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -6800,7 +6800,6 @@ check ovn-nbctl lsp-set-addresses public-lr0 router
> >   check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> >   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.10
> > -check_row_count ECMP_Nexthop 1
> >   ovn-sbctl dump-flows lr0 > lr0flows
> > @@ -6818,7 +6817,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | ovn_strip_lflows], [0], [dn
> >   ])
> >   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.20
> > -check_row_count ECMP_Nexthop 2
> >   ovn-sbctl dump-flows lr0 > lr0flows
> >   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> > @@ -6848,7 +6846,6 @@ AT_CHECK([grep -e "lr_in_arp_resolve.*ecmp" lr0flows | ovn_strip_lflows], [0], [
> >   # add ecmp route with wrong nexthop
> >   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.1.20
> > -check_row_count ECMP_Nexthop 2
> >   ovn-sbctl dump-flows lr0 > lr0flows
> >   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> > @@ -6868,7 +6865,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | sed 's/192\.168\.0\..0/192.
> >   check ovn-nbctl lr-route-del lr0
> >   wait_row_count nb:Logical_Router_Static_Route 0
> > -check_row_count ECMP_Nexthop 0
> >   check ovn-nbctl --wait=sb lr-route-add lr0 1.0.0.0/24 192.168.0.10
> >   ovn-sbctl dump-flows lr0 > lr0flows
>

On 8/22/24 17:17, Lorenzo Bianconi wrote:
>> Hi Lorenzo,
>>
>> I have one complaint below, otherwise the series looks good to me.
>>
> 
> [...]
> 
>>> @@ -742,12 +738,6 @@ void static_routes_destroy(struct static_routes_data *);
>>>   void bfd_init(struct bfd_data *);
>>>   void bfd_destroy(struct bfd_data *);
>>> -void build_ecmp_nexthop_table(struct ovsdb_idl_txn *,
>>> -                              struct hmap *, struct simap *,
>>> -                              const struct sbrec_ecmp_nexthop_table *);
>>> -void ecmp_nexthop_init(struct ecmp_nexthop_data *);
>>> -void ecmp_nexthop_destroy(struct ecmp_nexthop_data *);
>>> -
>>>   struct lflow_table;
>>>   struct lr_stateful_tracked_data;
>>>   struct ls_stateful_tracked_data;
>>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
>>> index 9d8e0ac46..ec39fdd81 100644
>>> --- a/ovn-sb.ovsschema
>>> +++ b/ovn-sb.ovsschema
>>> @@ -1,7 +1,7 @@
>>>   {
>>>       "name": "OVN_Southbound",
>>> -    "version": "20.36.0",
>>> -    "cksum": "1845967275 32154",
>>> +    "version": "20.35.0",
>>> +    "cksum": "2897301415 31493",
>>
>> Even though the table that bumped the version is being removed, I think it
>> makes more sense to bump the version to 23.37.0 instead of decreasing the
>> version number here.
> 
> ack, I am fine with it. If we have a consensus for it I will post v2.
> 

While I don't think there's anyone (there shouldn't be at least) using
an unreleased version of OVN with this table in the schema it doesn't
hurt to bump the version just to be sure.

On the other hand, if someone was using this version an upgrade to this
new version (without the SB table) will break their deployment anyway.

TL/DR: I'm fine either way. :)

Regards,
Dumitru

> Regards,
> Lorenzo
> 
>>
>>>       "tables": {
>>>           "SB_Global": {
>>>               "columns": {
>>> @@ -610,20 +610,6 @@
>>>                                             "refTable": "Datapath_Binding"}}}},
>>>               "indexes": [["logical_port", "ip"]],
>>>               "isRoot": true},
>>> -        "ECMP_Nexthop": {
>>> -            "columns": {
>>> -                "nexthop": {"type": "string"},
>>> -                "id": {"type": {"key": {"type": "integer",
>>> -                                        "minInteger": 0,
>>> -                                        "maxInteger": 65535}}},
>>> -                "external_ids": {
>>> -                    "type": {"key": "string", "value": "string",
>>> -                             "min": 0, "max": "unlimited"}},
>>> -                "options": {
>>> -                    "type": {"key": "string", "value": "string",
>>> -                             "min": 0, "max": "unlimited"}}},
>>> -            "indexes": [["nexthop"]],
>>> -            "isRoot": true},
>>>           "Chassis_Template_Var": {
>>>               "columns": {
>>>                   "chassis": {"type": "string"},
>>> diff --git a/ovn-sb.xml b/ovn-sb.xml
>>> index c11296d7c..746cc6308 100644
>>> --- a/ovn-sb.xml
>>> +++ b/ovn-sb.xml
>>> @@ -5178,35 +5178,4 @@ tcp.flags = RST;
>>>         The set of variable values for a given chassis.
>>>       </column>
>>>     </table>
>>> -
>>> -  <table name="ECMP_Nexthop">
>>> -    <p>
>>> -      Each record in this table represents an active ECMP route committed by
>>> -      <code>ovn-northd</code> to <code>ovs</code> connection-tracking table.
>>> -      <code>ECMP_Nexthop</code> table is used by <code>ovn-controller</code>
>>> -      to track active ct entries and to flush stale ones.
>>> -    </p>
>>> -    <column name="nexthop">
>>> -      <p>
>>> -        Nexthop IP address for this ECMP route. Nexthop IP address should
>>> -        be the IP address of a connected router port or the IP address of
>>> -        an external device used as nexthop for the given destination.
>>> -      </p>
>>> -    </column>
>>> -
>>> -    <column name="id">
>>> -      <p>
>>> -        Nexthop unique identifier. Nexthop ID is used to track active
>>> -        ecmp-symmetric-reply connections and flush stale ones.
>>> -      </p>
>>> -    </column>
>>> -
>>> -    <column name="options">
>>> -      Reserved for future use.
>>> -    </column>
>>> -
>>> -    <column name="external_ids">
>>> -      See <em>External IDs</em> at the beginning of this document.
>>> -    </column>
>>> -  </table>
>>>   </database>
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 93ccbce6b..fddf222b3 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -6800,7 +6800,6 @@ check ovn-nbctl lsp-set-addresses public-lr0 router
>>>   check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>>>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.10
>>> -check_row_count ECMP_Nexthop 1
>>>   ovn-sbctl dump-flows lr0 > lr0flows
>>> @@ -6818,7 +6817,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | ovn_strip_lflows], [0], [dn
>>>   ])
>>>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.20
>>> -check_row_count ECMP_Nexthop 2
>>>   ovn-sbctl dump-flows lr0 > lr0flows
>>>   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
>>> @@ -6848,7 +6846,6 @@ AT_CHECK([grep -e "lr_in_arp_resolve.*ecmp" lr0flows | ovn_strip_lflows], [0], [
>>>   # add ecmp route with wrong nexthop
>>>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.1.20
>>> -check_row_count ECMP_Nexthop 2
>>>   ovn-sbctl dump-flows lr0 > lr0flows
>>>   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
>>> @@ -6868,7 +6865,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | sed 's/192\.168\.0\..0/192.
>>>   check ovn-nbctl lr-route-del lr0
>>>   wait_row_count nb:Logical_Router_Static_Route 0
>>> -check_row_count ECMP_Nexthop 0
>>>   check ovn-nbctl --wait=sb lr-route-add lr0 1.0.0.0/24 192.168.0.10
>>>   ovn-sbctl dump-flows lr0 > lr0flows
>>

On Thu, Aug 22, 2024 at 8:21 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 8/22/24 17:17, Lorenzo Bianconi wrote:
> >> Hi Lorenzo,
> >>
> >> I have one complaint below, otherwise the series looks good to me.
> >>
> >
> > [...]
> >
> >>> @@ -742,12 +738,6 @@ void static_routes_destroy(struct static_routes_data *);
> >>>   void bfd_init(struct bfd_data *);
> >>>   void bfd_destroy(struct bfd_data *);
> >>> -void build_ecmp_nexthop_table(struct ovsdb_idl_txn *,
> >>> -                              struct hmap *, struct simap *,
> >>> -                              const struct sbrec_ecmp_nexthop_table *);
> >>> -void ecmp_nexthop_init(struct ecmp_nexthop_data *);
> >>> -void ecmp_nexthop_destroy(struct ecmp_nexthop_data *);
> >>> -
> >>>   struct lflow_table;
> >>>   struct lr_stateful_tracked_data;
> >>>   struct ls_stateful_tracked_data;
> >>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> >>> index 9d8e0ac46..ec39fdd81 100644
> >>> --- a/ovn-sb.ovsschema
> >>> +++ b/ovn-sb.ovsschema
> >>> @@ -1,7 +1,7 @@
> >>>   {
> >>>       "name": "OVN_Southbound",
> >>> -    "version": "20.36.0",
> >>> -    "cksum": "1845967275 32154",
> >>> +    "version": "20.35.0",
> >>> +    "cksum": "2897301415 31493",
> >>
> >> Even though the table that bumped the version is being removed, I think it
> >> makes more sense to bump the version to 23.37.0 instead of decreasing the
> >> version number here.
> >
> > ack, I am fine with it. If we have a consensus for it I will post v2.
> >
>
> While I don't think there's anyone (there shouldn't be at least) using
> an unreleased version of OVN with this table in the schema it doesn't
> hurt to bump the version just to be sure.
>
> On the other hand, if someone was using this version an upgrade to this
> new version (without the SB table) will break their deployment anyway.
>
> TL/DR: I'm fine either way. :)

So if we increase the version instead of reverting it, it means users
not deployed with the intermediate version (which is most cases if not
all) but deployed with the previous version may later encounter a
schema conversion without real change in the schema. I think that is
fine but never tried it myself. Probably better to have a quick test
to confirm.

Thanks,
Han

>
> Regards,
> Dumitru
>
> > Regards,
> > Lorenzo
> >
> >>
> >>>       "tables": {
> >>>           "SB_Global": {
> >>>               "columns": {
> >>> @@ -610,20 +610,6 @@
> >>>                                             "refTable": "Datapath_Binding"}}}},
> >>>               "indexes": [["logical_port", "ip"]],
> >>>               "isRoot": true},
> >>> -        "ECMP_Nexthop": {
> >>> -            "columns": {
> >>> -                "nexthop": {"type": "string"},
> >>> -                "id": {"type": {"key": {"type": "integer",
> >>> -                                        "minInteger": 0,
> >>> -                                        "maxInteger": 65535}}},
> >>> -                "external_ids": {
> >>> -                    "type": {"key": "string", "value": "string",
> >>> -                             "min": 0, "max": "unlimited"}},
> >>> -                "options": {
> >>> -                    "type": {"key": "string", "value": "string",
> >>> -                             "min": 0, "max": "unlimited"}}},
> >>> -            "indexes": [["nexthop"]],
> >>> -            "isRoot": true},
> >>>           "Chassis_Template_Var": {
> >>>               "columns": {
> >>>                   "chassis": {"type": "string"},
> >>> diff --git a/ovn-sb.xml b/ovn-sb.xml
> >>> index c11296d7c..746cc6308 100644
> >>> --- a/ovn-sb.xml
> >>> +++ b/ovn-sb.xml
> >>> @@ -5178,35 +5178,4 @@ tcp.flags = RST;
> >>>         The set of variable values for a given chassis.
> >>>       </column>
> >>>     </table>
> >>> -
> >>> -  <table name="ECMP_Nexthop">
> >>> -    <p>
> >>> -      Each record in this table represents an active ECMP route committed by
> >>> -      <code>ovn-northd</code> to <code>ovs</code> connection-tracking table.
> >>> -      <code>ECMP_Nexthop</code> table is used by <code>ovn-controller</code>
> >>> -      to track active ct entries and to flush stale ones.
> >>> -    </p>
> >>> -    <column name="nexthop">
> >>> -      <p>
> >>> -        Nexthop IP address for this ECMP route. Nexthop IP address should
> >>> -        be the IP address of a connected router port or the IP address of
> >>> -        an external device used as nexthop for the given destination.
> >>> -      </p>
> >>> -    </column>
> >>> -
> >>> -    <column name="id">
> >>> -      <p>
> >>> -        Nexthop unique identifier. Nexthop ID is used to track active
> >>> -        ecmp-symmetric-reply connections and flush stale ones.
> >>> -      </p>
> >>> -    </column>
> >>> -
> >>> -    <column name="options">
> >>> -      Reserved for future use.
> >>> -    </column>
> >>> -
> >>> -    <column name="external_ids">
> >>> -      See <em>External IDs</em> at the beginning of this document.
> >>> -    </column>
> >>> -  </table>
> >>>   </database>
> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >>> index 93ccbce6b..fddf222b3 100644
> >>> --- a/tests/ovn-northd.at
> >>> +++ b/tests/ovn-northd.at
> >>> @@ -6800,7 +6800,6 @@ check ovn-nbctl lsp-set-addresses public-lr0 router
> >>>   check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> >>>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.10
> >>> -check_row_count ECMP_Nexthop 1
> >>>   ovn-sbctl dump-flows lr0 > lr0flows
> >>> @@ -6818,7 +6817,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | ovn_strip_lflows], [0], [dn
> >>>   ])
> >>>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.0.20
> >>> -check_row_count ECMP_Nexthop 2
> >>>   ovn-sbctl dump-flows lr0 > lr0flows
> >>>   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> >>> @@ -6848,7 +6846,6 @@ AT_CHECK([grep -e "lr_in_arp_resolve.*ecmp" lr0flows | ovn_strip_lflows], [0], [
> >>>   # add ecmp route with wrong nexthop
> >>>   check ovn-nbctl --wait=sb --ecmp-symmetric-reply lr-route-add lr0 1.0.0.1 192.168.1.20
> >>> -check_row_count ECMP_Nexthop 2
> >>>   ovn-sbctl dump-flows lr0 > lr0flows
> >>>   AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> >>> @@ -6868,7 +6865,6 @@ AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | sed 's/192\.168\.0\..0/192.
> >>>   check ovn-nbctl lr-route-del lr0
> >>>   wait_row_count nb:Logical_Router_Static_Route 0
> >>> -check_row_count ECMP_Nexthop 0
> >>>   check ovn-nbctl --wait=sb lr-route-add lr0 1.0.0.0/24 192.168.0.10
> >>>   ovn-sbctl dump-flows lr0 > lr0flows
> >>
>