[ovs-dev] [PATCH v2] ofp-actions: Add "ingress" and "egress" options to "sample" action.

[Ben Pfaff]

On Fri, Nov 25, 2016 at 03:46:55PM +0100, Simon Horman wrote:
> Hi Ben,
> 
> On Wed, Nov 23, 2016 at 11:18:14PM -0800, Ben Pfaff wrote:
> > Before Open vSwitch 2.5.90, IPFIX reports from Open vSwitch didn't include
> > whether the packet was ingressing or egressing the switch.  Starting in
> > OVS 2.5.90, this information was available but only accurate if the action
> > included a port number that indicated a tunnel.  Conflating these two does
> > not always make sense (not every packet involves a tunnel!), so this patch
> > makes it possible for the sample action to simply say whether it's for
> > ingress or egress.
> > 
> > This is difficult to test, since the "tests" directory of OVS does not have
> > a proper IPFIX listener.  This passes those tests, plus a couple that just
> > verify that the actions are properly parsed and formatted.  Benli did test
> > it end-to-end in a VMware use case.
> 
> I am wondering if it is necessary to add NXAST_RAW_SAMPLE3 instead
> of just adding a direction field to NXAST_RAW_SAMPLE2 and updating
> branch-2.6. Is it too late for this change to change the behaviour of
> NXAST_RAW_SAMPLE2?

I'm wary of doing that because 2.6.0 was released months ago and because
trying to use this new with an older OVS would cause it to be silently
ignored.  (If we had been careful with the definition of the action, as
I try to be these days, then it would have provoked an
OFPERR_NXBRC_MUST_BE_ZERO error, and I'd be happy to just extend the
action.)

There's not too much cost to inventing NXAST_RAW_SAMPLE3, so I'm
comfortable with it.

> > @@ -4855,8 +4861,8 @@ decode_NXAST_RAW_SAMPLE(const struct nx_action_sample *nas,
> >      sample->collector_set_id = ntohl(nas->collector_set_id);
> >      sample->obs_domain_id = ntohl(nas->obs_domain_id);
> >      sample->obs_point_id = ntohl(nas->obs_point_id);
> > -    /* Default value for sampling port is OFPP_NONE */
> 
> The comment above still seems to be true.

It's still true, but I don't think there's much value in it.  It's
obvious that it's the default.

> >      sample->sampling_port = OFPP_NONE;
> > +    sample->direction = NX_ACTION_SAMPLE_DEFAULT;
> >  
> >      if (sample->probability == 0) {
> >          return OFPERR_OFPBAC_BAD_ARGUMENT;
> 
> ...
> 
> > @@ -1648,7 +1649,9 @@ ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
> >          data_common = dp_packet_put_zeros(&msg, sizeof *data_common);
> >          data_common->observation_point_id = htonl(obs_point_id);
> >          data_common->flow_direction =
> > -            (output_odp_port == ODPP_NONE) ? INGRESS_FLOW : EGRESS_FLOW;
> > +            (direction == NX_ACTION_SAMPLE_INGRESS ? INGRESS_FLOW
> > +             : direction == NX_ACTION_SAMPLE_EGRESS ? EGRESS_FLOW
> > +             : output_odp_port == ODPP_NONE ? INGRESS_FLOW : EGRESS_FLOW);
> 
> if, else if,... might be a bit easier on the eyes here.

I tend to prefer ?: when all of the conditions would be assigning to the
same object, when the conditions are simple enough that they each fit on
a line.