On Jan 17, Dumitru Ceara wrote:
> On 1/17/25 3:06 PM, Ales Musil wrote:
> > 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?
> >
>
> Sounds reasonable to me. Lorenzo, would you have time to prepare a new
> revision?
ack, thanks for the review. I will post a v3.
Regards,
Lorenzo
>
> Thanks,
> Dumitru
>
> >
> >>> }
> >>> }
> >>>
> >>> 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
