On Thu, Jan 16, 2025 at 6:49 PM Lorenzo Bianconi < [email protected]> wrote:
> > Allow the l3gateway and patch port to be peers for connected LRPs, > > previously it was only allowed to connect two ports of the same > > type. This allows the CMS to connect any router with GW router > > which is required for the concept of transit routers. > > Hi Ales, > > this patch is fine, just a couple of nits inline. > Hi Lorenzo, thank you for the review, see my reply inline. > Regards, > Lorenzo > > > > > Signed-off-by: Ales Musil <[email protected]> > > --- > > controller/physical.c | 32 ++++++++++---- > > tests/ovn-controller.at | 92 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 116 insertions(+), 8 deletions(-) > > > > diff --git a/controller/physical.c b/controller/physical.c > > index 1a3e7e20b..adf37600d 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -1067,7 +1067,8 @@ load_logical_ingress_metadata(const struct > sbrec_port_binding *binding, > > > > static const struct sbrec_port_binding * > > get_binding_peer(struct ovsdb_idl_index *sbrec_port_binding_by_name, > > - const struct sbrec_port_binding *binding) > > + const struct sbrec_port_binding *binding, > > + const enum en_lport_type type) > > { > > const char *peer_name = smap_get(&binding->options, "peer"); > > if (!peer_name) { > > @@ -1076,9 +1077,17 @@ get_binding_peer(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > > > const struct sbrec_port_binding *peer = lport_lookup_by_name( > > sbrec_port_binding_by_name, peer_name); > > - if (!peer || strcmp(peer->type, binding->type)) { > > + if (!peer) { > > return NULL; > > } > > + > > + enum en_lport_type peer_type = get_lport_type(peer); > > + if (type != peer_type && > > + !((type == LP_L3GATEWAY && peer_type == LP_PATCH) || > > + (type == LP_PATCH && peer_type == LP_L3GATEWAY))) { > > This is quite hard to read, what about moving it in a dedicated routine? > I agree it's a bit hard to decipher, I'll add a dedicated function in v2. > > > + return NULL; > > + } > > + > > const char *peer_peer_name = smap_get(&peer->options, "peer"); > > if (!peer_peer_name || strcmp(peer_peer_name, > binding->logical_port)) { > > return NULL; > > @@ -1087,6 +1096,15 @@ get_binding_peer(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > return peer; > > } > > > > +static bool > > +physical_should_eval_peer_port(const struct sbrec_port_binding *binding, > > + const struct sbrec_chassis *chassis, > > + const enum en_lport_type type) > > +{ > > + return type == LP_PATCH || > > + (type == LP_L3GATEWAY && binding->chassis == chassis); > > +} > > + > > enum access_type { > > PORT_LOCAL = 0, > > PORT_LOCALNET, > > @@ -1506,11 +1524,9 @@ consider_port_binding(const struct physical_ctx > *ctx, > > } > > > > struct match match; > > - if (type == LP_PATCH || > > - (type == LP_L3GATEWAY && binding->chassis == ctx->chassis)) { > > - > > + if (physical_should_eval_peer_port(binding, ctx->chassis, type)) { > > const struct sbrec_port_binding *peer = get_binding_peer( > > - ctx->sbrec_port_binding_by_name, binding); > > + ctx->sbrec_port_binding_by_name, binding, type); > > if (!peer) { > > return; > > } > > @@ -2373,9 +2389,9 @@ physical_handle_flows_for_lport(const struct > sbrec_port_binding *pb, > > > > if (!removed) { > > physical_eval_port_binding(p_ctx, pb, type, flow_table); > > - if (type == LP_PATCH) { > > + if (physical_should_eval_peer_port(pb, p_ctx->chassis, type)) { > > This is not actually the same condition, right? Can we just keep the > original > condition? > This condition actually needs to be adjusted otherwise the handler wouldn't process it incrementally. > > Regards, > Lorenzo > > > const struct sbrec_port_binding *peer = > > - get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb); > > + get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb, > type); > > if (peer) { > > physical_eval_port_binding(p_ctx, peer, > get_lport_type(peer), > > flow_table); > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > index fb5923d48..b2bb6e2d0 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -3444,3 +3444,95 @@ OVS_WAIT_FOR_OUTPUT([ovs-vsctl show | tail -n > +2], [], [dnl > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > AT_CLEANUP > > + > > +AT_SETUP([ovn-controller - LR peer ports combination]) > > +AT_KEYWORDS([ovn]) > > +ovn_start > > + > > +net_add n1 > > +sim_add hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.20 > > + > > +check ovn-nbctl ls-add ls0 > > +check ovn-nbctl ls-add ls1 > > + > > +check ovn-nbctl lsp-add ls0 vif0 > > +check ovn-nbctl lsp-add ls0 vif1 > > + > > +check ovn-nbctl lr-add lr0 > > +check ovn-nbctl lr-add lr1 > > + > > +check ovn-nbctl lrp-add lr0 lr0-ls0 00:00:00:00:20:00 192.168.20.1/24 > > +check ovn-nbctl lsp-add ls0 ls0-lr0 \ > > + -- lsp-set-type ls0-lr0 router \ > > + -- lsp-set-addresses ls0-lr0 router \ > > + -- lsp-set-options ls0-lr0 router-port=lr0-ls0 > > + > > +check ovn-nbctl lrp-add lr0 lr1-ls1 00:00:00:00:30:00 192.168.30.1/24 > > +check ovn-nbctl lsp-add ls0 ls1-lr1 \ > > + -- lsp-set-type ls1-lr1 router \ > > + -- lsp-set-addresses ls1-lr1 router \ > > + -- lsp-set-options ls1-lr1 router-port=lr1-ls1 > > + > > +check ovs-vsctl -- add-port br-int vif0 \ > > + -- set Interface vif0 external-ids:iface-id=vif0 > > + > > +check ovs-vsctl -- add-port br-int vif1 \ > > + -- set Interface vif1 external-ids:iface-id=vif1 > > + > > +check ovn-nbctl lrp-add lr0 lr0-lr1 00:00:00:00:30:01 192.168.30.1/31 > peer=lr1-lr0 > > +check ovn-nbctl lrp-add lr1 lr1-lr0 00:00:00:00:30:02 192.168.30.2/31 > peer=lr0-lr1 > > + > > +wait_for_ports_up > > +check ovn-nbctl --wait=hv sync > > + > > +pb_cookie() { > > + name=$1 > > + fetch_column port_binding _uuid logical_port=$name |\ > > + cut -d '-' -f 1 | tr -d '\n' | sed 's/^0\{0,8\}//' > > +} > > + > > +lr0_peer_cookie="0x$(pb_cookie lr0-lr1)" > > +lr1_peer_cookie="0x$(pb_cookie lr1-lr0)" > > + > > +# Patch to patch > > +check_row_count Port_Binding 1 logical_port="lr0-lr1" type=patch > > +check_row_count Port_Binding 1 logical_port="lr1-lr0" type=patch > > +ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY > log_to_phy_flows > > +AT_CHECK([grep -c "cookie=$lr0_peer_cookie," log_to_phy_flows], [0], > [dnl > > +1 > > +]) > > +AT_CHECK([grep -c "cookie=$lr1_peer_cookie," log_to_phy_flows], [0], > [dnl > > +1 > > +]) > > + > > +# L3 gateway to patch > > +check ovn-nbctl --wait=hv set Logical_Router lr0 options:chassis=hv1 > > + > > +check_row_count Port_Binding 1 logical_port="lr0-lr1" type=l3gateway > > +check_row_count Port_Binding 1 logical_port="lr1-lr0" type=patch > > +ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY > log_to_phy_flows > > +AT_CHECK([grep -c "cookie=$lr0_peer_cookie," log_to_phy_flows], [0], > [dnl > > +1 > > +]) > > +AT_CHECK([grep -c "cookie=$lr1_peer_cookie," log_to_phy_flows], [0], > [dnl > > +1 > > +]) > > + > > +# Patch to L3 gateway > > +check ovn-nbctl remove Logical_Router lr0 options chassis > > +check ovn-nbctl --wait=hv set Logical_Router lr1 options:chassis=hv1 > > + > > +check_row_count Port_Binding 1 logical_port="lr0-lr1" type=patch > > +check_row_count Port_Binding 1 logical_port="lr1-lr0" type=l3gateway > > +ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY > log_to_phy_flows > > +AT_CHECK([grep -c "cookie=$lr0_peer_cookie," log_to_phy_flows], [0], > [dnl > > +1 > > +]) > > +AT_CHECK([grep -c "cookie=$lr1_peer_cookie," log_to_phy_flows], [0], > [dnl > > +1 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > -- > > 2.47.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
