On Thu, Mar 27, 2014 at 05:10:31PM +0100, Thomas Graf wrote: > From: Andy Zhou <az...@nicira.com> > > ovs-vsctl is a command-line interface to the Open vSwitch database, > and as such it just modifies the desired Open vSwitch configuration in > the database. ovs-vswitchd, on the other hand, monitors the database > and implements the actual configuration specified in the database. > This can lead to surprises when the user makes a change to the > database, with ovs-vsctl, that ovs-vswitchd cannot actually > implement. In such a case, the ovs-vsctl command silently succeeds > (because the database was successfully updated) but its desired > effects don't actually take place. One good example of such a change > is attempting to add a port with a misspelled name (e.g. ``ovs-vsctl > add-port br0 fth0'', where fth0 should be eth0); another is creating > a bridge or a port whose name is longer than supported > (e.g. ``ovs-vsctl add-br'' with a 16-character bridge name on > Linux). It can take users a long time to realize the error, because it > requires looking through the ovs-vswitchd log. > > The patch improves the situation by checking whether operations that > ovs executes succeed and report an error when > they do not. This patch only report add-br and add-port > operation errors by examining the `ofport' value that > ovs-vswitchd stores into the database record for the newly created > interface. Until ovs-vswitchd finishes implementing the new > configuration, this column is empty, and after it finishes it is > either -1 (on failure) or a positive number (on success). > > Signed-off-by: Andy Zhou <az...@nicira.com> > Signed-off-by: Thomas Graf <tg...@redhat.com> > > --- > V3: Fixed tests/ovs-vsctl.at to expect improved error message > V2: Improved error message to indicate error occurs during setup and > that additional details can be found in the log.
I found this patch inspiring. For some reason I always thought that implementing this behavior would involve a lot of work, and a significant amount of tricky code, but this patch demonstrated that it isn't that bad. I learned a new word, "neoteric". On the other hand, I have a lot of actual comments, but on the third hand I'm going to post a v4 that incorporates those comments in a minute. So here are the comments. I'm afraid that checking for success on an interface based on just the name of the interface could have false positives in cases where clients are adding and deleting interfaces, that might be different, with common names. These cases aren't common, but it's possible to avoid them by using the UUID of the inserted row, as returned by the database transactions, instead of the name of the row, and it's not difficult to do it that way, so I'd prefer to do it that way. That's the bulk of what I changed. When we use UUID instead of name, there's no need to "unexpect" anything, since the database can tell us that the rows were never actually constructed, which simplifies things slightly. I don't think it's a good idea to actually exit with an error code, because it's a change in behavior that could break scripts. For example, I bet that some people use "ovs-vsctl add-port br0 <name>" and then separately "ovs-vsctl set interface <name> type=gre options:...". Even though it would be better to add and configure in a single command, it used to work fine, with exit code 0, to do add in one and configure in another, and I wouldn't want to break that if the script checks exit status (e.g. with "set -e"). I improved the log message slightly. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
