On Fri, Jan 17, 2025 at 2:49 PM Dumitru Ceara <[email protected]> wrote:
> On 1/13/25 1:11 PM, Lorenzo Bianconi wrote:
> > Similar to commit 1431276926 ("controller: fix ovn patch port
> > incremental processing"), update peer logical flows when the related
> > patch port is removed.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-947
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
>
> Hi Lorenzo,
>
> > Changes in v2:
> > - use OFTABLE_LOG_TO_PHY for table 65
> > ---
> > controller/physical.c | 14 ++++++++-----
> > tests/ovn.at | 47 +++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 56 insertions(+), 5 deletions(-)
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index c56c73c20..affa559d5 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -2413,12 +2413,16 @@ physical_handle_flows_for_lport(const struct
> sbrec_port_binding *pb,
> >
> > if (!removed) {
> > physical_eval_port_binding(p_ctx, pb, flow_table);
> > - if (!strcmp(pb->type, "patch")) {
> > - const struct sbrec_port_binding *peer =
> > - get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb);
> > - if (peer) {
> > - physical_eval_port_binding(p_ctx, peer, flow_table);
> > + }
> > +
> > + if (!strcmp(pb->type, "patch")) {
> > + const struct sbrec_port_binding *peer =
> > + get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb);
> > + if (peer) {
> > + if (removed) {
> > + ofctrl_remove_flows(flow_table, &peer->header_.uuid);
> > }
> > + physical_eval_port_binding(p_ctx, peer, flow_table);
>
> I'm not sure if this works correctly. If "!removed", e.g., when the LSP
> patch port is updated, we won't remove the flows. Won't that cause
> stale flows?
>
> I'm not sure of what the right way to fix this is. For example, if a
> pb.type changes from "patch" to something else we won't be removing the
> flows here. Because pb->type is already different.
>
> CC-ing Ales, Numan in case they have ideas.
>
> Regards,
> Dumitru
>
Hi Dumitru and Lorenzo,
As discussed offline we should probably fall back to full recompute
once we detect type change. The type shouldn't change that often so
it shouldn't be any performance hit. As for the part when the type
stays the same I think we need to always remove flows for peer and
install them again, does that make sense?
> > }
> > }
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index de01a649f..3aad19a19 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -40752,3 +40752,50 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
> table=OFTABLE_PHY_TO_LOG | grep -v
> > OVN_CLEANUP([hv1],[hv2])
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([requested-tnl-key-recompute])
> > +AT_KEYWORDS([requested-tnl-key-recompute])
> > +
> > +m4_define([OFTABLE_LOG_TO_PHY], [65])
> > +
> > +ovn_start
> > +net_add n1
> > +
> > +check ovn-nbctl ls-add ls
> > +check ovn-nbctl lsp-add ls lsp -- lsp-set-addresses lsp
> "00:00:10:01:02:01 10.0.0.1"
> > +
> > +check ovn-nbctl lr-add lr
> > +check ovn-nbctl set logical_router lr
> options:mac_binding_age_threshold=3600
> > +check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:ff:01 10.0.0.254/24
> > +check ovn-nbctl lsp-add ls ls-lr
> > +check ovn-nbctl lsp-set-type ls-lr router
> > +check ovn-nbctl lsp-set-addresses ls-lr router
> > +check ovn-nbctl lsp-set-options ls-lr router-port=lr-ls
> > +
> > +sim_add hv1
> > +
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys
> > +ovs-vsctl add-port br-int vif -- set Interface vif
> external-ids:iface-id=lsp
> > +
> > +check ovn-nbctl --wait=hv sync
> > +wait_for_ports_up
> > +
> > +check ovn-nbctl --wait=hv set logical_router_port lr-ls
> options:requested-tnl-key=42
> > +ls_tnl_key=$(fetch_column datapath_binding tunnel_key
> external_ids:name=ls)
> > +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY | grep
> metadata=0x${ls_tnl_key} | grep -q load:0x2a->NXM_NX_REG14])
> > +
> > +check ovn-nbctl lrp-del lr-ls
> > +check ovn-nbctl \
> > + -- lrp-add lr lr-ls 00:00:00:00:10:00 192.168.10.1/24 \
> > + -- set logical_router_port lr-ls options:requested-tnl-key=43
> > +check ovn-nbctl --wait=hv sync
> > +
> > +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY | grep
> metadata=0x${ls_tnl_key} | grep -q load:0x2b->NXM_NX_REG14])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
