On Fri, Jul 7, 2023 at 1:56 PM <num...@ovn.org> wrote:
>
> From: Numan Siddique <num...@ovn.org>
>
> 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 <num...@ovn.org>
> ---
> 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;
> + }
I think lrouters_changed is not needed. If lrouters related data is
changed, we should set change_tracked to false (call the
destroy_northd_data_tracked_changes), because lrouter data is part of
northd_data nad we don't track lrouter changes yet.
> +
> const struct engine_context *eng_ctx = engine_get_context();
> struct lflow_data *lflow_data = data;
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index b3c03c54bd..a321eff323 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -188,6 +188,26 @@ northd_nb_logical_switch_handler(struct engine_node
*node,
> return true;
> }
>
> +bool
> +northd_nb_logical_router_handler(struct engine_node *node,
> + void *data)
> +{
> + struct northd_data *nd = data;
> + struct northd_input input_data;
> +
> + northd_get_input_data(node, &input_data);
> +
> + if (!northd_handle_lr_changes(&input_data, nd)) {
> + return false;
> + }
> +
> + if (nd->change_tracked) {
> + engine_set_node_state(node, EN_UPDATED);
change_tracked is supposed to indicate if the changes in the engine node is
tracked, meaning the node depending on this one can potentailly
incrementally process the changes.
It is possible that the node is updated but the changes are not tracked. In
this case most likely the nodes depending on it will need to be recomputed.
Whether the node state is EN_UPDATED depends on if the node is updated. I
think the northd_handle_lr_changes can take a parameter "updated" to
indicate if it is updated.
> + }
> +
> + return true;
> +}
> +
> bool
> northd_sb_port_binding_handler(struct engine_node *node,
> void *data)
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 5674f4390c..739aa5e87f 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -16,6 +16,7 @@ void en_northd_cleanup(void *data);
> void en_northd_clear_tracked_data(void *data);
> bool northd_nb_nb_global_handler(struct engine_node *, void *data
OVS_UNUSED);
> bool northd_nb_logical_switch_handler(struct engine_node *, void *data);
> +bool northd_nb_logical_router_handler(struct engine_node *, void *data);
> bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> bool northd_northd_lb_data_handler(struct engine_node *, void *data);
>
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index f5b7998f1f..362dd5e87a 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -156,7 +156,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>
> engine_add_input(&en_northd, &en_nb_port_group, NULL);
> engine_add_input(&en_northd, &en_nb_acl, NULL);
> - engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> engine_add_input(&en_northd, &en_nb_mirror, NULL);
> engine_add_input(&en_northd, &en_nb_meter, NULL);
> engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> @@ -185,6 +184,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> northd_nb_nb_global_handler);
> engine_add_input(&en_northd, &en_nb_logical_switch,
> northd_nb_logical_switch_handler);
> + engine_add_input(&en_northd, &en_nb_logical_router,
> + northd_nb_logical_router_handler);
>
> engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index a7761285de..4393adc2ad 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4021,6 +4021,90 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od,
> }
> }
>
> +static void
> +associate_lr_lbs(struct ovn_datapath *lr_od, struct hmap
*lb_datapaths_map)
> +{
> + struct ovn_lb_datapaths *lb_dps;
> +
> + free(lr_od->lb_uuids);
> + lr_od->lb_uuids = xcalloc(lr_od->nbr->n_load_balancer,
> + sizeof *lr_od->lb_uuids);
> + lr_od->n_lb_uuids = lr_od->nbr->n_load_balancer;
> +
> + if (!lr_od->lb_ips) {
> + lr_od->lb_ips = ovn_lb_ip_set_create();
> + }
> +
> + for (size_t i = 0; i < lr_od->nbr->n_load_balancer; i++) {
> + const struct uuid *lb_uuid =
> + &lr_od->nbr->load_balancer[i]->header_.uuid;
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> + ovs_assert(lb_dps);
> + ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> + build_lrouter_lb_ips(lr_od->lb_ips, lb_dps->lb);
> + lr_od->lb_uuids[i] = *lb_uuid;
> + }
> +}
> +
> +static void
> +associate_lr_lb_groups(struct ovn_datapath *lr_od,
> + struct hmap *lb_group_datapaths_map,
> + struct hmap *lb_datapaths_map)
> +{
> + const struct nbrec_load_balancer_group *nbrec_lb_group;
> + struct ovn_lb_group_datapaths *lb_group_dps;
> +
> + free(lr_od->lb_group_uuids);
> + lr_od->lb_group_uuids = xcalloc(lr_od->nbr->n_load_balancer_group,
> + sizeof *lr_od->lb_group_uuids);
> + lr_od->n_lb_group_uuids = lr_od->nbr->n_load_balancer_group;
> +
> + /* Checking load balancer groups first, starting from the largest
one,
> + * to more efficiently copy IP sets. */
> + size_t largest_group = 0;
> +
> + for (size_t i = 1; i < lr_od->nbr->n_load_balancer_group; i++) {
> + if (lr_od->nbr->load_balancer_group[i]->n_load_balancer >
> +
lr_od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> + largest_group = i;
> + }
> + }
> +
> + for (size_t i = 0; i < lr_od->nbr->n_load_balancer_group; i++) {
> + size_t idx = (i + largest_group) %
lr_od->nbr->n_load_balancer_group;
> +
> + nbrec_lb_group = lr_od->nbr->load_balancer_group[idx];
> + const struct uuid *lb_group_uuid = &nbrec_lb_group->header_.uuid;
> +
> + lb_group_dps =
> + ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> + lb_group_uuid);
> + ovs_assert(lb_group_dps);
> + ovn_lb_group_datapaths_add_lr(lb_group_dps, lr_od);
> +
> + if (!lr_od->lb_ips) {
> + lr_od->lb_ips =
> + ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> + } else {
> + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> + build_lrouter_lb_ips(lr_od->lb_ips,
> + lb_group_dps->lb_group->lbs[j]);
> + }
> + }
> +
> + lr_od->lb_group_uuids[i] = *lb_group_uuid;
> +
> + struct ovn_lb_datapaths *lb_dps;
> + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
> + const struct uuid *lb_uuid =
> + &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> + ovs_assert(lb_dps);
> + ovn_lb_datapaths_add_lr(lb_dps, 1, &lr_od);
> + }
> + }
> +}
> +
> static void
> build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups,
> struct ovn_datapaths *ls_datapaths,
> @@ -4028,7 +4112,6 @@ build_lb_datapaths(const struct hmap *lbs, const
struct hmap *lb_groups,
> struct hmap *lb_datapaths_map,
> struct hmap *lb_group_datapaths_map)
> {
> - const struct nbrec_load_balancer_group *nbrec_lb_group;
> struct ovn_lb_group_datapaths *lb_group_dps;
> const struct ovn_lb_group *lb_group;
> struct ovn_lb_datapaths *lb_dps;
> @@ -4062,53 +4145,8 @@ build_lb_datapaths(const struct hmap *lbs, const
struct hmap *lb_groups,
>
> HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
> ovs_assert(od->nbr);
> -
> - /* Checking load balancer groups first, starting from the
largest one,
> - * to more efficiently copy IP sets. */
> - size_t largest_group = 0;
> -
> - for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) {
> - if (od->nbr->load_balancer_group[i]->n_load_balancer >
> -
od->nbr->load_balancer_group[largest_group]->n_load_balancer) {
> - largest_group = i;
> - }
> - }
> -
> - for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> - size_t idx = (i + largest_group) %
od->nbr->n_load_balancer_group;
> -
> - nbrec_lb_group = od->nbr->load_balancer_group[idx];
> - const struct uuid *lb_group_uuid =
&nbrec_lb_group->header_.uuid;
> -
> - lb_group_dps =
> - ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> - lb_group_uuid);
> - ovs_assert(lb_group_dps);
> - ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
> -
> - if (!od->lb_ips) {
> - od->lb_ips =
> - ovn_lb_ip_set_clone(lb_group_dps->lb_group->lb_ips);
> - } else {
> - for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs;
j++) {
> - build_lrouter_lb_ips(od->lb_ips,
> - lb_group_dps->lb_group->lbs[j]);
> - }
> - }
> - }
> -
> - if (!od->lb_ips) {
> - od->lb_ips = ovn_lb_ip_set_create();
> - }
> -
> - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
> - const struct uuid *lb_uuid =
> - &od->nbr->load_balancer[i]->header_.uuid;
> - lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> - ovs_assert(lb_dps);
> - ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
> - build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
> - }
> + associate_lr_lb_groups(od, lb_group_datapaths_map,
lb_datapaths_map);
> + associate_lr_lbs(od, lb_datapaths_map);
> }
>
> HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_datapaths_map) {
> @@ -4978,6 +5016,7 @@ destroy_northd_data_tracked_changes(struct
northd_data *nd)
> }
>
> nd->change_tracked = false;
> + nd->lrouters_changed = false;
> }
>
> /* Check if a changed LSP can be handled incrementally within the I-P
engine
> @@ -5513,6 +5552,263 @@ fail:
> return false;
> }
>
> +/* Returns true if the logical router has changes which are not
> + * incrementally handled.
> + * Presently supports i-p for the below changes:
> + * - load balancers and load balancer groups.
> + */
> +static bool
> +check_unsupported_inc_proc_for_lr_changes(
> + const struct nbrec_logical_router *lr)
> +{
> + /* Check if the columns are changed in this row. */
> + enum nbrec_logical_router_column_id col;
> + for (col = 0; col < NBREC_LOGICAL_ROUTER_N_COLUMNS; col++) {
> + if (nbrec_logical_router_is_updated(lr, col)) {
> + if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER ||
> + col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP) {
> + continue;
> + }
> + return true;
> + }
> + }
> +
> + /* Check if the referenced rows are changed.
> + XXX: Need a better OVSDB IDL interface for this check. */
> + for (size_t i = 0; i < lr->n_ports; i++) {
> + if (nbrec_logical_router_port_row_get_seqno(lr->ports[i],
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + return true;
> + }
> + }
> + if (lr->copp && nbrec_copp_row_get_seqno(lr->copp,
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + return true;
> + }
> + for (size_t i = 0; i < lr->n_nat; i++) {
> + if (nbrec_nat_row_get_seqno(lr->nat[i],
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + return true;
> + }
> + }
> + for (size_t i = 0; i < lr->n_policies; i++) {
> + if (nbrec_logical_router_policy_row_get_seqno(lr->policies[i],
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + return true;
> + }
> + }
> + for (size_t i = 0; i < lr->n_static_routes; i++) {
> + if (nbrec_logical_router_static_route_row_get_seqno(
> + lr->static_routes[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static bool
> +lr_check_and_handle_lb_and_lbgrp_changes(
> + const struct nbrec_logical_router *changed_lr,
> + struct hmap *lb_datapaths_map, struct hmap *lb_group_datapaths_map,
> + struct ovn_datapath *od, bool *updated)
> +{
> + ovs_assert(od->nbr);
> + bool lbs_changed = true;
> + if (!nbrec_logical_router_is_updated(changed_lr,
> + NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER) &&
> + !nbrec_logical_router_is_updated(changed_lr,
> + NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP)) {
> + lbs_changed = false;
> + for (size_t i = 0; i < changed_lr->n_load_balancer_group; i++) {
> + if (nbrec_load_balancer_group_row_get_seqno(
> + changed_lr->load_balancer_group[i],
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + lbs_changed = true;
> + break;
> + }
> + }
> +
> + if (!lbs_changed) {
> + for (size_t i = 0; i < changed_lr->n_load_balancer; i++) {
> + if (nbrec_load_balancer_row_get_seqno(
> + changed_lr->load_balancer[i],
> + OVSDB_IDL_CHANGE_MODIFY) > 0) {
> + lbs_changed = true;
> + break;
> + }
> + }
> + }
> +
> + if (!lbs_changed) {
> + return true;
> + }
> + }
> +
> + /* We need to do 2 things to handle the router for load balancer or
> + * load balancer groups.
> + *
> + * 1. Disassociate the logical router datapath from the already
associated
> + * load balancers/load balancer groups.
> + * 2. Associate the logical router datapath with the updated
> + * load balancers/groups.
> + * */
> + struct ovn_lb_datapaths *lb_dps;
> + size_t i, j;
> + for (i = 0; i < od->n_lb_uuids; i++) {
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
&od->lb_uuids[i]);
> + if (lb_dps) {
> + if (lb_dps->lb->routable) {
> + /* Can't handler if the load balancer has routable flag
set.
> + * This requires updating the router ports's routable
> + * addresses. */
> + return false;
> + }
> + ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> + }
> + }
> +
> + struct ovn_lb_group_datapaths *lbg_dps;
> + for (i = 0; i < od->n_lb_group_uuids; i++) {
> + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> + &od->lb_group_uuids[i]);
> + if (lbg_dps) {
> + ovn_lb_group_datapaths_remove_lr(lbg_dps, od);
> +
> + if (install_ls_lb_from_router && od->n_ls_peers) {
> + ovn_lb_group_datapaths_remove_ls(lbg_dps, od->n_ls_peers,
> + od->ls_peers);
> + }
> + }
> +
> + for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> + const struct uuid *lb_uuid =
> + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> + if (lb_dps) {
> + if (lb_dps->lb->routable) {
> + /* Can't handler if the load balancer has routable
flag
> + * set. This requires updating the router ports's
> + * routable addresses. */
> + return false;
> + }
> + ovn_lb_datapaths_remove_lr(lb_dps, 1, &od);
> +
> + if (install_ls_lb_from_router && od->n_ls_peers) {
> + ovn_lb_datapaths_remove_ls(lb_dps, od->n_ls_peers,
> + od->ls_peers);
> + }
> + }
> + }
> + }
> +
> + ovn_lb_ip_set_destroy(od->lb_ips);
> + od->lb_ips = NULL;
> + associate_lr_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map);
> + associate_lr_lbs(od, lb_datapaths_map);
> +
> + /* Do the job of build_lrouter_lbs_reachable_ips and
> + * build_lswitch_lbs_from_lrouter for this lrouter datapath. */
> + for (i = 0; i < od->nbr->n_load_balancer; i++) {
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> +
&od->nbr->load_balancer[i]->header_.uuid);
> + ovs_assert(lb_dps);
> + if (lb_dps->lb->routable) {
> + /* Can't handler if the load balancer has routable flag set.
> + * This requires updating the router ports's routable
addresses. */
> + return false;
> + }
> + build_lrouter_lb_reachable_ips(od, lb_dps->lb);
> +
> + if (install_ls_lb_from_router && od->n_ls_peers) {
> + ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
od->ls_peers);
> + }
> + }
> +
> + for (i = 0; i < od->nbr->n_load_balancer_group; i++) {
> + const struct nbrec_load_balancer_group *nbrec_lb_group =
> + od->nbr->load_balancer_group[i];
> + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> +
&nbrec_lb_group->header_.uuid);
> + ovs_assert(lbg_dps);
> + if (install_ls_lb_from_router && od->n_ls_peers) {
> + ovn_lb_group_datapaths_add_ls(lbg_dps, od->n_ls_peers,
> + od->ls_peers);
> + }
> +
> + for (j = 0; j < lbg_dps->lb_group->n_lbs; j++) {
> + build_lrouter_lb_reachable_ips(od,
lbg_dps->lb_group->lbs[j]);
> +
> + if (install_ls_lb_from_router && od->n_ls_peers) {
> + const struct uuid *lb_uuid =
> + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid;
> + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
lb_uuid);
> + ovs_assert(lb_dps);
> + if (lb_dps->lb->routable) {
> + /* Can't handler if the load balancer has routable
flag
> + * set. This requires updating the router ports's
> + * routable addresses. */
> + return false;
> + }
> + ovn_lb_datapaths_add_ls(lb_dps, od->n_ls_peers,
od->ls_peers);
> + }
> + }
> + }
> +
> + *updated = true;
> + /* Re-evaluate 'od->has_lb_vip' */
> + init_lb_for_datapath(od);
> + return true;
> +}
> +
> +
> +/* 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. */
> +bool
> +northd_handle_lr_changes(const struct northd_input *ni,
> + struct northd_data *nd)
> +{
> + const struct nbrec_logical_router *changed_lr;
> +
> + bool updated = false;
> +
> + NBREC_LOGICAL_ROUTER_TABLE_FOR_EACH_TRACKED (changed_lr,
> +
ni->nbrec_logical_router_table) {
> + if (nbrec_logical_router_is_new(changed_lr) ||
> + nbrec_logical_router_is_deleted(changed_lr)) {
> + goto fail;
> + }
> +
> + struct ovn_datapath *od = ovn_datapath_find(
> + &nd->lr_datapaths.datapaths,
> + &changed_lr->header_.uuid);
> +
> + /* Presently only able to handle load balancer and
> + * load balancer group changes. */
> + if (check_unsupported_inc_proc_for_lr_changes(changed_lr)) {
> + goto fail;
> + }
> +
> + if (!lr_check_and_handle_lb_and_lbgrp_changes(changed_lr,
> + &nd->lb_datapaths_map,
> +
&nd->lb_group_datapaths_map,
> + od, &updated)) {
> + goto fail;
> + }
> + }
> +
> + if (updated) {
> + nd->change_tracked = true;
Updated doesn't mean the change is tracked. Please see the earlier comments.
> + nd->lrouters_changed = true;
> + }
> +
> + return true;
> +fail:
> + destroy_northd_data_tracked_changes(nd);
I think we don't need "fail:" here. Instead, just return false and call the
destroy_northd_data_tracked_changes if lrouter is updated (but not tracked)
or can't be handled.
Thanks,
Han
> + return false;
> +}
> +
> bool
> northd_handle_sb_port_binding_changes(
> const struct sbrec_port_binding_table *sbrec_port_binding_table,
> diff --git a/northd/northd.h b/northd/northd.h
> index a79cc470c3..765720849a 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -119,8 +119,11 @@ struct northd_data {
> struct chassis_features features;
> struct sset svc_monitor_lsps;
> struct hmap svc_monitor_map;
> +
> + /* Tracked data. */
> bool change_tracked;
> struct tracked_ls_changes tracked_ls_changes;
> + bool lrouters_changed;
> };
>
> struct lflow_data {
> @@ -342,6 +345,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> const struct northd_input *,
> struct northd_data *);
> +bool northd_handle_lr_changes(const struct northd_input *,
> + struct northd_data *);
> void destroy_northd_data_tracked_changes(struct northd_data *);
> void northd_destroy(struct northd_data *data);
> void northd_init(struct northd_data *data);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index d3a5851e35..286b311242 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9760,6 +9760,14 @@ check as northd ovn-appctl -t NORTHD_TYPE
inc-engine/clear-stats
> check ovn-nbctl --wait=sb ls-lb-del sw0 lb1
> check_engine_stats norecompute recompute
>
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl lr-lb-add lr0 lb1
> +check_engine_stats norecompute recompute
> +
> +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> +check ovn-nbctl lr-lb-del lr0 lb1
> +check_engine_stats norecompute recompute
> +
> # Test load balancer group now
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> @@ -9796,11 +9804,11 @@ check_engine_stats norecompute recompute
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> -check_engine_stats recompute recompute
> +check_engine_stats norecompute recompute
>
> check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> check ovn-nbctl clear logical_router lr0 load_balancer_group
> -check_engine_stats recompute recompute
> +check_engine_stats norecompute recompute
>
> AT_CLEANUP
> ])
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
