On 6 August 2015 at 18:37, Andy Zhou <az...@nicira.com> wrote: > 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.
Sounds reasonable. >> >> 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. I was thinking this as well. >> >> 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. OK thanks, I've applied this patch to master. I plan to rebase and improve the ping output checking for the remaining patch and resend it. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
