[ovs-dev] [PATCH v2] vswitchd: Make Interface's ofport a persistent column.

Ben Pfaff blp at nicira.com
Wed Nov 21 17:53:16 UTC 2012


On Tue, Nov 20, 2012 at 02:19:53PM -0800, Gurucharan Shetty wrote:
> On Tue, Nov 20, 2012 at 1:14 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > On Tue, Nov 20, 2012 at 03:40:50AM -0800, Gurucharan Shetty wrote:
> > > From: Gurucharan Shetty <shettyg at nicira.com>
> > >
> > > Currently, the 'ofport' column in Interface table is
> > > ephemeral and is populated by vswitchd everytime it is
> > > started or when a new interface is created with vswitchd
> > > running.
> > >
> > > Making it persistent lets vswitchd try and assign the
> > > same ofport number to a particular interface across
> > > restarts. This is just a fallback option when
> > > 'ofport_request' column is empty.
> > >
> > > Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
> >
> > I know that you told me this in person just yesterday, but can you
> > remind me why we need to make ofport a read/write column?
> 
> If I do not do it, this is what happens.
> From my notes (sorry for the cryptic messages):
> 
> * Start openvswitch and create a few internal interfaces.
> * Stop openvswitch, remove kernel module, re-insmod it and start ovsdb-server.
> * Start ovs-vswitchd in gdb.
> 
> * Code stores the value of ofport  in the "struct if_cfg -> ofport"
> datastructure for each interface.
> * Go through each interface, and through iface_clear_db_record make
> its ofport as -1. Commit it to DB. Values can actually be seen in the
> database at this point and all are -1 for ofport column (with
> ovs-vsctl get).
> 
> * Call "iface_create" for each interface.
>  - For the first interface, if_cfg->cfg->ofport is -1. And when
> iface_set_ofp_port() is called with a +ve
> value (ofport value), ovsdb_idl_txn_write is called and row->new is
> set because the 2 values of ofport is different(one is -1 and the
> other +ve).
> 
>  - ovsdb_idl_txn_commit is called which commits this +ve value to DB.
> 
>  - In ovsdb_idl_txn_commit, ovsdb_idl_txn_disassemble is called which
> changes all the rows associated with "all other" interfaces to their
> "old" values (the old values have ofport as +ve) in the memory.
> 
>   -iface_create is called for second interface. Now,
> if_cfg->cfg->ofport is +ve and we are setting the same value with
> iface_set_ofp_port(). Since values are same, DB is not updated.
> 
> - So this creates a situation where except for one interface, all
> interfaces have their ofport set as "-1".

I think that you have uncovered a pre-existing bug in ovs-vswitchd.
ovs-vswitchd is supposed to set the ofport column to -1 only if there is
an error that prevents the port from being created.  But it sounds like,
instead, we populate every ofport column with -1 before we attempt to
add the ports.  This is probably a mistake.  I would think that,
instead, we should only modify ofport once we either know the new
OpenFlow port number or know that the port cannot be created.  I think
that would resolve the problem.  Can you take a look at solving it that
way?

Thanks,

Ben.



More information about the dev mailing list