[ovs-dev] [PATCH 2/3] kmod-macros: Don't unload kmod in VSWITCHD_STOP.
azhou at nicira.com
Fri Aug 7 01:37:28 UTC 2015
On Thu, Aug 6, 2015 at 3:58 PM, Joe Stringer <joestringer at nicira.com> wrote:
> 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.
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
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
> 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?
More information about the dev