Hi Daniel, I had the same reaction as Ben. I would prefer to see any efforts being made towards improving the existing OpenFlow “sample” action. Some more comments inline below.
> On Apr 25, 2016, at 8:48 PM, Daniel Ye <dani...@vmware.com> wrote: > > 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. I believe it should be straightforward to implement that. It’s a matter of extending the “sample” OpenFlow action to support a tunnel_out_port field which could be directly converted into the tunnel_out_port field of the corresponding “userspace” datapath action. > 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. You can’t get more granular than per-flow sampling. If granularity is concern, I would extend the OpenFlow “sample” action to support setting the output port, cf. the design of the datapath action. > 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. There shouldn’t be any significant impact on performance to monitor all ports of a bridge vs. only some of the ports. Since you didn't mention performance, I can only assume that’s not a concern, so there shouldn’t be any issue for users to enable monitoring of all ports of a bridge even when they are interested in only a subset of the ports. The per-bridge IPFIX exporter setting was only added to be consistent with the way the sFlow and netflow exporters can be configured in OVS, and that wasn’t meant to be the primary way of using IPFIX. Adding a per-port setting would break the consistency with sFlow and netflow in OVS, so I believe it’s not appropriate. The main feature was the OpenFlow “sample” action. Efforts should concentrate on that IMHO. > > Bests, > Daniel Benli Ye > > -----Original Message----- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Saturday, April 23, 2016 12:55 AM > To: Daniel Ye > Cc: Wenyu Zhang; dev@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 <dani...@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. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
