[ovs-dev] [PATCH 2/3] kmod-macros: Don't unload kmod in VSWITCHD_STOP.
joestringer at nicira.com
Thu Aug 6 22:58:54 UTC 2015
On 30 July 2015 at 12:51, Joe Stringer <joestringer at nicira.com> wrote:
> On 30 July 2015 at 11:57, Andy Zhou <azhou at nicira.com> wrote:
>> On Wed, Jul 29, 2015 at 4:52 PM, Joe Stringer <joestringer at 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.
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
trap ":" 0; AT_CHECK([fail])
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?
More information about the dev