date:20230802

Re: [ovs-dev] [PATCH] ovsdb-tool: Fix json leak while showing clustered log.

2023-08-02 Thread Dumitru Ceara

On 8/2/23 18:46, Ilya Maximets wrote:
> The json read from file is never freed in ovsdb-tool show-log
> for a clustered database:
> 
>  ERROR: LeakSanitizer: detected memory leaks
> 
>  Direct leak of 10774760 byte(s) in 269369 object(s) allocated from:
> 0 0x50cc32 in malloc (ovsdb/ovsdb-tool+0x50cc32)
> 1 0x6e7b6b in xmalloc__ lib/util.c:140:15
> 2 0x6e7b6b in xmalloc lib/util.c:175:12
> 3 0x6494f6 in json_create lib/json.c:1489:25
> 4 0x64a8a7 in json_object_create lib/json.c:263:25
> 5 0x6525f3 in json_parser_push_object lib/json.c:1311:25
> 6 0x6525f3 in json_parser_input lib/json.c:1409:13
> 7 0x64f6c4 in json_parser_feed lib/json.c:1126:17
> 8 0x5694b5 in parse_body ovsdb/log.c:412:9
> 9 0x5694b5 in ovsdb_log_read ovsdb/log.c:477:13
>10 0x54d294 in do_show_log_cluster ovsdb/ovsdb-tool.c:1069:27
>11 0x54d294 in do_show_log ovsdb/ovsdb-tool.c:1115:9
>12 0x63b7b1 in ovs_cmdl_run_command__ lib/command-line.c:247:17
>13 0x5488a5 in main ovsdb/ovsdb-tool.c:82:5
>14 0xe0eb49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49)
>15 0xe0ec0a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x27c0a)
>16 0x471fe4 in _start (ovsdb/ovsdb-tool+0x471fe4)
> 
> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered 
> databases.")
> Reported-by: Dumitru Ceara 
> Signed-off-by: Ilya Maximets 
> ---

Thanks for the quick fix!  Looks good to me:

Acked-by: Dumitru Ceara 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: Fix excessive memory usage on DB open.

2023-08-02 Thread Dumitru Ceara

On 8/2/23 15:45, Ilya Maximets wrote:
> During initial read of a database file all the file transactions are
> added to the transaction history.  The history run with the history
> size checks is only executed after the whole file is processed.
> If, for some reason, the file contains way too many transactions,
> this behavior may result in excessive memory consumption up to
> hundreds of GBs.  For example, here is a log entry about memory usage
> after reading a file with 100K+ OVN NbDB transactions:
> 
>   |4|memory|INFO|95650400 kB peak resident set size after 96.9 seconds
>   |5|memory|INFO|atoms:3083346 cells:1838767 monitors:0
> raft-log:123309 txn-history:123307 txn-history-atoms:1647022868
> 
> In this particular case ovsdb-server allocated 95 GB of RAM in order
> to accommodate 1.6 billion ovsdb atoms in the history, while only 3
> million atoms are in the actual database.

Wow..

> 
> Fix that by running history size checks after applying each file
> transaction.  This way the memory usage while reading the database
> from the example stays at about 1 GB mark.  History size checks are
> cheap in comparison with transaction replay, so the additional calls
> do not reduce performance.
> 
> We could've just moved the history run into ovsdb_txn_replay_commit(),
> but it seems more organic to call it externally, since we have init()
> and destroy() functions called externally as well.
> 
> Since the history run will be executed shortly after reading the
> database and actual memory consumption peak is not always logged,
> there seem to be no reliable way to unit test for the issue without
> adding extra testing infrastructure into the code.
> 
> Fixes: 695e81502794 ("ovsdb-server: Transaction history tracking.")
> Reported-at: https://bugzilla.redhat.com/2228464
> Signed-off-by: Ilya Maximets 
> ---
>  ovsdb/ovsdb-server.c | 3 ++-
>  ovsdb/relay.c| 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 8e623118b..cf09c9079 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -235,7 +235,7 @@ main_loop(struct server_config *config,
>  
>  SHASH_FOR_EACH_SAFE (node, all_dbs) {
>  struct db *db = node->data;
> -ovsdb_txn_history_run(db->db);
> +
>  ovsdb_storage_run(db->db->storage);
>  read_db(config, db);
>  /* Run triggers after storage_run and read_db to make sure new 
> raft
> @@ -678,6 +678,7 @@ parse_txn(struct server_config *config, struct db *db,
>  if (!error && !uuid_is_zero(txnid)) {
>  db->db->prereq = *txnid;
>  }
> +ovsdb_txn_history_run(db->db);
>  }
>  return error;
>  }
> diff --git a/ovsdb/relay.c b/ovsdb/relay.c
> index b035cb492..27ff196b7 100644
> --- a/ovsdb/relay.c
> +++ b/ovsdb/relay.c
> @@ -413,6 +413,7 @@ ovsdb_relay_run(void)
>  }
>  ovsdb_cs_event_destroy(event);
>  }
> +ovsdb_txn_history_run(ctx->db);
>  }
>  }
>  

It's always cool and somewhat worrying to see such a
commit-log-size-to-loc ratio but I think this is fine:

Acked-by: Dumitru Ceara 

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/4] tests: Add ovsdb execution cases for set size constraints.

2023-08-02 Thread Dumitru Ceara

On 7/25/23 11:32, Ilya Maximets wrote:
> Adding an extra check to one of the ovsdb execution cases that will
> verify that ovsdb-server is able to read back transactions previously
> written to a database file.  And also adding new execution tests
> that cover previously discovered issues with size checks on sets.
> 
> Signed-off-by: Ilya Maximets 
> ---
>  tests/ovsdb-execution.at | 54 ++--
>  tests/ovsdb-server.at| 15 ++-

tests++

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/4] ovsdb: relay: Fix handling of XOR updates with size constraints.

2023-08-02 Thread Dumitru Ceara

On 7/25/23 11:32, Ilya Maximets wrote:
> Relay servers apply updates via ovsdb_table_execute_update().  XOR
> updates contain datum diffs, and datum diffs can be larger than the
> type constraints.  Currently, relay will fail to parse such update
> into ovsdb row triggering a syntax error and a re-connection.
> 
> Fix that by relaxing the size constraints for this kind of updates.
> 
> Fixes: 026c77c58ddb ("ovsdb: New ovsdb 'relay' service model.")
> Signed-off-by: Ilya Maximets 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/4] ovsdb: file: Fix diff application to a default column value.

2023-08-02 Thread Dumitru Ceara

On 7/25/23 11:32, Ilya Maximets wrote:
> On a server size, default values are just normal values.  The only

Uhm, I guess you meant "server side", right?

> difference is that they are initialized from default atoms.  They are
> allocated on a separate piece of memory as any other values, so there
> should not be any special treatment.
> 
> Current code doesn't apply the diff to a column with default values
> after reading the file transaction and that breaks the logic.
> 
> For example, if we have a column with a set and a minimum number of
> elements for a type is 1, it will be initialized with one default atom.
> On mutation, new values can be added and the diff will contain only
> these new values, while the column will contain both the new values
> and the default atom.  While reading such transaction from a file
> with a diff, current code will replace the content loosing the default

Nit: losing

> atom.  The only case where we need to actually replace is if this row
> doesn't exist and it's not actually a diff, i.e. if this row was just
> created to be populated with a json content.
> 
> Fix that by removing the wrong check and not use values as a diff
> in case the row doesn't exist in a database.
> 
> Fixes: 2ccd66f594f7 ("ovsdb: Use column diffs for ovsdb and raft log 
> entries.")
> Signed-off-by: Ilya Maximets 
> ---

Acked-by: Dumitru Ceara 

Thanks!

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] ovsdb: file: Fix inability to read diffs that violate type size.

2023-08-02 Thread Dumitru Ceara

On 7/25/23 11:32, Ilya Maximets wrote:
> Diff records in a database file may contain sets larger than a maximum
> set size, so constraints should not be checked on read.  They will be
> checked later after applying the diff to a column.
> 
> Fixes: 2ccd66f594f7 ("ovsdb: Use column diffs for ovsdb and raft log 
> entries.")
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2023-July/406685.html
> Reported-by: Peng He 
> Signed-off-by: Ilya Maximets 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] system-tests: Wait for all interfaces to have IPv6

2023-08-02 Thread Mark Michelson


Thanks, Ales

Acked-by: Mark Michelson 

On 8/2/23 09:31, Ales Musil wrote:

The "SNAT in gateway router mode" could occasionally
fail, because the ext1 IPv6 address wasn't ready and
the NA packet arrived too late for the ping to succeed.

Signed-off-by: Ales Musil 
---
  tests/system-ovn.at | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index fef3cfbf0..766a250e5 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -8691,7 +8691,8 @@ ADD_VETH(ext1, ext1, br0, "172.16.1.1/24", 
"00:ee:00:01:01:01", \
  check ovn-nbctl --wait=hv sync
  wait_for_ports_up
  OVS_WAIT_UNTIL([test "$(ip netns exec ls1p1 ip a | grep 2001::1 | grep tentative)" = 
""])
-OVS_WAIT_UNTIL([test "$(ip netns exec ls1p2 ip a | grep 2002::1 | grep tentative)" = 
""])
+OVS_WAIT_UNTIL([test "$(ip netns exec ls1p2 ip a | grep 2002::2 | grep tentative)" = 
""])
+OVS_WAIT_UNTIL([test "$(ip netns exec ext1 ip a | grep 1711::1 | grep tentative)" = 
""])
  
  NS_CHECK_EXEC([ls1p1], [ping -q -c 3 -i 0.3 -w 2  172.16.1.1 | FORMAT_PING], \

  [0], [dnl


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1] lacp: Improve visibility with LACP

2023-08-02 Thread Mike Pattrick

Diagnosing connectivity issues involving a bond can be complicated by a
lack of logging in LACP. It is difficult to determine the health of
sending and receving LACP packets. This is further complicated by the
tendency of some switches to toggle the carrier on interfaces that
experience issues with LACP, which can cause confusion about why an
interface is flapping down and up again.

With this patch, OVS will log when LACP packets aren't sent or recieved
on time.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2223306
Signed-off-by: Mike Pattrick 
---
 lib/lacp.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/lib/lacp.c b/lib/lacp.c
index 3252f17eb..fd186347f 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -143,6 +143,8 @@ struct member {
 uint32_t count_link_expired;/* Num of times link expired */
 uint32_t count_link_defaulted;  /* Num of times link defaulted */
 uint32_t count_carrier_changed; /* Num of times link status changed */
+long long int last_tx;
+long long int last_rx;
 };
 
 static struct ovs_mutex mutex;
@@ -387,6 +389,13 @@ lacp_process_packet(struct lacp *lacp, const void *member_,
 goto out;
 }
 
+if (member->last_rx && member->status != LACP_CURRENT) {
+long long int delay = time_msec() - member->last_rx;
+VLOG_DBG("%s: %s recieved PDU after expiry, delayed by %lldms "
+ "seconds.", lacp->name, member->name, delay);
+}
+
+member->last_rx = time_msec();
 member->status = LACP_CURRENT;
 tx_rate = lacp->fast ? LACP_FAST_TIME_TX : LACP_SLOW_TIME_TX;
 timer_set_duration(>rx, LACP_RX_MULTIPLIER * tx_rate);
@@ -524,6 +533,11 @@ lacp_member_carrier_changed(const struct lacp *lacp, const 
void *member_,
 
 if (member->status == LACP_CURRENT || member->lacp->active) {
 member_set_expired(member);
+VLOG_DBG("%s: Expiring LACP due to %s carrier change.",
+ lacp->name, member->name);
+/* Do not warn about long LACP RX/TX interval if interface was down */
+member->last_rx = 0;
+member->last_tx = 0;
 }
 
 if (member->carrier_up != carrier_up) {
@@ -603,6 +617,8 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) 
OVS_EXCLUDED(mutex)
 
 if (member->status == LACP_CURRENT) {
 member_set_expired(member);
+VLOG_DBG("%s: Expired member %s because LACP PDU was not "
+ "received on time.", lacp->name, member->name);
 member->count_link_expired++;
 } else if (member->status == LACP_EXPIRED) {
 member_set_defaulted(member);
@@ -642,6 +658,13 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) 
OVS_EXCLUDED(mutex)
 ? LACP_FAST_TIME_TX
 : LACP_SLOW_TIME_TX);
 
+/* Log if we exceed the tx timer by double the tx rate. */
+if (member->last_tx &&
+(time_msec() - member->last_tx) > (duration * 2)) {
+VLOG_INFO("%s: Member %s failed to send LACP PCU on time.",
+  lacp->name, member->name);
+}
+member->last_tx = time_msec();
 timer_set_duration(>tx, duration);
 seq_change(connectivity_seq_get());
 }
@@ -800,6 +823,7 @@ member_set_expired(struct member *member) 
OVS_REQUIRES(mutex)
 member->partner.state &= ~LACP_STATE_SYNC;
 
 timer_set_duration(>rx, LACP_RX_MULTIPLIER * LACP_FAST_TIME_TX);
+member->last_tx = 0;
 }
 
 static void
-- 
2.39.3

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 2/2] ovn-ic: support learning routes in same AZ

2023-08-02 Thread Mark Michelson


Looks good to me.

Acked-by: Mark Michelson 

Having said that, I'd like for some OVN devs that are more familiar with 
the ins and outs of ovn-ic to make the determination if this is behavior 
that we want or not. For non-IC OVN, routers learn routes to their 
neighbor routers automatically, unless the "dynamic_neigh_routers" 
option is enabled. For ovn-ic, do we have a way to ensure that routers 
do not learn neighbor routes if they do not want to?


On 8/1/23 03:22, maximkorezkij via dev wrote:

when connecting multiple logical routers to a transit switch per az then
previously the routers in the same az would not learn each others
routes while the routers in the others az would learn all of them.

As this is confusing and would require each user to have additional
logical that configures static routing within each az.

Signed-off-by: Maxim Korezkij 
Signed-off-by: Felix Huettner 
---
  ic/ovn-ic.c | 52 -
  tests/ovn-ic.at |  2 ++
  2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 49850954f..cea14d008 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -861,6 +861,8 @@ struct ic_route_info {
  const char *origin;
  const char *route_table;

+const struct nbrec_logical_router *nb_lr;
+
  /* Either nb_route or nb_lrp is set and the other one must be NULL.
   * - For a route that is learned from IC-SB, or a static route that is
   *   generated from a route that is configured in NB, the "nb_route"
@@ -937,7 +939,8 @@ parse_route(const char *s_prefix, const char *s_nexthop,
  /* Return false if can't be added due to bad format. */
  static bool
  add_to_routes_learned(struct hmap *routes_learned,
-  const struct nbrec_logical_router_static_route *nb_route)
+  const struct nbrec_logical_router_static_route *nb_route,
+  const struct nbrec_logical_router *nb_lr)
  {
  struct in6_addr prefix, nexthop;
  unsigned int plen;
@@ -959,6 +962,7 @@ add_to_routes_learned(struct hmap *routes_learned,
  ic_route->nb_route = nb_route;
  ic_route->origin = origin;
  ic_route->route_table = nb_route->route_table;
+ic_route->nb_lr = nb_lr;
  hmap_insert(routes_learned, _route->node,
  ic_route_hash(, plen, , origin,
nb_route->route_table));
@@ -1099,7 +1103,8 @@ add_to_routes_ad(struct hmap *routes_ad, const struct 
in6_addr prefix,
   unsigned int plen, const struct in6_addr nexthop,
   const char *origin, const char *route_table,
   const struct nbrec_logical_router_port *nb_lrp,
- const struct nbrec_logical_router_static_route *nb_route)
+ const struct nbrec_logical_router_static_route *nb_route,
+ const struct nbrec_logical_router *nb_lr)
  {
  if (route_table == NULL) {
  route_table = "";
@@ -1117,6 +1122,7 @@ add_to_routes_ad(struct hmap *routes_ad, const struct 
in6_addr prefix,
  ic_route->origin = origin;
  ic_route->route_table = route_table;
  ic_route->nb_lrp = nb_lrp;
+ic_route->nb_lr = nb_lr;
  hmap_insert(routes_ad, _route->node, hash);
  } else {
  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
@@ -1130,6 +1136,7 @@ static void
  add_static_to_routes_ad(
  struct hmap *routes_ad,
  const struct nbrec_logical_router_static_route *nb_route,
+const struct nbrec_logical_router *nb_lr,
  const struct lport_addresses *nexthop_addresses,
  const struct smap *nb_options)
  {
@@ -1172,14 +1179,15 @@ add_static_to_routes_ad(
  }

  add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_STATIC,
- nb_route->route_table, NULL, nb_route);
+ nb_route->route_table, NULL, nb_route, nb_lr);
  }

  static void
  add_network_to_routes_ad(struct hmap *routes_ad, const char *network,
   const struct nbrec_logical_router_port *nb_lrp,
   const struct lport_addresses *nexthop_addresses,
- const struct smap *nb_options)
+ const struct smap *nb_options,
+ const struct nbrec_logical_router *nb_lr)
  {
  struct in6_addr prefix, nexthop;
  unsigned int plen;
@@ -1218,7 +1226,7 @@ add_network_to_routes_ad(struct hmap *routes_ad, const 
char *network,

  /* directly-connected routes go to  route table */
  add_to_routes_ad(routes_ad, prefix, plen, nexthop, ROUTE_ORIGIN_CONNECTED,
- NULL, nb_lrp, NULL);
+ NULL, nb_lrp, NULL, nb_lr);
  }

  static bool
@@ -1322,7 +1330,6 @@ lrp_is_ts_port(struct ic_context *ctx, struct 
ic_router_info *ic_lr,

  static void
  sync_learned_routes(struct ic_context *ctx,
-const struct icsbrec_availability_zone *az,

Re: [ovs-dev] [PATCH ovn 1/2] ovn-ic fix multiple routers in an az

2023-08-02 Thread Mark Michelson


Hi Maxim, thanks for the patch.

When submitting follow-up patches, please be sure to indicate the 
version of the patch in the subject line (e.g. "PATCH ovn v2"). I'm 
using the email date to assume this is the set of patches I should be 
reviewing.


I have a couple of findings down below.

On 8/1/23 03:22, Maxim Korezkij via dev wrote:

previously if multiple routers in the same az are connected to the same
transit switch then ovn-ic would only propagate the routes of one of
these routers to the ic-sb.
This commit fixes this behaviour and allows multiple routers in a single
az to use route advertisements.

Signed-off-by: Maxim Korezkij 
Signed-off-by: Felix Huettner 
---
  ic/ovn-ic.c | 26 +++
  tests/ovn-ic.at | 86 +
  2 files changed, 105 insertions(+), 7 deletions(-)

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 6f31037ec..49850954f 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -1583,8 +1583,8 @@ build_ts_routes_to_adv(struct ic_context *ctx,

  static void
  advertise_lr_routes(struct ic_context *ctx,


Patch 2 changes the name of this function to "collect_lr_routes". I 
think that change should be done in this patch since 
"advertise_lr_routes" isn't actually advertising the routes anymore.



-const struct icsbrec_availability_zone *az,
-struct ic_router_info *ic_lr)
+struct ic_router_info *ic_lr,
+struct shash *routes_ad_by_ts)
  {
  const struct nbrec_nb_global *nb_global =
  nbrec_nb_global_first(ctx->ovnnb_idl);
@@ -1595,7 +1595,7 @@ advertise_lr_routes(struct ic_context *ctx,
  struct lport_addresses ts_port_addrs;
  const struct icnbrec_transit_switch *key;

-struct hmap routes_ad = HMAP_INITIALIZER(_ad);
+struct hmap *routes_ad;
  for (int i = 0; i < ic_lr->n_isb_pbs; i++) {
  isb_pb = ic_lr->isb_pbs[i];
  key = icnbrec_transit_switch_index_init_row(
@@ -1604,6 +1604,12 @@ advertise_lr_routes(struct ic_context *ctx,
  ts_name = icnbrec_transit_switch_index_find(
  ctx->icnbrec_transit_switch_by_name, key)->name;
  icnbrec_transit_switch_index_destroy_row(key);
+routes_ad = shash_find_data(routes_ad_by_ts, ts_name);
+if (!routes_ad) {
+routes_ad = xzalloc(sizeof *routes_ad);
+hmap_init(routes_ad);
+shash_add(routes_ad_by_ts, ts_name, routes_ad);
+}

  if (!extract_lsp_addresses(isb_pb->address, _port_addrs)) {
  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
@@ -1615,12 +1621,10 @@ advertise_lr_routes(struct ic_context *ctx,
  }
  lrp_name = get_lrp_name_by_ts_port_name(ctx, isb_pb->logical_port);
  route_table = get_route_table_by_lrp_name(ctx, lrp_name);
-build_ts_routes_to_adv(ctx, ic_lr, _ad, _port_addrs,
+build_ts_routes_to_adv(ctx, ic_lr, routes_ad, _port_addrs,
 nb_global, route_table);
-advertise_routes(ctx, az, ts_name, _ad);
  destroy_lport_addresses(_port_addrs);
  }
-hmap_destroy(_ad);
  }

  static void
@@ -1721,14 +1725,22 @@ route_run(struct ic_context *ctx,
  icsbrec_port_binding_index_destroy_row(isb_pb_key);

  struct ic_router_info *ic_lr;
+struct shash routes_ad_by_ts = SHASH_INITIALIZER(_ad_by_ts);
  HMAP_FOR_EACH_SAFE (ic_lr, node, _lrs) {
-advertise_lr_routes(ctx, az, ic_lr);
+advertise_lr_routes(ctx, ic_lr, _ad_by_ts);
  sync_learned_routes(ctx, az, ic_lr);
  free(ic_lr->isb_pbs);
  hmap_destroy(_lr->routes_learned);
  hmap_remove(_lrs, _lr->node);
  free(ic_lr);
  }
+struct shash_node *node;
+SHASH_FOR_EACH_SAFE (node, _ad_by_ts) {
+advertise_routes(ctx, az, node->name, node->data);
+hmap_destroy(node->data);
+shash_delete(_ad_by_ts, node);
+}


You need to free(node->data) in this loop. Or as an alternative, you can 
call shash_delete_free_data() instead of shash_delete() at the end of 
the loop.



+shash_destroy(_ad_by_ts);
  hmap_destroy(_lrs);
  }

diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index ceee45092..4b470d5c8 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -1159,3 +1159,89 @@ AT_CHECK([ovn_as az2 ovn-nbctl lr-route-list lr12 | grep 
dst-ip | sort], [0], [d

  AT_CLEANUP
  ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-ic -- route sync -- multiple logical routers])
+
+ovn_init_ic_db
+ovn-ic-nbctl ts-add ts1
+
+for i in 1 2; do
+ovn_start az$i
+ovn_as az$i
+
+# Enable route learning at AZ level
+ovn-nbctl set nb_global . options:ic-route-learn=true
+# Enable route advertising at AZ level
+ovn-nbctl set nb_global . options:ic-route-adv=true
+done
+
+# Create new transit switches and LRs. Test topology is next:
+#
+# logical router (lr11) - transit switch (ts1) - logical router (lr21)
+#

[ovs-dev] [PATCH] ovsdb-tool: Fix json leak while showing clustered log.

2023-08-02 Thread Ilya Maximets

The json read from file is never freed in ovsdb-tool show-log
for a clustered database:

 ERROR: LeakSanitizer: detected memory leaks

 Direct leak of 10774760 byte(s) in 269369 object(s) allocated from:
0 0x50cc32 in malloc (ovsdb/ovsdb-tool+0x50cc32)
1 0x6e7b6b in xmalloc__ lib/util.c:140:15
2 0x6e7b6b in xmalloc lib/util.c:175:12
3 0x6494f6 in json_create lib/json.c:1489:25
4 0x64a8a7 in json_object_create lib/json.c:263:25
5 0x6525f3 in json_parser_push_object lib/json.c:1311:25
6 0x6525f3 in json_parser_input lib/json.c:1409:13
7 0x64f6c4 in json_parser_feed lib/json.c:1126:17
8 0x5694b5 in parse_body ovsdb/log.c:412:9
9 0x5694b5 in ovsdb_log_read ovsdb/log.c:477:13
   10 0x54d294 in do_show_log_cluster ovsdb/ovsdb-tool.c:1069:27
   11 0x54d294 in do_show_log ovsdb/ovsdb-tool.c:1115:9
   12 0x63b7b1 in ovs_cmdl_run_command__ lib/command-line.c:247:17
   13 0x5488a5 in main ovsdb/ovsdb-tool.c:82:5
   14 0xe0eb49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49)
   15 0xe0ec0a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x27c0a)
   16 0x471fe4 in _start (ovsdb/ovsdb-tool+0x471fe4)

Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered 
databases.")
Reported-by: Dumitru Ceara 
Signed-off-by: Ilya Maximets 
---
 ovsdb/ovsdb-tool.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index e26536532..facd680ff 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -1094,6 +1094,7 @@ do_show_log_cluster(struct ovsdb_log *log)
 free(s);
 }
 
+json_destroy(json);
 putchar('\n');
 }
 
-- 
2.40.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovsdb-server: Fix excessive memory usage on DB open.

2023-08-02 Thread Ilya Maximets

During initial read of a database file all the file transactions are
added to the transaction history.  The history run with the history
size checks is only executed after the whole file is processed.
If, for some reason, the file contains way too many transactions,
this behavior may result in excessive memory consumption up to
hundreds of GBs.  For example, here is a log entry about memory usage
after reading a file with 100K+ OVN NbDB transactions:

  |4|memory|INFO|95650400 kB peak resident set size after 96.9 seconds
  |5|memory|INFO|atoms:3083346 cells:1838767 monitors:0
raft-log:123309 txn-history:123307 txn-history-atoms:1647022868

In this particular case ovsdb-server allocated 95 GB of RAM in order
to accommodate 1.6 billion ovsdb atoms in the history, while only 3
million atoms are in the actual database.

Fix that by running history size checks after applying each file
transaction.  This way the memory usage while reading the database
from the example stays at about 1 GB mark.  History size checks are
cheap in comparison with transaction replay, so the additional calls
do not reduce performance.

We could've just moved the history run into ovsdb_txn_replay_commit(),
but it seems more organic to call it externally, since we have init()
and destroy() functions called externally as well.

Since the history run will be executed shortly after reading the
database and actual memory consumption peak is not always logged,
there seem to be no reliable way to unit test for the issue without
adding extra testing infrastructure into the code.

Fixes: 695e81502794 ("ovsdb-server: Transaction history tracking.")
Reported-at: https://bugzilla.redhat.com/2228464
Signed-off-by: Ilya Maximets 
---
 ovsdb/ovsdb-server.c | 3 ++-
 ovsdb/relay.c| 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 8e623118b..cf09c9079 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -235,7 +235,7 @@ main_loop(struct server_config *config,
 
 SHASH_FOR_EACH_SAFE (node, all_dbs) {
 struct db *db = node->data;
-ovsdb_txn_history_run(db->db);
+
 ovsdb_storage_run(db->db->storage);
 read_db(config, db);
 /* Run triggers after storage_run and read_db to make sure new raft
@@ -678,6 +678,7 @@ parse_txn(struct server_config *config, struct db *db,
 if (!error && !uuid_is_zero(txnid)) {
 db->db->prereq = *txnid;
 }
+ovsdb_txn_history_run(db->db);
 }
 return error;
 }
diff --git a/ovsdb/relay.c b/ovsdb/relay.c
index b035cb492..27ff196b7 100644
--- a/ovsdb/relay.c
+++ b/ovsdb/relay.c
@@ -413,6 +413,7 @@ ovsdb_relay_run(void)
 }
 ovsdb_cs_event_destroy(event);
 }
+ovsdb_txn_history_run(ctx->db);
 }
 }
 
-- 
2.40.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] system-tests: Wait for all interfaces to have IPv6

2023-08-02 Thread Ales Musil

The "SNAT in gateway router mode" could occasionally
fail, because the ext1 IPv6 address wasn't ready and
the NA packet arrived too late for the ping to succeed.

Signed-off-by: Ales Musil 
---
 tests/system-ovn.at | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index fef3cfbf0..766a250e5 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -8691,7 +8691,8 @@ ADD_VETH(ext1, ext1, br0, "172.16.1.1/24", 
"00:ee:00:01:01:01", \
 check ovn-nbctl --wait=hv sync
 wait_for_ports_up
 OVS_WAIT_UNTIL([test "$(ip netns exec ls1p1 ip a | grep 2001::1 | grep 
tentative)" = ""])
-OVS_WAIT_UNTIL([test "$(ip netns exec ls1p2 ip a | grep 2002::1 | grep 
tentative)" = ""])
+OVS_WAIT_UNTIL([test "$(ip netns exec ls1p2 ip a | grep 2002::2 | grep 
tentative)" = ""])
+OVS_WAIT_UNTIL([test "$(ip netns exec ext1 ip a | grep 1711::1 | grep 
tentative)" = ""])
 
 NS_CHECK_EXEC([ls1p1], [ping -q -c 3 -i 0.3 -w 2  172.16.1.1 | FORMAT_PING], \
 [0], [dnl
-- 
2.41.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] conntrack: Allow flush of SCTP protocol

2023-08-02 Thread Ales Musil

The SCTP protocol ports were excluded from
the netlink encoding. Which resulted in the
lookup failure in kernel, leading to the entry
not being flushed. Allow the flush of SCTP protocol
based on port numbers.

Signed-off-by: Ales Musil 
---
 lib/netlink-conntrack.c |  3 ++-
 tests/system-traffic.at | 26 ++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 4fcde9ba1..492bfcffb 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -579,7 +579,8 @@ nl_ct_put_tuple_proto(struct ofpbuf *buf, const struct 
ct_dpif_tuple *tuple)
 nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_TYPE, tuple->icmp_type);
 nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_CODE, tuple->icmp_code);
 } else if (tuple->ip_proto == IPPROTO_TCP ||
-   tuple->ip_proto == IPPROTO_UDP) {
+   tuple->ip_proto == IPPROTO_UDP ||
+   tuple->ip_proto == IPPROTO_SCTP) {
 nl_msg_put_be16(buf, CTA_PROTO_SRC_PORT, tuple->src_port);
 nl_msg_put_be16(buf, CTA_PROTO_DST_PORT, tuple->dst_port);
 } else {
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 945037ec0..78e2f9ab9 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2516,6 +2516,7 @@ AT_CLEANUP
 
 AT_SETUP([conntrack - ct flush])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_SCTP()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
@@ -2526,10 +2527,8 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
 AT_DATA([flows.txt], [dnl
 priority=1,action=drop
 priority=10,arp,action=normal
-priority=100,in_port=1,udp,action=ct(commit),2
-priority=100,in_port=2,udp,action=ct(zone=5,commit),1
-priority=100,in_port=1,icmp,action=ct(commit),2
-priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
+priority=100,in_port=1,ip,action=ct(commit),2
+priority=100,in_port=2,ip,action=ct(zone=5,commit),1
 ])
 
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
@@ -2692,6 +2691,25 @@ 
udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
 
 AT_CHECK([FLUSH_CMD])
 
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
+
+dnl Test SCTP flush based on port
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a50540009080045340001408464410a0101010a010102000100029178f7d3011470e18ccc000a000a
 actions=resubmit(,0)"])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 
packet=505400095054000a080045340001408464410a0101020a0101010002000198f29e47011470e18ccc000a000a
 actions=resubmit(,0)"])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed 
"s/,protoinfo=.*$//" | sort], [0], [dnl
+sctp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
+sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
+])
+
+AT_CHECK([FLUSH_CMD 
'ct_nw_src=10.1.1.1,ct_nw_proto=132,ct_tp_src=1,ct_tp_dst=2'])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed 
"s/,protoinfo=.*$//" | sort], [0], [dnl
+sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
+])
+
+AT_CHECK([FLUSH_CMD 
'ct_nw_src=10.1.1.2,ct_nw_proto=132,ct_tp_src=2,ct_tp_dst=1'])
+
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
 ])
 
-- 
2.41.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 2/2] netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite

2023-08-02 Thread Faicker Mo via dev

The warning message is
|1|tc(handler4)|WARN|can't offload rewrite of IP/IPV6 with ip_proto: X.

IPIP and GRE only need the checksum recalculation of the IP header if the
IP header is rewritten.

Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc")
Signed-off-by: Faicker Mo 
---
 lib/tc.c |  4 +++-
 tests/system-offloads-traffic.at | 34 
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/lib/tc.c b/lib/tc.c
index 52a74d9d0..a8cb86371 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2973,7 +2973,9 @@ csum_update_flag(struct tc_flower *flower,
 } else if (flower->key.ip_proto == IPPROTO_UDP) {
 flower->needs_full_ip_proto_mask = true;
 flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDP;
-} else if (flower->key.ip_proto == IPPROTO_ICMP) {
+} else if (flower->key.ip_proto == IPPROTO_ICMP ||
+   flower->key.ip_proto == IPPROTO_IPIP ||
+   flower->key.ip_proto == IPPROTO_GRE) {
 flower->needs_full_ip_proto_mask = true;
 } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
 flower->needs_full_ip_proto_mask = true;
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 7215e36e2..2ff74480d 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -855,3 +855,37 @@ AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | 
grep "eth_type(0x0800)
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

+AT_SETUP([offloads - Add IPIP/GRE protocols to offload in ip rewrite - 
offloads enabled])
+OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . 
other_config:hw-offload=true])
+
+AT_CHECK([ovs-ofctl add-flow br0 "priority=0 actions=normal"])
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Set up the ip field modify flow.
+AT_CHECK([ovs-ofctl add-flow br0 "priority=100 
in_port=ovs-p0,ip,nw_dst=10.1.1.2 actions=dec_ttl,output:ovs-p1"])
+AT_CHECK([ovs-ofctl add-flow br0 "priority=100 
in_port=ovs-p1,ip,nw_dst=10.1.1.1 actions=dec_ttl,output:ovs-p0"])
+
+dnl Set up ipip tunnel in NS.
+NS_CHECK_EXEC([at_ns0], [ip tunnel add ipip0 remote 10.1.1.2 2>/dev/null], [0])
+NS_CHECK_EXEC([at_ns0], [ip link set dev ipip0 up 2>/dev/null], [0])
+NS_CHECK_EXEC([at_ns0], [ip addr add dev ipip0 192.168.1.1/30 2>/dev/null], 
[0])
+NS_CHECK_EXEC([at_ns1], [ip tunnel add ipip0 remote 10.1.1.1 2>/dev/null], [0])
+NS_CHECK_EXEC([at_ns1], [ip link set dev ipip0 up 2>/dev/null], [0])
+NS_CHECK_EXEC([at_ns1], [ip addr add dev ipip0 192.168.1.2/30 2>/dev/null], 
[0])
+
+dnl Check the tunnel.
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 192.168.1.2 | FORMAT_PING], 
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+dnl Check the offloaded flow.
+AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep 
"eth_type(0x0800)" | wc -l], [0], [dnl
+2
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
--
2.39.3





___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/2] netdev-tc-offload: Add csum offload of protocols IGMP/UDPLITE/SCTP

2023-08-02 Thread Faicker Mo via dev

Add tc csum offload support of protocols IGMP/UDPLITE/SCTP

Signed-off-by: Faicker Mo 
---
 lib/tc.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/lib/tc.c b/lib/tc.c
index f49048cda..52a74d9d0 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2978,6 +2978,15 @@ csum_update_flag(struct tc_flower *flower,
 } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
 flower->needs_full_ip_proto_mask = true;
 flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP;
+} else if (flower->key.ip_proto == IPPROTO_IGMP) {
+flower->needs_full_ip_proto_mask = true;
+flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP;
+} else if (flower->key.ip_proto == IPPROTO_UDPLITE) {
+flower->needs_full_ip_proto_mask = true;
+flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE;
+} else if (flower->key.ip_proto == IPPROTO_SCTP) {
+flower->needs_full_ip_proto_mask = true;
+flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP;
 } else {
 VLOG_WARN_RL(_rl,
  "can't offload rewrite of IP/IPV6 with ip_proto: %d",
--
2.39.3





___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 0/2] Add more protocols to offload in ip rewrite

2023-08-02 Thread Faicker Mo via dev

v2:
  - seperated into two commits.
  - add the specific protocols to offload in ip rewrite.

Faicker Mo (2):
  netdev-tc-offload: Add csum offload of protocols IGMP/UDPLITE/SCTP
  netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite

 lib/tc.c | 13 +++-
 tests/system-offloads-traffic.at | 34 
 2 files changed, 46 insertions(+), 1 deletion(-)

--
2.39.3





___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 3/3] python: Use build to generate PEP517 compatible archives.

2023-08-02 Thread Robin Jarry

Quoting Paul Ganssle, setuptools maintainer:
> * The setuptools project has stopped maintaining all direct
>   invocations of setup.py years ago, and distutils is deprecated.
>   There are undoubtedly many ways that your setup.py-based system is
>   broken today, even if it's not failing loudly or obviously.
>
> * Direct invocations of setup.py cannot bootstrap their own
>   dependencies, and so some CLI is necessary for dependency
>   management.
>
> * The setuptools project no longer wants to provide any public CLI,
>   and will be actively removing the existing interface (though the
>   time scale for this is long).
>
> * PEP 517, 518 and other standards-based packaging are the future of
>   the Python ecosystem and a lot of progress has been made on making
>   this upgrade seamless.

As described in the recommendations in the end of the article:
`python3 setup.py sdist` should be replaced by `python3 -m build
--sdist`

Link: https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html#summary
Signed-off-by: Robin Jarry 
---
 python/automake.mk | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/python/automake.mk b/python/automake.mk
index 8854e656a5ae..19d9d91119c2 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -101,10 +101,11 @@ ovs-install-data-local:
rm python/ovs/dirs.py.tmp
 
 python-sdist: $(srcdir)/python/ovs/version.py $(ovs_pyfiles) python/ovs/dirs.py
-   (cd python/ && $(PYTHON3) setup.py sdist)
+   cd python/ && $(PYTHON3) -m build --sdist
 
 pypi-upload: $(srcdir)/python/ovs/version.py $(ovs_pyfiles) python/ovs/dirs.py
-   (cd python/ && $(PYTHON3) setup.py sdist && twine upload 
dist/ovs-$(VERSION).tar.gz)
+   cd python/ && $(PYTHON3) -m build --sdist && twine upload 
dist/ovs-$(VERSION).tar.gz
+
 install-data-local: ovs-install-data-local
 
 UNINSTALL_LOCAL += ovs-uninstall-local
-- 
2.41.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 2/3] python: Use twine to upload sdist package to pypi.org.

2023-08-02 Thread Robin Jarry

setup.py upload is now deprecated. When used, pypi.org returns an error:

> Upload failed (400): Invalid value for blake2_256_digest. Error: Use
> a valid, hex-encoded, BLAKE2 message digest.

Use twine which is the recommended replacement tool to upload on
pypi.org.

Link: https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html#summary
Reported-by: Terry Wilson 
Signed-off-by: Robin Jarry 
---
 python/automake.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/automake.mk b/python/automake.mk
index 8b6266de214e..8854e656a5ae 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -104,7 +104,7 @@ python-sdist: $(srcdir)/python/ovs/version.py 
$(ovs_pyfiles) python/ovs/dirs.py
(cd python/ && $(PYTHON3) setup.py sdist)
 
 pypi-upload: $(srcdir)/python/ovs/version.py $(ovs_pyfiles) python/ovs/dirs.py
-   (cd python/ && $(PYTHON3) setup.py sdist upload)
+   (cd python/ && $(PYTHON3) setup.py sdist && twine upload 
dist/ovs-$(VERSION).tar.gz)
 install-data-local: ovs-install-data-local
 
 UNINSTALL_LOCAL += ovs-uninstall-local
-- 
2.41.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/3] python: Move build related code into build-aux.

2023-08-02 Thread Robin Jarry

The python/build folder contents are completely unrelated to the ovs
python bindings. These files are only used during the build for various
subsystems (docs, man pages, code generation, etc.).

Having that folder in that location prevents from running:

cd python && python3 -m build

Which is a way to generate PEP517 compatible source archives and binary
wheel packages.

Move that folder into build-aux. Update PYTHONPATH accordingly.

Link: https://peps.python.org/pep-0517/
Link: https://pypi.org/project/build/
Signed-off-by: Robin Jarry 
---
 Makefile.am   |  6 +++---
 build-aux/automake.mk |  8 
 {python => build-aux}/build/__init__.py   |  0
 {python => build-aux}/build/extract_ofp_fields.py |  0
 {python => build-aux}/build/nroff.py  |  0
 {python => build-aux}/build/soutil.py |  0
 python/automake.mk| 12 
 7 files changed, 11 insertions(+), 15 deletions(-)
 rename {python => build-aux}/build/__init__.py (100%)
 rename {python => build-aux}/build/extract_ofp_fields.py (100%)
 rename {python => build-aux}/build/nroff.py (100%)
 rename {python => build-aux}/build/soutil.py (100%)

diff --git a/Makefile.am b/Makefile.am
index db341504d37f..fca138ea9fd6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -58,7 +58,7 @@ endif
 # foo/__init__.py into an (older) version with plain foo.py, since
 # foo/__init__.pyc will cause Python to ignore foo.py.
 run_python = \
-   PYTHONPATH=$(top_srcdir)/python$(psep)$$PYTHONPATH \
+   
PYTHONPATH=$(top_srcdir)/python$(psep)$(top_srcdir)/build-aux$(psep)$$PYTHONPATH
 \
PYTHONDONTWRITEBYTECODE=yes $(PYTHON3)
 
 ALL_LOCAL =
@@ -151,7 +151,7 @@ ro_shell = printf '\043 Generated automatically -- do not 
modify!-*- buffer-
 
 SUFFIXES += .in
 .in:
-   $(AM_V_GEN)PYTHONPATH=$$PYTHONPATH$(psep)$(srcdir)/python $(PYTHON3) 
$(srcdir)/build-aux/soexpand.py -I$(srcdir) < $< | \
+   $(AM_V_GEN)$(PYTHON3) $(srcdir)/build-aux/soexpand.py -I$(srcdir) < $< 
| \
  $(PYTHON3) $(srcdir)/build-aux/dpdkstrip.py $(DPDKSTRIP_FLAGS) | \
  sed \
-e 's,[@]PKIDIR[@],$(PKIDIR),g' \
@@ -416,7 +416,7 @@ CLEANFILES += flake8-check
 
 -include manpages.mk
 manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py python/build/soutil.py
-   @PYTHONPATH=$$PYTHONPATH$(psep)$(srcdir)/python $(PYTHON3) 
$(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) $(MAN_ROOTS) >$(@F).tmp
+   @$(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -I$(srcdir) 
$(MAN_ROOTS) >$(@F).tmp
@if cmp -s $(@F).tmp $@; then \
  touch $@; \
  rm -f $(@F).tmp; \
diff --git a/build-aux/automake.mk b/build-aux/automake.mk
index b9a77a51cfed..b4426d7b23dd 100644
--- a/build-aux/automake.mk
+++ b/build-aux/automake.mk
@@ -1,4 +1,8 @@
 EXTRA_DIST += \
+   build-aux/build/__init__.py \
+   build-aux/build/extract_ofp_fields.py \
+   build-aux/build/nroff.py \
+   build-aux/build/soutil.py \
build-aux/calculate-schema-cksum \
build-aux/cccl \
build-aux/cksum-schema-check \
@@ -13,6 +17,10 @@ EXTRA_DIST += \
build-aux/xml2nroff
 
 FLAKE8_PYFILES += \
+build-aux/build/__init__.py \
+build-aux/build/extract_ofp_fields.py \
+build-aux/build/nroff.py \
+build-aux/build/soutil.py \
 build-aux/dpdkstrip.py \
 build-aux/gen_ofp_field_decoders \
 build-aux/sodepends.py \
diff --git a/python/build/__init__.py b/build-aux/build/__init__.py
similarity index 100%
rename from python/build/__init__.py
rename to build-aux/build/__init__.py
diff --git a/python/build/extract_ofp_fields.py 
b/build-aux/build/extract_ofp_fields.py
similarity index 100%
rename from python/build/extract_ofp_fields.py
rename to build-aux/build/extract_ofp_fields.py
diff --git a/python/build/nroff.py b/build-aux/build/nroff.py
similarity index 100%
rename from python/build/nroff.py
rename to build-aux/build/nroff.py
diff --git a/python/build/soutil.py b/build-aux/build/soutil.py
similarity index 100%
rename from python/build/soutil.py
rename to build-aux/build/soutil.py
diff --git a/python/automake.mk b/python/automake.mk
index 82a50878741a..8b6266de214e 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -63,14 +63,6 @@ ovs_pytests = \
python/ovs/tests/test_odp.py \
python/ovs/tests/test_ofp.py
 
-# These python files are used at build time but not runtime,
-# so they are not installed.
-EXTRA_DIST += \
-   python/build/__init__.py \
-   python/build/extract_ofp_fields.py \
-   python/build/nroff.py \
-   python/build/soutil.py
-
 # PyPI support.
 EXTRA_DIST += \
python/ovs/compat/sortedcontainers/LICENSE \
@@ -88,10 +80,6 @@ PYCOV_CLEAN_FILES += $(PYFILES:.py=.py,cover)
 
 FLAKE8_PYFILES += \
$(filter-out python/ovs/compat/% python/ovs/dirs.py,$(PYFILES)) \
-   python/build/__init__.py \
-

[ovs-dev] [PATCH 0/3] Python ovsdb bindings distribution improvements

2023-08-02 Thread Robin Jarry

Hi all,

This is a small series to fix the python ovs package publication on
pypi.org.

Robin Jarry (3):
  python: Move build related code into build-aux.
  python: Use twine to upload sdist package to pypi.org.
  python: Use build to generate PEP517 compatible archives.

 Makefile.am |  6 +++---
 build-aux/automake.mk   |  8 
 {python => build-aux}/build/__init__.py |  0
 .../build/extract_ofp_fields.py |  0
 {python => build-aux}/build/nroff.py|  0
 {python => build-aux}/build/soutil.py   |  0
 python/automake.mk  | 17 +++--
 7 files changed, 14 insertions(+), 17 deletions(-)
 rename {python => build-aux}/build/__init__.py (100%)
 rename {python => build-aux}/build/extract_ofp_fields.py (100%)
 rename {python => build-aux}/build/nroff.py (100%)
 rename {python => build-aux}/build/soutil.py (100%)

-- 
2.41.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v4 6/8] northd: Handle load balancer group changes for a logical switch.

2023-08-02 Thread 0-day Robot

Bleep bloop.  Greetings Numan Siddique, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#80 FILE: northd/en-lb-data.c:307:
nbs->load_balancer[i], OVSDB_IDL_CHANGE_MODIFY) > 
0)) {

WARNING: Line is 80 characters long (recommended limit is 79)
#398 FILE: northd/northd.h:361:
   struct hmap *lb_group_datapaths_map);

Lines checked: 499, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v4 8/8] northd: Handle load balancer/group changes for a logical router.

2023-08-02 Thread numans

From: Numan Siddique 

When a logical router gets updated due to load balancer or load balancer
groups changes, it is now incrementally handled first in 'lb_data'
engine node similar to how logical switch changes are handled.  The
tracking data of 'lb_data' is updated similarly so that northd engine
handler - northd_handle_lb_data_changes_post_od() handles it.

A new handler northd_handle_lr_changes() is added in the 'northd' engine
node for logical router changes.  This handler returns true if only
load balancer or load balancer group columns are changed.  It returns
false for any other changes.

northd_handle_lb_data_changes_post_od() also sets the logical router
od's lb_ips accordingly.

Below are the scale testing results done with these patches applied
using ovn-heater.  The test ran the scenario  -
ocp-500-density-heavy.yml [1].

With these patches applied (with load balancer I-P handling in northd
engine node) the resuts are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1329292.1571033.314847
3.3315614.3786261.581889197.736147  125 0
Namespace.add_ports 0.0052170.0057600.006565
0.0133480.0210140.0061060.763214125 0
WorkerNode.bind_port0.0352050.0454580.052278
0.0598040.0639410.04565211.413122   250 0
WorkerNode.ping_port0.0050750.0068143.088548
3.1925774.2420260.726453181.613284  250 0
---

The results with the present main are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 4.3772606.4869627.502040
8.3225878.3347016.559002819.875306  125 0
Namespace.add_ports 0.0051120.0054840.005953
0.0091530.0114520.0056620.707752125 0
WorkerNode.bind_port0.0353600.0427320.049152
0.0536980.0566350.04321510.803700   250 0
WorkerNode.ping_port0.0053381.5999047.229649
7.7980398.2065373.209860802.464911  250 0
---

Few observations:

 - The total time taken has come down significantly from 819 seconds to 197.
 - 99%ile with these patches is 3.3 seconds compared to 8.3 seconds for the
   main.
 - 90%file with these patches is 3.3 seconds compared to 7.5 seconds for
   the main.
 - CPU utilization of northd during the test with these patches
   is between 100% to 300% which is almost the same as main.
   Main difference being that, with these patches the test duration is
   less and hence overall less CPU utilization.

[1] - 
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml

Signed-off-by: Numan Siddique 
---
 lib/lb.c |  46 +-
 lib/lb.h |   9 ++
 northd/en-lb-data.c  | 327 +++
 northd/en-lb-data.h  |  15 ++
 northd/en-northd.c   |  26 
 northd/en-northd.h   |   1 +
 northd/inc-proc-northd.c |   5 +-
 northd/northd.c  | 242 ++---
 northd/northd.h  |   3 +
 tests/ovn-northd.at  |  42 ++---
 10 files changed, 604 insertions(+), 112 deletions(-)

diff --git a/lib/lb.c b/lib/lb.c
index e6c9dc2be2..d0d562b6fb 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -794,6 +794,7 @@ ovn_lb_group_init(struct ovn_lb_group *lb_group,
 const struct uuid *lb_uuid =
 _lb_group->load_balancer[i]->header_.uuid;
 lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
+lb_group->has_routable_lb |= lb_group->lbs[i]->routable;
 }
 }
 
@@ -815,6 +816,7 @@ ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
 {

[ovs-dev] [PATCH ovn v4 7/8] northd: Sync SB Port bindings NAT column in a separate engine node.

2023-08-02 Thread numans

From: Numan Siddique 

A new engine node 'sync_to_sb_pb' is added within 'sync_to_sb'
node to sync NAT column of Port bindings table.  This separation
is required in order to add load balancer group I-P handling
in 'northd' engine node (which is handled in the next commit).

'sync_to_sb_pb' engine node can be later expanded to sync other
Port binding columns if required.

Signed-off-by: Numan Siddique 
---
 northd/en-sync-sb.c  |  31 +
 northd/en-sync-sb.h  |   4 +
 northd/inc-proc-northd.c |   8 +-
 northd/northd.c  | 243 +--
 northd/northd.h  |   2 +
 5 files changed, 174 insertions(+), 114 deletions(-)

diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index 9832fce30a..552ed56452 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -254,6 +254,37 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, 
void *data OVS_UNUSED)
 return false;
 }
 
+/* sync_to_sb_pb engine node functions.
+ * This engine node syncs the SB Port Bindings (partly).
+ * en_northd engine create the SB Port binding rows and
+ * updates most of the columns.
+ * This engine node updates the port binding columns which
+ * needs to be updated after northd engine is run.
+ */
+
+void *
+en_sync_to_sb_pb_init(struct engine_node *node OVS_UNUSED,
+  struct engine_arg *arg OVS_UNUSED)
+{
+return NULL;
+}
+
+void
+en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_data *northd_data = engine_get_input_data("northd", node);
+
+sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports);
+engine_set_node_state(node, EN_UPDATED);
+}
+
+void
+en_sync_to_sb_pb_cleanup(void *data OVS_UNUSED)
+{
+
+}
+
 /* static functions. */
 static void
 sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name,
diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h
index 06d2a57710..700d3340e4 100644
--- a/northd/en-sync-sb.h
+++ b/northd/en-sync-sb.h
@@ -22,4 +22,8 @@ void en_sync_to_sb_lb_run(struct engine_node *, void *data);
 void en_sync_to_sb_lb_cleanup(void *data);
 bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED);
 
+void *en_sync_to_sb_pb_init(struct engine_node *, struct engine_arg *);
+void en_sync_to_sb_pb_run(struct engine_node *, void *data);
+void en_sync_to_sb_pb_cleanup(void *data);
+
 #endif /* end of EN_SYNC_SB_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 402c94e88c..dc8b880fd8 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -141,6 +141,7 @@ static ENGINE_NODE(sync_to_sb_addr_set, 
"sync_to_sb_addr_set");
 static ENGINE_NODE(fdb_aging, "fdb_aging");
 static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
 static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
+static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb");
 static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
 
 void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
@@ -215,13 +216,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
  sync_to_sb_lb_northd_handler);
 engine_add_input(_sync_to_sb_lb, _sb_load_balancer, NULL);
 
+engine_add_input(_sync_to_sb_pb, _northd, NULL);
+
 /* en_sync_to_sb engine node syncs the SB database tables from
  * the NB database tables.
- * Right now this engine syncs the SB Address_Set table and
- * SB Load_Balancer table.
+ * Right now this engine syncs the SB Address_Set table,
+ * SB Load_Balancer table and (partly) SB Port_Binding table.
  */
 engine_add_input(_sync_to_sb, _sync_to_sb_addr_set, NULL);
 engine_add_input(_sync_to_sb, _sync_to_sb_lb, NULL);
+engine_add_input(_sync_to_sb, _sync_to_sb_pb, NULL);
 
 engine_add_input(_sync_from_sb, _northd,
  sync_from_sb_northd_handler);
diff --git a/northd/northd.c b/northd/northd.c
index 8d6fb2ea37..907bf0887b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3514,8 +3514,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 ds_destroy();
 
 sbrec_port_binding_set_external_ids(op->sb, >nbrp->external_ids);
-
-sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
 } else {
 if (!lsp_is_router(op->nbsp)) {
 uint32_t queue_id = smap_get_int(
@@ -3631,116 +3629,6 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 } else {
 sbrec_port_binding_set_options(op->sb, NULL);
 }
-const char *nat_addresses = smap_get(>nbsp->options,
-   "nat-addresses");
-size_t n_nats = 0;
-char **nats = NULL;
-bool l3dgw_ports = op->peer && op->peer->od &&
-   op->peer->od->n_l3dgw_ports;
-if (nat_addresses && !strcmp(nat_addresses, "router")) {
-if (op->peer && op->peer->od
-

[ovs-dev] [PATCH ovn v4 6/8] northd: Handle load balancer group changes for a logical switch.

2023-08-02 Thread numans

From: Numan Siddique 

For every a given load balancer group 'A', northd engine data maintains
a bitmap of datapaths associated to this lb group.  So when lb group 'A'
gets associated to a logical switch 's1', the bitmap index of 's1' is set
in its bitmap.

In order to handle the load balancer group changes incrementally for a
logical switch, we need to set and clear the bitmap bits accordingly.
And this patch does it.

Signed-off-by: Numan Siddique 
---
 northd/en-lb-data.c | 101 
 northd/en-lb-data.h |   4 ++
 northd/en-northd.c  |   9 +++-
 northd/northd.c |  64 
 northd/northd.h |   3 +-
 tests/ovn-northd.at |  30 +
 6 files changed, 164 insertions(+), 47 deletions(-)

diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
index e0c4db1422..0de8bfc29c 100644
--- a/northd/en-lb-data.c
+++ b/northd/en-lb-data.c
@@ -63,6 +63,7 @@ static struct crupdated_lb_group *
 static void add_deleted_lb_group_to_tracked_data(
 struct ovn_lb_group *, struct tracked_lb_data *);
 static bool is_ls_lbs_changed(const struct nbrec_logical_switch *nbs);
+static bool is_ls_lbgrps_changed(const struct nbrec_logical_switch *nbs);
 
 /* 'lb_data' engine node manages the NB load balancers and load balancer
  * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
@@ -271,12 +272,15 @@ lb_data_logical_switch_handler(struct engine_node *node, 
void *data)
 destroy_od_lb_data(od_lb_data);
 }
 } else {
-if (!is_ls_lbs_changed(nbs)) {
+bool ls_lbs_changed = is_ls_lbs_changed(nbs);
+bool ls_lbgrps_changed = is_ls_lbgrps_changed(nbs);
+if (!ls_lbs_changed && !ls_lbgrps_changed) {
 continue;
 }
 struct crupdated_od_lb_data *codlb = xzalloc(sizeof *codlb);
 codlb->od_uuid = nbs->header_.uuid;
 uuidset_init(>assoc_lbs);
+uuidset_init(>assoc_lbgrps);
 
 struct od_lb_data *od_lb_data =
 find_od_lb_data(_data->ls_lb_map, >header_.uuid);
@@ -285,38 +289,65 @@ lb_data_logical_switch_handler(struct engine_node *node, 
void *data)
 >header_.uuid);
 }
 
-struct uuidset *pre_lb_uuids = od_lb_data->lbs;
-od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
-uuidset_init(od_lb_data->lbs);
-
-for (size_t i = 0; i < nbs->n_load_balancer; i++) {
-const struct uuid *lb_uuid =
->load_balancer[i]->header_.uuid;
-uuidset_insert(od_lb_data->lbs, lb_uuid);
+if (ls_lbs_changed) {
+struct uuidset *pre_lb_uuids = od_lb_data->lbs;
+od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
+uuidset_init(od_lb_data->lbs);
+
+for (size_t i = 0; i < nbs->n_load_balancer; i++) {
+const struct uuid *lb_uuid =
+>load_balancer[i]->header_.uuid;
+uuidset_insert(od_lb_data->lbs, lb_uuid);
+
+struct uuidset_node *unode = uuidset_find(pre_lb_uuids,
+lb_uuid);
+
+if (!unode || (nbrec_load_balancer_row_get_seqno(
+nbs->load_balancer[i], OVSDB_IDL_CHANGE_MODIFY) > 
0)) {
+/* Add this lb to the tracked data. */
+uuidset_insert(>assoc_lbs, lb_uuid);
+changed = true;
+}
+
+if (unode) {
+uuidset_delete(pre_lb_uuids, unode);
+}
+}
+if (!uuidset_is_empty(pre_lb_uuids)) {
+trk_lb_data->has_dissassoc_lbs_from_od = true;
+changed = true;
+}
 
-struct uuidset_node *unode = uuidset_find(pre_lb_uuids,
-  lb_uuid);
+uuidset_destroy(pre_lb_uuids);
+free(pre_lb_uuids);
+}
 
-if (!unode || (nbrec_load_balancer_row_get_seqno(
-nbs->load_balancer[i], OVSDB_IDL_CHANGE_MODIFY) > 0)) {
-/* Add this lb to the tracked data. */
-uuidset_insert(>assoc_lbs, lb_uuid);
-changed = true;
+if (ls_lbgrps_changed) {
+struct uuidset *pre_lbgrp_uuids = od_lb_data->lbgrps;
+od_lb_data->lbgrps = xzalloc(sizeof *od_lb_data->lbgrps);
+uuidset_init(od_lb_data->lbgrps);
+for (size_t i = 0; i < nbs->n_load_balancer_group; i++) {
+const struct uuid *lbg_uuid =
+>load_balancer_group[i]->header_.uuid;
+

[ovs-dev] [PATCH ovn v4 5/8] northd: Handle load balancer changes for a logical switch.

2023-08-02 Thread numans

From: Numan Siddique 

'lb_data' engine node now also handles logical switch changes.
Its data maintains ls to lb related information. i.e if a
logical switch sw0 has lb1, lb2 and lb3 associated then
it stores this info in its data.  And when a new load balancer
lb4 is associated to it, it stores this information in its
tracked data so that 'northd' engine node can handle it
accordingly.  Tracked data will have information like:
  changed ls -> {sw0 : {associated_lbs: [lb4]}

In the 'northd' engne node, an additional handler for 'lb_data'
is added after the 'nbrec_logical_switch' changes.  With this patch
we now have 2 handlers for 'lb_data' in 'northd' engine node.


engine_add_input(_northd, _lb_data, northd_lb_data_handler_pre_od);
engine_add_input(_northd, _nb_logical_switch,
 northd_nb_logical_switch_handler);
engine_add_input(_northd, _lb_data, northd_lb_data_handler_post_od);


The first handler 'northd_lb_data_handler_pre_od' is called before the
'northd_nb_logical_switch_handler' handler and it just creates or
deletes the lb_datapaths hmap for the tracked lbs.

The second handler 'northd_lb_data_handler_post_od' is called after
the 'northd_nb_logical_switch_handler' handler and it updates the
ovn_lb_datapaths's 'nb_ls_map' bitmap.

Eg.  If the lb_data has the below tracked data:

tracked_data = {'crupdated_lbs': [lb1, lb2],
'deleted_lbs': [lb3],
'crupdated_lb_groups': [lbg1, lbg2],
'crupdated_ls_lbs': [{ls: sw0, assoc_lbs: [lb1],
 {ls: sw1, assoc_lbs: [lb1, lb2]}

then the first handler northd_lb_data_handler_pre_od(), creates the
ovn_lb_datapaths object for lb1 and lb2 and deletes lb3 from
the ovn_lb_datapaths hmap.  Similarly for the created or updated lb
groups lbg1 and lbg2.

The second handler northd_lb_data_handler_post_od() updates the
nb_ls_bitmap of lb1 for sw0 and sw1 and nb_ls_bitmap of lb2 for sw1.

This second handler is added mainly so that the actual logical switch
handler northd_nb_logical_switch_handler() has done modifying the
'od' struct.

Signed-off-by: Numan Siddique 
---
 lib/lb.c |   5 +-
 northd/en-lb-data.c  | 176 +++
 northd/en-lb-data.h  |  17 
 northd/en-lflow.c|   6 ++
 northd/en-northd.c   |  40 +++--
 northd/en-northd.h   |   3 +-
 northd/inc-proc-northd.c |   5 +-
 northd/northd.c  | 152 ++---
 northd/northd.h  |  15 ++--
 tests/ovn-northd.at  |  56 +
 10 files changed, 414 insertions(+), 61 deletions(-)

diff --git a/lib/lb.c b/lib/lb.c
index 6fd67e2218..e6c9dc2be2 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -1088,7 +1088,10 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, 
size_t n,
 struct ovn_datapath **ods)
 {
 for (size_t i = 0; i < n; i++) {
-bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
+if (!bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
+bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
+lb_dps->n_nb_ls++;
+}
 }
 }
 
diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
index 23f2cb1021..e0c4db1422 100644
--- a/northd/en-lb-data.c
+++ b/northd/en-lb-data.c
@@ -39,6 +39,14 @@ static void lb_data_destroy(struct ed_type_lb_data *);
 static void build_lbs(const struct nbrec_load_balancer_table *,
   const struct nbrec_load_balancer_group_table *,
   struct hmap *lbs, struct hmap *lb_groups);
+static void build_od_lb_map(const struct nbrec_logical_switch_table *,
+ struct hmap *od_lb_map);
+static struct od_lb_data *find_od_lb_data(struct hmap *od_lb_map,
+  const struct uuid *od_uuid);
+static void destroy_od_lb_data(struct od_lb_data *od_lb_data);
+static struct od_lb_data *create_od_lb_data(struct hmap *od_lb_map,
+const struct uuid *od_uuid);
+
 static struct ovn_lb_group *create_lb_group(
 const struct nbrec_load_balancer_group *, struct hmap *lbs,
 struct hmap *lb_groups);
@@ -54,6 +62,7 @@ static struct crupdated_lb_group *
struct tracked_lb_data *);
 static void add_deleted_lb_group_to_tracked_data(
 struct ovn_lb_group *, struct tracked_lb_data *);
+static bool is_ls_lbs_changed(const struct nbrec_logical_switch *nbs);
 
 /* 'lb_data' engine node manages the NB load balancers and load balancer
  * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
@@ -80,9 +89,13 @@ en_lb_data_run(struct engine_node *node, void *data)
 EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
 const struct nbrec_load_balancer_group_table *nb_lbg_table =
 EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
+const struct nbrec_logical_switch_table *nb_ls_table =
+

[ovs-dev] [PATCH ovn v4 4/8] northd: Refactor the 'northd' node code which handles logical switch changes.

2023-08-02 Thread numans

From: Numan Siddique 

This will help in handling other column changes of a logical switch.

Acked-by: Han Zhou 
Signed-off-by: Numan Siddique 
---
 northd/en-lb-data.c |   2 +-
 northd/northd.c | 310 +---
 2 files changed, 181 insertions(+), 131 deletions(-)

diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
index 328c003675..23f2cb1021 100644
--- a/northd/en-lb-data.c
+++ b/northd/en-lb-data.c
@@ -58,7 +58,7 @@ static void add_deleted_lb_group_to_tracked_data(
 /* 'lb_data' engine node manages the NB load balancers and load balancer
  * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
  * for each NB LB group, it creates 'struct ovn_lb_group' and stores in
- * the respective hmaps in it data (ed_type_lb_data).
+ * the respective hmaps in it's data (ed_type_lb_data).
  */
 void *
 en_lb_data_init(struct engine_node *node OVS_UNUSED,
diff --git a/northd/northd.c b/northd/northd.c
index f6d67f8283..526ec76b63 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4891,23 +4891,29 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
 sset_destroy(_ha_chassis_grps);
 }
 
+static void
+destroy_tracked_ls_change(struct ls_change *ls_change)
+{
+struct ovn_port *op;
+LIST_FOR_EACH (op, list, _change->added_ports) {
+ovs_list_remove(>list);
+}
+LIST_FOR_EACH (op, list, _change->updated_ports) {
+ovs_list_remove(>list);
+}
+LIST_FOR_EACH_SAFE (op, list, _change->deleted_ports) {
+ovs_list_remove(>list);
+ovn_port_destroy_orphan(op);
+}
+}
+
 void
 destroy_northd_data_tracked_changes(struct northd_data *nd)
 {
 struct ls_change *ls_change;
 LIST_FOR_EACH_SAFE (ls_change, list_node,
 >tracked_ls_changes.updated) {
-struct ovn_port *op;
-LIST_FOR_EACH (op, list, _change->added_ports) {
-ovs_list_remove(>list);
-}
-LIST_FOR_EACH (op, list, _change->updated_ports) {
-ovs_list_remove(>list);
-}
-LIST_FOR_EACH_SAFE (op, list, _change->deleted_ports) {
-ovs_list_remove(>list);
-ovn_port_destroy_orphan(op);
-}
+destroy_tracked_ls_change(ls_change);
 ovs_list_remove(_change->list_node);
 free(ls_change);
 }
@@ -5024,15 +5030,21 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct 
hmap *ls_ports,
 return op;
 }
 
+/* Returns true if the logical switch has changes which can be
+ * incrementally handled.
+ * Presently supports i-p for the below changes:
+ *- logical switch ports.
+ */
 static bool
-check_ls_changes_other_than_lsp(const struct nbrec_logical_switch *ls)
+ls_changes_can_be_handled(
+const struct nbrec_logical_switch *ls)
 {
 /* Check if the columns are changed in this row. */
 enum nbrec_logical_switch_column_id col;
 for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
 if (nbrec_logical_switch_is_updated(ls, col) &&
 col != NBREC_LOGICAL_SWITCH_COL_PORTS) {
-return true;
+return false;
 }
 }
 
@@ -5041,44 +5053,44 @@ check_ls_changes_other_than_lsp(const struct 
nbrec_logical_switch *ls)
 for (size_t i = 0; i < ls->n_acls; i++) {
 if (nbrec_acl_row_get_seqno(ls->acls[i],
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 }
 if (ls->copp && nbrec_copp_row_get_seqno(ls->copp,
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 for (size_t i = 0; i < ls->n_dns_records; i++) {
 if (nbrec_dns_row_get_seqno(ls->dns_records[i],
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 }
 for (size_t i = 0; i < ls->n_forwarding_groups; i++) {
 if (nbrec_forwarding_group_row_get_seqno(ls->forwarding_groups[i],
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 }
 for (size_t i = 0; i < ls->n_load_balancer; i++) {
 if (nbrec_load_balancer_row_get_seqno(ls->load_balancer[i],
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 }
 for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
 if (nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 }
 for (size_t i = 0; i < ls->n_qos_rules; i++) {
 if (nbrec_qos_row_get_seqno(ls->qos_rules[i],
 OVSDB_IDL_CHANGE_MODIFY) > 0) {
-return true;
+return false;
 }
 }
-return false;
+return true;
 }
 
 static bool
@@ -5119,6 +5131,149

[ovs-dev] [PATCH ovn v4 3/8] northd: Add initial I-P for load balancer and load balancer groups

2023-08-02 Thread numans

From: Numan Siddique 

Any changes to load balancers and load balancer groups
are handled incrementally in the newly added 'lb_data'
engine node.  'lb_data' is input to 'northd' node
and the handler - northd_lb_data_handler in 'northd'
node handles the changes.

If a load balancer or load balancer group is associated to
a logical switch or router then 'northd' node falls back
to full recompute.  Upcoming patch will handle this scenario.

Signed-off-by: Numan Siddique 
---
 lib/lb.c |  82 +--
 lib/lb.h |   9 +
 northd/en-lb-data.c  | 273 ++-
 northd/en-lb-data.h  |  50 +
 northd/en-northd.c   |  41 
 northd/en-northd.h   |   1 +
 northd/inc-proc-northd.c |  12 +-
 northd/northd.c  |  59 +
 northd/northd.h  |   7 +
 tests/ovn-northd.at  | 457 +++
 10 files changed, 960 insertions(+), 31 deletions(-)

diff --git a/lib/lb.c b/lib/lb.c
index e874680a3f..6fd67e2218 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct nbrec_load_balancer 
*nbrec_lb,
 return NULL;
 }
 
-struct ovn_northd_lb *
-ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
+static void
+ovn_northd_lb_init(struct ovn_northd_lb *lb,
+   const struct nbrec_load_balancer *nbrec_lb)
 {
 bool template = smap_get_bool(_lb->options, "template", false);
 bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
 bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol, "sctp");
-struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
 int address_family = !strcmp(smap_get_def(_lb->options,
   "address-family", "ipv4"),
  "ipv4")
@@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct nbrec_load_balancer 
*nbrec_lb)
   "reject", false);
 ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
node->key, node->value, template);
+if (lb_vip_nb->lb_health_check) {
+lb->health_checks = true;
+}
+
 if (IN6_IS_ADDR_V4MAPPED(_vip->vip)) {
 sset_add(>ips_v4, lb_vip->vip_str);
 } else {
@@ -711,6 +715,13 @@ ovn_northd_lb_create(const struct nbrec_load_balancer 
*nbrec_lb)
 ds_chomp(_fields, ',');
 lb->selection_fields = ds_steal_cstr(_fields);
 }
+}
+
+struct ovn_northd_lb *
+ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
+{
+struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
+ovn_northd_lb_init(lb, nbrec_lb);
 return lb;
 }
 
@@ -736,8 +747,8 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb *lb)
 return >nlb->vips;
 }
 
-void
-ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
+static void
+ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
 {
 for (size_t i = 0; i < lb->n_vips; i++) {
 ovn_lb_vip_destroy(>vips[i]);
@@ -745,24 +756,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
 }
 free(lb->vips);
 free(lb->vips_nb);
+lb->vips = NULL;
+lb->vips_nb = NULL;
 smap_destroy(>template_vips);
 sset_destroy(>ips_v4);
 sset_destroy(>ips_v6);
 free(lb->selection_fields);
+lb->selection_fields = NULL;
+lb->health_checks = false;
+}
+
+void
+ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
+{
+ovn_northd_lb_cleanup(lb);
 free(lb);
 }
 
-/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group record
- * and an array of 'struct ovn_northd_lb' objects for its associated
- * load balancers. */
-struct ovn_lb_group *
-ovn_lb_group_create(const struct nbrec_load_balancer_group *nbrec_lb_group,
-const struct hmap *lbs)
+void
+ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
+ const struct nbrec_load_balancer *nbrec_lb)
 {
-struct ovn_lb_group *lb_group;
+ovn_northd_lb_cleanup(lb);
+ovn_northd_lb_init(lb, nbrec_lb);
+}
 
-lb_group = xzalloc(sizeof *lb_group);
-lb_group->uuid = nbrec_lb_group->header_.uuid;
+static void
+ovn_lb_group_init(struct ovn_lb_group *lb_group,
+  const struct nbrec_load_balancer_group *nbrec_lb_group,
+  const struct hmap *lbs)
+{
 lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
 lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
 lb_group->lb_ips = ovn_lb_ip_set_create();
@@ -772,10 +795,29 @@ ovn_lb_group_create(const struct 
nbrec_load_balancer_group *nbrec_lb_group,
 _lb_group->load_balancer[i]->header_.uuid;
 lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
 }
+}
 
+/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group record
+ * and an array of 'struct ovn_northd_lb' objects for its associated
+ * load balancers. */
+struct ovn_lb_group *
+ovn_lb_group_create(const struct nbrec_load_balancer_group

[ovs-dev] [PATCH ovn v4 2/8] northd: Add a new engine node - lb_data.

2023-08-02 Thread numans

From: Numan Siddique 

This patch separates out the 'lbs' and 'lb_groups' from the 'northd' engine
node data into a new engine node 'lb_data'.  This new node becomes
an input to the 'northd' node.

This makes handling the NB load balancer and load balancer group changes
easier.

Prior to this patch, 'struct ovn_northd_lb' and 'struct ovn_lb_group'
were maintaing the logical switch and logical router association
with a load balancer.  This data is now maintained by 'northd' engine
node.  New structs 'struct ovn_lb_datapaths' and 'struct
ovn_lb_group_datapaths' is added for this purpose.

Acked-by: Han Zhou 
Signed-off-by: Numan Siddique 
---
 lib/lb.c | 205 ++--
 lib/lb.h |  86 +++--
 northd/automake.mk   |   2 +
 northd/en-lb-data.c  | 125 
 northd/en-lb-data.h  |  23 ++
 northd/en-lflow.c|   3 +-
 northd/en-northd.c   |  11 +-
 northd/en-sync-sb.c  |   2 +-
 northd/inc-proc-northd.c |   8 +-
 northd/northd.c  | 669 ++-
 northd/northd.h  |  15 +-
 11 files changed, 781 insertions(+), 368 deletions(-)
 create mode 100644 northd/en-lb-data.c
 create mode 100644 northd/en-lb-data.h

diff --git a/lib/lb.c b/lib/lb.c
index 7afdaed65b..e874680a3f 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -26,6 +26,7 @@
 #include "openvswitch/vlog.h"
 #include "lib/bitmap.h"
 #include "lib/smap.h"
+#include "socket-util.h"
 
 VLOG_DEFINE_THIS_MODULE(lb);
 
@@ -431,11 +432,62 @@ void ovn_northd_lb_vip_init(struct ovn_northd_lb_vip 
*lb_vip_nb,
 ovn_lb_get_health_check(nbrec_lb, vip_port_str, template);
 }
 
+static void
+ovn_lb_vip_backends_health_check_init(const struct ovn_northd_lb *lb,
+  const struct ovn_lb_vip *lb_vip,
+  struct ovn_northd_lb_vip *lb_vip_nb)
+{
+struct ds key = DS_EMPTY_INITIALIZER;
+
+for (size_t j = 0; j < lb_vip->n_backends; j++) {
+struct ovn_lb_backend *backend = _vip->backends[j];
+ds_clear();
+ds_put_format(, IN6_IS_ADDR_V4MAPPED(_vip->vip)
+  ? "%s" : "[%s]", backend->ip_str);
+
+const char *s = smap_get(>nlb->ip_port_mappings, ds_cstr());
+if (!s) {
+continue;
+}
+
+char *svc_mon_src_ip = NULL;
+char *port_name = xstrdup(s);
+char *p = strstr(port_name, ":");
+if (p) {
+*p = 0;
+p++;
+struct sockaddr_storage svc_mon_src_addr;
+if (!inet_parse_address(p, _mon_src_addr)) {
+static struct vlog_rate_limit rl =
+VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "Invalid svc mon src IP %s", p);
+} else {
+struct ds src_ip_s = DS_EMPTY_INITIALIZER;
+ss_format_address_nobracks(_mon_src_addr,
+_ip_s);
+svc_mon_src_ip = ds_steal_cstr(_ip_s);
+}
+}
+
+if (svc_mon_src_ip) {
+struct ovn_northd_lb_backend *backend_nb =
+_vip_nb->backends_nb[j];
+backend_nb->health_check = true;
+backend_nb->logical_port = xstrdup(port_name);
+backend_nb->svc_mon_src_ip = svc_mon_src_ip;
+}
+free(port_name);
+}
+
+ds_destroy();
+}
+
 static
 void ovn_northd_lb_vip_destroy(struct ovn_northd_lb_vip *vip)
 {
 free(vip->backend_ips);
 for (size_t i = 0; i < vip->n_backends; i++) {
+free(vip->backends_nb[i].logical_port);
 free(vip->backends_nb[i].svc_mon_src_ip);
 }
 free(vip->backends_nb);
@@ -555,8 +607,7 @@ ovn_lb_get_health_check(const struct nbrec_load_balancer 
*nbrec_lb,
 }
 
 struct ovn_northd_lb *
-ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb,
- size_t n_ls_datapaths, size_t n_lr_datapaths)
+ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
 {
 bool template = smap_get_bool(_lb->options, "template", false);
 bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
@@ -595,9 +646,6 @@ ovn_northd_lb_create(const struct nbrec_load_balancer 
*nbrec_lb,
 }
 lb->affinity_timeout = affinity_timeout;
 
-lb->nb_ls_map = bitmap_allocate(n_ls_datapaths);
-lb->nb_lr_map = bitmap_allocate(n_lr_datapaths);
-
 sset_init(>ips_v4);
 sset_init(>ips_v6);
 struct smap_node *node;
@@ -632,6 +680,10 @@ ovn_northd_lb_create(const struct nbrec_load_balancer 
*nbrec_lb,
 xstrdup(node->value));
 }
 n_vips++;
+
+if (lb_vip_nb->lb_health_check) {
+ovn_lb_vip_backends_health_check_init(lb, lb_vip, lb_vip_nb);
+}
 }
 
 /* It's possible that parsing VIPs fails.  Update the lb->n_vips to the
@@ -684,24 +736,6 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb *lb)
 return >nlb->vips;
 }
 
-void

[ovs-dev] [PATCH ovn v4 1/8] northd I-P: Sync SB load balancers in a separate engine node.

2023-08-02 Thread numans

From: Numan Siddique 

Similar to the commit [1], a new sub-engine node "sync_to_sb_lb"
is added with-in the "sync_to_sb" to sync the SB load balancers.
Its main input nodes are "northd" (to access the "lbs" hmap built
by this node) and "sb_load_balancer" to access the SB load balancer.
"sync_to_sb_lb" doesn't add any handlers and falls back to full
recompute all the time.

[1] - ccafcc2dc321("northd I-P: Add a new engine node 'en_sync_to_sb' to sync 
SB tables.")

Signed-off-by: Numan Siddique 
---
 northd/en-northd.c   |  2 --
 northd/en-sync-sb.c  | 43 
 northd/en-sync-sb.h  |  6 ++
 northd/inc-proc-northd.c | 10 --
 northd/northd.c  |  4 +---
 northd/northd.h  |  6 +-
 tests/ovn-northd.at  | 23 +
 7 files changed, 86 insertions(+), 8 deletions(-)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index 044fa70190..f9f2d04452 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -101,8 +101,6 @@ northd_get_input_data(struct engine_node *node,
 EN_OVSDB_GET(engine_get_input("SB_chassis", node));
 input_data->sbrec_fdb_table =
 EN_OVSDB_GET(engine_get_input("SB_fdb", node));
-input_data->sbrec_load_balancer_table =
-EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
 input_data->sbrec_service_monitor_table =
 EN_OVSDB_GET(engine_get_input("SB_service_monitor", node));
 input_data->sbrec_port_group_table =
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index d7fea981f2..dbe3c63752 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -211,6 +211,49 @@ sync_to_sb_addr_set_nb_port_group_handler(struct 
engine_node *node,
 return true;
 }
 
+/* sync_to_sb_lb engine node functions.
+ * This engine node syncs the SB load balancers.
+ */
+void *
+en_sync_to_sb_lb_init(struct engine_node *node OVS_UNUSED,
+  struct engine_arg *arg OVS_UNUSED)
+{
+return NULL;
+}
+
+void
+en_sync_to_sb_lb_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+const struct sbrec_load_balancer_table *sb_load_balancer_table =
+EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_data *northd_data = engine_get_input_data("northd", node);
+
+sync_lbs(eng_ctx->ovnsb_idl_txn, sb_load_balancer_table,
+ _data->ls_datapaths, _data->lbs);
+engine_set_node_state(node, EN_UPDATED);
+}
+
+void
+en_sync_to_sb_lb_cleanup(void *data OVS_UNUSED)
+{
+
+}
+
+bool
+sync_to_sb_lb_northd_handler(struct engine_node *node, void *data OVS_UNUSED)
+{
+struct northd_data *nd = engine_get_input_data("northd", node);
+if (nd->change_tracked) {
+/* There are only NB LSP related changes and these can be safely
+ * ignore and returned true.  However in case the northd engine
+ * tracking data includes other changes, we need to do additional
+ * checks before safely ignoring. */
+return true;
+}
+return false;
+}
+
 /* static functions. */
 static void
 sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name,
diff --git a/northd/en-sync-sb.h b/northd/en-sync-sb.h
index bcb9799d24..06d2a57710 100644
--- a/northd/en-sync-sb.h
+++ b/northd/en-sync-sb.h
@@ -16,4 +16,10 @@ bool sync_to_sb_addr_set_nb_address_set_handler(struct 
engine_node *,
 bool sync_to_sb_addr_set_nb_port_group_handler(struct engine_node *,
void *data);
 
+
+void *en_sync_to_sb_lb_init(struct engine_node *, struct engine_arg *);
+void en_sync_to_sb_lb_run(struct engine_node *, void *data);
+void en_sync_to_sb_lb_cleanup(void *data);
+bool sync_to_sb_lb_northd_handler(struct engine_node *, void *data OVS_UNUSED);
+
 #endif /* end of EN_SYNC_SB_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index d328deb222..c0874d5294 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -139,6 +139,7 @@ static ENGINE_NODE(sync_to_sb, "sync_to_sb");
 static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set");
 static ENGINE_NODE(fdb_aging, "fdb_aging");
 static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
+static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
 
 void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
   struct ovsdb_idl_loop *sb)
@@ -166,7 +167,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_northd, _sb_ha_chassis_group, NULL);
 engine_add_input(_northd, _sb_ip_multicast, NULL);
 engine_add_input(_northd, _sb_service_monitor, NULL);
-engine_add_input(_northd, _sb_load_balancer, NULL);
 engine_add_input(_northd, _sb_fdb, NULL);
 engine_add_input(_northd, _sb_static_mac_binding, NULL);
 engine_add_input(_northd, _sb_chassis_template_var, NULL);
@@ -202,11 +202,17 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,

[ovs-dev] [PATCH ovn v4 0/8] northd: I-P for load balancer and lb groups

2023-08-02 Thread numans

From: Numan Siddique 

This patch series adds the support to handle load balancer and
load balancer group changes incrementally in the "northd" engine
node.  "flow" engine node doesn't support I-P yet and falls back
to full recompute.  Changes to logical switches and router's load
balancer and load balancer group columns are also handled incrementally
provided those are the only changes to them.

Below are the scale testing results done with these patches applied
using ovn-heater.  The test ran the scenario  -
ocp-500-density-heavy.yml [1].

With these patches applied (with load balancer I-P handling in northd
engine node) the resuts are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1329292.1571033.314847
3.3315614.3786261.581889197.736147  125 0
Namespace.add_ports 0.0052170.0057600.006565
0.0133480.0210140.0061060.763214125 0
WorkerNode.bind_port0.0352050.0454580.052278
0.0598040.0639410.04565211.413122   250 0
WorkerNode.ping_port0.0050750.0068143.088548
3.1925774.2420260.726453181.613284  250 0
---

The results with the present main are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 4.3772606.4869627.502040
8.3225878.3347016.559002819.875306  125 0
Namespace.add_ports 0.0051120.0054840.005953
0.0091530.0114520.0056620.707752125 0
WorkerNode.bind_port0.0353600.0427320.049152
0.0536980.0566350.04321510.803700   250 0
WorkerNode.ping_port0.0053381.5999047.229649
7.7980398.2065373.209860802.464911  250 0
---

Few observations:

 - The total time taken has come down significantly from 819 seconds to 197
   to complete the density heavy tests (excluding the base cluster
   bringup)
 - 99%ile with these patches is 3.3 seconds compared to 8.3 seconds for the
   main.
 - 90%file with these patches is 3.3 seconds compared to 7.5 seconds for
   the main.
 - CPU utilization of northd during the test with these patches
   is between 100% to 300% which is almost the same as main.
   Main difference being that, with these patches the test duration is
   less and hence overall less CPU utilization.

[1] - 
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml


v3 -> v4
---
  * Covered more test scearios.
  * Found few issues and fixed them.  v3 was not handling the scenario of
a vip getting added or removed from a load balancer.

v2 -> v3

  * v2 was very inefficient in handling the load balancer group changes
and in associating the load balancers of the lb group to the
datapaths. This was the main reason for the regression in the full
recompute time taken.
v3 addressed these by more efficiently handling the lb group changes
incrementally.

Numan Siddique (8):
  northd I-P: Sync SB load balancers in a separate engine node.
  northd: Add a new engine node - lb_data.
  northd: Add initial I-P for load balancer and load balancer groups
  northd: Refactor the 'northd' node code which handles logical switch
changes.
  northd: Handle load balancer changes for a logical switch.
  northd: Handle load balancer group changes for a logical switch.
  northd: Sync SB Port bindings NAT column in a separate engine node.
  northd: Handle load balancer/group changes for a logical router.

 lib/lb.c |  318 ++--
 lib/lb.h |  102 ++-
 northd/automake.mk   |2 +
 northd/en-lb-data.c  |  800

Re: [ovs-dev] [PATCH ovn v2 8/8] northd: Handle load balancer changes for a logical router.

2023-08-02 Thread Numan Siddique

On Mon, Jul 31, 2023 at 8:18 PM Han Zhou  wrote:
>
> On Wed, Jul 19, 2023 at 2:01 AM Numan Siddique  wrote:
> >
> > On Tue, Jul 18, 2023 at 11:08 AM Han Zhou  wrote:
> > >
> > > On Fri, Jul 7, 2023 at 1:56 PM  wrote:
> > > >
> > > > From: Numan Siddique 
> > > >
> > > > When a logical router gets updated due to load balancer
> > > > or load balancer groups changes, it is now incrementally
> > > > handled in the 'northd' engine node.  Other logical router
> > > > updates result in the full recompute of 'northd' engine node.
> > > >
> > > > lflow engine node still falls back to recompute due to
> > > > logical router changes.
> > > >
> > > > Signed-off-by: Numan Siddique 
> > > > ---
> > > >  lib/lb.c |  32 +++-
> > > >  lib/lb.h |   5 +
> > > >  northd/en-lflow.c|   5 +
> > > >  northd/en-northd.c   |  20 ++
> > > >  northd/en-northd.h   |   1 +
> > > >  northd/inc-proc-northd.c |   3 +-
> > > >  northd/northd.c  | 392
> ++-
> > > >  northd/northd.h  |   5 +
> > > >  tests/ovn-northd.at  |  12 +-
> > > >  9 files changed, 422 insertions(+), 53 deletions(-)
> > > >
> > > > diff --git a/lib/lb.c b/lib/lb.c
> > > > index a0426cc42e..a8b694d0b3 100644
> > > > --- a/lib/lb.c
> > > > +++ b/lib/lb.c
> > > > @@ -1082,7 +1082,10 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
> > > *lb_dps, size_t n,
> > > >  struct ovn_datapath **ods)
> > > >  {
> > > >  for (size_t i = 0; i < n; i++) {
> > > > -bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > > +if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > > +bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
> > > > +lb_dps->n_nb_lr++;
> > > > +}
> > > >  }
> > > >  }
> > > >
> > > > @@ -1110,6 +1113,18 @@ ovn_lb_datapaths_remove_ls(struct
> ovn_lb_datapaths
> > > *lb_dps, size_t n,
> > > >  }
> > > >  }
> > > >
> > > > +void
> > > > +ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
> > > > +   struct ovn_datapath **ods)
> > > > +{
> > > > +for (size_t i = 0; i < n; i++) {
> > > > +if (bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
> > > > +bitmap_set0(lb_dps->nb_lr_map, ods[i]->index);
> > > > +lb_dps->n_nb_lr--;
> > > > +}
> > > > +}
> > > > +}
> > > > +
> > > >  struct ovn_lb_group_datapaths *
> > > >  ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
> > > >size_t n_ls_datapaths,
> > > > @@ -1175,5 +1190,18 @@ void
> > > >  ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
> > > >struct ovn_datapath *lr)
> > > >  {
> > > > -bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > +if (!bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > > +bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > +lbg_dps->n_nb_lr++;
> > > > +}
> > > > +}
> > > > +
> > > > +void
> > > > +ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
> *lbg_dps,
> > > > + struct ovn_datapath *lr)
> > > > +{
> > > > +if (bitmap_is_set(lbg_dps->nb_lr_map, lr->index)) {
> > > > +bitmap_set1(lbg_dps->nb_lr_map, lr->index);
> > > > +lbg_dps->n_nb_lr--;
> > > > +}
> > > >  }
> > > > diff --git a/lib/lb.h b/lib/lb.h
> > > > index 08723e8ef3..91eec0199b 100644
> > > > --- a/lib/lb.h
> > > > +++ b/lib/lb.h
> > > > @@ -185,6 +185,8 @@ void ovn_lb_datapaths_set_ls(struct
> ovn_lb_datapaths
> > > *, size_t n,
> > > >
> > > >  void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n,
> > > >  struct ovn_datapath **);
> > > > +void ovn_lb_datapaths_remove_lr(struct ovn_lb_datapaths *, size_t n,
> > > > +struct ovn_datapath **);
> > > >
> > > >  struct ovn_lb_group_datapaths {
> > > >  struct hmap_node hmap_node;
> > > > @@ -214,6 +216,9 @@ void ovn_lb_group_datapaths_remove_ls(struct
> > > ovn_lb_group_datapaths *,
> > > >
> > > >  void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *,
> > > > struct ovn_datapath *lr);
> > > > +void ovn_lb_group_datapaths_remove_lr(struct ovn_lb_group_datapaths
> *,
> > > > +  struct ovn_datapath *lr);
> > > > +
> > > >  struct ovn_controller_lb {
> > > >  struct hmap_node hmap_node;
> > > >
> > > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > > > index db1bcbccd6..ea51f4e3e0 100644
> > > > --- a/northd/en-lflow.c
> > > > +++ b/northd/en-lflow.c
> > > > @@ -102,6 +102,11 @@ lflow_northd_handler(struct engine_node *node,
> > > >  if (!northd_data->change_tracked) {
> > > >  return false;
> > > >  }
> > > > +
> > > > +if (northd_data->lrouters_changed) {
> > > > +return false;
> > > > +}
> > >
> >

Re: [ovs-dev] [PATCH] ovsdb-tool: Fix json leak while showing clustered log.

Re: [ovs-dev] [PATCH] ovsdb-server: Fix excessive memory usage on DB open.

Re: [ovs-dev] [PATCH 4/4] tests: Add ovsdb execution cases for set size constraints.

Re: [ovs-dev] [PATCH 3/4] ovsdb: relay: Fix handling of XOR updates with size constraints.

Re: [ovs-dev] [PATCH 2/4] ovsdb: file: Fix diff application to a default column value.

Re: [ovs-dev] [PATCH 1/4] ovsdb: file: Fix inability to read diffs that violate type size.

Re: [ovs-dev] [PATCH ovn] system-tests: Wait for all interfaces to have IPv6

[ovs-dev] [PATCH v1] lacp: Improve visibility with LACP

Re: [ovs-dev] [PATCH ovn 2/2] ovn-ic: support learning routes in same AZ

Re: [ovs-dev] [PATCH ovn 1/2] ovn-ic fix multiple routers in an az

[ovs-dev] [PATCH] ovsdb-tool: Fix json leak while showing clustered log.

[ovs-dev] [PATCH] ovsdb-server: Fix excessive memory usage on DB open.

[ovs-dev] [PATCH ovn] system-tests: Wait for all interfaces to have IPv6

[ovs-dev] [PATCH] conntrack: Allow flush of SCTP protocol

[ovs-dev] [PATCH v2 2/2] netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite

[ovs-dev] [PATCH v2 1/2] netdev-tc-offload: Add csum offload of protocols IGMP/UDPLITE/SCTP

[ovs-dev] [PATCH v2 0/2] Add more protocols to offload in ip rewrite

[ovs-dev] [PATCH 3/3] python: Use build to generate PEP517 compatible archives.

[ovs-dev] [PATCH 2/3] python: Use twine to upload sdist package to pypi.org.

[ovs-dev] [PATCH 1/3] python: Move build related code into build-aux.

[ovs-dev] [PATCH 0/3] Python ovsdb bindings distribution improvements

Re: [ovs-dev] [PATCH ovn v4 6/8] northd: Handle load balancer group changes for a logical switch.

[ovs-dev] [PATCH ovn v4 8/8] northd: Handle load balancer/group changes for a logical router.

[ovs-dev] [PATCH ovn v4 7/8] northd: Sync SB Port bindings NAT column in a separate engine node.

[ovs-dev] [PATCH ovn v4 6/8] northd: Handle load balancer group changes for a logical switch.

[ovs-dev] [PATCH ovn v4 5/8] northd: Handle load balancer changes for a logical switch.

[ovs-dev] [PATCH ovn v4 4/8] northd: Refactor the 'northd' node code which handles logical switch changes.

[ovs-dev] [PATCH ovn v4 3/8] northd: Add initial I-P for load balancer and load balancer groups

[ovs-dev] [PATCH ovn v4 2/8] northd: Add a new engine node - lb_data.

[ovs-dev] [PATCH ovn v4 1/8] northd I-P: Sync SB load balancers in a separate engine node.

[ovs-dev] [PATCH ovn v4 0/8] northd: I-P for load balancer and lb groups

Re: [ovs-dev] [PATCH ovn v2 8/8] northd: Handle load balancer changes for a logical router.

32 matches

Site Navigation

Mail list logo

Footer information