From patchwork Thu Jan 11 15:28:27 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1885670 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 4T9pVD2b5Tz1yPf for ; Fri, 12 Jan 2024 02:28:52 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 6F06F813B2; Thu, 11 Jan 2024 15:28:50 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 6F06F813B2 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 X_LfkKen5arn; Thu, 11 Jan 2024 15:28:48 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp1.osuosl.org (Postfix) with ESMTPS id 7D48781330; Thu, 11 Jan 2024 15:28:47 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 7D48781330 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4FB44C007C; Thu, 11 Jan 2024 15:28:47 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) by lists.linuxfoundation.org (Postfix) with ESMTP id C2CEAC0077 for ; Thu, 11 Jan 2024 15:28:45 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 9150B81330 for ; Thu, 11 Jan 2024 15:28:45 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 9150B81330 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 qGGdt64TlAMZ for ; Thu, 11 Jan 2024 15:28:43 +0000 (UTC) Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by smtp1.osuosl.org (Postfix) with ESMTPS id 6DA718130A for ; Thu, 11 Jan 2024 15:28:43 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 6DA718130A Received: by mail.gandi.net (Postfix) with ESMTPSA id ECC9540008; Thu, 11 Jan 2024 15:28:39 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Thu, 11 Jan 2024 10:28:27 -0500 Message-ID: <20240111152827.2789937-1-numans@ovn.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240111152752.2789854-1-numans@ovn.org> References: <20240111152752.2789854-1-numans@ovn.org> MIME-Version: 1.0 X-GND-Sasl: numans@ovn.org Subject: [ovs-dev] [PATCH ovn v5 01/16] northd: Refactor the northd change tracking. 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 northd engine tracking data now has the following tracking data - changed ovn_ports (right now only changed logical switch ports are tracked.) - changed load balancers. This separation becomes easier to add lflow handling for these changes in lflow northd engine handler. This patch doesn't handle the load balancer changes in lflow handler. It will be handled in upcoming commits. Before this patch, any changes to load balancers or lb groups resulted in full recompute of 'sync_to_sb_lb' and 'lflow' engine nodes. Now this scenario is optimized and it results in recomputes of these nodes only if 'northd' engine node adds any changed load balancers in its tracked data. One example is created or updating an empty load balancer group. Signed-off-by: Numan Siddique Acked-by: Dumitru Ceara Acked-by: Han Zhou --- northd/en-lflow.c | 12 +- northd/en-northd.c | 18 +- northd/en-sync-from-sb.c | 2 +- northd/en-sync-sb.c | 9 +- northd/northd.c | 436 ++++++++++++++++++++------------------- northd/northd.h | 86 +++++--- tests/ovn-northd.at | 10 +- 7 files changed, 313 insertions(+), 260 deletions(-) diff --git a/northd/en-lflow.c b/northd/en-lflow.c index 2b84fef0ef..6ba26006e0 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -104,12 +104,12 @@ lflow_northd_handler(struct engine_node *node, void *data) { struct northd_data *northd_data = engine_get_input_data("northd", node); - if (!northd_data->change_tracked) { + if (!northd_has_tracked_data(&northd_data->trk_data)) { return false; } - /* Fall back to recompute if lb related data has changed. */ - if (northd_data->lb_changed) { + /* Fall back to recompute if load balancers have changed. */ + if (northd_has_lbs_in_tracked_data(&northd_data->trk_data)) { return false; } @@ -119,9 +119,9 @@ lflow_northd_handler(struct engine_node *node, struct lflow_input lflow_input; lflow_get_input_data(node, &lflow_input); - if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn, - &northd_data->tracked_ls_changes, - &lflow_input, &lflow_data->lflows)) { + if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn, + &northd_data->trk_data.trk_lsps, + &lflow_input, &lflow_data->lflows)) { return false; } diff --git a/northd/en-northd.c b/northd/en-northd.c index aa0f20f0c2..28559ed211 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -171,7 +171,7 @@ northd_nb_logical_switch_handler(struct engine_node *node, return false; } - if (nd->change_tracked) { + if (northd_has_lsps_in_tracked_data(&nd->trk_data)) { engine_set_node_state(node, EN_UPDATED); } @@ -209,10 +209,6 @@ northd_nb_logical_router_handler(struct engine_node *node, return false; } - if (nd->change_tracked) { - engine_set_node_state(node, EN_UPDATED); - } - return true; } @@ -230,15 +226,15 @@ northd_lb_data_handler(struct engine_node *node, void *data) &nd->ls_datapaths, &nd->lr_datapaths, &nd->lb_datapaths_map, - &nd->lb_group_datapaths_map)) { + &nd->lb_group_datapaths_map, + &nd->trk_data)) { return false; } - /* Indicate the depedendant engine nodes that load balancer/group - * related data has changed (including association to logical - * switch/router). */ - nd->lb_changed = true; - engine_set_node_state(node, EN_UPDATED); + if (northd_has_lbs_in_tracked_data(&nd->trk_data)) { + engine_set_node_state(node, EN_UPDATED); + } + return true; } diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c index 48b1576173..8c05239b49 100644 --- a/northd/en-sync-from-sb.c +++ b/northd/en-sync-from-sb.c @@ -65,7 +65,7 @@ sync_from_sb_northd_handler(struct engine_node *node, void *data OVS_UNUSED) { struct northd_data *nd = engine_get_input_data("northd", node); - if (nd->change_tracked) { + if (northd_has_tracked_data(&nd->trk_data)) { /* So far the changes tracked in northd don't impact this node. * * In particular, for the LS related changes, the only field this node diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 2ec3bf54f8..3aaab8d005 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -236,7 +236,8 @@ 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 || nd->lb_changed) { + if (!northd_has_tracked_data(&nd->trk_data) || + northd_has_lbs_in_tracked_data(&nd->trk_data)) { /* Return false if no tracking data or if lbs changed. */ return false; } @@ -306,11 +307,13 @@ sync_to_sb_pb_northd_handler(struct engine_node *node, void *data OVS_UNUSED) } struct northd_data *nd = engine_get_input_data("northd", node); - if (!nd->change_tracked) { + if (!northd_has_tracked_data(&nd->trk_data) || + northd_has_lbs_in_tracked_data(&nd->trk_data)) { + /* Return false if no tracking data or if lbs changed. */ return false; } - if (!sync_pbs_for_northd_ls_changes(&nd->tracked_ls_changes)) { + if (!sync_pbs_for_northd_changed_ovn_ports(&nd->trk_data.trk_lsps)) { return false; } diff --git a/northd/northd.c b/northd/northd.c index 952f8200d4..f32e3bf21a 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4910,21 +4910,20 @@ sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) } /* Sync the SB Port bindings for the added and updated logical switch ports - * of the tracked logical switches (from the northd engine node). */ + * of the tracked northd engine data. */ bool -sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *ls_changes) +sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports) { - struct ls_change *ls_change; - LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) { - struct ovn_port *op; - - LIST_FOR_EACH (op, list, &ls_change->added_ports) { - sync_pb_for_op(op); - } + struct hmapx_node *hmapx_node; + struct ovn_port *op; + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { + op = hmapx_node->data; + sync_pb_for_op(op); + } - LIST_FOR_EACH (op, list, &ls_change->updated_ports) { - sync_pb_for_op(op); - } + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { + op = hmapx_node->data; + sync_pb_for_op(op); } return true; @@ -5126,34 +5125,68 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn, } static void -destroy_tracked_ls_change(struct ls_change *ls_change) +destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports) { - struct ovn_port *op; - LIST_FOR_EACH (op, list, &ls_change->added_ports) { - ovs_list_remove(&op->list); - } - LIST_FOR_EACH (op, list, &ls_change->updated_ports) { - ovs_list_remove(&op->list); + struct hmapx_node *hmapx_node; + HMAPX_FOR_EACH_SAFE (hmapx_node, &trk_ovn_ports->deleted) { + ovn_port_destroy_orphan(hmapx_node->data); + hmapx_delete(&trk_ovn_ports->deleted, hmapx_node); } - LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { - ovs_list_remove(&op->list); - ovn_port_destroy_orphan(op); + + hmapx_clear(&trk_ovn_ports->created); + hmapx_clear(&trk_ovn_ports->updated); +} + +static void +destroy_tracked_lbs(struct tracked_lbs *trk_lbs) +{ + struct hmapx_node *hmapx_node; + HMAPX_FOR_EACH_SAFE (hmapx_node, &trk_lbs->deleted) { + ovn_lb_datapaths_destroy(hmapx_node->data); + hmapx_delete(&trk_lbs->deleted, hmapx_node); } + + hmapx_clear(&trk_lbs->crupdated); +} + +static void +add_op_to_northd_tracked_ports(struct hmapx *tracked_ovn_ports, + struct ovn_port *op) +{ + hmapx_add(tracked_ovn_ports, op); } void destroy_northd_data_tracked_changes(struct northd_data *nd) { - struct ls_change *ls_change; - LIST_FOR_EACH_SAFE (ls_change, list_node, - &nd->tracked_ls_changes.updated) { - destroy_tracked_ls_change(ls_change); - ovs_list_remove(&ls_change->list_node); - free(ls_change); - } + struct northd_tracked_data *trk_changes = &nd->trk_data; + destroy_tracked_ovn_ports(&trk_changes->trk_lsps); + destroy_tracked_lbs(&trk_changes->trk_lbs); + nd->trk_data.type = NORTHD_TRACKED_NONE; +} + +static void +init_northd_tracked_data(struct northd_data *nd) +{ + struct northd_tracked_data *trk_data = &nd->trk_data; + trk_data->type = NORTHD_TRACKED_NONE; + hmapx_init(&trk_data->trk_lsps.created); + hmapx_init(&trk_data->trk_lsps.updated); + hmapx_init(&trk_data->trk_lsps.deleted); + hmapx_init(&trk_data->trk_lbs.crupdated); + hmapx_init(&trk_data->trk_lbs.deleted); +} - nd->change_tracked = false; - nd->lb_changed = false; +static void +destroy_northd_tracked_data(struct northd_data *nd) +{ + struct northd_tracked_data *trk_data = &nd->trk_data; + trk_data->type = NORTHD_TRACKED_NONE; + hmapx_destroy(&trk_data->trk_lsps.created); + hmapx_destroy(&trk_data->trk_lsps.updated); + hmapx_destroy(&trk_data->trk_lsps.deleted); + hmapx_destroy(&trk_data->trk_lbs.crupdated); + hmapx_destroy(&trk_data->trk_lbs.deleted); } /* Check if a changed LSP can be handled incrementally within the I-P engine @@ -5358,12 +5391,11 @@ check_lsp_changes_other_than_up(const struct nbrec_logical_switch_port *nbsp) */ static bool ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, - const struct nbrec_logical_switch *changed_ls, - const struct northd_input *ni, - struct northd_data *nd, - struct ovn_datapath *od, - struct ls_change *ls_change, - bool *updated) + const struct nbrec_logical_switch *changed_ls, + const struct northd_input *ni, + struct northd_data *nd, + struct ovn_datapath *od, + struct tracked_ovn_ports *trk_lsps) { bool ls_ports_changed = false; if (!nbrec_logical_switch_is_updated(changed_ls, @@ -5384,7 +5416,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, return true; } - ls_change->had_only_router_ports = (od->n_router_ports + bool ls_had_only_router_ports = (od->n_router_ports && (od->n_router_ports == hmap_count(&od->ports))); struct ovn_port *op; @@ -5410,8 +5442,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, if (!op) { goto fail; } - ovs_list_push_back(&ls_change->added_ports, - &op->list); + add_op_to_northd_tracked_ports(&trk_lsps->created, op); } else if (ls_port_has_changed(new_nbsp)) { /* Existing port updated */ bool temp = false; @@ -5447,7 +5478,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, if (!op) { goto fail; } - ovs_list_push_back(&ls_change->updated_ports, &op->list); + add_op_to_northd_tracked_ports(&trk_lsps->updated, op); } op->visited = true; } @@ -5456,15 +5487,14 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { if (!op->visited) { if (!op->lsp_can_be_inc_processed) { - goto fail_clean_deleted; + goto fail; } if (sset_contains(&nd->svc_monitor_lsps, op->key)) { /* This port was used for svc monitor, which may be * impacted by this deletion. Fallback to recompute. */ - goto fail_clean_deleted; + goto fail; } - ovs_list_push_back(&ls_change->deleted_ports, - &op->list); + add_op_to_northd_tracked_ports(&trk_lsps->deleted, op); hmap_remove(&nd->ls_ports, &op->key_node); hmap_remove(&od->ports, &op->dp_node); sbrec_port_binding_delete(op->sb); @@ -5473,27 +5503,32 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, } } - if (!ovs_list_is_empty(&ls_change->added_ports) || - !ovs_list_is_empty(&ls_change->updated_ports) || - !ovs_list_is_empty(&ls_change->deleted_ports)) { - *updated = true; + bool ls_has_only_router_ports = (od->n_router_ports + && (od->n_router_ports == hmap_count(&od->ports))); + + /* There are lflows related to router ports that depends on whether + * there are switch ports on the logical switch (see + * build_lswitch_rport_arp_req_flow() for more details). Check if this + * dependency has changed and if it has, then add the router ports + * to the tracked 'updated' ovn ports so that lflow engine can + * regenerate lflows for these router ports. */ + if (ls_had_only_router_ports != ls_has_only_router_ports) { + for (size_t i = 0; i < od->n_router_ports; i++) { + op = od->router_ports[i]; + add_op_to_northd_tracked_ports(&trk_lsps->updated, op); + } } return true; -fail_clean_deleted: - LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { - ovn_port_destroy_orphan(op); - } - fail: + destroy_tracked_ovn_ports(trk_lsps); return false; } /* Return true if changes are handled incrementally, false otherwise. * When there are any changes, try to track what's exactly changed and set - * northd_data->change_tracked accordingly: change tracked - true, otherwise, - * false. + * northd_data->trk_data accordingly. * * Note: Changes to load balancer and load balancer groups associated with * the logical switches are handled separately in the lb_data change handlers. @@ -5504,7 +5539,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, struct northd_data *nd) { const struct nbrec_logical_switch *changed_ls; - struct ls_change *ls_change = NULL; + struct northd_tracked_data *trk_data = &nd->trk_data; NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls, ni->nbrec_logical_switch_table) { @@ -5528,31 +5563,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, goto fail; } - ls_change = xzalloc(sizeof *ls_change); - ls_change->od = od; - ovs_list_init(&ls_change->added_ports); - ovs_list_init(&ls_change->deleted_ports); - ovs_list_init(&ls_change->updated_ports); - - bool updated = false; if (!ls_handle_lsp_changes(ovnsb_idl_txn, changed_ls, - ni, nd, od, ls_change, - &updated)) { - destroy_tracked_ls_change(ls_change); - free(ls_change); + ni, nd, od, &trk_data->trk_lsps)) { goto fail; } - - if (updated) { - ovs_list_push_back(&nd->tracked_ls_changes.updated, - &ls_change->list_node); - } else { - free(ls_change); - } } - if (!ovs_list_is_empty(&nd->tracked_ls_changes.updated)) { - nd->change_tracked = true; + if (!hmapx_is_empty(&trk_data->trk_lsps.created) + || !hmapx_is_empty(&trk_data->trk_lsps.updated) + || !hmapx_is_empty(&trk_data->trk_lsps.deleted)) { + trk_data->type |= NORTHD_TRACKED_PORTS; } return true; @@ -5617,13 +5637,10 @@ lr_changes_can_be_handled( } /* Return true if changes are handled incrementally, false otherwise. - * When there are any changes, try to track what's exactly changed and set - * northd_data->change_tracked accordingly: change tracked - true, otherwise, - * false. + * * Note: Changes to load balancer and load balancer groups associated with * the logical routers are handled separately in the lb_data change - * handlers (northd_handle_lb_data_changes_pre_od and - * northd_handle_lb_data_changes_post_od). + * handler - northd_handle_lb_data_changes(). * */ bool northd_handle_lr_changes(const struct northd_input *ni, @@ -5729,7 +5746,8 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, struct ovn_datapaths *ls_datapaths, struct ovn_datapaths *lr_datapaths, struct hmap *lb_datapaths_map, - struct hmap *lbgrp_datapaths_map) + struct hmap *lbgrp_datapaths_map, + struct northd_tracked_data *nd_changes) { if (trk_lb_data->has_health_checks) { /* Fall back to recompute since a tracked load balancer @@ -5792,7 +5810,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, } hmap_remove(lb_datapaths_map, &lb_dps->hmap_node); - ovn_lb_datapaths_destroy(lb_dps); + + /* Add the deleted lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.deleted, lb_dps); } /* Create the 'lb_dps' if not already created for each @@ -5809,6 +5829,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, hmap_insert(lb_datapaths_map, &lb_dps->hmap_node, uuid_hash(lb_uuid)); } + + /* Add the updated lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps); } struct ovn_lb_group_datapaths *lbgrp_dps; @@ -5839,6 +5862,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, &uuidnode->uuid); ovs_assert(lb_dps); ovn_lb_datapaths_add_ls(lb_dps, 1, &od); + + /* Add the lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps); } UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) { @@ -5854,6 +5880,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid); ovs_assert(lb_dps); ovn_lb_datapaths_add_ls(lb_dps, 1, &od); + + /* Add the lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps); } } @@ -5874,6 +5903,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, /* Add the lb_ips of lb_dps to the od. */ build_lrouter_lb_ips(od->lb_ips, lb_dps->lb); build_lrouter_lb_reachable_ips(od, lb_dps->lb); + + /* Add the lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps); } UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) { @@ -5893,6 +5925,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, /* Add the lb_ips of lb_dps to the od. */ build_lrouter_lb_ips(od->lb_ips, lb_dps->lb); build_lrouter_lb_reachable_ips(od, lb_dps->lb); + + /* Add the lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps); } } @@ -5971,9 +6006,17 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, /* Re-evaluate 'od->has_lb_vip' */ init_lb_for_datapath(od); } + + /* Add the lb to the northd tracked data. */ + hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps); } } + if (!hmapx_is_empty(&nd_changes->trk_lbs.crupdated) + || !hmapx_is_empty(&nd_changes->trk_lbs.deleted)) { + nd_changes->type |= NORTHD_TRACKED_LBS; + } + return true; } @@ -17029,149 +17072,122 @@ delete_lflow_for_lsp(struct ovn_port *op, bool is_update, return true; } -bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_ls_changes *ls_changes, - struct lflow_input *lflow_input, - struct hmap *lflows) +bool +lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, + struct tracked_ovn_ports *trk_lsps, + struct lflow_input *lflow_input, + struct hmap *lflows) { - struct ls_change *ls_change; + struct hmapx_node *hmapx_node; + struct ovn_port *op; - LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) { - const struct sbrec_multicast_group *sbmc_flood = - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, - MC_FLOOD, ls_change->od->sb); - const struct sbrec_multicast_group *sbmc_flood_l2 = - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, - MC_FLOOD_L2, ls_change->od->sb); - const struct sbrec_multicast_group *sbmc_unknown = - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, - MC_UNKNOWN, ls_change->od->sb); + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->deleted) { + op = hmapx_node->data; + /* Make sure 'op' is an lsp and not lrp. */ + ovs_assert(op->nbsp); - struct ovn_port *op; - LIST_FOR_EACH (op, list, &ls_change->deleted_ports) { - if (!delete_lflow_for_lsp(op, false, - lflow_input->sbrec_logical_flow_table, - lflows)) { + if (!delete_lflow_for_lsp(op, false, + lflow_input->sbrec_logical_flow_table, + lflows)) { return false; } - /* No need to update SB multicast groups, thanks to weak - * references. */ - } + /* No need to update SB multicast groups, thanks to weak + * references. */ + } - LIST_FOR_EACH (op, list, &ls_change->updated_ports) { - /* Delete old lflows. */ - if (!delete_lflow_for_lsp(op, true, - lflow_input->sbrec_logical_flow_table, - lflows)) { - return false; - } + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->updated) { + op = hmapx_node->data; + /* Make sure 'op' is an lsp and not lrp. */ + ovs_assert(op->nbsp); - /* Generate new lflows. */ - struct ds match = DS_EMPTY_INITIALIZER; - struct ds actions = DS_EMPTY_INITIALIZER; - build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports, - lflow_input->lr_ports, - lflow_input->meter_groups, - &match, &actions, - lflows); - ds_destroy(&match); - ds_destroy(&actions); - - /* SB port_binding is not deleted, so don't update SB multicast - * groups. */ - - /* Sync the new flows to SB. */ - struct lflow_ref_node *lfrn; - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, - lfrn->lflow); - } + /* Delete old lflows. */ + if (!delete_lflow_for_lsp(op, true, + lflow_input->sbrec_logical_flow_table, + lflows)) { + return false; } - LIST_FOR_EACH (op, list, &ls_change->added_ports) { - struct ds match = DS_EMPTY_INITIALIZER; - struct ds actions = DS_EMPTY_INITIALIZER; - build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports, - lflow_input->lr_ports, - lflow_input->meter_groups, - &match, &actions, - lflows); - ds_destroy(&match); - ds_destroy(&actions); - - /* Update SB multicast groups for the new port. */ - if (!sbmc_flood) { - sbmc_flood = create_sb_multicast_group(ovnsb_txn, - ls_change->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY); - } - sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb); + /* Generate new lflows. */ + struct ds match = DS_EMPTY_INITIALIZER; + struct ds actions = DS_EMPTY_INITIALIZER; + build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports, + lflow_input->lr_ports, + lflow_input->meter_groups, + &match, &actions, + lflows); + ds_destroy(&match); + ds_destroy(&actions); - if (!sbmc_flood_l2) { - sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn, - ls_change->od->sb, MC_FLOOD_L2, - OVN_MCAST_FLOOD_L2_TUNNEL_KEY); - } - sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2, op->sb); + /* SB port_binding is not deleted, so don't update SB multicast + * groups. */ - if (op->has_unknown) { - if (!sbmc_unknown) { - sbmc_unknown = create_sb_multicast_group(ovnsb_txn, - ls_change->od->sb, MC_UNKNOWN, - OVN_MCAST_UNKNOWN_TUNNEL_KEY); - } - sbrec_multicast_group_update_ports_addvalue(sbmc_unknown, - op->sb); - } - - /* Sync the newly added flows to SB. */ - struct lflow_ref_node *lfrn; - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, - lfrn->lflow); - } + /* Sync the new flows to SB. */ + struct lflow_ref_node *lfrn; + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, + lfrn->lflow); } + } - bool ls_has_only_router_ports = (ls_change->od->n_router_ports && - (ls_change->od->n_router_ports == - hmap_count(&ls_change->od->ports))); - - if (ls_change->had_only_router_ports != ls_has_only_router_ports) { - /* There are lflows related to router ports that depends on whether - * there are switch ports on the logical switch (see - * build_lswitch_rport_arp_req_flow() for more details). Since this - * dependency changed, we need to regenerate lflows for each router - * port on this logical switch. */ - for (size_t i = 0; i < ls_change->od->n_router_ports; i++) { - op = ls_change->od->router_ports[i]; - - /* Delete old lflows. */ - if (!delete_lflow_for_lsp(op, "affected router", - lflow_input->sbrec_logical_flow_table, - lflows)) { - return false; - } + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) { + op = hmapx_node->data; + /* Make sure 'op' is an lsp and not lrp. */ + ovs_assert(op->nbsp); - /* Generate new lflows. */ - struct ds match = DS_EMPTY_INITIALIZER; - struct ds actions = DS_EMPTY_INITIALIZER; - build_lswitch_and_lrouter_iterate_by_lsp(op, - lflow_input->ls_ports, lflow_input->lr_ports, - lflow_input->meter_groups, &match, &actions, lflows); - ds_destroy(&match); - ds_destroy(&actions); - - /* Sync the new flows to SB. */ - struct lflow_ref_node *lfrn; - LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { - sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, - lfrn->lflow); - } + const struct sbrec_multicast_group *sbmc_flood = + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, + MC_FLOOD, op->od->sb); + const struct sbrec_multicast_group *sbmc_flood_l2 = + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, + MC_FLOOD_L2, op->od->sb); + const struct sbrec_multicast_group *sbmc_unknown = + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, + MC_UNKNOWN, op->od->sb); + + struct ds match = DS_EMPTY_INITIALIZER; + struct ds actions = DS_EMPTY_INITIALIZER; + build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports, + lflow_input->lr_ports, + lflow_input->meter_groups, + &match, &actions, + lflows); + ds_destroy(&match); + ds_destroy(&actions); + + /* Update SB multicast groups for the new port. */ + if (!sbmc_flood) { + sbmc_flood = create_sb_multicast_group(ovnsb_txn, + op->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY); + } + sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb); + + if (!sbmc_flood_l2) { + sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn, + op->od->sb, MC_FLOOD_L2, + OVN_MCAST_FLOOD_L2_TUNNEL_KEY); + } + sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2, op->sb); + + if (op->has_unknown) { + if (!sbmc_unknown) { + sbmc_unknown = create_sb_multicast_group(ovnsb_txn, + op->od->sb, MC_UNKNOWN, + OVN_MCAST_UNKNOWN_TUNNEL_KEY); } + sbrec_multicast_group_update_ports_addvalue(sbmc_unknown, + op->sb); + } + + /* Sync the newly added flows to SB. */ + struct lflow_ref_node *lfrn; + LIST_FOR_EACH (lfrn, lflow_list_node, &op->lflows) { + sync_lsp_lflows_to_sb(ovnsb_txn, lflow_input, lflows, + lfrn->lflow); } } - return true; + return true; } static bool @@ -17816,8 +17832,7 @@ northd_init(struct northd_data *data) data->ovn_internal_version_changed = false; sset_init(&data->svc_monitor_lsps); hmap_init(&data->svc_monitor_map); - data->change_tracked = false; - ovs_list_init(&data->tracked_ls_changes.updated); + init_northd_tracked_data(data); } void @@ -17857,6 +17872,7 @@ northd_destroy(struct northd_data *data) destroy_debug_config(); sset_destroy(&data->svc_monitor_lsps); + destroy_northd_tracked_data(data); } void diff --git a/northd/northd.h b/northd/northd.h index 5be7b5384d..23521065e8 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -83,22 +83,44 @@ struct ovn_datapaths { struct ovn_datapath **array; }; -/* Track what's changed for a single LS. - * Now only track port changes. */ -struct ls_change { - struct ovs_list list_node; - struct ovn_datapath *od; - struct ovs_list added_ports; - struct ovs_list deleted_ports; - struct ovs_list updated_ports; - bool had_only_router_ports; +struct tracked_ovn_ports { + /* tracked created ports. + * hmapx node data is 'struct ovn_port *' */ + struct hmapx created; + + /* tracked updated ports. + * hmapx node data is 'struct ovn_port *' */ + struct hmapx updated; + + /* tracked deleted ports. + * hmapx node data is 'struct ovn_port *' */ + struct hmapx deleted; }; -/* Track what's changed for logical switches. - * Now only track updated ones (added or deleted may be supported in the - * future). */ -struct tracked_ls_changes { - struct ovs_list updated; /* Contains struct ls_change */ +struct tracked_lbs { + /* Tracked created or updated load balancers. + * hmapx node data is 'struct ovn_lb_datapaths' */ + struct hmapx crupdated; + + /* Tracked deleted lbs. + * hmapx node data is 'struct ovn_lb_datapaths' */ + struct hmapx deleted; +}; + +enum northd_tracked_data_type { + NORTHD_TRACKED_NONE, + NORTHD_TRACKED_PORTS = (1 << 0), + NORTHD_TRACKED_LBS = (1 << 1), +}; + +/* Track what's changed in the northd engine node. + * Now only tracks ovn_ports (of vif type) - created, updated + * and deleted. */ +struct northd_tracked_data { + /* Indicates the type of data tracked. One or all of NORTHD_TRACKED_*. */ + enum northd_tracked_data_type type; + struct tracked_ovn_ports trk_lsps; + struct tracked_lbs trk_lbs; }; struct northd_data { @@ -114,10 +136,9 @@ struct northd_data { struct chassis_features features; struct sset svc_monitor_lsps; struct hmap svc_monitor_map; - bool change_tracked; - struct tracked_ls_changes tracked_ls_changes; - bool lb_changed; /* Indicates if load balancers changed or association of - * load balancer to logical switch/router changed. */ + + /* Change tracking data. */ + struct northd_tracked_data trk_data; }; struct lflow_data { @@ -338,9 +359,10 @@ void northd_indices_create(struct northd_data *data, void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, struct lflow_input *input_data, struct hmap *lflows); -bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, - struct tracked_ls_changes *, - struct lflow_input *, struct hmap *lflows); +bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, + struct tracked_ovn_ports *, + struct lflow_input *, + struct hmap *lflows); bool northd_handle_sb_port_binding_changes( const struct sbrec_port_binding_table *, struct hmap *ls_ports); @@ -349,7 +371,8 @@ bool northd_handle_lb_data_changes(struct tracked_lb_data *, struct ovn_datapaths *ls_datapaths, struct ovn_datapaths *lr_datapaths, struct hmap *lb_datapaths_map, - struct hmap *lbgrp_datapaths_map); + struct hmap *lbgrp_datapaths_map, + struct northd_tracked_data *); void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn, const struct nbrec_bfd_table *, @@ -372,6 +395,23 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); -bool sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *); +bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *); + +static inline bool +northd_has_tracked_data(struct northd_tracked_data *trk_nd_changes) { + return trk_nd_changes->type != NORTHD_TRACKED_NONE; +} + +static inline bool +northd_has_lbs_in_tracked_data(struct northd_tracked_data *trk_nd_changes) +{ + return (trk_nd_changes->type & NORTHD_TRACKED_LBS); +} + +static inline bool +northd_has_lsps_in_tracked_data(struct northd_tracked_data *trk_nd_changes) +{ + return (trk_nd_changes->type & NORTHD_TRACKED_PORTS); +} #endif /* NORTHD_H */ diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 9a0d418e46..7bcd113d8f 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -10644,9 +10644,8 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats lbg1_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg1) check_engine_stats lb_data norecompute compute check_engine_stats northd norecompute compute -check_engine_stats lflow recompute nocompute -check_engine_stats sync_to_sb_lb recompute nocompute - +check_engine_stats lflow norecompute nocompute +check_engine_stats sync_to_sb_lb norecompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats @@ -10674,7 +10673,6 @@ check_engine_stats lb_data norecompute compute check_engine_stats northd norecompute compute check_engine_stats lflow recompute nocompute check_engine_stats sync_to_sb_lb recompute nocompute - CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats @@ -10822,8 +10820,8 @@ check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats lbg1_uuid=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg1) check_engine_stats lb_data norecompute compute check_engine_stats northd norecompute compute -check_engine_stats lflow recompute nocompute -check_engine_stats sync_to_sb_lb recompute nocompute +check_engine_stats lflow norecompute nocompute +check_engine_stats sync_to_sb_lb norecompute nocompute check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats check ovn-nbctl --wait=sb set load_balancer_group . load_balancer="$lb2_uuid,$lb3_uuid,$lb4_uuid"