| Message ID | 20220117092708.20050-1-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto: fix ipfix not always sampling on egress | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <amorenoz@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? 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <amorenoz@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? > 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <amorenoz@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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > -- > Adrián Moreno
On 1/20/22 11:31, Eelco Chaudron wrote: > > > 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 <amorenoz@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. > Thanks, I'll take a look. >>> 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 >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >> -- >> Adrián Moreno >
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; }
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 <amorenoz@redhat.com> --- ofproto/ofproto-dpif-xlate.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)