| Message ID | 20231026181427.3366991-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | northd lflow incremental processing | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Thu, Oct 26, 2023 at 11:15 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > It also moves the logical router port IPv6 prefix delegation > updates to "sync-from-sb" engine node. > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/en-northd.c | 2 +- > northd/en-sync-sb.c | 3 +- > northd/northd.c | 283 ++++++++++++++++++++++++++------------------ > northd/northd.h | 6 +- > tests/ovn-northd.at | 31 ++++- > 5 files changed, 198 insertions(+), 127 deletions(-) > > diff --git a/northd/en-northd.c b/northd/en-northd.c > index 96c2ce9f69..13e731cad9 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node, > northd_get_input_data(node, &input_data); > > if (!northd_handle_sb_port_binding_changes( > - input_data.sbrec_port_binding_table, &nd->ls_ports)) { > + input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) { > return false; > } > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 2540fcfb97..a14c609acd 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) > const struct engine_context *eng_ctx = engine_get_context(); > struct northd_data *northd_data = engine_get_input_data("northd", node); > > - sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, > + &northd_data->lr_ports); > engine_set_node_state(node, EN_UPDATED); > } > > diff --git a/northd/northd.c b/northd/northd.c > index 9ce1b2cb5a..c9c7045755 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > { > sbrec_port_binding_set_datapath(op->sb, op->od->sb); > if (op->nbrp) { > + /* Note: SB port binding options for router ports are set in > + * sync_pbs(). */ > + > /* If the router is for l3 gateway, it resides on a chassis > * and its port type is "l3gateway". */ > const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > sbrec_port_binding_set_type(op->sb, "patch"); > } > > - struct smap new; > - smap_init(&new); > if (is_cr_port(op)) { > ovs_assert(sbrec_chassis_by_name); > ovs_assert(sbrec_chassis_by_hostname); > ovs_assert(sbrec_ha_chassis_grp_by_name); > ovs_assert(active_ha_chassis_grps); > - const char *redirect_type = smap_get(&op->nbrp->options, > - "redirect-type"); > > if (op->nbrp->ha_chassis_group) { > if (op->nbrp->n_gateway_chassis) { > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > /* Delete the legacy gateway_chassis from the pb. */ > sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0); > } > - smap_add(&new, "distributed-port", op->nbrp->name); > - > - bool always_redirect = > - !op->od->has_distributed_nat && > - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > - > - if (redirect_type) { > - smap_add(&new, "redirect-type", redirect_type); > - /* XXX Why can't we enable always-redirect when redirect-type > - * is bridged? */ > - if (!strcmp(redirect_type, "bridged")) { > - always_redirect = false; > - } > - } > - > - if (always_redirect) { > - smap_add(&new, "always-redirect", "true"); > - } > - } else { > - if (op->peer) { > - smap_add(&new, "peer", op->peer->key); > - if (op->nbrp->ha_chassis_group || > - op->nbrp->n_gateway_chassis) { > - char *redirect_name = > - ovn_chassis_redirect_name(op->nbrp->name); > - smap_add(&new, "chassis-redirect-port", redirect_name); > - free(redirect_name); > - } > - } > - if (chassis_name) { > - smap_add(&new, "l3gateway-chassis", chassis_name); > - } > - } > - > - const char *ipv6_pd_list = smap_get(&op->sb->options, > - "ipv6_ra_pd_list"); > - if (ipv6_pd_list) { > - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > } > > - sbrec_port_binding_set_options(op->sb, &new); > - smap_destroy(&new); > - > sbrec_port_binding_set_parent_port(op->sb, NULL); > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > @@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) > return duplicates; > } > > -/* Syncs the SB port binding for the ovn_port 'op'. Caller should make sure > - * that the OVN SB IDL txn is not NULL. Presently it only syncs the nat > - * column of port binding corresponding to the 'op->nbsp' */ > +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port. > + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it > + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */ > static void > -sync_pb_for_op(struct ovn_port *op) > +sync_pb_for_lsp(struct ovn_port *op) > { > + ovs_assert(op->nbsp); > + > if (lsp_is_router(op->nbsp)) { > const char *chassis = NULL; > if (op->peer && op->peer->od && op->peer->od->nbr) { > @@ -4879,18 +4839,87 @@ sync_pb_for_op(struct ovn_port *op) > } > } > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port. > + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it > + * only sets the port binding options column for the router ports */ > +static void > +sync_pb_for_lrp(struct ovn_port *op) > +{ > + ovs_assert(op->nbrp); > + > + struct smap new; > + smap_init(&new); > + > + const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); > + if (is_cr_port(op)) { > + smap_add(&new, "distributed-port", op->nbrp->name); > + > + bool always_redirect = > + !op->od->has_distributed_nat && > + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > + > + const char *redirect_type = smap_get(&op->nbrp->options, > + "redirect-type"); > + if (redirect_type) { > + smap_add(&new, "redirect-type", redirect_type); > + /* XXX Why can't we enable always-redirect when redirect-type > + * is bridged? */ > + if (!strcmp(redirect_type, "bridged")) { > + always_redirect = false; > + } > + } > + > + if (always_redirect) { > + smap_add(&new, "always-redirect", "true"); > + } > + } else { > + if (op->peer) { > + smap_add(&new, "peer", op->peer->key); > + if (op->nbrp->ha_chassis_group || > + op->nbrp->n_gateway_chassis) { > + char *redirect_name = > + ovn_chassis_redirect_name(op->nbrp->name); > + smap_add(&new, "chassis-redirect-port", redirect_name); > + free(redirect_name); > + } > + } > + if (chassis_name) { > + smap_add(&new, "l3gateway-chassis", chassis_name); > + } > + } > + > + const char *ipv6_pd_list = smap_get(&op->sb->options, > + "ipv6_ra_pd_list"); > + if (ipv6_pd_list) { > + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > + } > + > + sbrec_port_binding_set_options(op->sb, &new); > + smap_destroy(&new); > +} > + > +static void ovn_update_ipv6_options(struct hmap *lr_ports); > +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op); > + > /* Sync the SB Port bindings which needs to be updated. > * Presently it syncs the nat column of port bindings corresponding to > * the logical switch ports. */ > void > -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, > + struct hmap *lr_ports) > { > ovs_assert(ovnsb_idl_txn); > > struct ovn_port *op; > HMAP_FOR_EACH (op, key_node, ls_ports) { > - sync_pb_for_op(op); > + sync_pb_for_lsp(op); > + } > + > + HMAP_FOR_EACH (op, key_node, lr_ports) { > + sync_pb_for_lrp(op); > } > + > + ovn_update_ipv6_options(lr_ports); > } > > /* Sync the SB Port bindings for the added and updated logical switch ports > @@ -4902,12 +4931,22 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports) > struct ovn_port *op; > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { > op = hmapx_node->data; > - sync_pb_for_op(op); > + if (op->nbsp) { > + sync_pb_for_lsp(op); > + } else { > + sync_pb_for_lrp(op); > + ovn_update_ipv6_opt_for_op(op); > + } > } > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { > op = hmapx_node->data; > - sync_pb_for_op(op); > + if (op->nbsp) { > + sync_pb_for_lsp(op); > + } else { > + sync_pb_for_lrp(op); > + ovn_update_ipv6_opt_for_op(op); > + } > } > > return true; > @@ -5703,20 +5742,21 @@ fail: > bool > northd_handle_sb_port_binding_changes( > const struct sbrec_port_binding_table *sbrec_port_binding_table, > - struct hmap *ls_ports) > + struct hmap *ls_ports, struct hmap *lr_ports) > { > const struct sbrec_port_binding *pb; > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) { > - /* Return false if the 'pb' belongs to a router port. We don't handle > - * I-P for router ports yet. */ > - if (is_pb_router_type(pb)) { > - return false; > - } > + bool is_router_port = is_pb_router_type(pb); > + struct ovn_port *op; > > - struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port); > - if (op && !op->lsp_can_be_inc_processed) { > - return false; > + if (is_router_port) { It looks as if we can handle any router type port PB updates but in the else branch we are very strict about what switch port PB updates we can handle, which sounds not right because I think router type handling are more complex. I am wondering either we missed some checks for the router type ports, or we were too strict about switch ports. I am not sure but just want to double check. > + op = ovn_port_find(lr_ports, pb->logical_port); > + } else { > + op = ovn_port_find(ls_ports, pb->logical_port); > + if (op && !op->lsp_can_be_inc_processed) { > + return false; > + } > } > > if (sbrec_port_binding_is_new(pb)) { > @@ -5725,7 +5765,8 @@ northd_handle_sb_port_binding_changes( > * pointer in northd data. Fallback to recompute otherwise. */ > if (!op) { > VLOG_WARN_RL(&rl, "A port-binding for %s is created but the " > - "LSP is not found.", pb->logical_port); > + "%s is not found.", pb->logical_port, > + is_router_port ? "LRP" : "LSP"); > return false; > } > op->sb = pb; > @@ -5736,7 +5777,7 @@ northd_handle_sb_port_binding_changes( > * sb idl pointers and other unexpected behavior. */ > if (op) { > VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the " > - "LSP still exists.", pb->logical_port); > + "LSP/LRP still exists.", pb->logical_port); > return false; > } > } else { > @@ -5746,7 +5787,8 @@ northd_handle_sb_port_binding_changes( Here is a comment that needs to be updated: /* The PB is updated, most likely because of binding/unbinding * to/from a chassis, and we can ignore the change (updating NB * "up" will be handled in the engine node "sync_from_sb"). This part of the comment needs an update for the router port handling added regarding the IPv6 prefix delegation. Thanks, Han > * Fallback to recompute for anything unexpected. */ > if (!op) { > VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the " > - "LSP is not found.", pb->logical_port); > + "%s is not found.", pb->logical_port, > + is_router_port ? "LRP" : "LSP"); > return false; > } > if (op->sb != pb) { > @@ -7895,67 +7937,72 @@ static void > copy_ra_to_sb(struct ovn_port *op, const char *address_mode); > > static void > -ovn_update_ipv6_options(struct hmap *lr_ports) > +ovn_update_ipv6_opt_for_op(struct ovn_port *op) > { > - struct ovn_port *op; > - HMAP_FOR_EACH (op, key_node, lr_ports) { > - ovs_assert(op->nbrp); > - > - if (op->nbrp->peer || !op->peer) { > - continue; > - } > + if (op->nbrp->peer || !op->peer) { > + return; > + } > > - if (!op->lrp_networks.n_ipv6_addrs) { > - continue; > - } > + if (!op->lrp_networks.n_ipv6_addrs) { > + return; > + } > > - struct smap options; > - smap_clone(&options, &op->sb->options); > + struct smap options; > + smap_clone(&options, &op->sb->options); > > - /* enable IPv6 prefix delegation */ > - bool prefix_delegation = smap_get_bool(&op->nbrp->options, > - "prefix_delegation", false); > - if (!lrport_is_enabled(op->nbrp)) { > - prefix_delegation = false; > - } > - if (smap_get_bool(&options, "ipv6_prefix_delegation", > - false) != prefix_delegation) { > - smap_add(&options, "ipv6_prefix_delegation", > - prefix_delegation ? "true" : "false"); > - } > + /* enable IPv6 prefix delegation */ > + bool prefix_delegation = smap_get_bool(&op->nbrp->options, > + "prefix_delegation", false); > + if (!lrport_is_enabled(op->nbrp)) { > + prefix_delegation = false; > + } > + if (smap_get_bool(&options, "ipv6_prefix_delegation", > + false) != prefix_delegation) { > + smap_add(&options, "ipv6_prefix_delegation", > + prefix_delegation ? "true" : "false"); > + } > > - bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > + bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > "prefix", false); > - if (!lrport_is_enabled(op->nbrp)) { > - ipv6_prefix = false; > - } > - if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > - smap_add(&options, "ipv6_prefix", > - ipv6_prefix ? "true" : "false"); > - } > - sbrec_port_binding_set_options(op->sb, &options); > + if (!lrport_is_enabled(op->nbrp)) { > + ipv6_prefix = false; > + } > + if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > + smap_add(&options, "ipv6_prefix", > + ipv6_prefix ? "true" : "false"); > + } > + sbrec_port_binding_set_options(op->sb, &options); > > - smap_destroy(&options); > + smap_destroy(&options); > > - const char *address_mode = smap_get( > - &op->nbrp->ipv6_ra_configs, "address_mode"); > + const char *address_mode = smap_get( > + &op->nbrp->ipv6_ra_configs, "address_mode"); > > - if (!address_mode) { > - continue; > - } > - if (strcmp(address_mode, "slaac") && > - strcmp(address_mode, "dhcpv6_stateful") && > - strcmp(address_mode, "dhcpv6_stateless")) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > - VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > - address_mode); > - continue; > - } > + if (!address_mode) { > + return; > + } > + if (strcmp(address_mode, "slaac") && > + strcmp(address_mode, "dhcpv6_stateful") && > + strcmp(address_mode, "dhcpv6_stateless")) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > + address_mode); > + return; > + } > > - if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > - false)) { > - copy_ra_to_sb(op, address_mode); > - } > + if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > + false)) { > + copy_ra_to_sb(op, address_mode); > + } > +} > + > +static void > +ovn_update_ipv6_options(struct hmap *lr_ports) > +{ > + struct ovn_port *op; > + HMAP_FOR_EACH (op, key_node, lr_ports) { > + ovs_assert(op->nbrp); > + ovn_update_ipv6_opt_for_op(op); > } > } > > @@ -18027,8 +18074,6 @@ ovnnb_db_run(struct northd_input *input_data, > &data->lr_ports); > stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > - ovn_update_ipv6_options(&data->lr_ports); > - ovn_update_ipv6_prefix(&data->lr_ports); > > sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, > input_data->sbrec_mirror_table); > @@ -18359,6 +18404,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn, > &ha_ref_chassis_map); > } > shash_destroy(&ha_ref_chassis_map); > + > + ovn_update_ipv6_prefix(lr_ports); > } > > const char * > diff --git a/northd/northd.h b/northd/northd.h > index 0478826f0a..3945e84bb8 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -368,7 +368,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct lflow_input *, > struct hmap *lflows); > bool northd_handle_sb_port_binding_changes( > - const struct sbrec_port_binding_table *, struct hmap *ls_ports); > + const struct sbrec_port_binding_table *, struct hmap *ls_ports, > + struct hmap *lr_ports); > > struct tracked_lb_data; > bool northd_handle_lb_data_changes(struct tracked_lb_data *, > @@ -398,7 +399,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, > struct chassis_features *chassis_features); > bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, > + struct hmap *lr_ports); > bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *); > > bool northd_has_tracked_data(struct northd_tracked_data *); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 699f6cfdce..55a244c8c4 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -11018,7 +11018,9 @@ check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-add lr0 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Adding a logical router port should result in recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > @@ -11026,8 +11028,10 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > # for northd engine there will be both recompute and compute > # first it will be recompute to handle lr0-sw0 and then a compute > # for the SB port binding change. > -check_engine_stats northd recompute nocompute > +check_engine_stats northd recompute compute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > ovn-nbctl lsp-add sw0 sw0-lr0 > ovn-nbctl lsp-set-type sw0-lr0 router > @@ -11035,7 +11039,9 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > ovn-nbctl ls-add public > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > @@ -11057,9 +11063,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > - > -check as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Do checks for NATs. > # Add a NAT. This should not result in recompute of both northd and lflow > @@ -11068,6 +11074,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT options column > @@ -11075,6 +11082,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . options:foo=bar > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT external_ip column > @@ -11082,6 +11090,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120 > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT logical_ip column > @@ -11089,6 +11098,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10 > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT type > @@ -11096,13 +11106,15 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . type=snat > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Create a dnat_and_snat NAT with external_mac and logical_port > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03 > -check_engine_stats northd recompute nocompute > +check_engine_stats northd recompute compute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4) > @@ -11111,6 +11123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"' > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Create a load balancer and add the lb vip as NAT > @@ -11124,31 +11137,35 @@ check ovn-nbctl lr-lb-add lr0 lb2 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Delete the NAT > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb clear logical_router lr0 nat > -check_engine_stats northd recompute nocompute > +check_engine_stats northd recompute compute > check_engine_stats lflow recompute nocompute > check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -11157,12 +11174,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > OVN_CLEANUP([hv1]) > AT_CLEANUP > -- > 2.41.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Nov 14, 2023 at 9:40 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Oct 26, 2023 at 11:15 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > It also moves the logical router port IPv6 prefix delegation > > updates to "sync-from-sb" engine node. > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > northd/en-northd.c | 2 +- > > northd/en-sync-sb.c | 3 +- > > northd/northd.c | 283 ++++++++++++++++++++++++++------------------ > > northd/northd.h | 6 +- > > tests/ovn-northd.at | 31 ++++- > > 5 files changed, 198 insertions(+), 127 deletions(-) > > > > diff --git a/northd/en-northd.c b/northd/en-northd.c > > index 96c2ce9f69..13e731cad9 100644 > > --- a/northd/en-northd.c > > +++ b/northd/en-northd.c > > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node, > > northd_get_input_data(node, &input_data); > > > > if (!northd_handle_sb_port_binding_changes( > > - input_data.sbrec_port_binding_table, &nd->ls_ports)) { > > + input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) { > > return false; > > } > > > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > > index 2540fcfb97..a14c609acd 100644 > > --- a/northd/en-sync-sb.c > > +++ b/northd/en-sync-sb.c > > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) > > const struct engine_context *eng_ctx = engine_get_context(); > > struct northd_data *northd_data = engine_get_input_data("northd", node); > > > > - sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, > > + &northd_data->lr_ports); > > engine_set_node_state(node, EN_UPDATED); > > } > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 9ce1b2cb5a..c9c7045755 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > > { > > sbrec_port_binding_set_datapath(op->sb, op->od->sb); > > if (op->nbrp) { > > + /* Note: SB port binding options for router ports are set in > > + * sync_pbs(). */ > > + > > /* If the router is for l3 gateway, it resides on a chassis > > * and its port type is "l3gateway". */ > > const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); > > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > > sbrec_port_binding_set_type(op->sb, "patch"); > > } > > > > - struct smap new; > > - smap_init(&new); > > if (is_cr_port(op)) { > > ovs_assert(sbrec_chassis_by_name); > > ovs_assert(sbrec_chassis_by_hostname); > > ovs_assert(sbrec_ha_chassis_grp_by_name); > > ovs_assert(active_ha_chassis_grps); > > - const char *redirect_type = smap_get(&op->nbrp->options, > > - "redirect-type"); > > > > if (op->nbrp->ha_chassis_group) { > > if (op->nbrp->n_gateway_chassis) { > > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > > /* Delete the legacy gateway_chassis from the pb. */ > > sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0); > > } > > - smap_add(&new, "distributed-port", op->nbrp->name); > > - > > - bool always_redirect = > > - !op->od->has_distributed_nat && > > - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > > - > > - if (redirect_type) { > > - smap_add(&new, "redirect-type", redirect_type); > > - /* XXX Why can't we enable always-redirect when redirect-type > > - * is bridged? */ > > - if (!strcmp(redirect_type, "bridged")) { > > - always_redirect = false; > > - } > > - } > > - > > - if (always_redirect) { > > - smap_add(&new, "always-redirect", "true"); > > - } > > - } else { > > - if (op->peer) { > > - smap_add(&new, "peer", op->peer->key); > > - if (op->nbrp->ha_chassis_group || > > - op->nbrp->n_gateway_chassis) { > > - char *redirect_name = > > - ovn_chassis_redirect_name(op->nbrp->name); > > - smap_add(&new, "chassis-redirect-port", redirect_name); > > - free(redirect_name); > > - } > > - } > > - if (chassis_name) { > > - smap_add(&new, "l3gateway-chassis", chassis_name); > > - } > > - } > > - > > - const char *ipv6_pd_list = smap_get(&op->sb->options, > > - "ipv6_ra_pd_list"); > > - if (ipv6_pd_list) { > > - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > > } > > > > - sbrec_port_binding_set_options(op->sb, &new); > > - smap_destroy(&new); > > - > > sbrec_port_binding_set_parent_port(op->sb, NULL); > > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > > > @@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) > > return duplicates; > > } > > > > -/* Syncs the SB port binding for the ovn_port 'op'. Caller should make sure > > - * that the OVN SB IDL txn is not NULL. Presently it only syncs the nat > > - * column of port binding corresponding to the 'op->nbsp' */ > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port. > > + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it > > + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */ > > static void > > -sync_pb_for_op(struct ovn_port *op) > > +sync_pb_for_lsp(struct ovn_port *op) > > { > > + ovs_assert(op->nbsp); > > + > > if (lsp_is_router(op->nbsp)) { > > const char *chassis = NULL; > > if (op->peer && op->peer->od && op->peer->od->nbr) { > > @@ -4879,18 +4839,87 @@ sync_pb_for_op(struct ovn_port *op) > > } > > } > > > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port. > > + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it > > + * only sets the port binding options column for the router ports */ > > +static void > > +sync_pb_for_lrp(struct ovn_port *op) > > +{ > > + ovs_assert(op->nbrp); > > + > > + struct smap new; > > + smap_init(&new); > > + > > + const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); > > + if (is_cr_port(op)) { > > + smap_add(&new, "distributed-port", op->nbrp->name); > > + > > + bool always_redirect = > > + !op->od->has_distributed_nat && > > + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > > + > > + const char *redirect_type = smap_get(&op->nbrp->options, > > + "redirect-type"); > > + if (redirect_type) { > > + smap_add(&new, "redirect-type", redirect_type); > > + /* XXX Why can't we enable always-redirect when redirect-type > > + * is bridged? */ > > + if (!strcmp(redirect_type, "bridged")) { > > + always_redirect = false; > > + } > > + } > > + > > + if (always_redirect) { > > + smap_add(&new, "always-redirect", "true"); > > + } > > + } else { > > + if (op->peer) { > > + smap_add(&new, "peer", op->peer->key); > > + if (op->nbrp->ha_chassis_group || > > + op->nbrp->n_gateway_chassis) { > > + char *redirect_name = > > + ovn_chassis_redirect_name(op->nbrp->name); > > + smap_add(&new, "chassis-redirect-port", redirect_name); > > + free(redirect_name); > > + } > > + } > > + if (chassis_name) { > > + smap_add(&new, "l3gateway-chassis", chassis_name); > > + } > > + } > > + > > + const char *ipv6_pd_list = smap_get(&op->sb->options, > > + "ipv6_ra_pd_list"); > > + if (ipv6_pd_list) { > > + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > > + } > > + > > + sbrec_port_binding_set_options(op->sb, &new); > > + smap_destroy(&new); > > +} > > + > > +static void ovn_update_ipv6_options(struct hmap *lr_ports); > > +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op); > > + > > /* Sync the SB Port bindings which needs to be updated. > > * Presently it syncs the nat column of port bindings corresponding to > > * the logical switch ports. */ > > void > > -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > > +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, > > + struct hmap *lr_ports) > > { > > ovs_assert(ovnsb_idl_txn); > > > > struct ovn_port *op; > > HMAP_FOR_EACH (op, key_node, ls_ports) { > > - sync_pb_for_op(op); > > + sync_pb_for_lsp(op); > > + } > > + > > + HMAP_FOR_EACH (op, key_node, lr_ports) { > > + sync_pb_for_lrp(op); > > } > > + > > + ovn_update_ipv6_options(lr_ports); > > } > > > > /* Sync the SB Port bindings for the added and updated logical switch ports > > @@ -4902,12 +4931,22 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports) > > struct ovn_port *op; > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { > > op = hmapx_node->data; > > - sync_pb_for_op(op); > > + if (op->nbsp) { > > + sync_pb_for_lsp(op); > > + } else { Sorry that I missed a comment in my previous reply. I didn't find any place that the trk_ovn_ports would have a LRP (i.e. op->nbrp != NULL) added. How could this branch be executed? Thanks, Han > > + sync_pb_for_lrp(op); > > + ovn_update_ipv6_opt_for_op(op); > > + } > > } > > > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { > > op = hmapx_node->data; > > - sync_pb_for_op(op); > > + if (op->nbsp) { > > + sync_pb_for_lsp(op); > > + } else { > > + sync_pb_for_lrp(op); > > + ovn_update_ipv6_opt_for_op(op); > > + } > > } > > > > return true; > > @@ -5703,20 +5742,21 @@ fail: > > bool > > northd_handle_sb_port_binding_changes( > > const struct sbrec_port_binding_table *sbrec_port_binding_table, > > - struct hmap *ls_ports) > > + struct hmap *ls_ports, struct hmap *lr_ports) > > { > > const struct sbrec_port_binding *pb; > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) { > > - /* Return false if the 'pb' belongs to a router port. We don't handle > > - * I-P for router ports yet. */ > > - if (is_pb_router_type(pb)) { > > - return false; > > - } > > + bool is_router_port = is_pb_router_type(pb); > > + struct ovn_port *op; > > > > - struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port); > > - if (op && !op->lsp_can_be_inc_processed) { > > - return false; > > + if (is_router_port) { > > It looks as if we can handle any router type port PB updates but in the else branch we are very strict about what switch port PB updates we can handle, which sounds not right because I think router type handling are more complex. I am wondering either we missed some checks for the router type ports, or we were too strict about switch ports. I am not sure but just want to double check. > > > + op = ovn_port_find(lr_ports, pb->logical_port); > > + } else { > > + op = ovn_port_find(ls_ports, pb->logical_port); > > + if (op && !op->lsp_can_be_inc_processed) { > > + return false; > > + } > > } > > > > if (sbrec_port_binding_is_new(pb)) { > > @@ -5725,7 +5765,8 @@ northd_handle_sb_port_binding_changes( > > * pointer in northd data. Fallback to recompute otherwise. */ > > if (!op) { > > VLOG_WARN_RL(&rl, "A port-binding for %s is created but the " > > - "LSP is not found.", pb->logical_port); > > + "%s is not found.", pb->logical_port, > > + is_router_port ? "LRP" : "LSP"); > > return false; > > } > > op->sb = pb; > > @@ -5736,7 +5777,7 @@ northd_handle_sb_port_binding_changes( > > * sb idl pointers and other unexpected behavior. */ > > if (op) { > > VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the " > > - "LSP still exists.", pb->logical_port); > > + "LSP/LRP still exists.", pb->logical_port); > > return false; > > } > > } else { > > @@ -5746,7 +5787,8 @@ northd_handle_sb_port_binding_changes( > > Here is a comment that needs to be updated: > /* The PB is updated, most likely because of binding/unbinding > * to/from a chassis, and we can ignore the change (updating NB > * "up" will be handled in the engine node "sync_from_sb"). > > This part of the comment needs an update for the router port handling added regarding the IPv6 prefix delegation. > > Thanks, > Han > > > * Fallback to recompute for anything unexpected. */ > > if (!op) { > > VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the " > > - "LSP is not found.", pb->logical_port); > > + "%s is not found.", pb->logical_port, > > + is_router_port ? "LRP" : "LSP"); > > return false; > > } > > if (op->sb != pb) { > > @@ -7895,67 +7937,72 @@ static void > > copy_ra_to_sb(struct ovn_port *op, const char *address_mode); > > > > static void > > -ovn_update_ipv6_options(struct hmap *lr_ports) > > +ovn_update_ipv6_opt_for_op(struct ovn_port *op) > > { > > - struct ovn_port *op; > > - HMAP_FOR_EACH (op, key_node, lr_ports) { > > - ovs_assert(op->nbrp); > > - > > - if (op->nbrp->peer || !op->peer) { > > - continue; > > - } > > + if (op->nbrp->peer || !op->peer) { > > + return; > > + } > > > > - if (!op->lrp_networks.n_ipv6_addrs) { > > - continue; > > - } > > + if (!op->lrp_networks.n_ipv6_addrs) { > > + return; > > + } > > > > - struct smap options; > > - smap_clone(&options, &op->sb->options); > > + struct smap options; > > + smap_clone(&options, &op->sb->options); > > > > - /* enable IPv6 prefix delegation */ > > - bool prefix_delegation = smap_get_bool(&op->nbrp->options, > > - "prefix_delegation", false); > > - if (!lrport_is_enabled(op->nbrp)) { > > - prefix_delegation = false; > > - } > > - if (smap_get_bool(&options, "ipv6_prefix_delegation", > > - false) != prefix_delegation) { > > - smap_add(&options, "ipv6_prefix_delegation", > > - prefix_delegation ? "true" : "false"); > > - } > > + /* enable IPv6 prefix delegation */ > > + bool prefix_delegation = smap_get_bool(&op->nbrp->options, > > + "prefix_delegation", false); > > + if (!lrport_is_enabled(op->nbrp)) { > > + prefix_delegation = false; > > + } > > + if (smap_get_bool(&options, "ipv6_prefix_delegation", > > + false) != prefix_delegation) { > > + smap_add(&options, "ipv6_prefix_delegation", > > + prefix_delegation ? "true" : "false"); > > + } > > > > - bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > > + bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > > "prefix", false); > > - if (!lrport_is_enabled(op->nbrp)) { > > - ipv6_prefix = false; > > - } > > - if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > > - smap_add(&options, "ipv6_prefix", > > - ipv6_prefix ? "true" : "false"); > > - } > > - sbrec_port_binding_set_options(op->sb, &options); > > + if (!lrport_is_enabled(op->nbrp)) { > > + ipv6_prefix = false; > > + } > > + if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > > + smap_add(&options, "ipv6_prefix", > > + ipv6_prefix ? "true" : "false"); > > + } > > + sbrec_port_binding_set_options(op->sb, &options); > > > > - smap_destroy(&options); > > + smap_destroy(&options); > > > > - const char *address_mode = smap_get( > > - &op->nbrp->ipv6_ra_configs, "address_mode"); > > + const char *address_mode = smap_get( > > + &op->nbrp->ipv6_ra_configs, "address_mode"); > > > > - if (!address_mode) { > > - continue; > > - } > > - if (strcmp(address_mode, "slaac") && > > - strcmp(address_mode, "dhcpv6_stateful") && > > - strcmp(address_mode, "dhcpv6_stateless")) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > - VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > > - address_mode); > > - continue; > > - } > > + if (!address_mode) { > > + return; > > + } > > + if (strcmp(address_mode, "slaac") && > > + strcmp(address_mode, "dhcpv6_stateful") && > > + strcmp(address_mode, "dhcpv6_stateless")) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > + VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > > + address_mode); > > + return; > > + } > > > > - if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > > - false)) { > > - copy_ra_to_sb(op, address_mode); > > - } > > + if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > > + false)) { > > + copy_ra_to_sb(op, address_mode); > > + } > > +} > > + > > +static void > > +ovn_update_ipv6_options(struct hmap *lr_ports) > > +{ > > + struct ovn_port *op; > > + HMAP_FOR_EACH (op, key_node, lr_ports) { > > + ovs_assert(op->nbrp); > > + ovn_update_ipv6_opt_for_op(op); > > } > > } > > > > @@ -18027,8 +18074,6 @@ ovnnb_db_run(struct northd_input *input_data, > > &data->lr_ports); > > stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > - ovn_update_ipv6_options(&data->lr_ports); > > - ovn_update_ipv6_prefix(&data->lr_ports); > > > > sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, > > input_data->sbrec_mirror_table); > > @@ -18359,6 +18404,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn, > > &ha_ref_chassis_map); > > } > > shash_destroy(&ha_ref_chassis_map); > > + > > + ovn_update_ipv6_prefix(lr_ports); > > } > > > > const char * > > diff --git a/northd/northd.h b/northd/northd.h > > index 0478826f0a..3945e84bb8 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -368,7 +368,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > > struct lflow_input *, > > struct hmap *lflows); > > bool northd_handle_sb_port_binding_changes( > > - const struct sbrec_port_binding_table *, struct hmap *ls_ports); > > + const struct sbrec_port_binding_table *, struct hmap *ls_ports, > > + struct hmap *lr_ports); > > > > struct tracked_lb_data; > > bool northd_handle_lb_data_changes(struct tracked_lb_data *, > > @@ -398,7 +399,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, > > struct chassis_features *chassis_features); > > bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > > > -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, > > + struct hmap *lr_ports); > > bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *); > > > > bool northd_has_tracked_data(struct northd_tracked_data *); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 699f6cfdce..55a244c8c4 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -11018,7 +11018,9 @@ check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-add lr0 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Adding a logical router port should result in recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > @@ -11026,8 +11028,10 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > > # for northd engine there will be both recompute and compute > > # first it will be recompute to handle lr0-sw0 and then a compute > > # for the SB port binding change. > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd recompute compute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > ovn-nbctl lsp-add sw0 sw0-lr0 > > ovn-nbctl lsp-set-type sw0-lr0 router > > @@ -11035,7 +11039,9 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > ovn-nbctl ls-add public > > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > > @@ -11057,9 +11063,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > - > > -check as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Do checks for NATs. > > # Add a NAT. This should not result in recompute of both northd and lflow > > @@ -11068,6 +11074,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT options column > > @@ -11075,6 +11082,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . options:foo=bar > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT external_ip column > > @@ -11082,6 +11090,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120 > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT logical_ip column > > @@ -11089,6 +11098,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10 > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT type > > @@ -11096,13 +11106,15 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . type=snat > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Create a dnat_and_snat NAT with external_mac and logical_port > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03 > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd recompute compute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4) > > @@ -11111,6 +11123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"' > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Create a load balancer and add the lb vip as NAT > > @@ -11124,31 +11137,35 @@ check ovn-nbctl lr-lb-add lr0 lb2 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Delete the NAT > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb clear logical_router lr0 nat > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd recompute compute > > check_engine_stats lflow recompute nocompute > > check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -11157,12 +11174,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > -- > > 2.41.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Nov 15, 2023 at 1:24 AM Han Zhou <hzhou@ovn.org> wrote: > > On Tue, Nov 14, 2023 at 9:40 PM Han Zhou <hzhou@ovn.org> wrote: > > > > > > > > On Thu, Oct 26, 2023 at 11:15 AM <numans@ovn.org> wrote: > > > > > > From: Numan Siddique <numans@ovn.org> > > > > > > It also moves the logical router port IPv6 prefix delegation > > > updates to "sync-from-sb" engine node. > > > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > > --- > > > northd/en-northd.c | 2 +- > > > northd/en-sync-sb.c | 3 +- > > > northd/northd.c | 283 ++++++++++++++++++++++++++------------------ > > > northd/northd.h | 6 +- > > > tests/ovn-northd.at | 31 ++++- > > > 5 files changed, 198 insertions(+), 127 deletions(-) > > > > > > diff --git a/northd/en-northd.c b/northd/en-northd.c > > > index 96c2ce9f69..13e731cad9 100644 > > > --- a/northd/en-northd.c > > > +++ b/northd/en-northd.c > > > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node > *node, > > > northd_get_input_data(node, &input_data); > > > > > > if (!northd_handle_sb_port_binding_changes( > > > - input_data.sbrec_port_binding_table, &nd->ls_ports)) { > > > + input_data.sbrec_port_binding_table, &nd->ls_ports, > &nd->lr_ports)) { > > > return false; > > > } > > > > > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > > > index 2540fcfb97..a14c609acd 100644 > > > --- a/northd/en-sync-sb.c > > > +++ b/northd/en-sync-sb.c > > > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void > *data OVS_UNUSED) > > > const struct engine_context *eng_ctx = engine_get_context(); > > > struct northd_data *northd_data = engine_get_input_data("northd", > node); > > > > > > - sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > > > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, > > > + &northd_data->lr_ports); > > > engine_set_node_state(node, EN_UPDATED); > > > } > > > > > > diff --git a/northd/northd.c b/northd/northd.c > > > index 9ce1b2cb5a..c9c7045755 100644 > > > --- a/northd/northd.c > > > +++ b/northd/northd.c > > > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > > { > > > sbrec_port_binding_set_datapath(op->sb, op->od->sb); > > > if (op->nbrp) { > > > + /* Note: SB port binding options for router ports are set in > > > + * sync_pbs(). */ > > > + > > > /* If the router is for l3 gateway, it resides on a chassis > > > * and its port type is "l3gateway". */ > > > const char *chassis_name = smap_get(&op->od->nbr->options, > "chassis"); > > > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > > sbrec_port_binding_set_type(op->sb, "patch"); > > > } > > > > > > - struct smap new; > > > - smap_init(&new); > > > if (is_cr_port(op)) { > > > ovs_assert(sbrec_chassis_by_name); > > > ovs_assert(sbrec_chassis_by_hostname); > > > ovs_assert(sbrec_ha_chassis_grp_by_name); > > > ovs_assert(active_ha_chassis_grps); > > > - const char *redirect_type = smap_get(&op->nbrp->options, > > > - "redirect-type"); > > > > > > if (op->nbrp->ha_chassis_group) { > > > if (op->nbrp->n_gateway_chassis) { > > > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > > /* Delete the legacy gateway_chassis from the pb. */ > > > sbrec_port_binding_set_gateway_chassis(op->sb, NULL, > 0); > > > } > > > - smap_add(&new, "distributed-port", op->nbrp->name); > > > - > > > - bool always_redirect = > > > - !op->od->has_distributed_nat && > > > - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > > > - > > > - if (redirect_type) { > > > - smap_add(&new, "redirect-type", redirect_type); > > > - /* XXX Why can't we enable always-redirect when > redirect-type > > > - * is bridged? */ > > > - if (!strcmp(redirect_type, "bridged")) { > > > - always_redirect = false; > > > - } > > > - } > > > - > > > - if (always_redirect) { > > > - smap_add(&new, "always-redirect", "true"); > > > - } > > > - } else { > > > - if (op->peer) { > > > - smap_add(&new, "peer", op->peer->key); > > > - if (op->nbrp->ha_chassis_group || > > > - op->nbrp->n_gateway_chassis) { > > > - char *redirect_name = > > > - ovn_chassis_redirect_name(op->nbrp->name); > > > - smap_add(&new, "chassis-redirect-port", > redirect_name); > > > - free(redirect_name); > > > - } > > > - } > > > - if (chassis_name) { > > > - smap_add(&new, "l3gateway-chassis", chassis_name); > > > - } > > > - } > > > - > > > - const char *ipv6_pd_list = smap_get(&op->sb->options, > > > - "ipv6_ra_pd_list"); > > > - if (ipv6_pd_list) { > > > - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > > > } > > > > > > - sbrec_port_binding_set_options(op->sb, &new); > > > - smap_destroy(&new); > > > - > > > sbrec_port_binding_set_parent_port(op->sb, NULL); > > > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > > > > > @@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct > sbrec_load_balancer_table *table) > > > return duplicates; > > > } > > > > > > -/* Syncs the SB port binding for the ovn_port 'op'. Caller should > make sure > > > - * that the OVN SB IDL txn is not NULL. Presently it only syncs the > nat > > > - * column of port binding corresponding to the 'op->nbsp' */ > > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch > port. > > > + * Caller should make sure that the OVN SB IDL txn is not NULL. > Presently it > > > + * only syncs the nat column of port binding corresponding to the > 'op->nbsp' */ > > > static void > > > -sync_pb_for_op(struct ovn_port *op) > > > +sync_pb_for_lsp(struct ovn_port *op) > > > { > > > + ovs_assert(op->nbsp); > > > + > > > if (lsp_is_router(op->nbsp)) { > > > const char *chassis = NULL; > > > if (op->peer && op->peer->od && op->peer->od->nbr) { > > > @@ -4879,18 +4839,87 @@ sync_pb_for_op(struct ovn_port *op) > > > } > > > } > > > > > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical router > port. > > > + * Caller should make sure that the OVN SB IDL txn is not NULL. > Presently it > > > + * only sets the port binding options column for the router ports */ > > > +static void > > > +sync_pb_for_lrp(struct ovn_port *op) > > > +{ > > > + ovs_assert(op->nbrp); > > > + > > > + struct smap new; > > > + smap_init(&new); > > > + > > > + const char *chassis_name = smap_get(&op->od->nbr->options, > "chassis"); > > > + if (is_cr_port(op)) { > > > + smap_add(&new, "distributed-port", op->nbrp->name); > > > + > > > + bool always_redirect = > > > + !op->od->has_distributed_nat && > > > + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > > > + > > > + const char *redirect_type = smap_get(&op->nbrp->options, > > > + "redirect-type"); > > > + if (redirect_type) { > > > + smap_add(&new, "redirect-type", redirect_type); > > > + /* XXX Why can't we enable always-redirect when > redirect-type > > > + * is bridged? */ > > > + if (!strcmp(redirect_type, "bridged")) { > > > + always_redirect = false; > > > + } > > > + } > > > + > > > + if (always_redirect) { > > > + smap_add(&new, "always-redirect", "true"); > > > + } > > > + } else { > > > + if (op->peer) { > > > + smap_add(&new, "peer", op->peer->key); > > > + if (op->nbrp->ha_chassis_group || > > > + op->nbrp->n_gateway_chassis) { > > > + char *redirect_name = > > > + ovn_chassis_redirect_name(op->nbrp->name); > > > + smap_add(&new, "chassis-redirect-port", redirect_name); > > > + free(redirect_name); > > > + } > > > + } > > > + if (chassis_name) { > > > + smap_add(&new, "l3gateway-chassis", chassis_name); > > > + } > > > + } > > > + > > > + const char *ipv6_pd_list = smap_get(&op->sb->options, > > > + "ipv6_ra_pd_list"); > > > + if (ipv6_pd_list) { > > > + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > > > + } > > > + > > > + sbrec_port_binding_set_options(op->sb, &new); > > > + smap_destroy(&new); > > > +} > > > + > > > +static void ovn_update_ipv6_options(struct hmap *lr_ports); > > > +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op); > > > + > > > /* Sync the SB Port bindings which needs to be updated. > > > * Presently it syncs the nat column of port bindings corresponding to > > > * the logical switch ports. */ > > > void > > > -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > > > +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, > > > + struct hmap *lr_ports) > > > { > > > ovs_assert(ovnsb_idl_txn); > > > > > > struct ovn_port *op; > > > HMAP_FOR_EACH (op, key_node, ls_ports) { > > > - sync_pb_for_op(op); > > > + sync_pb_for_lsp(op); > > > + } > > > + > > > + HMAP_FOR_EACH (op, key_node, lr_ports) { > > > + sync_pb_for_lrp(op); > > > } > > > + > > > + ovn_update_ipv6_options(lr_ports); > > > } > > > > > > /* Sync the SB Port bindings for the added and updated logical switch > ports > > > @@ -4902,12 +4931,22 @@ sync_pbs_for_northd_changed_ovn_ports( struct > tracked_ovn_ports *trk_ovn_ports) > > > struct ovn_port *op; > > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { > > > op = hmapx_node->data; > > > - sync_pb_for_op(op); > > > + if (op->nbsp) { > > > + sync_pb_for_lsp(op); > > > + } else { > > Sorry that I missed a comment in my previous reply. I didn't find any place > that the trk_ovn_ports would have a LRP (i.e. op->nbrp != NULL) added. How > could this branch be executed? Good catch. I'll remove it. > > Thanks, > Han > > > > + sync_pb_for_lrp(op); > > > + ovn_update_ipv6_opt_for_op(op); > > > + } > > > } > > > > > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { > > > op = hmapx_node->data; > > > - sync_pb_for_op(op); > > > + if (op->nbsp) { > > > + sync_pb_for_lsp(op); > > > + } else { > > > + sync_pb_for_lrp(op); > > > + ovn_update_ipv6_opt_for_op(op); > > > + } > > > } > > > > > > return true; > > > @@ -5703,20 +5742,21 @@ fail: > > > bool > > > northd_handle_sb_port_binding_changes( > > > const struct sbrec_port_binding_table *sbrec_port_binding_table, > > > - struct hmap *ls_ports) > > > + struct hmap *ls_ports, struct hmap *lr_ports) > > > { > > > const struct sbrec_port_binding *pb; > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > sbrec_port_binding_table) { > > > - /* Return false if the 'pb' belongs to a router port. We > don't handle > > > - * I-P for router ports yet. */ > > > - if (is_pb_router_type(pb)) { > > > - return false; > > > - } > > > + bool is_router_port = is_pb_router_type(pb); > > > + struct ovn_port *op; > > > > > > - struct ovn_port *op = ovn_port_find(ls_ports, > pb->logical_port); > > > - if (op && !op->lsp_can_be_inc_processed) { > > > - return false; > > > + if (is_router_port) { > > > > It looks as if we can handle any router type port PB updates but in the > else branch we are very strict about what switch port PB updates we can > handle, which sounds not right because I think router type handling are > more complex. I am wondering either we missed some checks for the router > type ports, or we were too strict about switch ports. I am not sure but > just want to double check. The check - "if (op && !op->lsp_can_be_inc_processed)" was present before this patch and I just kept it. I think you added it for LSP VIF I-P. IMO SB port binding handler for northd engine node (northd_handle_sb_port_binding_changes()), doesn't need to do much other than updating "op->sb = sb". I think for all port binding updates (be it switch ports or router ports) we don't have to do anything special. And anything specific we need to do, like updating the up column of lsp and updating the IPv6 PD to the lrp we are doing in the "sync_from_sb" node. I'll remove the condition below to return false for lsp PB and test it out. If it works out fine I'll remove it in the next version. -- if (op && !op->lsp_can_be_inc_processed) { return false; } --- Let me know if you think otherwise. Thanks > > > > > + op = ovn_port_find(lr_ports, pb->logical_port); > > > + } else { > > > + op = ovn_port_find(ls_ports, pb->logical_port); > > > + if (op && !op->lsp_can_be_inc_processed) { > > > + return false; > > > + } > > > } > > > > > > if (sbrec_port_binding_is_new(pb)) { > > > @@ -5725,7 +5765,8 @@ northd_handle_sb_port_binding_changes( > > > * pointer in northd data. Fallback to recompute > otherwise. */ > > > if (!op) { > > > VLOG_WARN_RL(&rl, "A port-binding for %s is created > but the " > > > - "LSP is not found.", pb->logical_port); > > > + "%s is not found.", pb->logical_port, > > > + is_router_port ? "LRP" : "LSP"); > > > return false; > > > } > > > op->sb = pb; > > > @@ -5736,7 +5777,7 @@ northd_handle_sb_port_binding_changes( > > > * sb idl pointers and other unexpected behavior. */ > > > if (op) { > > > VLOG_WARN_RL(&rl, "A port-binding for %s is deleted > but the " > > > - "LSP still exists.", pb->logical_port); > > > + "LSP/LRP still exists.", pb->logical_port); > > > return false; > > > } > > > } else { > > > @@ -5746,7 +5787,8 @@ northd_handle_sb_port_binding_changes( > > > > Here is a comment that needs to be updated: > > /* The PB is updated, most likely because of > binding/unbinding > > * to/from a chassis, and we can ignore the change (updating > NB > > * "up" will be handled in the engine node "sync_from_sb"). > > > > This part of the comment needs an update for the router port handling > added regarding the IPv6 prefix delegation. Ack. Thanks Numan > > > > Thanks, > > Han > > > > > * Fallback to recompute for anything unexpected. */ > > > if (!op) { > > > VLOG_WARN_RL(&rl, "A port-binding for %s is updated > but the " > > > - "LSP is not found.", pb->logical_port); > > > + "%s is not found.", pb->logical_port, > > > + is_router_port ? "LRP" : "LSP"); > > > return false; > > > } > > > if (op->sb != pb) { > > > @@ -7895,67 +7937,72 @@ static void > > > copy_ra_to_sb(struct ovn_port *op, const char *address_mode); > > > > > > static void > > > -ovn_update_ipv6_options(struct hmap *lr_ports) > > > +ovn_update_ipv6_opt_for_op(struct ovn_port *op) > > > { > > > - struct ovn_port *op; > > > - HMAP_FOR_EACH (op, key_node, lr_ports) { > > > - ovs_assert(op->nbrp); > > > - > > > - if (op->nbrp->peer || !op->peer) { > > > - continue; > > > - } > > > + if (op->nbrp->peer || !op->peer) { > > > + return; > > > + } > > > > > > - if (!op->lrp_networks.n_ipv6_addrs) { > > > - continue; > > > - } > > > + if (!op->lrp_networks.n_ipv6_addrs) { > > > + return; > > > + } > > > > > > - struct smap options; > > > - smap_clone(&options, &op->sb->options); > > > + struct smap options; > > > + smap_clone(&options, &op->sb->options); > > > > > > - /* enable IPv6 prefix delegation */ > > > - bool prefix_delegation = smap_get_bool(&op->nbrp->options, > > > - "prefix_delegation", false); > > > - if (!lrport_is_enabled(op->nbrp)) { > > > - prefix_delegation = false; > > > - } > > > - if (smap_get_bool(&options, "ipv6_prefix_delegation", > > > - false) != prefix_delegation) { > > > - smap_add(&options, "ipv6_prefix_delegation", > > > - prefix_delegation ? "true" : "false"); > > > - } > > > + /* enable IPv6 prefix delegation */ > > > + bool prefix_delegation = smap_get_bool(&op->nbrp->options, > > > + "prefix_delegation", false); > > > + if (!lrport_is_enabled(op->nbrp)) { > > > + prefix_delegation = false; > > > + } > > > + if (smap_get_bool(&options, "ipv6_prefix_delegation", > > > + false) != prefix_delegation) { > > > + smap_add(&options, "ipv6_prefix_delegation", > > > + prefix_delegation ? "true" : "false"); > > > + } > > > > > > - bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > > > + bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > > > "prefix", false); > > > - if (!lrport_is_enabled(op->nbrp)) { > > > - ipv6_prefix = false; > > > - } > > > - if (smap_get_bool(&options, "ipv6_prefix", false) != > ipv6_prefix) { > > > - smap_add(&options, "ipv6_prefix", > > > - ipv6_prefix ? "true" : "false"); > > > - } > > > - sbrec_port_binding_set_options(op->sb, &options); > > > + if (!lrport_is_enabled(op->nbrp)) { > > > + ipv6_prefix = false; > > > + } > > > + if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > > > + smap_add(&options, "ipv6_prefix", > > > + ipv6_prefix ? "true" : "false"); > > > + } > > > + sbrec_port_binding_set_options(op->sb, &options); > > > > > > - smap_destroy(&options); > > > + smap_destroy(&options); > > > > > > - const char *address_mode = smap_get( > > > - &op->nbrp->ipv6_ra_configs, "address_mode"); > > > + const char *address_mode = smap_get( > > > + &op->nbrp->ipv6_ra_configs, "address_mode"); > > > > > > - if (!address_mode) { > > > - continue; > > > - } > > > - if (strcmp(address_mode, "slaac") && > > > - strcmp(address_mode, "dhcpv6_stateful") && > > > - strcmp(address_mode, "dhcpv6_stateless")) { > > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > > > - VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > > > - address_mode); > > > - continue; > > > - } > > > + if (!address_mode) { > > > + return; > > > + } > > > + if (strcmp(address_mode, "slaac") && > > > + strcmp(address_mode, "dhcpv6_stateful") && > > > + strcmp(address_mode, "dhcpv6_stateless")) { > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > > + VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > > > + address_mode); > > > + return; > > > + } > > > > > > - if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > > > - false)) { > > > - copy_ra_to_sb(op, address_mode); > > > - } > > > + if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > > > + false)) { > > > + copy_ra_to_sb(op, address_mode); > > > + } > > > +} > > > + > > > +static void > > > +ovn_update_ipv6_options(struct hmap *lr_ports) > > > +{ > > > + struct ovn_port *op; > > > + HMAP_FOR_EACH (op, key_node, lr_ports) { > > > + ovs_assert(op->nbrp); > > > + ovn_update_ipv6_opt_for_op(op); > > > } > > > } > > > > > > @@ -18027,8 +18074,6 @@ ovnnb_db_run(struct northd_input *input_data, > > > &data->lr_ports); > > > stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > > stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > > - ovn_update_ipv6_options(&data->lr_ports); > > > - ovn_update_ipv6_prefix(&data->lr_ports); > > > > > > sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, > > > input_data->sbrec_mirror_table); > > > @@ -18359,6 +18404,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn, > > > &ha_ref_chassis_map); > > > } > > > shash_destroy(&ha_ref_chassis_map); > > > + > > > + ovn_update_ipv6_prefix(lr_ports); > > > } > > > > > > const char * > > > diff --git a/northd/northd.h b/northd/northd.h > > > index 0478826f0a..3945e84bb8 100644 > > > --- a/northd/northd.h > > > +++ b/northd/northd.h > > > @@ -368,7 +368,8 @@ bool lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > > struct lflow_input *, > > > struct hmap *lflows); > > > bool northd_handle_sb_port_binding_changes( > > > - const struct sbrec_port_binding_table *, struct hmap *ls_ports); > > > + const struct sbrec_port_binding_table *, struct hmap *ls_ports, > > > + struct hmap *lr_ports); > > > > > > struct tracked_lb_data; > > > bool northd_handle_lb_data_changes(struct tracked_lb_data *, > > > @@ -398,7 +399,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct > sbrec_load_balancer_table *, > > > struct chassis_features *chassis_features); > > > bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > > > > > -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > > > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, > > > + struct hmap *lr_ports); > > > bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports > *); > > > > > > bool northd_has_tracked_data(struct northd_tracked_data *); > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > index 699f6cfdce..55a244c8c4 100644 > > > --- a/tests/ovn-northd.at > > > +++ b/tests/ovn-northd.at > > > @@ -11018,7 +11018,9 @@ check ovn-nbctl lsp-add sw0 sw0p1 -- > lsp-set-addresses sw0p1 "00:00:20:20:12:01 > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-add lr0 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Adding a logical router port should result in recompute > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > @@ -11026,8 +11028,10 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 > 00:00:00:00:ff:01 10.0.0.1/24 > > > # for northd engine there will be both recompute and compute > > > # first it will be recompute to handle lr0-sw0 and then a compute > > > # for the SB port binding change. > > > -check_engine_stats northd recompute nocompute > > > +check_engine_stats northd recompute compute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > ovn-nbctl lsp-add sw0 sw0-lr0 > > > ovn-nbctl lsp-set-type sw0-lr0 router > > > @@ -11035,7 +11039,9 @@ ovn-nbctl lsp-set-addresses sw0-lr0 > 00:00:00:00:ff:01 > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > ovn-nbctl ls-add public > > > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > > > @@ -11057,9 +11063,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis > lr0-public hv1 20 > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > - > > > -check as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg > > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Do checks for NATs. > > > # Add a NAT. This should not result in recompute of both northd and > lflow > > > @@ -11068,6 +11074,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 > 10.0.0.4 > > > check_engine_stats northd recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Update the NAT options column > > > @@ -11075,6 +11082,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > > check ovn-nbctl --wait=sb set NAT . options:foo=bar > > > check_engine_stats northd recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Update the NAT external_ip column > > > @@ -11082,6 +11090,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > > check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120 > > > check_engine_stats northd recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Update the NAT logical_ip column > > > @@ -11089,6 +11098,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > > check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10 > > > check_engine_stats northd recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Update the NAT type > > > @@ -11096,13 +11106,15 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > > check ovn-nbctl --wait=sb set NAT . type=snat > > > check_engine_stats northd recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Create a dnat_and_snat NAT with external_mac and logical_port > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 > 10.0.0.4 sw0p1 30:54:00:00:00:03 > > > -check_engine_stats northd recompute nocompute > > > +check_engine_stats northd recompute compute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat > logical_ip=10.0.0.4) > > > @@ -11111,6 +11123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > > check ovn-nbctl --wait=sb set NAT $nat2_uuid > external_mac='"30:54:00:00:00:04"' > > > check_engine_stats northd recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Create a load balancer and add the lb vip as NAT > > > @@ -11124,31 +11137,35 @@ check ovn-nbctl lr-lb-add lr0 lb2 > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 > 10.0.0.20 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 > 10.0.0.41 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > # Delete the NAT > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb clear logical_router lr0 nat > > > -check_engine_stats northd recompute nocompute > > > +check_engine_stats northd recompute compute > > > check_engine_stats lflow recompute nocompute > > > check_engine_stats sync_to_sb_pb recompute nocompute > > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > @@ -11157,12 +11174,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" > reroute 172.168.0.101,172.168.0.102 > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > > check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" > > > check_engine_stats northd recompute nocompute > > > +check_engine_stats sync_to_sb_pb recompute nocompute > > > check_engine_stats lflow recompute nocompute > > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > > > OVN_CLEANUP([hv1]) > > > AT_CLEANUP > > > -- > > > 2.41.0 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 10/26/23 20:14, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > It also moves the logical router port IPv6 prefix delegation > updates to "sync-from-sb" engine node. > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- I think I agree in general with the patch; I'd like to see the new revision however because the check that Han also commented on [0] in northd_handle_sb_port_binding_changes() makes me wonder too if we're not missing cases (or if we should just relax the LSP check as well). [0] https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409451.html Aside from that I left a few minor comments below. Thanks, Dumitru > northd/en-northd.c | 2 +- > northd/en-sync-sb.c | 3 +- > northd/northd.c | 283 ++++++++++++++++++++++++++------------------ > northd/northd.h | 6 +- > tests/ovn-northd.at | 31 ++++- > 5 files changed, 198 insertions(+), 127 deletions(-) > > diff --git a/northd/en-northd.c b/northd/en-northd.c > index 96c2ce9f69..13e731cad9 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node, > northd_get_input_data(node, &input_data); > > if (!northd_handle_sb_port_binding_changes( > - input_data.sbrec_port_binding_table, &nd->ls_ports)) { > + input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) { > return false; > } > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 2540fcfb97..a14c609acd 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) > const struct engine_context *eng_ctx = engine_get_context(); > struct northd_data *northd_data = engine_get_input_data("northd", node); > > - sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, > + &northd_data->lr_ports); > engine_set_node_state(node, EN_UPDATED); > } > > diff --git a/northd/northd.c b/northd/northd.c > index 9ce1b2cb5a..c9c7045755 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > { > sbrec_port_binding_set_datapath(op->sb, op->od->sb); > if (op->nbrp) { > + /* Note: SB port binding options for router ports are set in > + * sync_pbs(). */ > + > /* If the router is for l3 gateway, it resides on a chassis > * and its port type is "l3gateway". */ > const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > sbrec_port_binding_set_type(op->sb, "patch"); > } > > - struct smap new; > - smap_init(&new); > if (is_cr_port(op)) { > ovs_assert(sbrec_chassis_by_name); > ovs_assert(sbrec_chassis_by_hostname); > ovs_assert(sbrec_ha_chassis_grp_by_name); > ovs_assert(active_ha_chassis_grps); > - const char *redirect_type = smap_get(&op->nbrp->options, > - "redirect-type"); > > if (op->nbrp->ha_chassis_group) { > if (op->nbrp->n_gateway_chassis) { > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > /* Delete the legacy gateway_chassis from the pb. */ > sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0); > } > - smap_add(&new, "distributed-port", op->nbrp->name); > - > - bool always_redirect = > - !op->od->has_distributed_nat && > - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > - > - if (redirect_type) { > - smap_add(&new, "redirect-type", redirect_type); > - /* XXX Why can't we enable always-redirect when redirect-type > - * is bridged? */ > - if (!strcmp(redirect_type, "bridged")) { > - always_redirect = false; > - } > - } > - > - if (always_redirect) { > - smap_add(&new, "always-redirect", "true"); > - } > - } else { > - if (op->peer) { > - smap_add(&new, "peer", op->peer->key); > - if (op->nbrp->ha_chassis_group || > - op->nbrp->n_gateway_chassis) { > - char *redirect_name = > - ovn_chassis_redirect_name(op->nbrp->name); > - smap_add(&new, "chassis-redirect-port", redirect_name); > - free(redirect_name); > - } > - } > - if (chassis_name) { > - smap_add(&new, "l3gateway-chassis", chassis_name); > - } > - } > - > - const char *ipv6_pd_list = smap_get(&op->sb->options, > - "ipv6_ra_pd_list"); > - if (ipv6_pd_list) { > - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > } > > - sbrec_port_binding_set_options(op->sb, &new); > - smap_destroy(&new); > - > sbrec_port_binding_set_parent_port(op->sb, NULL); > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > @@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) > return duplicates; > } > > -/* Syncs the SB port binding for the ovn_port 'op'. Caller should make sure > - * that the OVN SB IDL txn is not NULL. Presently it only syncs the nat > - * column of port binding corresponding to the 'op->nbsp' */ > +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port. > + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it > + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */ > static void > -sync_pb_for_op(struct ovn_port *op) > +sync_pb_for_lsp(struct ovn_port *op) > { > + ovs_assert(op->nbsp); > + > if (lsp_is_router(op->nbsp)) { > const char *chassis = NULL; > if (op->peer && op->peer->od && op->peer->od->nbr) { > @@ -4879,18 +4839,87 @@ sync_pb_for_op(struct ovn_port *op) > } > } > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port. > + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it > + * only sets the port binding options column for the router ports */ > +static void > +sync_pb_for_lrp(struct ovn_port *op) > +{ > + ovs_assert(op->nbrp); > + > + struct smap new; > + smap_init(&new); > + > + const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); > + if (is_cr_port(op)) { > + smap_add(&new, "distributed-port", op->nbrp->name); > + > + bool always_redirect = > + !op->od->has_distributed_nat && > + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > + > + const char *redirect_type = smap_get(&op->nbrp->options, > + "redirect-type"); > + if (redirect_type) { > + smap_add(&new, "redirect-type", redirect_type); > + /* XXX Why can't we enable always-redirect when redirect-type > + * is bridged? */ > + if (!strcmp(redirect_type, "bridged")) { > + always_redirect = false; > + } > + } > + > + if (always_redirect) { > + smap_add(&new, "always-redirect", "true"); > + } > + } else { > + if (op->peer) { > + smap_add(&new, "peer", op->peer->key); > + if (op->nbrp->ha_chassis_group || > + op->nbrp->n_gateway_chassis) { > + char *redirect_name = > + ovn_chassis_redirect_name(op->nbrp->name); > + smap_add(&new, "chassis-redirect-port", redirect_name); > + free(redirect_name); > + } > + } > + if (chassis_name) { > + smap_add(&new, "l3gateway-chassis", chassis_name); > + } > + } > + > + const char *ipv6_pd_list = smap_get(&op->sb->options, > + "ipv6_ra_pd_list"); Nit: this now fits on a single line. > + if (ipv6_pd_list) { > + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > + } > + > + sbrec_port_binding_set_options(op->sb, &new); > + smap_destroy(&new); > +} > + > +static void ovn_update_ipv6_options(struct hmap *lr_ports); > +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op); > + > /* Sync the SB Port bindings which needs to be updated. > * Presently it syncs the nat column of port bindings corresponding to > * the logical switch ports. */ > void > -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, > + struct hmap *lr_ports) > { > ovs_assert(ovnsb_idl_txn); > > struct ovn_port *op; > HMAP_FOR_EACH (op, key_node, ls_ports) { > - sync_pb_for_op(op); > + sync_pb_for_lsp(op); > + } > + > + HMAP_FOR_EACH (op, key_node, lr_ports) { > + sync_pb_for_lrp(op); > } > + > + ovn_update_ipv6_options(lr_ports); > } > > /* Sync the SB Port bindings for the added and updated logical switch ports > @@ -4902,12 +4931,22 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports) > struct ovn_port *op; > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { > op = hmapx_node->data; > - sync_pb_for_op(op); > + if (op->nbsp) { > + sync_pb_for_lsp(op); > + } else { > + sync_pb_for_lrp(op); > + ovn_update_ipv6_opt_for_op(op); > + } > } > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { > op = hmapx_node->data; > - sync_pb_for_op(op); > + if (op->nbsp) { > + sync_pb_for_lsp(op); > + } else { > + sync_pb_for_lrp(op); > + ovn_update_ipv6_opt_for_op(op); > + } > } > > return true; > @@ -5703,20 +5742,21 @@ fail: > bool > northd_handle_sb_port_binding_changes( > const struct sbrec_port_binding_table *sbrec_port_binding_table, > - struct hmap *ls_ports) > + struct hmap *ls_ports, struct hmap *lr_ports) > { > const struct sbrec_port_binding *pb; > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) { > - /* Return false if the 'pb' belongs to a router port. We don't handle > - * I-P for router ports yet. */ > - if (is_pb_router_type(pb)) { > - return false; > - } > + bool is_router_port = is_pb_router_type(pb); > + struct ovn_port *op; > > - struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port); > - if (op && !op->lsp_can_be_inc_processed) { > - return false; > + if (is_router_port) { > + op = ovn_port_find(lr_ports, pb->logical_port); > + } else { > + op = ovn_port_find(ls_ports, pb->logical_port); > + if (op && !op->lsp_can_be_inc_processed) { > + return false; > + } > } > > if (sbrec_port_binding_is_new(pb)) { > @@ -5725,7 +5765,8 @@ northd_handle_sb_port_binding_changes( > * pointer in northd data. Fallback to recompute otherwise. */ > if (!op) { > VLOG_WARN_RL(&rl, "A port-binding for %s is created but the " > - "LSP is not found.", pb->logical_port); > + "%s is not found.", pb->logical_port, > + is_router_port ? "LRP" : "LSP"); > return false; > } > op->sb = pb; > @@ -5736,7 +5777,7 @@ northd_handle_sb_port_binding_changes( > * sb idl pointers and other unexpected behavior. */ > if (op) { > VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the " > - "LSP still exists.", pb->logical_port); > + "LSP/LRP still exists.", pb->logical_port); > return false; > } > } else { > @@ -5746,7 +5787,8 @@ northd_handle_sb_port_binding_changes( > * Fallback to recompute for anything unexpected. */ > if (!op) { > VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the " > - "LSP is not found.", pb->logical_port); > + "%s is not found.", pb->logical_port, > + is_router_port ? "LRP" : "LSP"); > return false; > } > if (op->sb != pb) { > @@ -7895,67 +7937,72 @@ static void > copy_ra_to_sb(struct ovn_port *op, const char *address_mode); > > static void > -ovn_update_ipv6_options(struct hmap *lr_ports) > +ovn_update_ipv6_opt_for_op(struct ovn_port *op) > { > - struct ovn_port *op; > - HMAP_FOR_EACH (op, key_node, lr_ports) { > - ovs_assert(op->nbrp); > - > - if (op->nbrp->peer || !op->peer) { > - continue; > - } > + if (op->nbrp->peer || !op->peer) { > + return; > + } > > - if (!op->lrp_networks.n_ipv6_addrs) { > - continue; > - } > + if (!op->lrp_networks.n_ipv6_addrs) { > + return; > + } > > - struct smap options; > - smap_clone(&options, &op->sb->options); > + struct smap options; > + smap_clone(&options, &op->sb->options); > > - /* enable IPv6 prefix delegation */ > - bool prefix_delegation = smap_get_bool(&op->nbrp->options, > - "prefix_delegation", false); > - if (!lrport_is_enabled(op->nbrp)) { > - prefix_delegation = false; > - } > - if (smap_get_bool(&options, "ipv6_prefix_delegation", > - false) != prefix_delegation) { > - smap_add(&options, "ipv6_prefix_delegation", > - prefix_delegation ? "true" : "false"); > - } > + /* enable IPv6 prefix delegation */ > + bool prefix_delegation = smap_get_bool(&op->nbrp->options, > + "prefix_delegation", false); Nit: please indent and align under the first function argument. > + if (!lrport_is_enabled(op->nbrp)) { > + prefix_delegation = false; > + } > + if (smap_get_bool(&options, "ipv6_prefix_delegation", > + false) != prefix_delegation) { > + smap_add(&options, "ipv6_prefix_delegation", > + prefix_delegation ? "true" : "false"); > + } > > - bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > + bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > "prefix", false); > - if (!lrport_is_enabled(op->nbrp)) { > - ipv6_prefix = false; > - } > - if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > - smap_add(&options, "ipv6_prefix", > - ipv6_prefix ? "true" : "false"); > - } > - sbrec_port_binding_set_options(op->sb, &options); > + if (!lrport_is_enabled(op->nbrp)) { > + ipv6_prefix = false; > + } > + if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > + smap_add(&options, "ipv6_prefix", > + ipv6_prefix ? "true" : "false"); Nit: indent > + } > + sbrec_port_binding_set_options(op->sb, &options); > > - smap_destroy(&options); > + smap_destroy(&options); > > - const char *address_mode = smap_get( > - &op->nbrp->ipv6_ra_configs, "address_mode"); > + const char *address_mode = smap_get( > + &op->nbrp->ipv6_ra_configs, "address_mode"); Nit: This can now be written as: const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs, "address_mode"); > > - if (!address_mode) { > - continue; > - } > - if (strcmp(address_mode, "slaac") && > - strcmp(address_mode, "dhcpv6_stateful") && > - strcmp(address_mode, "dhcpv6_stateless")) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > - VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > - address_mode); > - continue; > - } > + if (!address_mode) { > + return; > + } > + if (strcmp(address_mode, "slaac") && > + strcmp(address_mode, "dhcpv6_stateful") && > + strcmp(address_mode, "dhcpv6_stateless")) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > + address_mode); Nit: indent > + return; > + } > > - if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > - false)) { > - copy_ra_to_sb(op, address_mode); > - } > + if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > + false)) { Nit: It all fits on one line now. > + copy_ra_to_sb(op, address_mode); > + } > +} > + > +static void > +ovn_update_ipv6_options(struct hmap *lr_ports) > +{ > + struct ovn_port *op; > + HMAP_FOR_EACH (op, key_node, lr_ports) { > + ovs_assert(op->nbrp); > + ovn_update_ipv6_opt_for_op(op); > } > } > > @@ -18027,8 +18074,6 @@ ovnnb_db_run(struct northd_input *input_data, > &data->lr_ports); > stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > - ovn_update_ipv6_options(&data->lr_ports); > - ovn_update_ipv6_prefix(&data->lr_ports); > > sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, > input_data->sbrec_mirror_table); > @@ -18359,6 +18404,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn, > &ha_ref_chassis_map); > } > shash_destroy(&ha_ref_chassis_map); > + > + ovn_update_ipv6_prefix(lr_ports); > } > > const char * > diff --git a/northd/northd.h b/northd/northd.h > index 0478826f0a..3945e84bb8 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -368,7 +368,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct lflow_input *, > struct hmap *lflows); > bool northd_handle_sb_port_binding_changes( > - const struct sbrec_port_binding_table *, struct hmap *ls_ports); > + const struct sbrec_port_binding_table *, struct hmap *ls_ports, > + struct hmap *lr_ports); > > struct tracked_lb_data; > bool northd_handle_lb_data_changes(struct tracked_lb_data *, > @@ -398,7 +399,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, > struct chassis_features *chassis_features); > bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, > + struct hmap *lr_ports); > bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *); > > bool northd_has_tracked_data(struct northd_tracked_data *); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 699f6cfdce..55a244c8c4 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -11018,7 +11018,9 @@ check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-add lr0 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Adding a logical router port should result in recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > @@ -11026,8 +11028,10 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > # for northd engine there will be both recompute and compute > # first it will be recompute to handle lr0-sw0 and then a compute > # for the SB port binding change. > -check_engine_stats northd recompute nocompute > +check_engine_stats northd recompute compute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > ovn-nbctl lsp-add sw0 sw0-lr0 > ovn-nbctl lsp-set-type sw0-lr0 router > @@ -11035,7 +11039,9 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > ovn-nbctl ls-add public > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > @@ -11057,9 +11063,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > - > -check as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Do checks for NATs. > # Add a NAT. This should not result in recompute of both northd and lflow > @@ -11068,6 +11074,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT options column > @@ -11075,6 +11082,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . options:foo=bar > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT external_ip column > @@ -11082,6 +11090,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120 > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT logical_ip column > @@ -11089,6 +11098,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10 > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Update the NAT type > @@ -11096,13 +11106,15 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT . type=snat > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Create a dnat_and_snat NAT with external_mac and logical_port > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03 > -check_engine_stats northd recompute nocompute > +check_engine_stats northd recompute compute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4) > @@ -11111,6 +11123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"' > check_engine_stats northd recompute nocompute > check_engine_stats lflow recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Create a load balancer and add the lb vip as NAT > @@ -11124,31 +11137,35 @@ check ovn-nbctl lr-lb-add lr0 lb2 > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > # Delete the NAT > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb clear logical_router lr0 nat > -check_engine_stats northd recompute nocompute > +check_engine_stats northd recompute compute > check_engine_stats lflow recompute nocompute > check_engine_stats sync_to_sb_pb recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > @@ -11157,12 +11174,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102 > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" > check_engine_stats northd recompute nocompute > +check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats lflow recompute nocompute > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > OVN_CLEANUP([hv1]) > AT_CLEANUP
On Thu, Nov 23, 2023 at 8:24 AM Dumitru Ceara <dceara@redhat.com> wrote: > On 10/26/23 20:14, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > It also moves the logical router port IPv6 prefix delegation > > updates to "sync-from-sb" engine node. > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > I think I agree in general with the patch; I'd like to see the new > revision however because the check that Han also commented on [0] in > northd_handle_sb_port_binding_changes() makes me wonder too if we're not > missing cases (or if we should just relax the LSP check as well). > > [0] > https://mail.openvswitch.org/pipermail/ovs-dev/2023-November/409451.html > > > Aside from that I left a few minor comments below. > > Thanks, > Dumitru > > > northd/en-northd.c | 2 +- > > northd/en-sync-sb.c | 3 +- > > northd/northd.c | 283 ++++++++++++++++++++++++++------------------ > > northd/northd.h | 6 +- > > tests/ovn-northd.at | 31 ++++- > > 5 files changed, 198 insertions(+), 127 deletions(-) > > > > diff --git a/northd/en-northd.c b/northd/en-northd.c > > index 96c2ce9f69..13e731cad9 100644 > > --- a/northd/en-northd.c > > +++ b/northd/en-northd.c > > @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node > *node, > > northd_get_input_data(node, &input_data); > > > > if (!northd_handle_sb_port_binding_changes( > > - input_data.sbrec_port_binding_table, &nd->ls_ports)) { > > + input_data.sbrec_port_binding_table, &nd->ls_ports, > &nd->lr_ports)) { > > return false; > > } > > > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > > index 2540fcfb97..a14c609acd 100644 > > --- a/northd/en-sync-sb.c > > +++ b/northd/en-sync-sb.c > > @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void > *data OVS_UNUSED) > > const struct engine_context *eng_ctx = engine_get_context(); > > struct northd_data *northd_data = engine_get_input_data("northd", > node); > > > > - sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); > > + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, > > + &northd_data->lr_ports); > > engine_set_node_state(node, EN_UPDATED); > > } > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 9ce1b2cb5a..c9c7045755 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > { > > sbrec_port_binding_set_datapath(op->sb, op->od->sb); > > if (op->nbrp) { > > + /* Note: SB port binding options for router ports are set in > > + * sync_pbs(). */ > > + > > /* If the router is for l3 gateway, it resides on a chassis > > * and its port type is "l3gateway". */ > > const char *chassis_name = smap_get(&op->od->nbr->options, > "chassis"); > > @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > sbrec_port_binding_set_type(op->sb, "patch"); > > } > > > > - struct smap new; > > - smap_init(&new); > > if (is_cr_port(op)) { > > ovs_assert(sbrec_chassis_by_name); > > ovs_assert(sbrec_chassis_by_hostname); > > ovs_assert(sbrec_ha_chassis_grp_by_name); > > ovs_assert(active_ha_chassis_grps); > > - const char *redirect_type = smap_get(&op->nbrp->options, > > - "redirect-type"); > > > > if (op->nbrp->ha_chassis_group) { > > if (op->nbrp->n_gateway_chassis) { > > @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn > *ovnsb_txn, > > /* Delete the legacy gateway_chassis from the pb. */ > > sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0); > > } > > - smap_add(&new, "distributed-port", op->nbrp->name); > > - > > - bool always_redirect = > > - !op->od->has_distributed_nat && > > - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > > - > > - if (redirect_type) { > > - smap_add(&new, "redirect-type", redirect_type); > > - /* XXX Why can't we enable always-redirect when > redirect-type > > - * is bridged? */ > > - if (!strcmp(redirect_type, "bridged")) { > > - always_redirect = false; > > - } > > - } > > - > > - if (always_redirect) { > > - smap_add(&new, "always-redirect", "true"); > > - } > > - } else { > > - if (op->peer) { > > - smap_add(&new, "peer", op->peer->key); > > - if (op->nbrp->ha_chassis_group || > > - op->nbrp->n_gateway_chassis) { > > - char *redirect_name = > > - ovn_chassis_redirect_name(op->nbrp->name); > > - smap_add(&new, "chassis-redirect-port", > redirect_name); > > - free(redirect_name); > > - } > > - } > > - if (chassis_name) { > > - smap_add(&new, "l3gateway-chassis", chassis_name); > > - } > > - } > > - > > - const char *ipv6_pd_list = smap_get(&op->sb->options, > > - "ipv6_ra_pd_list"); > > - if (ipv6_pd_list) { > > - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > > } > > > > - sbrec_port_binding_set_options(op->sb, &new); > > - smap_destroy(&new); > > - > > sbrec_port_binding_set_parent_port(op->sb, NULL); > > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > > > @@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct > sbrec_load_balancer_table *table) > > return duplicates; > > } > > > > -/* Syncs the SB port binding for the ovn_port 'op'. Caller should make > sure > > - * that the OVN SB IDL txn is not NULL. Presently it only syncs the nat > > - * column of port binding corresponding to the 'op->nbsp' */ > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch > port. > > + * Caller should make sure that the OVN SB IDL txn is not NULL. > Presently it > > + * only syncs the nat column of port binding corresponding to the > 'op->nbsp' */ > > static void > > -sync_pb_for_op(struct ovn_port *op) > > +sync_pb_for_lsp(struct ovn_port *op) > > { > > + ovs_assert(op->nbsp); > > + > > if (lsp_is_router(op->nbsp)) { > > const char *chassis = NULL; > > if (op->peer && op->peer->od && op->peer->od->nbr) { > > @@ -4879,18 +4839,87 @@ sync_pb_for_op(struct ovn_port *op) > > } > > } > > > > +/* Syncs the SB port binding for the ovn_port 'op' of a logical router > port. > > + * Caller should make sure that the OVN SB IDL txn is not NULL. > Presently it > > + * only sets the port binding options column for the router ports */ > > +static void > > +sync_pb_for_lrp(struct ovn_port *op) > > +{ > > + ovs_assert(op->nbrp); > > + > > + struct smap new; > > + smap_init(&new); > > + > > + const char *chassis_name = smap_get(&op->od->nbr->options, > "chassis"); > > + if (is_cr_port(op)) { > > + smap_add(&new, "distributed-port", op->nbrp->name); > > + > > + bool always_redirect = > > + !op->od->has_distributed_nat && > > + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); > > + > > + const char *redirect_type = smap_get(&op->nbrp->options, > > + "redirect-type"); > > + if (redirect_type) { > > + smap_add(&new, "redirect-type", redirect_type); > > + /* XXX Why can't we enable always-redirect when > redirect-type > > + * is bridged? */ > > + if (!strcmp(redirect_type, "bridged")) { > > + always_redirect = false; > > + } > > + } > > + > > + if (always_redirect) { > > + smap_add(&new, "always-redirect", "true"); > > + } > > + } else { > > + if (op->peer) { > > + smap_add(&new, "peer", op->peer->key); > > + if (op->nbrp->ha_chassis_group || > > + op->nbrp->n_gateway_chassis) { > > + char *redirect_name = > > + ovn_chassis_redirect_name(op->nbrp->name); > > + smap_add(&new, "chassis-redirect-port", redirect_name); > > + free(redirect_name); > > + } > > + } > > + if (chassis_name) { > > + smap_add(&new, "l3gateway-chassis", chassis_name); > > + } > > + } > > + > > + const char *ipv6_pd_list = smap_get(&op->sb->options, > > + "ipv6_ra_pd_list"); > > Nit: this now fits on a single line. > > > + if (ipv6_pd_list) { > > + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); > > + } > > + > > + sbrec_port_binding_set_options(op->sb, &new); > > + smap_destroy(&new); > > +} > > + > > +static void ovn_update_ipv6_options(struct hmap *lr_ports); > > +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op); > > + > > /* Sync the SB Port bindings which needs to be updated. > > * Presently it syncs the nat column of port bindings corresponding to > > * the logical switch ports. */ > > void > > -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) > > +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, > > + struct hmap *lr_ports) > > { > > ovs_assert(ovnsb_idl_txn); > > > > struct ovn_port *op; > > HMAP_FOR_EACH (op, key_node, ls_ports) { > > - sync_pb_for_op(op); > > + sync_pb_for_lsp(op); > > + } > > + > > + HMAP_FOR_EACH (op, key_node, lr_ports) { > > + sync_pb_for_lrp(op); > > } > > + > > + ovn_update_ipv6_options(lr_ports); > > } > > > > /* Sync the SB Port bindings for the added and updated logical switch > ports > > @@ -4902,12 +4931,22 @@ sync_pbs_for_northd_changed_ovn_ports( struct > tracked_ovn_ports *trk_ovn_ports) > > struct ovn_port *op; > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { > > op = hmapx_node->data; > > - sync_pb_for_op(op); > > + if (op->nbsp) { > > + sync_pb_for_lsp(op); > > + } else { > > + sync_pb_for_lrp(op); > > + ovn_update_ipv6_opt_for_op(op); > > + } > > } > > > > HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { > > op = hmapx_node->data; > > - sync_pb_for_op(op); > > + if (op->nbsp) { > > + sync_pb_for_lsp(op); > > + } else { > > + sync_pb_for_lrp(op); > > + ovn_update_ipv6_opt_for_op(op); > > + } > > } > > > > return true; > > @@ -5703,20 +5742,21 @@ fail: > > bool > > northd_handle_sb_port_binding_changes( > > const struct sbrec_port_binding_table *sbrec_port_binding_table, > > - struct hmap *ls_ports) > > + struct hmap *ls_ports, struct hmap *lr_ports) > > { > > const struct sbrec_port_binding *pb; > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > sbrec_port_binding_table) { > > - /* Return false if the 'pb' belongs to a router port. We don't > handle > > - * I-P for router ports yet. */ > > - if (is_pb_router_type(pb)) { > > - return false; > > - } > > + bool is_router_port = is_pb_router_type(pb); > > + struct ovn_port *op; > > > > - struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port); > > - if (op && !op->lsp_can_be_inc_processed) { > > - return false; > > + if (is_router_port) { > > + op = ovn_port_find(lr_ports, pb->logical_port); > > + } else { > > + op = ovn_port_find(ls_ports, pb->logical_port); > > + if (op && !op->lsp_can_be_inc_processed) { > > + return false; > > + } > > } > > > > if (sbrec_port_binding_is_new(pb)) { > > @@ -5725,7 +5765,8 @@ northd_handle_sb_port_binding_changes( > > * pointer in northd data. Fallback to recompute otherwise. > */ > > if (!op) { > > VLOG_WARN_RL(&rl, "A port-binding for %s is created but > the " > > - "LSP is not found.", pb->logical_port); > > + "%s is not found.", pb->logical_port, > > + is_router_port ? "LRP" : "LSP"); > > return false; > > } > > op->sb = pb; > > @@ -5736,7 +5777,7 @@ northd_handle_sb_port_binding_changes( > > * sb idl pointers and other unexpected behavior. */ > > if (op) { > > VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but > the " > > - "LSP still exists.", pb->logical_port); > > + "LSP/LRP still exists.", pb->logical_port); > > return false; > > } > > } else { > > @@ -5746,7 +5787,8 @@ northd_handle_sb_port_binding_changes( > > * Fallback to recompute for anything unexpected. */ > > if (!op) { > > VLOG_WARN_RL(&rl, "A port-binding for %s is updated but > the " > > - "LSP is not found.", pb->logical_port); > > + "%s is not found.", pb->logical_port, > > + is_router_port ? "LRP" : "LSP"); > > return false; > > } > > if (op->sb != pb) { > > @@ -7895,67 +7937,72 @@ static void > > copy_ra_to_sb(struct ovn_port *op, const char *address_mode); > > > > static void > > -ovn_update_ipv6_options(struct hmap *lr_ports) > > +ovn_update_ipv6_opt_for_op(struct ovn_port *op) > > { > > - struct ovn_port *op; > > - HMAP_FOR_EACH (op, key_node, lr_ports) { > > - ovs_assert(op->nbrp); > > - > > - if (op->nbrp->peer || !op->peer) { > > - continue; > > - } > > + if (op->nbrp->peer || !op->peer) { > > + return; > > + } > > > > - if (!op->lrp_networks.n_ipv6_addrs) { > > - continue; > > - } > > + if (!op->lrp_networks.n_ipv6_addrs) { > > + return; > > + } > > > > - struct smap options; > > - smap_clone(&options, &op->sb->options); > > + struct smap options; > > + smap_clone(&options, &op->sb->options); > > > > - /* enable IPv6 prefix delegation */ > > - bool prefix_delegation = smap_get_bool(&op->nbrp->options, > > - "prefix_delegation", false); > > - if (!lrport_is_enabled(op->nbrp)) { > > - prefix_delegation = false; > > - } > > - if (smap_get_bool(&options, "ipv6_prefix_delegation", > > - false) != prefix_delegation) { > > - smap_add(&options, "ipv6_prefix_delegation", > > - prefix_delegation ? "true" : "false"); > > - } > > + /* enable IPv6 prefix delegation */ > > + bool prefix_delegation = smap_get_bool(&op->nbrp->options, > > + "prefix_delegation", false); > > Nit: please indent and align under the first function argument. > > > + if (!lrport_is_enabled(op->nbrp)) { > > + prefix_delegation = false; > > + } > > + if (smap_get_bool(&options, "ipv6_prefix_delegation", > > + false) != prefix_delegation) { > > + smap_add(&options, "ipv6_prefix_delegation", > > + prefix_delegation ? "true" : "false"); > > + } > > > > - bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > > + bool ipv6_prefix = smap_get_bool(&op->nbrp->options, > > "prefix", false); > > - if (!lrport_is_enabled(op->nbrp)) { > > - ipv6_prefix = false; > > - } > > - if (smap_get_bool(&options, "ipv6_prefix", false) != > ipv6_prefix) { > > - smap_add(&options, "ipv6_prefix", > > - ipv6_prefix ? "true" : "false"); > > - } > > - sbrec_port_binding_set_options(op->sb, &options); > > + if (!lrport_is_enabled(op->nbrp)) { > > + ipv6_prefix = false; > > + } > > + if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { > > + smap_add(&options, "ipv6_prefix", > > + ipv6_prefix ? "true" : "false"); > > Nit: indent > > Ack. Addressed in v3. > + } > > + sbrec_port_binding_set_options(op->sb, &options); > > > > - smap_destroy(&options); > > + smap_destroy(&options); > > > > - const char *address_mode = smap_get( > > - &op->nbrp->ipv6_ra_configs, "address_mode"); > > + const char *address_mode = smap_get( > > + &op->nbrp->ipv6_ra_configs, "address_mode"); > > Nit: This can now be written as: > > const char *address_mode = smap_get(&op->nbrp->ipv6_ra_configs, > "address_mode"); > > Ack. Addressed in v3. > > > - if (!address_mode) { > > - continue; > > - } > > - if (strcmp(address_mode, "slaac") && > > - strcmp(address_mode, "dhcpv6_stateful") && > > - strcmp(address_mode, "dhcpv6_stateless")) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > > - VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > > - address_mode); > > - continue; > > - } > > + if (!address_mode) { > > + return; > > + } > > + if (strcmp(address_mode, "slaac") && > > + strcmp(address_mode, "dhcpv6_stateful") && > > + strcmp(address_mode, "dhcpv6_stateless")) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > + VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", > > + address_mode); > > Nit: indent > > Ack. Addressed in v3. > + return; > > + } > > > > - if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > > - false)) { > > - copy_ra_to_sb(op, address_mode); > > - } > > + if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", > > + false)) { > > Nit: It all fits on one line now. > Ack. Addressed in v3. Numan > > + copy_ra_to_sb(op, address_mode); > > + } > > +} > > + > > +static void > > +ovn_update_ipv6_options(struct hmap *lr_ports) > > +{ > > + struct ovn_port *op; > > + HMAP_FOR_EACH (op, key_node, lr_ports) { > > + ovs_assert(op->nbrp); > > + ovn_update_ipv6_opt_for_op(op); > > } > > } > > > > @@ -18027,8 +18074,6 @@ ovnnb_db_run(struct northd_input *input_data, > > &data->lr_ports); > > stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); > > - ovn_update_ipv6_options(&data->lr_ports); > > - ovn_update_ipv6_prefix(&data->lr_ports); > > > > sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, > > input_data->sbrec_mirror_table); > > @@ -18359,6 +18404,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn, > > &ha_ref_chassis_map); > > } > > shash_destroy(&ha_ref_chassis_map); > > + > > + ovn_update_ipv6_prefix(lr_ports); > > } > > > > const char * > > diff --git a/northd/northd.h b/northd/northd.h > > index 0478826f0a..3945e84bb8 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -368,7 +368,8 @@ bool lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > struct lflow_input *, > > struct hmap *lflows); > > bool northd_handle_sb_port_binding_changes( > > - const struct sbrec_port_binding_table *, struct hmap *ls_ports); > > + const struct sbrec_port_binding_table *, struct hmap *ls_ports, > > + struct hmap *lr_ports); > > > > struct tracked_lb_data; > > bool northd_handle_lb_data_changes(struct tracked_lb_data *, > > @@ -398,7 +399,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct > sbrec_load_balancer_table *, > > struct chassis_features *chassis_features); > > bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); > > > > -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); > > +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, > > + struct hmap *lr_ports); > > bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *); > > > > bool northd_has_tracked_data(struct northd_tracked_data *); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 699f6cfdce..55a244c8c4 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -11018,7 +11018,9 @@ check ovn-nbctl lsp-add sw0 sw0p1 -- > lsp-set-addresses sw0p1 "00:00:20:20:12:01 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-add lr0 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Adding a logical router port should result in recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > @@ -11026,8 +11028,10 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 > 00:00:00:00:ff:01 10.0.0.1/24 > > # for northd engine there will be both recompute and compute > > # first it will be recompute to handle lr0-sw0 and then a compute > > # for the SB port binding change. > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd recompute compute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > ovn-nbctl lsp-add sw0 sw0-lr0 > > ovn-nbctl lsp-set-type sw0-lr0 router > > @@ -11035,7 +11039,9 @@ ovn-nbctl lsp-set-addresses sw0-lr0 > 00:00:00:00:ff:01 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > ovn-nbctl ls-add public > > ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > > @@ -11057,9 +11063,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis > lr0-public hv1 20 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > - > > -check as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Do checks for NATs. > > # Add a NAT. This should not result in recompute of both northd and > lflow > > @@ -11068,6 +11074,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 > 10.0.0.4 > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT options column > > @@ -11075,6 +11082,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . options:foo=bar > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT external_ip column > > @@ -11082,6 +11090,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120 > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT logical_ip column > > @@ -11089,6 +11098,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10 > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Update the NAT type > > @@ -11096,13 +11106,15 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT . type=snat > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Create a dnat_and_snat NAT with external_mac and logical_port > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 > 10.0.0.4 sw0p1 30:54:00:00:00:03 > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd recompute compute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat > logical_ip=10.0.0.4) > > @@ -11111,6 +11123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE > inc-engine/clear-stats > > check ovn-nbctl --wait=sb set NAT $nat2_uuid > external_mac='"30:54:00:00:00:04"' > > check_engine_stats northd recompute nocompute > > check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Create a load balancer and add the lb vip as NAT > > @@ -11124,31 +11137,35 @@ check ovn-nbctl lr-lb-add lr0 lb2 > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 > 10.0.0.20 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 > 10.0.0.41 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > # Delete the NAT > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb clear logical_router lr0 nat > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd recompute compute > > check_engine_stats lflow recompute nocompute > > check_engine_stats sync_to_sb_pb recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > @@ -11157,12 +11174,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" > reroute 172.168.0.101,172.168.0.102 > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" > > check_engine_stats northd recompute nocompute > > +check_engine_stats sync_to_sb_pb recompute nocompute > > check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/northd/en-northd.c b/northd/en-northd.c index 96c2ce9f69..13e731cad9 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node, northd_get_input_data(node, &input_data); if (!northd_handle_sb_port_binding_changes( - input_data.sbrec_port_binding_table, &nd->ls_ports)) { + input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) { return false; } diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 2540fcfb97..a14c609acd 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data OVS_UNUSED) const struct engine_context *eng_ctx = engine_get_context(); struct northd_data *northd_data = engine_get_input_data("northd", node); - sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports); + sync_pbs(eng_ctx->ovnsb_idl_txn, &northd_data->ls_ports, + &northd_data->lr_ports); engine_set_node_state(node, EN_UPDATED); } diff --git a/northd/northd.c b/northd/northd.c index 9ce1b2cb5a..c9c7045755 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, { sbrec_port_binding_set_datapath(op->sb, op->od->sb); if (op->nbrp) { + /* Note: SB port binding options for router ports are set in + * sync_pbs(). */ + /* If the router is for l3 gateway, it resides on a chassis * and its port type is "l3gateway". */ const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); @@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, sbrec_port_binding_set_type(op->sb, "patch"); } - struct smap new; - smap_init(&new); if (is_cr_port(op)) { ovs_assert(sbrec_chassis_by_name); ovs_assert(sbrec_chassis_by_hostname); ovs_assert(sbrec_ha_chassis_grp_by_name); ovs_assert(active_ha_chassis_grps); - const char *redirect_type = smap_get(&op->nbrp->options, - "redirect-type"); if (op->nbrp->ha_chassis_group) { if (op->nbrp->n_gateway_chassis) { @@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, /* Delete the legacy gateway_chassis from the pb. */ sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0); } - smap_add(&new, "distributed-port", op->nbrp->name); - - bool always_redirect = - !op->od->has_distributed_nat && - !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); - - if (redirect_type) { - smap_add(&new, "redirect-type", redirect_type); - /* XXX Why can't we enable always-redirect when redirect-type - * is bridged? */ - if (!strcmp(redirect_type, "bridged")) { - always_redirect = false; - } - } - - if (always_redirect) { - smap_add(&new, "always-redirect", "true"); - } - } else { - if (op->peer) { - smap_add(&new, "peer", op->peer->key); - if (op->nbrp->ha_chassis_group || - op->nbrp->n_gateway_chassis) { - char *redirect_name = - ovn_chassis_redirect_name(op->nbrp->name); - smap_add(&new, "chassis-redirect-port", redirect_name); - free(redirect_name); - } - } - if (chassis_name) { - smap_add(&new, "l3gateway-chassis", chassis_name); - } - } - - const char *ipv6_pd_list = smap_get(&op->sb->options, - "ipv6_ra_pd_list"); - if (ipv6_pd_list) { - smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); } - sbrec_port_binding_set_options(op->sb, &new); - smap_destroy(&new); - sbrec_port_binding_set_parent_port(op->sb, NULL); sbrec_port_binding_set_tag(op->sb, NULL, 0); @@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct sbrec_load_balancer_table *table) return duplicates; } -/* Syncs the SB port binding for the ovn_port 'op'. Caller should make sure - * that the OVN SB IDL txn is not NULL. Presently it only syncs the nat - * column of port binding corresponding to the 'op->nbsp' */ +/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port. + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it + * only syncs the nat column of port binding corresponding to the 'op->nbsp' */ static void -sync_pb_for_op(struct ovn_port *op) +sync_pb_for_lsp(struct ovn_port *op) { + ovs_assert(op->nbsp); + if (lsp_is_router(op->nbsp)) { const char *chassis = NULL; if (op->peer && op->peer->od && op->peer->od->nbr) { @@ -4879,18 +4839,87 @@ sync_pb_for_op(struct ovn_port *op) } } +/* Syncs the SB port binding for the ovn_port 'op' of a logical router port. + * Caller should make sure that the OVN SB IDL txn is not NULL. Presently it + * only sets the port binding options column for the router ports */ +static void +sync_pb_for_lrp(struct ovn_port *op) +{ + ovs_assert(op->nbrp); + + struct smap new; + smap_init(&new); + + const char *chassis_name = smap_get(&op->od->nbr->options, "chassis"); + if (is_cr_port(op)) { + smap_add(&new, "distributed-port", op->nbrp->name); + + bool always_redirect = + !op->od->has_distributed_nat && + !l3dgw_port_has_associated_vtep_lports(op->l3dgw_port); + + const char *redirect_type = smap_get(&op->nbrp->options, + "redirect-type"); + if (redirect_type) { + smap_add(&new, "redirect-type", redirect_type); + /* XXX Why can't we enable always-redirect when redirect-type + * is bridged? */ + if (!strcmp(redirect_type, "bridged")) { + always_redirect = false; + } + } + + if (always_redirect) { + smap_add(&new, "always-redirect", "true"); + } + } else { + if (op->peer) { + smap_add(&new, "peer", op->peer->key); + if (op->nbrp->ha_chassis_group || + op->nbrp->n_gateway_chassis) { + char *redirect_name = + ovn_chassis_redirect_name(op->nbrp->name); + smap_add(&new, "chassis-redirect-port", redirect_name); + free(redirect_name); + } + } + if (chassis_name) { + smap_add(&new, "l3gateway-chassis", chassis_name); + } + } + + const char *ipv6_pd_list = smap_get(&op->sb->options, + "ipv6_ra_pd_list"); + if (ipv6_pd_list) { + smap_add(&new, "ipv6_ra_pd_list", ipv6_pd_list); + } + + sbrec_port_binding_set_options(op->sb, &new); + smap_destroy(&new); +} + +static void ovn_update_ipv6_options(struct hmap *lr_ports); +static void ovn_update_ipv6_opt_for_op(struct ovn_port *op); + /* Sync the SB Port bindings which needs to be updated. * Presently it syncs the nat column of port bindings corresponding to * the logical switch ports. */ void -sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports) +sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *ls_ports, + struct hmap *lr_ports) { ovs_assert(ovnsb_idl_txn); struct ovn_port *op; HMAP_FOR_EACH (op, key_node, ls_ports) { - sync_pb_for_op(op); + sync_pb_for_lsp(op); + } + + HMAP_FOR_EACH (op, key_node, lr_ports) { + sync_pb_for_lrp(op); } + + ovn_update_ipv6_options(lr_ports); } /* Sync the SB Port bindings for the added and updated logical switch ports @@ -4902,12 +4931,22 @@ sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports) struct ovn_port *op; HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) { op = hmapx_node->data; - sync_pb_for_op(op); + if (op->nbsp) { + sync_pb_for_lsp(op); + } else { + sync_pb_for_lrp(op); + ovn_update_ipv6_opt_for_op(op); + } } HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->updated) { op = hmapx_node->data; - sync_pb_for_op(op); + if (op->nbsp) { + sync_pb_for_lsp(op); + } else { + sync_pb_for_lrp(op); + ovn_update_ipv6_opt_for_op(op); + } } return true; @@ -5703,20 +5742,21 @@ fail: bool northd_handle_sb_port_binding_changes( const struct sbrec_port_binding_table *sbrec_port_binding_table, - struct hmap *ls_ports) + struct hmap *ls_ports, struct hmap *lr_ports) { const struct sbrec_port_binding *pb; static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table) { - /* Return false if the 'pb' belongs to a router port. We don't handle - * I-P for router ports yet. */ - if (is_pb_router_type(pb)) { - return false; - } + bool is_router_port = is_pb_router_type(pb); + struct ovn_port *op; - struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port); - if (op && !op->lsp_can_be_inc_processed) { - return false; + if (is_router_port) { + op = ovn_port_find(lr_ports, pb->logical_port); + } else { + op = ovn_port_find(ls_ports, pb->logical_port); + if (op && !op->lsp_can_be_inc_processed) { + return false; + } } if (sbrec_port_binding_is_new(pb)) { @@ -5725,7 +5765,8 @@ northd_handle_sb_port_binding_changes( * pointer in northd data. Fallback to recompute otherwise. */ if (!op) { VLOG_WARN_RL(&rl, "A port-binding for %s is created but the " - "LSP is not found.", pb->logical_port); + "%s is not found.", pb->logical_port, + is_router_port ? "LRP" : "LSP"); return false; } op->sb = pb; @@ -5736,7 +5777,7 @@ northd_handle_sb_port_binding_changes( * sb idl pointers and other unexpected behavior. */ if (op) { VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the " - "LSP still exists.", pb->logical_port); + "LSP/LRP still exists.", pb->logical_port); return false; } } else { @@ -5746,7 +5787,8 @@ northd_handle_sb_port_binding_changes( * Fallback to recompute for anything unexpected. */ if (!op) { VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the " - "LSP is not found.", pb->logical_port); + "%s is not found.", pb->logical_port, + is_router_port ? "LRP" : "LSP"); return false; } if (op->sb != pb) { @@ -7895,67 +7937,72 @@ static void copy_ra_to_sb(struct ovn_port *op, const char *address_mode); static void -ovn_update_ipv6_options(struct hmap *lr_ports) +ovn_update_ipv6_opt_for_op(struct ovn_port *op) { - struct ovn_port *op; - HMAP_FOR_EACH (op, key_node, lr_ports) { - ovs_assert(op->nbrp); - - if (op->nbrp->peer || !op->peer) { - continue; - } + if (op->nbrp->peer || !op->peer) { + return; + } - if (!op->lrp_networks.n_ipv6_addrs) { - continue; - } + if (!op->lrp_networks.n_ipv6_addrs) { + return; + } - struct smap options; - smap_clone(&options, &op->sb->options); + struct smap options; + smap_clone(&options, &op->sb->options); - /* enable IPv6 prefix delegation */ - bool prefix_delegation = smap_get_bool(&op->nbrp->options, - "prefix_delegation", false); - if (!lrport_is_enabled(op->nbrp)) { - prefix_delegation = false; - } - if (smap_get_bool(&options, "ipv6_prefix_delegation", - false) != prefix_delegation) { - smap_add(&options, "ipv6_prefix_delegation", - prefix_delegation ? "true" : "false"); - } + /* enable IPv6 prefix delegation */ + bool prefix_delegation = smap_get_bool(&op->nbrp->options, + "prefix_delegation", false); + if (!lrport_is_enabled(op->nbrp)) { + prefix_delegation = false; + } + if (smap_get_bool(&options, "ipv6_prefix_delegation", + false) != prefix_delegation) { + smap_add(&options, "ipv6_prefix_delegation", + prefix_delegation ? "true" : "false"); + } - bool ipv6_prefix = smap_get_bool(&op->nbrp->options, + bool ipv6_prefix = smap_get_bool(&op->nbrp->options, "prefix", false); - if (!lrport_is_enabled(op->nbrp)) { - ipv6_prefix = false; - } - if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { - smap_add(&options, "ipv6_prefix", - ipv6_prefix ? "true" : "false"); - } - sbrec_port_binding_set_options(op->sb, &options); + if (!lrport_is_enabled(op->nbrp)) { + ipv6_prefix = false; + } + if (smap_get_bool(&options, "ipv6_prefix", false) != ipv6_prefix) { + smap_add(&options, "ipv6_prefix", + ipv6_prefix ? "true" : "false"); + } + sbrec_port_binding_set_options(op->sb, &options); - smap_destroy(&options); + smap_destroy(&options); - const char *address_mode = smap_get( - &op->nbrp->ipv6_ra_configs, "address_mode"); + const char *address_mode = smap_get( + &op->nbrp->ipv6_ra_configs, "address_mode"); - if (!address_mode) { - continue; - } - if (strcmp(address_mode, "slaac") && - strcmp(address_mode, "dhcpv6_stateful") && - strcmp(address_mode, "dhcpv6_stateless")) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", - address_mode); - continue; - } + if (!address_mode) { + return; + } + if (strcmp(address_mode, "slaac") && + strcmp(address_mode, "dhcpv6_stateful") && + strcmp(address_mode, "dhcpv6_stateless")) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_WARN_RL(&rl, "Invalid address mode [%s] defined", + address_mode); + return; + } - if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", - false)) { - copy_ra_to_sb(op, address_mode); - } + if (smap_get_bool(&op->nbrp->ipv6_ra_configs, "send_periodic", + false)) { + copy_ra_to_sb(op, address_mode); + } +} + +static void +ovn_update_ipv6_options(struct hmap *lr_ports) +{ + struct ovn_port *op; + HMAP_FOR_EACH (op, key_node, lr_ports) { + ovs_assert(op->nbrp); + ovn_update_ipv6_opt_for_op(op); } } @@ -18027,8 +18074,6 @@ ovnnb_db_run(struct northd_input *input_data, &data->lr_ports); stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); - ovn_update_ipv6_options(&data->lr_ports); - ovn_update_ipv6_prefix(&data->lr_ports); sync_mirrors(ovnsb_txn, input_data->nbrec_mirror_table, input_data->sbrec_mirror_table); @@ -18359,6 +18404,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn, &ha_ref_chassis_map); } shash_destroy(&ha_ref_chassis_map); + + ovn_update_ipv6_prefix(lr_ports); } const char * diff --git a/northd/northd.h b/northd/northd.h index 0478826f0a..3945e84bb8 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -368,7 +368,8 @@ bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, struct lflow_input *, struct hmap *lflows); bool northd_handle_sb_port_binding_changes( - const struct sbrec_port_binding_table *, struct hmap *ls_ports); + const struct sbrec_port_binding_table *, struct hmap *ls_ports, + struct hmap *lr_ports); struct tracked_lb_data; bool northd_handle_lb_data_changes(struct tracked_lb_data *, @@ -398,7 +399,8 @@ void sync_lbs(struct ovsdb_idl_txn *, const struct sbrec_load_balancer_table *, struct chassis_features *chassis_features); bool check_sb_lb_duplicates(const struct sbrec_load_balancer_table *); -void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports); +void sync_pbs(struct ovsdb_idl_txn *, struct hmap *ls_ports, + struct hmap *lr_ports); bool sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *); bool northd_has_tracked_data(struct northd_tracked_data *); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 699f6cfdce..55a244c8c4 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -11018,7 +11018,9 @@ check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20:20:12:01 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-add lr0 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE # Adding a logical router port should result in recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats @@ -11026,8 +11028,10 @@ check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 # for northd engine there will be both recompute and compute # first it will be recompute to handle lr0-sw0 and then a compute # for the SB port binding change. -check_engine_stats northd recompute nocompute +check_engine_stats northd recompute compute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE ovn-nbctl lsp-add sw0 sw0-lr0 ovn-nbctl lsp-set-type sw0-lr0 router @@ -11035,7 +11039,9 @@ ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE ovn-nbctl ls-add public ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 @@ -11057,9 +11063,9 @@ ovn-nbctl --wait=hv lrp-set-gateway-chassis lr0-public hv1 20 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl set logical_router_port lr0-sw0 options:foo=bar check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute - -check as northd ovn-appctl -t NORTHD_TYPE vlog/set dbg +CHECK_NO_CHANGE_AFTER_RECOMPUTE # Do checks for NATs. # Add a NAT. This should not result in recompute of both northd and lflow @@ -11068,6 +11074,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Update the NAT options column @@ -11075,6 +11082,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set NAT . options:foo=bar check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Update the NAT external_ip column @@ -11082,6 +11090,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set NAT . external_ip=172.168.0.120 check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Update the NAT logical_ip column @@ -11089,6 +11098,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set NAT . logical_ip=10.0.0.10 check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Update the NAT type @@ -11096,13 +11106,15 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set NAT . type=snat check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Create a dnat_and_snat NAT with external_mac and logical_port check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.110 10.0.0.4 sw0p1 30:54:00:00:00:03 -check_engine_stats northd recompute nocompute +check_engine_stats northd recompute compute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE nat2_uuid=$(ovn-nbctl --bare --columns _uuid find nat logical_ip=10.0.0.4) @@ -11111,6 +11123,7 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set NAT $nat2_uuid external_mac='"30:54:00:00:00:04"' check_engine_stats northd recompute nocompute check_engine_stats lflow recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Create a load balancer and add the lb vip as NAT @@ -11124,31 +11137,35 @@ check ovn-nbctl lr-lb-add lr0 lb2 check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.140 10.0.0.20 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-nat-add lr0 dnat_and_snat 172.168.0.150 10.0.0.41 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.150 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-nat-del lr0 dnat_and_snat 172.168.0.140 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE # Delete the NAT check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb clear logical_router lr0 nat -check_engine_stats northd recompute nocompute +check_engine_stats northd recompute compute check_engine_stats lflow recompute nocompute check_engine_stats sync_to_sb_pb recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE @@ -11157,12 +11174,16 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-policy-add lr0 10 "ip4.src == 10.0.0.3" reroute 172.168.0.101,172.168.0.102 check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb lr-policy-del lr0 10 "ip4.src == 10.0.0.3" check_engine_stats northd recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE OVN_CLEANUP([hv1]) AT_CLEANUP