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?
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
