On Thu, Mar 27, 2014 at 8:29 PM, Ben Pfaff <b...@nicira.com> wrote: > 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.
Thanks for the review and writing up the V4 patch. > > 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. Using uuid is indeed much cleaner and simpler all around. Thanks for other clean ups as well. > > 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 agree with you on being nice to existing scripts. What's is our answer to (future) scripts that can benefit from such error code? I suppose we can suggest them to monitor stderr, but not sure if this is what you had in mind. > > I improved the log message slightly. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
