Hi Dumitru yes, I will take care of it. In this case, I’m waiting for a response from the maintainers
> On 14 Feb 2025, at 17:55, Dumitru Ceara <[email protected]> wrote: > > Hi Numan, Mark, Han, Aleksandra, Vladislav, Ales, > > Today is the day we're supposed to branch for 25.03 and we still have > this feature that is a candidate for 25.03. I'll try to summarize its > status here (sorry for top posting). > > Based on the discussion between Numan and Aleksandra below it seems > there's a rather clear plan about how this feature should be improved in > the 25.09 cycle. > > However, as far as I understand, there's still no conclusion reached > about accepting this feature in 25.03 as "experimental". I'm OK with > that (if the conditions below are satisfied) and if I'm not wrong Numan > also said he'd be OK with it. > > The reason for it to be marked "experimental" is that we expect with a > high probability that the way this functionality is implemented will > change fundamentally in 25.09 - potentially with user visible modifications. > > From my perspective, what I think this feature needs for it to be merged > in 25.03 (as experimental) is: > > 1. A NEWS item: > - the news item should mention the addition of the new feature and a > brief description of what the feature does (this is a requirement for > any new feature, in general). That item should also clearly indicate > that the feature is experimental. > > 2. The ovn-nb.xml man page change for this functionality should contain > a clear note saying that the new tables (and feature) are experimental. > > For (1), (2), please see 'Q: What does it mean when a feature is marked > "experimental"?' and 'Q: How is a feature marked "experimental"?' in our > documentation: > https://github.com/ovn-org/ovn/blob/cd4ad2f561794a1f67427b2aa904f876a76b6e28/Documentation/faq/general.rst#L4 > > 3. At least one of the maintainers must sign-off on the patch - in this > case I guess that would mean that the maintainer reviewed the code (to a > reasonable extent) and is OK with accepting the experimental feature. > > Unfortunately, I'm not going to be able to do more reviews today. I did > start looking and I pushed the rebased latest (v5) version of the patch > here: > > https://github.com/dceara/ovn/tree/refs/heads/review-pws443995-port-mirroring-v5 > > v5: > https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > > FWIW, CI seems to be green with the patch applied. > > Aleksandra, Vladislav, would you be able of taking care of (1) and (2) > above, that is, adding a NEWS item about this feature and the man page > note about it being experimental? > > But for (3) we need maintainers to agree on what to do in 25.03. Numan, > Mark, Han, what do you guys think? > > Thank you! > > Best regards, > Dumitru > > On 2/14/25 2:50 PM, Aleksandra Rukomoinikova wrote: >> Hi Numan, >> >> On 14 Feb 2025, at 07:23, Numan Siddique <[email protected]> wrote: >> I guess with this, you can mirror to any logical port and it's fine >> if the logical switch of this mirror port >> is not connected to the source port via any router. Which I guess is >> your goal too. >> >> Yes, we pursued this goal. >> >> >> I don't think it is necessary for the cloned packet to go through all >> the stages of the datapath pipeline once the packet is cloned. >> I think it can be directly output to the destination mirror port. >> >> If I understood your point correctly, then logical flow is now being >> added to the egress pipeline of logical pipes containing the target ports >> in the pre_acl reset to the last table, which ensures this. >> >> >> I'm fine if we want to use a new action or not as a short term approach. >> >> >> I've seen your vision of the mirror action below, and I'll take it to work. >> >> In this patch, what happens if the packet from a source port is >> dropped in the ACL stage ? >> Looks like we end up mirroring this packet. Is this fine ? >> >> yes, we wanted to make the mirroring as close to the source port >> as possible. >> >> I also noticed that if a user creates a chain, like below, the >> behavior is weird >> and the packets are dropped. I didn't dig further. >> >> ovn-nbctl mirror-add mirror0 both from-lport sw1-p1 >> ovn-nbctl lsp-attach-mirror sw0-p1 mirror0 >> >> ovn-nbctl mirror-add mirror1 both from-lport sw0-p1 >> ovn-nbctl lsp-attach-mirror sw1-p1 mirror1 >> >> Regarding the case you described, I tend to believe that the v5 >> patch fixes this situation. when pinging from port s1-p1 to >> s0-p1, I see a request on port s1-p1 and then 2 replays from port s0-p1. >> >> In the long term (hopefully in 25.09), below is what I think we >> should design this feature as : >> >> Suppose if I create a mirror like this: >> >> ovn-nbctl mirror-add mirror0 both from-lport sw1-p1 >> ovn-nbctl lsp-attach-mirror sw0-p1 mirror0 >> >> then >> 1 . Consider a mirror as a separate datapath. Hopefully we can make >> use of composable services features. >> 2. Create an internal patch port connecting the logical switch sw0 to >> the mirror datapath ( SB port binding) >> 3. If the packet needs to be mirrored, in the ingress or egress >> pipeline we just do something like >> table=2 (ls_in_mirror ), priority=100 , match=(inport == >> "sw0-p1"), action=(mirror(outport == sw0-mirror0); next;) >> or >> table=7 (ls_out_mirror ), priority=100 , match=(outport == >> "sw01-port1"), action=(mirror(outport == sw0-mirror0); ; next;) >> >> In the mirror pipeline, we can add logical flows for the mirror rules >> (or mirror ACLs). These mirror rules >> can drop the packet if there is no match or output to the mirror output port. >> >> 3. We can consider that each mirror has one output port. >> 4. If we want we can enhance a mirror to have multiple output ports. >> Depending on the match, the output port >> can be chosen. >> >> Regarding your ideas on how it should be designed in 25.09: >> I understand your ideas, I will think in this way for version 25.09. >> >> Thank you for taking the time to test this patch, >> I am extremely grateful. >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
