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?

...

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

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

...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to