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. > > > > 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 > -- Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA [email protected] @RedHat Red Hat Red Hat _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
