[ovs-dev] [PATCH v1] Support port level IPFIX

[Daniel Ye]

Hi Ben, 

Thanks for your review. I have sent a second patch with signed-off tag. Please review it again.
For your comments, I listed the answers of mine below:
1. "I worry that we'll end up with a fourth, and a fifth, ..."
    With flow IPFIX, Bridge IPFIX and Port level IPFIX, we can handle almost all the IPFIX cases. There is no need to add other kinds of IPFIX.

2. " why can't the details of what packets it selects, etc., be controlled from flows in the flow table, rather than by configuration in the database?"
    We can't do this, because flow based IPFIX does not support to export output tunnel information. However, in overlay network, users want to
    know tunnel information.  With Bridge IPFIX, output tunnel information can be retrieved. But, Bridge IPFIX will enable all ports on the bridge to
    collects the IPFIX information and it's not granular. With Port IPFIX, users can just enable IPFIX on the ports, which they are interested in and the
    IPFIX information is preciser. Output tunnel information can be got if you enable port IPFIX on the tunnel port. What's more, IPFIX
    configuration of each port can be different. While Bridge IPFIX can only support one IPFIX configuration per bridge. Users need configure IPFIX
    granularly like port-based IPFIX.

3. If we want to use IPFIX to monitor the packets which ingress and egress the port, it's easier to be configured by port than by flows.

Bests,
Daniel Benli Ye

-----Original Message-----
From: Ben Pfaff [mailto:blp at ovn.org] 
Sent: Saturday, April 23, 2016 12:55 AM
To: Daniel Ye
Cc: Wenyu Zhang; dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1] Support port level IPFIX

On Tue, Apr 19, 2016 at 03:55:10PM +0800, Daniel Benli Ye wrote:
> From: Benli Ye <daniely at vmware.com>
> 
> This patch enables port level IPFIX. Before this patch, OVS supported 
> per bridge IPFIX and per flow IPFX, and exporting packet tunnel 
> headers is only supported by bridge IPFIX. This patch adds port level 
> IPFIX for easy configuration and port level IPFIX also supports 
> exporting packet tunnel headers, just the same with bridge level IPFIX.
> Three main things are done in this patch.
>   1) Add a column ipfix in Port table to ref IPFIX table
>   2) Each interface in the port should use the port IPFiX configuration
>   3) A hash map is used to manage the port which is configured IPFIX
> 
> CLI to configure Port IPFIX:
>   1) Configure
>      ovs-vsctl -- set Port port0 ipfix=@i -- --id=@i create IPFIX \
>          targets=\"10.24.122.72:4739\" sampling=1 obs_domain_id=123 \
>          obs_point_id=456 cache_active_timeout=1 cache_max_flows=128 \
>          other_config:enable-tunnel-sampling=true
>   2) Clear
>      ovs-vsctl clear Port port0 ipfix

Thanks for working on IPFIX!  We don't have enough IPFIX expertise around here, so new contributors are always welcome.

The patch lacks a Signed-off-by.  We will need it before it can be applied.  CONTRIBUTING.md explains the format and the meaning, which is to agree to the Developer's Certificate of Origin, which is also in CONTRIBUTING.md; please read it.

Due to the lack of signoff, I did not do a detailed review, but I have some general comments.  First, this patch follows the coding style remarkably well, especially for a first patch--well done, thank you!

Second, this is the third form of configuration to be introduced for IPFIX.  I worry that we'll end up with a fourth, and a fifth, ...  Why does IPFIX need so many kinds of configuration; that is, why can't the details of what packets it selects, etc., be controlled from flows in the flow table, rather than by configuration in the database?  If that were the case, then we would not need to so many forms of configuration:
the controller could control it through the flows over which it already has so much control.

Thanks,

Ben.