On Dec 6, 2011, at 2:18 PM, Luca Giraudo wrote:
> Hi Neil,
> replies in-line.
>
> On Tue, Dec 6, 2011 at 1:43 PM, Neil Mckee <neil.mc...@inmon.com> wrote:
> Hi Luca,
>
> Just checking.... does this mean that the sFlow-agent-address might change
> just because the first collector in the list changed? Or because there was a
> routing change and the packet path to the collector went out a different
> interface?
>
> I think you are right in the first case: if the first collector changes and
> if the sflow agent is reconfigured then you may have a different agent
> address if the outgoing interface/bridge is different from the previous one
> (i.e. collectors not in the same subnet or different routing). You are
> partially right in the second case (it should be a different interface on a
> different OVS bridge, since the IP address is assigned to the bridge)
>
>
> The primary purpose of the sflow-agent-address is to provide a persistent
> identifier. One that can be used as a key to store the data under. It's
> helpful if that is also an address that can be talked to, but connectivity
> is not the main thing. The main thing is for it to stay the same across
> reboots, routing-changes and collector-changes.
>
> It sounds like you can still set this address explicitly, which is good,
> but if you are choosing one on the fly then please consider ways to make that
> decision as "sticky" as possible. For example, one approach is to select
> the lowest-numbered loopback-address (if there are any) and fall back on the
> lowest numbered interface address (perhaps preferring IPv4 addresses over
> IPv6 addresses). Another approach is to choose one any way you like, but
> then keep it the same for as long as that IP belongs to the switch.
>
> The previous configuration method works as before (agent address or
> controller address) so the persistence is maintained. Regarding your
> suggestion, eventually we can choose firstly the control_ip as the agent
> address and then the outgoing interface as the last resource to make the
> selection more "sticky". What do you think?
That would certainly be an improvement.
It still seems like it might fluctuate in the case where the control_ip is not
set, but I don't have a feel for how often that will happen? (Or, more
specifically, how often that will happen in a switch where there really are
multiple IP's to choose from.)
Neil
>
> Luca
>
>
> Neil
>
>
>
> On Dec 6, 2011, at 12:42 PM, Luca Giraudo wrote:
>
> > ---
> > NEWS | 3 ++
> > lib/netdev.c | 21 +++++++++++++++
> > lib/netdev.h | 1 +
> > ofproto/ofproto-dpif-sflow.c | 56
> > +++++++++++++++++++++++++++++++----------
> > vswitchd/vswitch.xml | 4 ++-
> > 5 files changed, 70 insertions(+), 15 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 8d7addf..6dc6a3a 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -26,6 +26,9 @@ v1.4.0 - xx xxx xxxx
> > - A new distributed testing tool that allows one to diagnose
> > performance
> > and connectivity issues. This tool currently is not included in RH
> > or
> > Xen packages.
> > + - ovs-vsctl:
> > + - sFlow configuration without "agent" parameter in the case the
> > routing
> > + table contains a route to the first sFlow target in the list.
> > - RHEL packaging now supports integration with Red Hat network scripts.
> >
> >
> > diff --git a/lib/netdev.c b/lib/netdev.c
> > index e77fcdf..053bf48 100644
> > --- a/lib/netdev.c
> > +++ b/lib/netdev.c
> > @@ -692,6 +692,27 @@ netdev_set_in4(struct netdev *netdev, struct in_addr
> > addr, struct in_addr mask)
> > : EOPNOTSUPP);
> > }
> >
> > +/* Obtains ad IPv4 address from device name and save the address in in4.
> > Returns
> > + true is successful, false on failure.
> > + */
> > +bool
> > +netdev_get_in4_by_name(const char *device_name, struct in_addr *in4)
> > +{
> > + struct netdev *netdev;
> > +
> > + int error = netdev_open(device_name, "system", &netdev);
> > + if (!error) {
> > + error = netdev_get_in4(netdev, in4, NULL);
> > + netdev_close(netdev);
> > +
> > + if(!error) {
> > + return true;
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > /* Adds 'router' as a default IP gateway for the TCP/IP stack that
> > corresponds
> > * to 'netdev'. */
> > int
> > diff --git a/lib/netdev.h b/lib/netdev.h
> > index 02271d4..10923b6 100644
> > --- a/lib/netdev.h
> > +++ b/lib/netdev.h
> > @@ -131,6 +131,7 @@ int netdev_set_advertisements(struct netdev *, uint32_t
> > advertise);
> > int netdev_get_in4(const struct netdev *, struct in_addr *address,
> > struct in_addr *netmask);
> > int netdev_set_in4(struct netdev *, struct in_addr addr, struct in_addr
> > mask);
> > +bool netdev_get_in4_by_name(const char *device_name, struct in_addr *in4);
> > int netdev_get_in6(const struct netdev *, struct in6_addr *);
> > int netdev_add_router(struct netdev *, struct in_addr router);
> > int netdev_get_next_hop(const struct netdev *, const struct in_addr *host,
> > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> > index d17b0be..a6c5517 100644
> > --- a/ofproto/ofproto-dpif-sflow.c
> > +++ b/ofproto/ofproto-dpif-sflow.c
> > @@ -18,6 +18,7 @@
> > #include <config.h>
> > #include "ofproto-dpif-sflow.h"
> > #include <inttypes.h>
> > +#include <net/if.h>
> > #include <stdlib.h>
> > #include "collectors.h"
> > #include "compiler.h"
> > @@ -30,6 +31,7 @@
> > #include "ofproto.h"
> > #include "packets.h"
> > #include "poll-loop.h"
> > +#include "route-table.h"
> > #include "sflow_api.h"
> > #include "socket-util.h"
> > #include "timeval.h"
> > @@ -223,9 +225,14 @@ sflow_agent_get_counters(void *ds_, SFLPoller *poller,
> > * The sFlow agent address should be a local IP address that is persistent
> > and
> > * reachable over the network, if possible. The IP address associated with
> > * 'agent_device' is used if it has one, and otherwise 'control_ip', the IP
> > - * address used to talk to the controller. */
> > + * address used to talk to the controller. If the agent device is not
> > specified
> > + * then it is figured out by taking a look at the routing table and at
> > target
> > + * list.
> > + */
> > static bool
> > -sflow_choose_agent_address(const char *agent_device, const char
> > *control_ip,
> > +sflow_choose_agent_address(const char *agent_device,
> > + const struct sset *targets,
> > + const char *control_ip,
> > SFLAddress *agent_addr)
> > {
> > struct in_addr in4;
> > @@ -234,12 +241,28 @@ sflow_choose_agent_address(const char *agent_device,
> > const char *control_ip,
> > agent_addr->type = SFLADDRESSTYPE_IP_V4;
> >
> > if (agent_device) {
> > - struct netdev *netdev;
> > + bool ret = netdev_get_in4_by_name(agent_device, &in4);
> > +
> > + if (ret == true) {
> > + goto success;
> > + }
> > + } else if (!sset_is_empty(targets)) {
> > + const char *target;
> > + ovs_be32 target_ip = 0;
> > + char name[IFNAMSIZ];
> > +
> > + SSET_FOR_EACH (target, targets) {
> > + struct sockaddr_in target_addr;
> > + if (inet_parse_active(target, SFL_DEFAULT_COLLECTOR_PORT,
> > &target_addr)) {
> > + target_ip = target_addr.sin_addr.s_addr;
> > + break;
> > + }
> > + }
> >
> > - if (!netdev_open(agent_device, "system", &netdev)) {
> > - int error = netdev_get_in4(netdev, &in4, NULL);
> > - netdev_close(netdev);
> > - if (!error) {
> > + if (target_ip && route_table_get_name(target_ip, name)) {
> > + bool ret = netdev_get_in4_by_name(name, &in4);
> > +
> > + if (ret == true) {
> > goto success;
> > }
> > }
> > @@ -289,6 +312,8 @@ dpif_sflow_create(struct dpif *dpif)
> > ds->next_tick = time_now() + 1;
> > hmap_init(&ds->ports);
> > ds->probability = 0;
> > + route_table_register();
> > +
> > return ds;
> > }
> >
> > @@ -307,6 +332,7 @@ dpif_sflow_destroy(struct dpif_sflow *ds)
> > if (ds) {
> > struct dpif_sflow_port *dsp, *next;
> >
> > + route_table_unregister();
> > dpif_sflow_clear(ds);
> > HMAP_FOR_EACH_SAFE (dsp, next, hmap_node, &ds->ports) {
> > dpif_sflow_del_port__(ds, dsp);
> > @@ -430,6 +456,14 @@ dpif_sflow_set_options(struct dpif_sflow *ds,
> > }
> > }
> >
> > + /* Choose agent IP address and agent device (if not yet setup) */
> > + if (!sflow_choose_agent_address(options->agent_device,
> > + &options->targets,
> > + options->control_ip, &agentIP)) {
> > + dpif_sflow_clear(ds);
> > + return;
> > + }
> > +
> > /* Avoid reconfiguring if options didn't change. */
> > if (!options_changed) {
> > return;
> > @@ -437,13 +471,6 @@ dpif_sflow_set_options(struct dpif_sflow *ds,
> > ofproto_sflow_options_destroy(ds->options);
> > ds->options = ofproto_sflow_options_clone(options);
> >
> > - /* Choose agent IP address. */
> > - if (!sflow_choose_agent_address(options->agent_device,
> > - options->control_ip, &agentIP)) {
> > - dpif_sflow_clear(ds);
> > - return;
> > - }
> > -
> > /* Create agent. */
> > VLOG_INFO("creating sFlow agent %d", options->sub_id);
> > if (ds->sflow_agent) {
> > @@ -572,6 +599,7 @@ dpif_sflow_run(struct dpif_sflow *ds)
> > {
> > if (dpif_sflow_is_enabled(ds)) {
> > time_t now = time_now();
> > + route_table_run();
> > if (now >= ds->next_tick) {
> > sfl_agent_tick(ds->sflow_agent, time_wall());
> > ds->next_tick = now + 1;
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 51309bf..948e756 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -2771,7 +2771,9 @@
> >
> > <column name="agent">
> > Name of the network device whose IP address should be reported as the
> > - ``agent address'' to collectors. If not specified, the IP address
> > + ``agent address'' to collectors. If not specified, the agent device
> > is
> > + figured from the first target address and the routing table. If the
> > routing
> > + table does not contain a route to the target, the IP address
> > defaults to the <ref table="Controller" column="local_ip"/> in the
> > collector's <ref table="Controller"/>. If an agent IP address cannot
> > be
> > determined either way, sFlow is disabled.
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
