On 20 Jan 2022, at 11:06, Adrian Moreno wrote:
> On 1/20/22 10:13, Eelco Chaudron wrote: >> >> >> On 17 Jan 2022, at 10:27, Adrian Moreno wrote: >> >>> We are currently requiring in_port to be a valid port number for ipfix >>> sampling even if the sampling is done on the output port (egress). >>> >>> This restriction is not really needed and can affect pipelines that >>> intentionally set the in_port to OFPP_NONE during flow processing. For >>> instance, OVN does this, see: >>> >>> cfa547821 Fix ovn-controller generated packets from getting dropped for >>> reject ACL action. >>> >>> This patch skips ipfix sampling only if both (ingress and egress) ports >>> are invalid. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2016346 >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >> >> Adrian, the change looks good to me. Maybe you could add a test case for >> this specific configuration, i.e., ingress and egress only? >> > > I agree we need a test. I'm not very familiar with the unit test system so I > was looking into how to verify what datapath flows are being programmed. Any > suggestions/pointers? My first idea would be to use “ovs-appctl ofproto/trace” which will give you the megaflow and datapath actions, but the existing tests are probably better. The “ofproto-dpif - Bridge IPFIX sanity check” (and the others) just use dpctl/dump-flows, guess you can use those as a baseline. >> Cheers, >> >> Eelco >> >>> --- >>> ofproto/ofproto-dpif-xlate.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index cafcd014a..189276bc1 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -3272,7 +3272,9 @@ compose_ipfix_action(struct xlate_ctx *ctx, >>> odp_port_t output_odp_port) >>> struct dpif_ipfix *ipfix = ctx->xbridge->ipfix; >>> odp_port_t tunnel_out_port = ODPP_NONE; >>> >>> - if (!ipfix || ctx->xin->flow.in_port.ofp_port == OFPP_NONE) { >>> + if (!ipfix || >>> + (output_odp_port == ODPP_NONE && >>> + ctx->xin->flow.in_port.ofp_port == OFPP_NONE)) { >>> return; >>> } >>> >>> -- >>> 2.34.1 >>> >>> _______________________________________________ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > -- > Adrián Moreno _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
