[ovs-dev] [PATCH 2/3] kmod-macros: Don't unload kmod in VSWITCHD_STOP.

Joe Stringer joestringer at nicira.com
Fri Aug 7 18:17:31 UTC 2015


On 6 August 2015 at 18:37, Andy Zhou <azhou at nicira.com> wrote:
> 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
> 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.



More information about the dev mailing list