On 12/13/24 10:49 AM, Enrique Llorente Pastora wrote: > On Thu, Dec 12, 2024 at 10:16 PM Vladislav Odintsov <[email protected]> > wrote: >> >> Hi Enrique, Dumitru, >> >> On 12.12.2024 15:24, Dumitru Ceara wrote: >>> On 12/12/24 12:59 PM, Enrique Llorente wrote: >>>> For scenarios where a logical switch has empty type LSP and localnet >>>> type LSP, disabling the normal LSP should end up steering traffic to the >>>> localnet LSP. This change do that by removing the l2 lookup table entry >>>> for this LSP, previously when disabled the entry was changing from: >>>> >>>> match=(eth.dst == 00:00:00:00:00:01), action=(outport = "sw0p1"; output;) >>>> >>>> and after enabled=false: >>>> >>>> match=(eth.dst == 00:00:00:00:00:01), action=(drop;) >>>> >>>> So it was still matching and traffic will be drop instead of being >>>> steering to localnet port. >>>> >>>> Reported-at: https://issues.redhat.com/browse/FDP-1033 >>>> Signed-off-by: Enrique Llorente <[email protected]> >>>> --- >>> Hi Enrique, >>> >>> We're essentially reverting 91934ad7c3a6 ("northd: drop traffic to >>> disabled LSPs in ingress pipeline") [0]. >>> >>> Vladislav, what are your thoughts on this? >> Agree. This looks like a slight regression: we do want to drop traffic >> destined to disabled port as soon as possible, I mean on the source node. > > We found that we may not need this after all, removing LSP Addresses > at the same time we set Enabled = false also > removes the problematic flows. >
Thanks for the follow up! I marked the patch as "Rejected" in patchwork. Please do let us know if you find you do need a different code change in the end. Regards, Dumitru >>> >>> I guess the special case here is when we have a disabled LSP that has >>> the same mac address as an enabled one. Should we make the drop >>> conditional, only if there's no other LSP (disabled or enabled) with the >>> same mac configured? >> >> It was an opening for me, that it is possible to create more than one >> logical switch port with the same LS with same MAC. ;) >> >> IIUC, the wanted behavior is to pass traffic to ports, which should >> handle "unknown" addresses. So yes, your approach seems covering both >> cases and I'd kindly ask not to break existing behavior. >> >>> >>> Regards, >>> Dumitru >>> >>> [0] https://github.com/ovn-org/ovn/commit/91934ad7c3a6 >>> >>>> northd/northd.c | 12 +++++++----- >>>> tests/ovn-northd.at | 4 ---- >>>> 2 files changed, 7 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index 3a488ff3d..1ec7442a3 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -10283,14 +10283,16 @@ build_lswitch_ip_unicast_lookup(struct ovn_port >>>> *op, >>>> return; >>>> } >>>> >>>> + /* Skip adding the unicast lookup flows the LSP is explicitly >>>> disabled */ >>>> + if (!lsp_is_enabled(op->nbsp)) { >>>> + return; >>>> + } >>>> + >>>> bool lsp_clone_to_unknown = lsp_is_clone_to_unknown(op->nbsp); >>>> - bool lsp_enabled = lsp_is_enabled(op->nbsp); >>>> - const char *action = lsp_enabled >>>> - ? ((lsp_clone_to_unknown && op->od->has_unknown) >>>> + const char *action = lsp_clone_to_unknown && op->od->has_unknown >>>> ? "clone {outport = %s; output; };" >>>> "outport = \""MC_UNKNOWN "\"; output;" >>>> - : "outport = %s; output;") >>>> - : debug_drop_action(); >>>> + : "outport = %s; output;"; >>>> >>>> if (lsp_is_router(op->nbsp) && op->peer && op->peer->nbrp) { >>>> /* For ports connected to logical routers add flows to bypass the >>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>>> index 4eae1c67c..d95be69c3 100644 >>>> --- a/tests/ovn-northd.at >>>> +++ b/tests/ovn-northd.at >>>> @@ -9095,9 +9095,7 @@ ovn_strip_lflows ], [0], [dnl >>>> table=??(ls_in_check_port_sec), priority=50 , match=(1), >>>> action=(reg0[[15]] = check_in_port_sec(); next;) >>>> table=??(ls_in_l2_lkup ), priority=0 , match=(1), >>>> action=(outport = get_fdb(eth.dst); next;) >>>> table=??(ls_in_l2_lkup ), priority=110 , match=(eth.dst == >>>> $svc_monitor_mac && (tcp || icmp || icmp6)), >>>> action=(handle_svc_check(inport);) >>>> - table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:00:01), action=(drop;) >>>> table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:00:02), action=(outport = "sw0p2"; output;) >>>> - table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:01:01), action=(drop;) >>>> table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:02:02), action=(outport = "sw0p2"; output;) >>>> table=??(ls_in_l2_lkup ), priority=70 , match=(eth.mcast), >>>> action=(outport = "_MC_flood"; output;) >>>> table=??(ls_in_l2_unknown ), priority=0 , match=(1), >>>> action=(output;) >>>> @@ -9129,9 +9127,7 @@ ovn_strip_lflows ], [0], [dnl >>>> table=??(ls_in_check_port_sec), priority=70 , match=(inport == >>>> "sw0p2"), action=(set_queue(10); reg0[[15]] = check_in_port_sec(); next;) >>>> table=??(ls_in_l2_lkup ), priority=0 , match=(1), >>>> action=(outport = get_fdb(eth.dst); next;) >>>> table=??(ls_in_l2_lkup ), priority=110 , match=(eth.dst == >>>> $svc_monitor_mac && (tcp || icmp || icmp6)), >>>> action=(handle_svc_check(inport);) >>>> - table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:00:01), action=(drop;) >>>> table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:00:02), action=(outport = "sw0p2"; output;) >>>> - table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:01:01), action=(drop;) >>>> table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst == >>>> 00:00:00:00:02:02), action=(outport = "sw0p2"; output;) >>>> table=??(ls_in_l2_lkup ), priority=70 , match=(eth.mcast), >>>> action=(outport = "_MC_flood"; output;) >>>> table=??(ls_in_l2_unknown ), priority=0 , match=(1), >>>> action=(output;) >> >> -- >> Regards, >> Vladislav Odintsov >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
