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

[Simon Horman]

On Sat, Nov 26, 2016 at 01:36:00PM -0800, Ben Pfaff wrote:
> 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.

Understood. I agree it makes sense to be cautious.

Acked-by: Simon Horman <simon.horman at netronome.com>


> > > @@ -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.

Point taken.

> > >      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.

I guess its just a matter of personal preference.
I'm happy to keep the above.