On Thu, Jan 23, 2025 at 11:39 AM Lorenzo Bianconi
<lorenzo.bianc...@redhat.com> 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.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
>
> Hi Ales,
>
> the patch looks fine to me, just a question inline.
>
> Regards,
> Lorenzo
>
> > ---
> > v2: Rebase on top of current main.
> > Add function to compare the types.
> > ---
> > controller/physical.c | 49 ++++++++++++++++++----
> > tests/ovn-controller.at | 92 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 133 insertions(+), 8 deletions(-)
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 7fbdb424e..e089e3740 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -1066,9 +1066,28 @@ load_logical_ingress_metadata(const struct
> > sbrec_port_binding *binding,
> > }
> > }
> >
> > +static bool
> > +pbs_are_peers(const enum en_lport_type type_a,
> > + const enum en_lport_type type_b)
> > +{
> > + /* Allow PBs of the same type to be peers. */
> > + if (type_a == type_b) {
> > + return true;
> > + }
> > +
> > + /* Allow L3GW port to be peered with patch port. */
> > + if ((type_a == LP_L3GATEWAY && type_b == LP_PATCH) ||
> > + (type_a == LP_PATCH && type_b == LP_L3GATEWAY)) {
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > 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) {
> > @@ -1077,9 +1096,15 @@ 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 (!pbs_are_peers(type, peer_type)) {
> > + 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;
> > @@ -1088,6 +1113,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,
> > @@ -1507,11 +1541,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;
> > }
> > @@ -2379,8 +2411,9 @@ physical_handle_flows_for_lport(const struct
> > sbrec_port_binding *pb,
> > physical_eval_port_binding(p_ctx, pb, type, flow_table);
> > }
> >
> > - const struct sbrec_port_binding *peer = type == LP_PATCH
> > - ? get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb)
> > + const struct sbrec_port_binding *peer = type ==
> > + physical_should_eval_peer_port(pb, p_ctx->chassis, type)
>
> now we are checking type with a boolean value returned from
> physical_should_eval_peer_port(). Is it correct?
Good catch. It should be
const struct sbrec_port_binding *peer =
physical_should_eval_peer_port(pb, p_ctx->chassis, type)
? get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb, type)
: NULL;
With this addressed:
Acked-by: Numan Siddique <num...@ovn.org>
Numan
>
> > + ? get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb, type)
> > : NULL;
> > if (peer) {
> > ofctrl_remove_flows(flow_table, &peer->header_.uuid);
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index 988de7bb3..27bef27e3 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -3504,3 +3504,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.1
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
