[ovs-dev] [PATCH 2/2] debian: force-reload-kmod while package upgrading.

[Ben Pfaff]

On Tue, Apr 16, 2013 at 07:32:43PM -0700, Gurucharan Shetty wrote:
> On Tue, Apr 16, 2013 at 2:54 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > On Thu, Apr 11, 2013 at 04:40:13PM -0700, Ben Pfaff wrote:
> > > On Wed, Apr 10, 2013 at 03:41:23PM -0700, Gurucharan Shetty wrote:
> > > > Currently, when we upgrade openvswitch packages, we do restart
> > > > of userspace daemons. This does not replace the kernel module.
> > > >
> > > > But almost everytime, we want to also use the new kernel module
> > > > that comes with the new version. This means that we need to
> > > > manually do a "force-reload-kmod". This step, along with reloading
> > > > kernel module also restarts userspace daemons. This gives us a
> > > > total of 2 restarts. This is quite expensive in a hypervisor with
> > > > hundreds of VMs sending real traffic. This also hurts the controller
> > > > as it gets 2 reconnections in a short amount of time.
> > > >
> > > > With this patch, during a package upgrade, we will automatically do
> > > > a force-reload-kmod while openvswitch-switch is installed. Usually,
> > > > when openvswitch is upgraded, both kernel package and userspace
> > > > package is upgraded together with a "dpkg -i" on command line. This
> > > > results in both packages unpacked first and then configured. This
> > > > ensures that we use the new kernel module during the postinst of
> > > > openvswitch-switch package. One can also install the kernel package
> > > > first and then install the userspace package in 2 separate steps.
> > > >
> > > > If anyone wants to just restart the userspace package instead of
> > > > force-reload-kmod, they can set the value of OVS_FORCE_RELOAD_KMOD=no
> > > > while installing the package.
> > > > Ex: OVS_FORCE_RELOAD_KMOD=no dpkg -i openvswitch-switch*
> > > >
> > > > Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
> > >
> > > Did you consider using dpkg "triggers" to accomplish this instead?
> > > Installing either openvswitch-switch or a kernel module package would
> > > fire the appropriate trigger, but the implementation of the triggers
> > > would only do either "force-reload-kmod" or "restart" even if both
> > > triggers had fired.
> >
> > Guru told me off-list that he'd tried triggers but they didn't work
> > for this purpose.
> >
> > Given that, the patch seems OK, but I worry about a few things:
> >
> >         * We used to be very conservative about doing
> >           force-reload-kmod, doing it only when the user told us to,
> >           because we were worried that it would be disruptive.  I
> >           don't think we've heard about big problems with it, so maybe
> >           we can ignore that, but I thought I'd point it out just in
> >           case.
> >
> >         * force-reload-kmod is no better than restart, and might be
> >           worse, when the kernel module hasn't actually changed.  For
> >           example, if dhclient is running, then it is definitely
> >           worse.
> >
> >         * force-reload-kmod is definitely worse if the kernel module
> >           got removed.
> >
> > I think we could mitigate the second and third problems by only doing
> > force-reload-kmod if the module has changed.  Maybe with logic that
> > compares the srcversion from "/sbin/modinfo openvswitch" against
> > /sys/module/openvswitch/srcversion, so that we only reload the module
> > if the on-disk version is different from the loaded version.
> 
> > What do you think?
> >
> I think it is a good idea. Currently we do a depmod in the postinst script
> of the datapath module(through DEBHELPER). So, there can be a case where
> the new kenel module has been placed in the disk and depmod not yet run.
> But openvswitch-switch configure may be called and it thinks that the
> kernel module is still the old one and do a "restart" instead of a
> "force-reload-kmod". I think we can avoid it if we do a "depmod" inside the
> openvswitch-switch init script before deciding whether to do a
> 'force-reload-kmod' or 'restart'. It sounds a little hacky, but should work.

Thanks for thinking of that.  I am not certain that it is necessary,
because I am not certain whether the dependencies file is necessary to
find a module (as opposed to loading a module and its dependencies).  It
can't hurt, though.