From patchwork Fri Aug 18 08:58:45 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1822766 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::138; helo=smtp1.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=patchwork.ozlabs.org) Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4RRwnM53V9z1ygW for ; Fri, 18 Aug 2023 19:00:23 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id DD9BF840B0; Fri, 18 Aug 2023 09:00:21 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org DD9BF840B0 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2SypjEOCjYMX; Fri, 18 Aug 2023 09:00:17 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp1.osuosl.org (Postfix) with ESMTPS id 141AD8419B; Fri, 18 Aug 2023 09:00:14 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 141AD8419B Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id A4DD9C0039; Fri, 18 Aug 2023 09:00:13 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 1BCE7C0072 for ; Fri, 18 Aug 2023 09:00:12 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 7E216615EE for ; Fri, 18 Aug 2023 08:58:55 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 7E216615EE X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id T8UmHPcC7-49 for ; Fri, 18 Aug 2023 08:58:54 +0000 (UTC) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::224]) by smtp3.osuosl.org (Postfix) with ESMTPS id B8554615F0 for ; Fri, 18 Aug 2023 08:58:53 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org B8554615F0 Received: by mail.gandi.net (Postfix) with ESMTPSA id AB662E0009; Fri, 18 Aug 2023 08:58:49 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Fri, 18 Aug 2023 14:28:45 +0530 Message-Id: <20230818085845.1031138-1-numans@ovn.org> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230818085606.1030792-1-numans@ovn.org> References: <20230818085606.1030792-1-numans@ovn.org> MIME-Version: 1.0 X-GND-Sasl: numans@ovn.org Subject: [ovs-dev] [PATCH ovn v6 15/16] northd: Move router ports SB PB options sync to sync_to_sb_pb node. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" From: Numan Siddique With this we don't fall back to recompute of northd engine node when SB port bindings (of router ports) updates from SB ovsdb-server are received. Signed-off-by: Numan Siddique --- northd/en-northd.c | 2 +- northd/en-sync-sb.c | 3 +- northd/northd.c | 197 +++++++++++++++++++++++++++----------------- northd/northd.h | 6 +- tests/ovn-northd.at | 49 +++++++++++ 5 files changed, 176 insertions(+), 81 deletions(-) diff --git a/northd/en-northd.c b/northd/en-northd.c index 651c53a6ce..b3e84a6af6 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -199,7 +199,7 @@ northd_sb_port_binding_handler(struct engine_node *node, northd_get_input_data(node, &input_data); if (!northd_handle_sb_port_binding_changes( - input_data.sbrec_port_binding_table, &nd->ls_ports)) { + input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) { return false; } diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 3ef3be6af1..3b82bd0718 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -276,7 +276,8 @@ 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, &northd_data->ls_ports); + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, + &northd_data->lr_ports); engine_set_node_state(node, EN_UPDATED); } diff --git a/northd/northd.c b/northd/northd.c index 2a490e9ab6..2ae41b5d8a 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3222,27 +3222,33 @@ op_get_name(const struct ovn_port *op) } static void -ovn_update_ipv6_prefix(struct hmap *lr_ports) +update_nb_ipv6_prefix(const struct ovn_port *op) { - const struct ovn_port *op; - HMAP_FOR_EACH (op, key_node, lr_ports) { - ovs_assert(op->nbrp); + ovs_assert(op->nbrp); - if (!smap_get_bool(&op->nbrp->options, "prefix", false)) { - continue; - } + if (!smap_get_bool(&op->nbrp->options, "prefix", false)) { + return; + } - char prefix[IPV6_SCAN_LEN + 6]; - unsigned aid; - const char *ipv6_pd_list = smap_get(&op->sb->options, - "ipv6_ra_pd_list"); - if (!ipv6_pd_list || - !ovs_scan(ipv6_pd_list, "%u:%s", &aid, prefix)) { - continue; - } + char prefix[IPV6_SCAN_LEN + 6]; + unsigned aid; + const char *ipv6_pd_list = smap_get(&op->sb->options, + "ipv6_ra_pd_list"); + if (!ipv6_pd_list || + !ovs_scan(ipv6_pd_list, "%u:%s", &aid, prefix)) { + return; + } + + const char *prefix_ptr = prefix; + nbrec_logical_router_port_set_ipv6_prefix(op->nbrp, &prefix_ptr, 1); +} - const char *prefix_ptr = prefix; - nbrec_logical_router_port_set_ipv6_prefix(op->nbrp, &prefix_ptr, 1); +static void +ovn_update_ipv6_prefix(struct hmap *lr_ports) +{ + const struct ovn_port *op; + HMAP_FOR_EACH (op, key_node, lr_ports) { + update_nb_ipv6_prefix(op); } } @@ -3390,6 +3396,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, { sbrec_port_binding_set_datapath(op->sb, op->od->sb); if (op->nbrp) { + /* Note: SB port binding options for router ports are set in + * sync_pbs(). */ + /* If the router is for l3 gateway, it resides on a chassis * and its port type is "l3gateway". */ const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); @@ -3401,15 +3410,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, sbrec_port_binding_set_type(op->sb, "patch"); } - struct smap new; - smap_init(&new); if (is_cr_port(op)) { ovs_assert(sbrec_chassis_by_name); ovs_assert(sbrec_chassis_by_hostname); ovs_assert(sbrec_ha_chassis_grp_by_name); ovs_assert(active_ha_chassis_grps); - const char *redirect_type = smap_get(&op->nbrp->options, - "redirect-type"); if (op->nbrp->ha_chassis_group) { if (op->nbrp->n_gateway_chassis) { @@ -3451,49 +3456,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, /* Delete the legacy gateway_chassis from the pb. */ sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0); } - smap_add(&new, "distributed-port", op->nbrp->name); - - bool always_redirect = - !op->od->has_distributed_nat && - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); - - if (redirect_type) { - smap_add(&new, "redirect-type", redirect_type); - /* XXX Why can't we enable always-redirect when redirect-type - * is bridged? */ - if (!strcmp(redirect_type, "bridged")) { - always_redirect = false; - } - } - - if (always_redirect) { - smap_add(&new, "always-redirect", "true"); - } - } else { - if (op->peer) { - smap_add(&new, "peer", op->peer->key); - if (op->nbrp->ha_chassis_group || - op->nbrp->n_gateway_chassis) { - char *redirect_name = - ovn_chassis_redirect_name(op->nbrp->name); - smap_add(&new, "chassis-redirect-port", redirect_name); - free(redirect_name); - } - } - if (chassis_name) { - smap_add(&new, "l3gateway-chassis", chassis_name); - } } - const char *ipv6_pd_list = smap_get(&op->sb->options, - "ipv6_ra_pd_list"); - if (ipv6_pd_list) { - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); - } - - sbrec_port_binding_set_options(op->sb, &new); - smap_destroy(&new); - sbrec_port_binding_set_parent_port(op->sb, NULL); sbrec_port_binding_set_tag(op->sb, NULL, 0); @@ -4667,10 +4631,23 @@ sync_lbs(struct ovsdb_idl_txn *ovnsb_txn, } } -/* Sync the SB Port bindings which needs to be updated. - * Presently it syncs the nat column of port bindings corresponding to - * the logical switch ports. */ -void sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) +static void ovn_update_ipv6_options(struct hmap *lr_ports); +static void ovn_update_ipv6_prefix(struct hmap *lr_ports); + +/* Sync the SB Port bindings which needs to be updated after the northd + * engine is run. + * Presently it syncs + * - the nat column of port bindings corresponding to + * the logical switch ports. + * - 'always-redirect' flag of chassis resident gateway ports. + * + * Note: Ideally we should move ovn_port_update_sbrec() here. + * But until we add full incremental processing to northd engine, + * it is difficult. + * */ +void +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, + struct hmap *lr_ports) { ovs_assert(ovnsb_idl_txn); @@ -4796,6 +4773,63 @@ void sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0); } } + + /* Only sets the port binding options column for the router ports. + * */ + HMAP_FOR_EACH (op, key_node, lr_ports) { + ovs_assert(op->nbrp); + struct smap new; + smap_init(&new); + + const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); + if (is_cr_port(op)) { + smap_add(&new, "distributed-port", op->nbrp->name); + + bool always_redirect = + !op->od->has_distributed_nat && + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); + + const char *redirect_type = smap_get(&op->nbrp->options, + "redirect-type"); + if (redirect_type) { + smap_add(&new, "redirect-type", redirect_type); + /* XXX Why can't we enable always-redirect when redirect-type + * is bridged? */ + if (!strcmp(redirect_type, "bridged")) { + always_redirect = false; + } + } + + if (always_redirect) { + smap_add(&new, "always-redirect", "true"); + } + } else { + if (op->peer) { + smap_add(&new, "peer", op->peer->key); + if (op->nbrp->ha_chassis_group || + op->nbrp->n_gateway_chassis) { + char *redirect_name = + ovn_chassis_redirect_name(op->nbrp->name); + smap_add(&new, "chassis-redirect-port", redirect_name); + free(redirect_name); + } + } + if (chassis_name) { + smap_add(&new, "l3gateway-chassis", chassis_name); + } + } + + const char *ipv6_pd_list = smap_get(&op->sb->options, + "ipv6_ra_pd_list"); + if (ipv6_pd_list) { + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); + } + + sbrec_port_binding_set_options(op->sb, &new); + smap_destroy(&new); + } + + ovn_update_ipv6_options(lr_ports); } static bool @@ -5690,20 +5724,23 @@ fail: bool northd_handle_sb_port_binding_changes( const struct sbrec_port_binding_table *sbrec_port_binding_table, - struct hmap *ls_ports) + struct hmap *ls_ports, struct hmap *lr_ports) { const struct sbrec_port_binding *pb; static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) { /* Return false if the 'pb' belongs to a router port. We don't handle * I-P for router ports yet. */ - if (is_pb_router_type(pb)) { - return false; - } + bool is_router_port = is_pb_router_type(pb); - struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port); - if (op && !op->lsp_can_be_inc_processed) { - return false; + struct ovn_port *op; + if (is_router_port) { + op = ovn_port_find(lr_ports, pb->logical_port); + } else { + op = ovn_port_find(ls_ports, pb->logical_port); + if (op && !op->lsp_can_be_inc_processed) { + return false; + } } if (sbrec_port_binding_is_new(pb)) { @@ -5712,7 +5749,8 @@ northd_handle_sb_port_binding_changes( * pointer in northd data. Fallback to recompute otherwise. */ if (!op) { VLOG_WARN_RL(&rl, "A port-binding for %s is created but the " - "LSP is not found.", pb->logical_port); + "%s is not found.", pb->logical_port, + is_router_port ? "LRP" : "LSP"); return false; } op->sb = pb; @@ -5723,7 +5761,8 @@ northd_handle_sb_port_binding_changes( * sb idl pointers and other unexpected behavior. */ if (op) { VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the " - "LSP still exists.", pb->logical_port); + "%s still exists.", pb->logical_port, + is_router_port ? "LRP" : "LSP"); return false; } } else { @@ -5733,7 +5772,8 @@ northd_handle_sb_port_binding_changes( * Fallback to recompute for anything unexpected. */ if (!op) { VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the " - "LSP is not found.", pb->logical_port); + "%s is not found.", pb->logical_port, + is_router_port ? "LRP" : "LSP"); return false; } if (op->sb != pb) { @@ -5741,6 +5781,10 @@ northd_handle_sb_port_binding_changes( "IDL row, which is unusual.", pb->logical_port); return false; } + + if (op && is_router_port) { + update_nb_ipv6_prefix(op); + } } } return true; @@ -19487,7 +19531,6 @@ ovnnb_db_run(struct northd_input *input_data, &data->lr_ports); stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); - ovn_update_ipv6_options(&data->lr_ports); ovn_update_ipv6_prefix(&data->lr_ports); sync_port_groups(ovnsb_txn, input_data->sbrec_port_group_table, diff --git a/northd/northd.h b/northd/northd.h index b836aa737f..fc2c083612 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -394,7 +394,8 @@ bool lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, struct lflow_input *lflow_input, struct lflow_data *lflow_data); bool northd_handle_sb_port_binding_changes( - const struct sbrec_port_binding_table *, struct hmap *ls_ports); + const struct sbrec_port_binding_table *, struct hmap *ls_ports, + struct hmap *lr_ports); struct tracked_lb_data; bool northd_handle_lb_data_changes_pre_od(struct tracked_lb_data *, @@ -426,6 +427,7 @@ const char *northd_get_svc_monitor_mac(void); void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, struct ovn_datapaths *ls_datapaths, struct hmap *lbs); -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, + struct hmap *lr_ports); #endif /* NORTHD_H */ diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 4424a1f64d..37bfc3e0cf 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -10346,3 +10346,52 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([SB port binding incremental processing]) +ovn_start + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0-port1 +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:03 10.0.0.3" + +# Create a logical router and attach both logical switches +check ovn-nbctl lr-add lr0 +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 +# northd engine node should have only one recompute count. +node_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) +check test "$node_recompute_ct" -eq "1" +lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) +check test "$node_recompute_ct" -eq "1" + +# Add a gw port to lr0 +ovn-sbctl chassis-add gw1 geneve 127.0.0.1 +ovn-nbctl --wait=sb sync +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb lrp-add lr0 lr0-public 00:00:00:00:ff:01 10.0.0.1/24 + +node_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) +check test "$node_recompute_ct" -eq "1" +lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) +check test "$node_recompute_ct" -eq "1" + +as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg + +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl lrp-set-gateway-chassis lr0-public gw1 10 +node_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) +# recompute count would be 2 because we don't handle SB HA_Chassis_Group changes. +check test "$node_recompute_ct" -eq "2" +lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) +check test "$node_recompute_ct" -eq "2" + +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl set logical_router_port lr0-public options:foo=bar +node_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute) +check test "$node_recompute_ct" -eq "1" +lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute) +check test "$node_recompute_ct" -eq "1" + +AT_CLEANUP +])