On Thu, Aug 6, 2015 at 3:58 PM, Joe Stringer <joestrin...@nicira.com> wrote: > On 30 July 2015 at 12:51, Joe Stringer <joestrin...@nicira.com> wrote: >> On 30 July 2015 at 11:57, Andy Zhou <az...@nicira.com> wrote: >>> On Wed, Jul 29, 2015 at 4:52 PM, Joe Stringer <joestrin...@nicira.com> >>> wrote: >>>> We already queue the removal of the kernel module in OVS_VSWITCHD_START, >>>> via an ON_EXIT() call. This redundant removal also doesn't interact very >>>> well with usage of vports: If the datapath still exists in the kernel, >>>> we cannot remove a vport module; if we cannot remove a vport module, we >>>> cannot remove the openvswitch module. Having the call here prevents >>>> tunnel tests from manually cleaning up their datapath on exit, so this >>>> patch removes it. >>>> >>> >>> I don't understand this. If we consider each test as an independent >>> sessions, should we be able >>> to be add and remove kernel modules in between? Some kernel bugs only >>> shows up during module >>> clean ups. >> >> The issue is that as long as the datapath has a tunnel port configured, you >> cannot remove the vport module (and you need to remove the vport >> module before removing openvswitch module). The solution I use here is >> to queue up three commands using ON_EXIT(): >> - During OVS_VSWITCHD_START, ON_EXIT(modprobe -r openvswitch) >> - At the beginning of vport tests, ON_EXIT(modprobe -r vport_vxlan) >> - Following this, ON_EXIT(ovs-dpctl del-dp ovs-system). >> >> These should be executed in reverse order to remove tunnel ports, >> remove tunnel modules, and remove the ovs module. > > Andy, we spoke offline about this and as I understand your thought is > that tests should begin with OVS_VSWITCHD_START and finish with > OVS_VSWITCHD_STOP. I'm all for that, but I believe that the START > operation (also, ADD_VETH, ADD_VLAN, etc.) should queue up cleanup of > those devices where necessary so that whether it is a successful test > run or a test failure, cleanup should occur. This will cause the > minimum amount of disruption for subsequent tests. OVS_VSWITCHD_STOP > still provides a way to safely cleanup the OVS processes and check > logs for unexpected output, but it can leave out cleanup which is > necessary to execute in both success and failure conditions.
I am fine with the ON_EXIT() approach proposed here. In terms of ovs modules, I wonder if it will be simpler if we simply load all modules for all kernel tests. OVS kmod used to be a single module. > > For cases where people want to debug a test in the middle, they can > always clear the trap and nothing will be cleaned up, allowing > debugging: > trap ":" 0; AT_CHECK([fail]) This is a good ideal. Providing an macro for it would be nice. The cleanup file we generate can also be enhanced so that it can be invoked by hand. > > Given that the trap that's set up by ON_EXIT() is always run (even if > OVS_VSWITCHD_STOP occurs first), I believe that this patch has the > correct behaviour. Do you have any remaining reservations? No. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
