Message ID | 20230818085736.1030963-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | northd: I-P for load balancer and lb groups | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Fri, Aug 18, 2023 at 1:57 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > This will help in handling other column changes of a logical switch. > > Acked-by: Han Zhou <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/en-lb-data.c | 2 +- > northd/northd.c | 356 ++++++++++++++++++++++++++------------------ > 2 files changed, 209 insertions(+), 149 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). nit: this change belongs to a previous patch. Other than this, LGTM (already acked :)) > */ > void * > en_lb_data_init(struct engine_node *node OVS_UNUSED, > diff --git a/northd/northd.c b/northd/northd.c > index e9afa07177..4cc9ef8c8d 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4891,23 +4891,29 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn, > sset_destroy(&active_ha_chassis_grps); > } > > +static void > +destroy_tracked_ls_change(struct ls_change *ls_change) > +{ > + 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); > + } > + LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { > + ovs_list_remove(&op->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, > &nd->tracked_ls_changes.updated) { > - 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); > - } > - LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { > - ovs_list_remove(&op->list); > - ovn_port_destroy_orphan(op); > - } > + destroy_tracked_ls_change(ls_change); > ovs_list_remove(&ls_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,172 @@ check_lsp_changes_other_than_up(const struct nbrec_logical_switch_port *nbsp) > return false; > } > > +/* Handles logical switch port changes of a changed logical switch. > + * Returns false, if any logical port can't be incrementally handled. > + */ > +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) > +{ > + bool ls_ports_changed = false; > + if (!nbrec_logical_switch_is_updated(changed_ls, > + NBREC_LOGICAL_SWITCH_COL_PORTS)) { > + > + for (size_t i = 0; i < changed_ls->n_ports; i++) { > + if (nbrec_logical_switch_port_row_get_seqno( > + changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0) { > + ls_ports_changed = true; > + break; > + } > + } > + } else { > + ls_ports_changed = true; > + } > + > + if (!ls_ports_changed) { > + return true; > + } > + > + /* Check if the logical switch has only router ports before this change. > + * If so, fall back to recompute. > + * lflow engine node while building the lflows checks if the logical switch > + * has any router ports and depending on that it adds different flows. > + * See build_lswitch_rport_arp_req_flow() for more details. > + * Note: We can definitely handle this scenario incrementally in the > + * northd engine node and fall back to recompute in lflow engine node > + * and even handle this incrementally in lflow node. Until we do that > + * resort to full recompute of northd node. > + */ > + bool only_rports = (od->n_router_ports > + && (od->n_router_ports == hmap_count(&od->ports))); > + if (only_rports) { > + return false; > + } > + > + struct ovn_port *op; > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > + op->visited = false; > + } > + > + /* Compare the individual ports in the old and new Logical Switches */ > + for (size_t j = 0; j < changed_ls->n_ports; ++j) { > + struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; > + op = ovn_port_find_in_datapath(od, new_nbsp->name); > + > + if (!op) { > + if (!lsp_can_be_inc_processed(new_nbsp)) { > + goto fail; > + } > + op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > + new_nbsp->name, new_nbsp, od, NULL, NULL, > + ni->sbrec_mirror_table, > + ni->sbrec_chassis_table, > + ni->sbrec_chassis_by_name, > + ni->sbrec_chassis_by_hostname); > + if (!op) { > + goto fail; > + } > + ovs_list_push_back(&ls_change->added_ports, > + &op->list); > + } else if (ls_port_has_changed(op->nbsp, new_nbsp)) { > + /* Existing port updated */ > + bool temp = false; > + if (lsp_is_type_changed(op->sb, new_nbsp, &temp) || > + !op->lsp_can_be_inc_processed || > + !lsp_can_be_inc_processed(new_nbsp)) { > + goto fail; > + } > + const struct sbrec_port_binding *sb = op->sb; > + if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) { > + /* This port is used for svc monitor, which may be impacted > + * by this change. Fallback to recompute. */ > + goto fail; > + } > + if (!check_lsp_is_up && > + !check_lsp_changes_other_than_up(new_nbsp)) { > + /* If the only change is the "up" column while the > + * "ignore_lsp_down" is set to true, just ignore this > + * change. */ > + op->visited = true; > + continue; > + } > + struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows); > + ovs_list_splice(&lflows, op->lflows.next, &op->lflows); > + ovn_port_destroy(&nd->ls_ports, op); > + op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > + new_nbsp->name, new_nbsp, od, sb, &lflows, > + ni->sbrec_mirror_table, > + ni->sbrec_chassis_table, > + ni->sbrec_chassis_by_name, > + ni->sbrec_chassis_by_hostname); > + ovs_assert(ovs_list_is_empty(&lflows)); > + if (!op) { > + goto fail; > + } > + ovs_list_push_back(&ls_change->updated_ports, &op->list); > + } > + op->visited = true; > + } > + > + /* Check for deleted ports */ > + HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { > + if (!op->visited) { > + if (!op->lsp_can_be_inc_processed) { > + goto fail_clean_deleted; > + } > + 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; > + } > + ovs_list_push_back(&ls_change->deleted_ports, > + &op->list); > + hmap_remove(&nd->ls_ports, &op->key_node); > + hmap_remove(&od->ports, &op->dp_node); > + sbrec_port_binding_delete(op->sb); > + delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, > + op->tunnel_key); > + } > + } > + > + /* Check if the logical switch has only router ports after this change. > + * If so, fall back to recompute. > + * lflow engine node while building the lflows checks if the logical switch > + * has any router ports and depending on that it adds different flows. > + * See build_lswitch_rport_arp_req_flow() for more details. > + * Note: We can definitely handle this scenario incrementally in the > + * northd engine node and fall back to recompute in lflow engine node > + * and even handle this incrementally in lflow node. Until we do that > + * resort to full recompute of northd node. > + */ > + only_rports = (od->n_router_ports > + && (od->n_router_ports == hmap_count(&od->ports))); > + if (only_rports) { > + goto fail_clean_deleted; > + } > + > + 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; > + } > + > + return true; > + > +fail_clean_deleted: > + LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { > + ovn_port_destroy_orphan(op); > + } > + > +fail: > + 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, > @@ -5130,11 +5308,9 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > { > const struct nbrec_logical_switch *changed_ls; > struct ls_change *ls_change = NULL; > - struct ovn_port *op; > > NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls, > ni->nbrec_logical_switch_table) { > - ls_change = NULL; > if (nbrec_logical_switch_is_new(changed_ls) || > nbrec_logical_switch_is_deleted(changed_ls)) { > goto fail; > @@ -5150,8 +5326,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > goto fail; > } > > - /* Now only able to handle lsp changes. */ > - if (check_ls_changes_other_than_lsp(changed_ls)) { > + /* Check if the ls changes can be handled or not. */ > + if (!ls_changes_can_be_handled(changed_ls)) { > goto fail; > } > > @@ -5161,126 +5337,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > ovs_list_init(&ls_change->deleted_ports); > ovs_list_init(&ls_change->updated_ports); > > - HMAP_FOR_EACH (op, dp_node, &od->ports) { > - op->visited = false; > - } > - > - /* Check if the logical switch has only router ports before this change. > - * If so, fall back to recompute. > - * lflow engine node while building the lflows checks if the logical switch > - * has any router ports and depending on that it adds different flows. > - * See build_lswitch_rport_arp_req_flow() for more details. > - * Note: We can definitely handle this scenario incrementally in the > - * northd engine node and fall back to recompute in lflow engine node > - * and even handle this incrementally in lflow node. Until we do that > - * resort to full recompute of northd node. > - */ > - bool only_rports = (od->n_router_ports > - && (od->n_router_ports == hmap_count(&od->ports))); > - if (only_rports) { > + 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); > goto fail; > } > > - /* Compare the individual ports in the old and new Logical Switches */ > - for (size_t j = 0; j < changed_ls->n_ports; ++j) { > - struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; > - op = ovn_port_find_in_datapath(od, new_nbsp->name); > - > - if (!op) { > - if (!lsp_can_be_inc_processed(new_nbsp)) { > - goto fail; > - } > - op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > - new_nbsp->name, new_nbsp, od, NULL, NULL, > - ni->sbrec_mirror_table, > - ni->sbrec_chassis_table, > - ni->sbrec_chassis_by_name, > - ni->sbrec_chassis_by_hostname); > - if (!op) { > - goto fail; > - } > - ovs_list_push_back(&ls_change->added_ports, > - &op->list); > - } else if (ls_port_has_changed(op->nbsp, new_nbsp)) { > - /* Existing port updated */ > - bool temp = false; > - if (lsp_is_type_changed(op->sb, new_nbsp, &temp) || > - !op->lsp_can_be_inc_processed || > - !lsp_can_be_inc_processed(new_nbsp)) { > - goto fail; > - } > - const struct sbrec_port_binding *sb = op->sb; > - if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) { > - /* This port is used for svc monitor, which may be impacted > - * by this change. Fallback to recompute. */ > - goto fail; > - } > - if (!check_lsp_is_up && > - !check_lsp_changes_other_than_up(new_nbsp)) { > - /* If the only change is the "up" column while the > - * "ignore_lsp_down" is set to true, just ignore this > - * change. */ > - op->visited = true; > - continue; > - } > - struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows); > - ovs_list_splice(&lflows, op->lflows.next, &op->lflows); > - ovn_port_destroy(&nd->ls_ports, op); > - op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > - new_nbsp->name, new_nbsp, od, sb, &lflows, > - ni->sbrec_mirror_table, > - ni->sbrec_chassis_table, > - ni->sbrec_chassis_by_name, > - ni->sbrec_chassis_by_hostname); > - ovs_assert(ovs_list_is_empty(&lflows)); > - if (!op) { > - goto fail; > - } > - ovs_list_push_back(&ls_change->updated_ports, &op->list); > - } > - op->visited = true; > - } > - > - /* Check for deleted ports */ > - HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { > - if (!op->visited) { > - if (!op->lsp_can_be_inc_processed) { > - goto fail_clean_deleted; > - } > - 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; > - } > - ovs_list_push_back(&ls_change->deleted_ports, > - &op->list); > - hmap_remove(&nd->ls_ports, &op->key_node); > - hmap_remove(&od->ports, &op->dp_node); > - sbrec_port_binding_delete(op->sb); > - delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, > - op->tunnel_key); > - } > - } > - > - /* Check if the logical switch has only router ports after this change. > - * If so, fall back to recompute. > - * lflow engine node while building the lflows checks if the logical switch > - * has any router ports and depending on that it adds different flows. > - * See build_lswitch_rport_arp_req_flow() for more details. > - * Note: We can definitely handle this scenario incrementally in the > - * northd engine node and fall back to recompute in lflow engine node > - * and even handle this incrementally in lflow node. Until we do that > - * resort to full recompute of northd node. > - */ > - only_rports = (od->n_router_ports > - && (od->n_router_ports == hmap_count(&od->ports))); > - if (only_rports) { > - goto fail_clean_deleted; > - } > - > - 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)) { > + if (updated) { > ovs_list_push_back(&nd->tracked_ls_changes.updated, > &ls_change->list_node); > } else { > @@ -5293,13 +5359,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > } > return true; > > -fail_clean_deleted: > - LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { > - ovn_port_destroy_orphan(op); > - } > - > fail: > - free(ls_change); > destroy_northd_data_tracked_changes(nd); > return false; > } > -- > 2.40.1 >
On Fri, Aug 18, 2023 at 10:58 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > This will help in handling other column changes of a logical switch. > > Acked-by: Han Zhou <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/en-lb-data.c | 2 +- > northd/northd.c | 356 ++++++++++++++++++++++++++------------------ > 2 files changed, 209 insertions(+), 149 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 e9afa07177..4cc9ef8c8d 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4891,23 +4891,29 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn, > sset_destroy(&active_ha_chassis_grps); > } > > +static void > +destroy_tracked_ls_change(struct ls_change *ls_change) > +{ > + 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); > + } > + LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { > + ovs_list_remove(&op->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, > &nd->tracked_ls_changes.updated) { > - 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); > - } > - LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { > - ovs_list_remove(&op->list); > - ovn_port_destroy_orphan(op); > - } > + destroy_tracked_ls_change(ls_change); > ovs_list_remove(&ls_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,172 @@ check_lsp_changes_other_than_up(const struct nbrec_logical_switch_port *nbsp) > return false; > } > > +/* Handles logical switch port changes of a changed logical switch. > + * Returns false, if any logical port can't be incrementally handled. > + */ > +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) > +{ > + bool ls_ports_changed = false; > + if (!nbrec_logical_switch_is_updated(changed_ls, > + NBREC_LOGICAL_SWITCH_COL_PORTS)) { > + > + for (size_t i = 0; i < changed_ls->n_ports; i++) { > + if (nbrec_logical_switch_port_row_get_seqno( > + changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0) { > + ls_ports_changed = true; > + break; > + } > + } > + } else { > + ls_ports_changed = true; > + } > + > + if (!ls_ports_changed) { > + return true; > + } > + > + /* Check if the logical switch has only router ports before this change. > + * If so, fall back to recompute. > + * lflow engine node while building the lflows checks if the logical switch > + * has any router ports and depending on that it adds different flows. > + * See build_lswitch_rport_arp_req_flow() for more details. > + * Note: We can definitely handle this scenario incrementally in the > + * northd engine node and fall back to recompute in lflow engine node > + * and even handle this incrementally in lflow node. Until we do that > + * resort to full recompute of northd node. > + */ > + bool only_rports = (od->n_router_ports > + && (od->n_router_ports == hmap_count(&od->ports))); > + if (only_rports) { > + return false; > + } > + > + struct ovn_port *op; > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > + op->visited = false; > + } > + > + /* Compare the individual ports in the old and new Logical Switches */ > + for (size_t j = 0; j < changed_ls->n_ports; ++j) { > + struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; > + op = ovn_port_find_in_datapath(od, new_nbsp->name); > + > + if (!op) { > + if (!lsp_can_be_inc_processed(new_nbsp)) { > + goto fail; > + } > + op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > + new_nbsp->name, new_nbsp, od, NULL, NULL, > + ni->sbrec_mirror_table, > + ni->sbrec_chassis_table, > + ni->sbrec_chassis_by_name, > + ni->sbrec_chassis_by_hostname); > + if (!op) { > + goto fail; > + } > + ovs_list_push_back(&ls_change->added_ports, > + &op->list); > + } else if (ls_port_has_changed(op->nbsp, new_nbsp)) { > + /* Existing port updated */ > + bool temp = false; > + if (lsp_is_type_changed(op->sb, new_nbsp, &temp) || > + !op->lsp_can_be_inc_processed || > + !lsp_can_be_inc_processed(new_nbsp)) { > + goto fail; > + } > + const struct sbrec_port_binding *sb = op->sb; > + if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) { > + /* This port is used for svc monitor, which may be impacted > + * by this change. Fallback to recompute. */ > + goto fail; > + } > + if (!check_lsp_is_up && > + !check_lsp_changes_other_than_up(new_nbsp)) { > + /* If the only change is the "up" column while the > + * "ignore_lsp_down" is set to true, just ignore this > + * change. */ > + op->visited = true; > + continue; > + } > + struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows); > + ovs_list_splice(&lflows, op->lflows.next, &op->lflows); > + ovn_port_destroy(&nd->ls_ports, op); > + op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > + new_nbsp->name, new_nbsp, od, sb, &lflows, > + ni->sbrec_mirror_table, > + ni->sbrec_chassis_table, > + ni->sbrec_chassis_by_name, > + ni->sbrec_chassis_by_hostname); > + ovs_assert(ovs_list_is_empty(&lflows)); > + if (!op) { > + goto fail; > + } > + ovs_list_push_back(&ls_change->updated_ports, &op->list); > + } > + op->visited = true; > + } > + > + /* Check for deleted ports */ > + HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { > + if (!op->visited) { > + if (!op->lsp_can_be_inc_processed) { > + goto fail_clean_deleted; > + } > + 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; > + } > + ovs_list_push_back(&ls_change->deleted_ports, > + &op->list); > + hmap_remove(&nd->ls_ports, &op->key_node); > + hmap_remove(&od->ports, &op->dp_node); > + sbrec_port_binding_delete(op->sb); > + delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, > + op->tunnel_key); > + } > + } > + > + /* Check if the logical switch has only router ports after this change. > + * If so, fall back to recompute. > + * lflow engine node while building the lflows checks if the logical switch > + * has any router ports and depending on that it adds different flows. > + * See build_lswitch_rport_arp_req_flow() for more details. > + * Note: We can definitely handle this scenario incrementally in the > + * northd engine node and fall back to recompute in lflow engine node > + * and even handle this incrementally in lflow node. Until we do that > + * resort to full recompute of northd node. > + */ > + only_rports = (od->n_router_ports > + && (od->n_router_ports == hmap_count(&od->ports))); > + if (only_rports) { > + goto fail_clean_deleted; > + } > + > + 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; > + } > + > + return true; > + > +fail_clean_deleted: > + LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { > + ovn_port_destroy_orphan(op); > + } > + > +fail: > + 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, > @@ -5130,11 +5308,9 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > { > const struct nbrec_logical_switch *changed_ls; > struct ls_change *ls_change = NULL; > - struct ovn_port *op; > > NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls, > ni->nbrec_logical_switch_table) { > - ls_change = NULL; > if (nbrec_logical_switch_is_new(changed_ls) || > nbrec_logical_switch_is_deleted(changed_ls)) { > goto fail; > @@ -5150,8 +5326,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > goto fail; > } > > - /* Now only able to handle lsp changes. */ > - if (check_ls_changes_other_than_lsp(changed_ls)) { > + /* Check if the ls changes can be handled or not. */ > + if (!ls_changes_can_be_handled(changed_ls)) { > goto fail; > } > > @@ -5161,126 +5337,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > ovs_list_init(&ls_change->deleted_ports); > ovs_list_init(&ls_change->updated_ports); > > - HMAP_FOR_EACH (op, dp_node, &od->ports) { > - op->visited = false; > - } > - > - /* Check if the logical switch has only router ports before this change. > - * If so, fall back to recompute. > - * lflow engine node while building the lflows checks if the logical switch > - * has any router ports and depending on that it adds different flows. > - * See build_lswitch_rport_arp_req_flow() for more details. > - * Note: We can definitely handle this scenario incrementally in the > - * northd engine node and fall back to recompute in lflow engine node > - * and even handle this incrementally in lflow node. Until we do that > - * resort to full recompute of northd node. > - */ > - bool only_rports = (od->n_router_ports > - && (od->n_router_ports == hmap_count(&od->ports))); > - if (only_rports) { > + 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); > goto fail; > } > > - /* Compare the individual ports in the old and new Logical Switches */ > - for (size_t j = 0; j < changed_ls->n_ports; ++j) { > - struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; > - op = ovn_port_find_in_datapath(od, new_nbsp->name); > - > - if (!op) { > - if (!lsp_can_be_inc_processed(new_nbsp)) { > - goto fail; > - } > - op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > - new_nbsp->name, new_nbsp, od, NULL, NULL, > - ni->sbrec_mirror_table, > - ni->sbrec_chassis_table, > - ni->sbrec_chassis_by_name, > - ni->sbrec_chassis_by_hostname); > - if (!op) { > - goto fail; > - } > - ovs_list_push_back(&ls_change->added_ports, > - &op->list); > - } else if (ls_port_has_changed(op->nbsp, new_nbsp)) { > - /* Existing port updated */ > - bool temp = false; > - if (lsp_is_type_changed(op->sb, new_nbsp, &temp) || > - !op->lsp_can_be_inc_processed || > - !lsp_can_be_inc_processed(new_nbsp)) { > - goto fail; > - } > - const struct sbrec_port_binding *sb = op->sb; > - if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) { > - /* This port is used for svc monitor, which may be impacted > - * by this change. Fallback to recompute. */ > - goto fail; > - } > - if (!check_lsp_is_up && > - !check_lsp_changes_other_than_up(new_nbsp)) { > - /* If the only change is the "up" column while the > - * "ignore_lsp_down" is set to true, just ignore this > - * change. */ > - op->visited = true; > - continue; > - } > - struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows); > - ovs_list_splice(&lflows, op->lflows.next, &op->lflows); > - ovn_port_destroy(&nd->ls_ports, op); > - op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, > - new_nbsp->name, new_nbsp, od, sb, &lflows, > - ni->sbrec_mirror_table, > - ni->sbrec_chassis_table, > - ni->sbrec_chassis_by_name, > - ni->sbrec_chassis_by_hostname); > - ovs_assert(ovs_list_is_empty(&lflows)); > - if (!op) { > - goto fail; > - } > - ovs_list_push_back(&ls_change->updated_ports, &op->list); > - } > - op->visited = true; > - } > - > - /* Check for deleted ports */ > - HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { > - if (!op->visited) { > - if (!op->lsp_can_be_inc_processed) { > - goto fail_clean_deleted; > - } > - 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; > - } > - ovs_list_push_back(&ls_change->deleted_ports, > - &op->list); > - hmap_remove(&nd->ls_ports, &op->key_node); > - hmap_remove(&od->ports, &op->dp_node); > - sbrec_port_binding_delete(op->sb); > - delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, > - op->tunnel_key); > - } > - } > - > - /* Check if the logical switch has only router ports after this change. > - * If so, fall back to recompute. > - * lflow engine node while building the lflows checks if the logical switch > - * has any router ports and depending on that it adds different flows. > - * See build_lswitch_rport_arp_req_flow() for more details. > - * Note: We can definitely handle this scenario incrementally in the > - * northd engine node and fall back to recompute in lflow engine node > - * and even handle this incrementally in lflow node. Until we do that > - * resort to full recompute of northd node. > - */ > - only_rports = (od->n_router_ports > - && (od->n_router_ports == hmap_count(&od->ports))); > - if (only_rports) { > - goto fail_clean_deleted; > - } > - > - 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)) { > + if (updated) { > ovs_list_push_back(&nd->tracked_ls_changes.updated, > &ls_change->list_node); > } else { > @@ -5293,13 +5359,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > } > return true; > > -fail_clean_deleted: > - LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { > - ovn_port_destroy_orphan(op); > - } > - > fail: > - free(ls_change); > destroy_northd_data_tracked_changes(nd); > return false; > } > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > Looks good to me, thanks. Reviewed-by: Ales Musil <amusil@redhat.com>
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 e9afa07177..4cc9ef8c8d 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4891,23 +4891,29 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn, sset_destroy(&active_ha_chassis_grps); } +static void +destroy_tracked_ls_change(struct ls_change *ls_change) +{ + 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); + } + LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { + ovs_list_remove(&op->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, &nd->tracked_ls_changes.updated) { - 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); - } - LIST_FOR_EACH_SAFE (op, list, &ls_change->deleted_ports) { - ovs_list_remove(&op->list); - ovn_port_destroy_orphan(op); - } + destroy_tracked_ls_change(ls_change); ovs_list_remove(&ls_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,172 @@ check_lsp_changes_other_than_up(const struct nbrec_logical_switch_port *nbsp) return false; } +/* Handles logical switch port changes of a changed logical switch. + * Returns false, if any logical port can't be incrementally handled. + */ +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) +{ + bool ls_ports_changed = false; + if (!nbrec_logical_switch_is_updated(changed_ls, + NBREC_LOGICAL_SWITCH_COL_PORTS)) { + + for (size_t i = 0; i < changed_ls->n_ports; i++) { + if (nbrec_logical_switch_port_row_get_seqno( + changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0) { + ls_ports_changed = true; + break; + } + } + } else { + ls_ports_changed = true; + } + + if (!ls_ports_changed) { + return true; + } + + /* Check if the logical switch has only router ports before this change. + * If so, fall back to recompute. + * lflow engine node while building the lflows checks if the logical switch + * has any router ports and depending on that it adds different flows. + * See build_lswitch_rport_arp_req_flow() for more details. + * Note: We can definitely handle this scenario incrementally in the + * northd engine node and fall back to recompute in lflow engine node + * and even handle this incrementally in lflow node. Until we do that + * resort to full recompute of northd node. + */ + bool only_rports = (od->n_router_ports + && (od->n_router_ports == hmap_count(&od->ports))); + if (only_rports) { + return false; + } + + struct ovn_port *op; + HMAP_FOR_EACH (op, dp_node, &od->ports) { + op->visited = false; + } + + /* Compare the individual ports in the old and new Logical Switches */ + for (size_t j = 0; j < changed_ls->n_ports; ++j) { + struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; + op = ovn_port_find_in_datapath(od, new_nbsp->name); + + if (!op) { + if (!lsp_can_be_inc_processed(new_nbsp)) { + goto fail; + } + op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, + new_nbsp->name, new_nbsp, od, NULL, NULL, + ni->sbrec_mirror_table, + ni->sbrec_chassis_table, + ni->sbrec_chassis_by_name, + ni->sbrec_chassis_by_hostname); + if (!op) { + goto fail; + } + ovs_list_push_back(&ls_change->added_ports, + &op->list); + } else if (ls_port_has_changed(op->nbsp, new_nbsp)) { + /* Existing port updated */ + bool temp = false; + if (lsp_is_type_changed(op->sb, new_nbsp, &temp) || + !op->lsp_can_be_inc_processed || + !lsp_can_be_inc_processed(new_nbsp)) { + goto fail; + } + const struct sbrec_port_binding *sb = op->sb; + if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) { + /* This port is used for svc monitor, which may be impacted + * by this change. Fallback to recompute. */ + goto fail; + } + if (!check_lsp_is_up && + !check_lsp_changes_other_than_up(new_nbsp)) { + /* If the only change is the "up" column while the + * "ignore_lsp_down" is set to true, just ignore this + * change. */ + op->visited = true; + continue; + } + struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows); + ovs_list_splice(&lflows, op->lflows.next, &op->lflows); + ovn_port_destroy(&nd->ls_ports, op); + op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, + new_nbsp->name, new_nbsp, od, sb, &lflows, + ni->sbrec_mirror_table, + ni->sbrec_chassis_table, + ni->sbrec_chassis_by_name, + ni->sbrec_chassis_by_hostname); + ovs_assert(ovs_list_is_empty(&lflows)); + if (!op) { + goto fail; + } + ovs_list_push_back(&ls_change->updated_ports, &op->list); + } + op->visited = true; + } + + /* Check for deleted ports */ + HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { + if (!op->visited) { + if (!op->lsp_can_be_inc_processed) { + goto fail_clean_deleted; + } + 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; + } + ovs_list_push_back(&ls_change->deleted_ports, + &op->list); + hmap_remove(&nd->ls_ports, &op->key_node); + hmap_remove(&od->ports, &op->dp_node); + sbrec_port_binding_delete(op->sb); + delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, + op->tunnel_key); + } + } + + /* Check if the logical switch has only router ports after this change. + * If so, fall back to recompute. + * lflow engine node while building the lflows checks if the logical switch + * has any router ports and depending on that it adds different flows. + * See build_lswitch_rport_arp_req_flow() for more details. + * Note: We can definitely handle this scenario incrementally in the + * northd engine node and fall back to recompute in lflow engine node + * and even handle this incrementally in lflow node. Until we do that + * resort to full recompute of northd node. + */ + only_rports = (od->n_router_ports + && (od->n_router_ports == hmap_count(&od->ports))); + if (only_rports) { + goto fail_clean_deleted; + } + + 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; + } + + return true; + +fail_clean_deleted: + LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { + ovn_port_destroy_orphan(op); + } + +fail: + 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, @@ -5130,11 +5308,9 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, { const struct nbrec_logical_switch *changed_ls; struct ls_change *ls_change = NULL; - struct ovn_port *op; NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls, ni->nbrec_logical_switch_table) { - ls_change = NULL; if (nbrec_logical_switch_is_new(changed_ls) || nbrec_logical_switch_is_deleted(changed_ls)) { goto fail; @@ -5150,8 +5326,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, goto fail; } - /* Now only able to handle lsp changes. */ - if (check_ls_changes_other_than_lsp(changed_ls)) { + /* Check if the ls changes can be handled or not. */ + if (!ls_changes_can_be_handled(changed_ls)) { goto fail; } @@ -5161,126 +5337,16 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, ovs_list_init(&ls_change->deleted_ports); ovs_list_init(&ls_change->updated_ports); - HMAP_FOR_EACH (op, dp_node, &od->ports) { - op->visited = false; - } - - /* Check if the logical switch has only router ports before this change. - * If so, fall back to recompute. - * lflow engine node while building the lflows checks if the logical switch - * has any router ports and depending on that it adds different flows. - * See build_lswitch_rport_arp_req_flow() for more details. - * Note: We can definitely handle this scenario incrementally in the - * northd engine node and fall back to recompute in lflow engine node - * and even handle this incrementally in lflow node. Until we do that - * resort to full recompute of northd node. - */ - bool only_rports = (od->n_router_ports - && (od->n_router_ports == hmap_count(&od->ports))); - if (only_rports) { + 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); goto fail; } - /* Compare the individual ports in the old and new Logical Switches */ - for (size_t j = 0; j < changed_ls->n_ports; ++j) { - struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j]; - op = ovn_port_find_in_datapath(od, new_nbsp->name); - - if (!op) { - if (!lsp_can_be_inc_processed(new_nbsp)) { - goto fail; - } - op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, - new_nbsp->name, new_nbsp, od, NULL, NULL, - ni->sbrec_mirror_table, - ni->sbrec_chassis_table, - ni->sbrec_chassis_by_name, - ni->sbrec_chassis_by_hostname); - if (!op) { - goto fail; - } - ovs_list_push_back(&ls_change->added_ports, - &op->list); - } else if (ls_port_has_changed(op->nbsp, new_nbsp)) { - /* Existing port updated */ - bool temp = false; - if (lsp_is_type_changed(op->sb, new_nbsp, &temp) || - !op->lsp_can_be_inc_processed || - !lsp_can_be_inc_processed(new_nbsp)) { - goto fail; - } - const struct sbrec_port_binding *sb = op->sb; - if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) { - /* This port is used for svc monitor, which may be impacted - * by this change. Fallback to recompute. */ - goto fail; - } - if (!check_lsp_is_up && - !check_lsp_changes_other_than_up(new_nbsp)) { - /* If the only change is the "up" column while the - * "ignore_lsp_down" is set to true, just ignore this - * change. */ - op->visited = true; - continue; - } - struct ovs_list lflows = OVS_LIST_INITIALIZER(&lflows); - ovs_list_splice(&lflows, op->lflows.next, &op->lflows); - ovn_port_destroy(&nd->ls_ports, op); - op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports, - new_nbsp->name, new_nbsp, od, sb, &lflows, - ni->sbrec_mirror_table, - ni->sbrec_chassis_table, - ni->sbrec_chassis_by_name, - ni->sbrec_chassis_by_hostname); - ovs_assert(ovs_list_is_empty(&lflows)); - if (!op) { - goto fail; - } - ovs_list_push_back(&ls_change->updated_ports, &op->list); - } - op->visited = true; - } - - /* Check for deleted ports */ - HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { - if (!op->visited) { - if (!op->lsp_can_be_inc_processed) { - goto fail_clean_deleted; - } - 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; - } - ovs_list_push_back(&ls_change->deleted_ports, - &op->list); - hmap_remove(&nd->ls_ports, &op->key_node); - hmap_remove(&od->ports, &op->dp_node); - sbrec_port_binding_delete(op->sb); - delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key, - op->tunnel_key); - } - } - - /* Check if the logical switch has only router ports after this change. - * If so, fall back to recompute. - * lflow engine node while building the lflows checks if the logical switch - * has any router ports and depending on that it adds different flows. - * See build_lswitch_rport_arp_req_flow() for more details. - * Note: We can definitely handle this scenario incrementally in the - * northd engine node and fall back to recompute in lflow engine node - * and even handle this incrementally in lflow node. Until we do that - * resort to full recompute of northd node. - */ - only_rports = (od->n_router_ports - && (od->n_router_ports == hmap_count(&od->ports))); - if (only_rports) { - goto fail_clean_deleted; - } - - 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)) { + if (updated) { ovs_list_push_back(&nd->tracked_ls_changes.updated, &ls_change->list_node); } else { @@ -5293,13 +5359,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, } return true; -fail_clean_deleted: - LIST_FOR_EACH_POP (op, list, &ls_change->deleted_ports) { - ovn_port_destroy_orphan(op); - } - fail: - free(ls_change); destroy_northd_data_tracked_changes(nd); return false; }