[ovs-dev] [PATCH 2/2] rhel: Add post installation check for kernel modules

[Ansis Atteka]

On 11 January 2018 at 16:13, Greg Rose <gvrose8192 at gmail.com> wrote:
> From: Greg Rose <roseg at vmware.com>
>
> A bug in RHEL 7.2 has been found in which a customer who installed
> a RHEL 7.2 openvswitch kernel module rpm with a slightly different
> minor build number than the rnning kernel found that the kernel

s/rnning/currently running?

> modules were installed to the wrong directory.
>
> After the installation the new openvswitch kernel modules were
> installed to:
> /lib/modules/3.10.0-327.22.2.el7.x86_64/extra/openvswitch
>
> But the running kernel was 3.10.0-327.el7.x86_64 and after the
> installation was complete the kernel modules in the installed
> directory were not linked to the "weak-updates" directory in
> the running kernel.  So the customer was not able to load the
> correct kernel modules and a critical bug was encountered.

I think it may be helpful to explain that "critical bug" here is that
openvswitch in-tree kernel module would be loaded opposed to the one
customer explicitly installed with rpm and expected to be loaded. It
would help reader to understand intent of this commit message a little
better.

>
> This patch does a post installation sanity check to make sure
> that the ../extra/openvswitch directory or the
> ../weak-updates/openvswitch directory at least exist.

I am a little bit confused about wording of the paragraph above. Would
it make sense to reword it to something like:

"""
This patch replicates ./extra/openvswitch directory with kernel
modules, if for currently running kernel there is neither
./extra/openvswitch nor ./weak-update/openvswitch directory?
"""


>
> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
> ---
>  rhel/openvswitch.spec.in | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
> index e510d35..fbcd868 100644
> --- a/rhel/openvswitch.spec.in
> +++ b/rhel/openvswitch.spec.in
> @@ -169,6 +169,25 @@ fi
>  /sbin/chkconfig --add openvswitch
>  /sbin/chkconfig openvswitch on
>
> +# In some cases a kernel module rpm will have a different minor build
> +# version than the current running kernel.  Check and copy modules if

s/current/currently?

> +# necessary.
> +if [[ ! -d /lib/modules/$(uname -r)/extra/openvswitch && \
> +      ! -d /lib/modules/$(uname -r)/weak-updates/openvswitch ]]; then
> +    found="false"
> +    for i in `ls -t /lib/modules`

1. The iteration in for loop will act weird if in /lib/modules you
have a file or directory with whitespace character. Should you replace
"`ls -t`" simply with "/lib/modules/*"?
2. And then I would use quotes around any paths where you expand $i to
prevent  bogus filenames to confuse execution flow once $i expands.

> +    do
> +         if [ -d /lib/modules/$i/extra/openvswitch ]; then
> +             cp -r --preserve /lib/modules/$i/extra /lib/modules/$(uname -r)
> +             /usr/sbin/depmod -a

I am not sure if I understand the intended behavior of the lines
above. Here are my [possibly unjustified] concerns:

First. isn't it sub-optimal to call `depmod -a` each time in the for
loop? Would it be more optimal and still achieve the same behavior if
you would reverse lines in `ls -t` and break out early from the
for-loop once "found" becomes "true"?

Second, is it ok to copy&paste all kernel modules like that between
different $i versions? Especially w.r.t. thirdparty kernel modules
that are provided neither by us or Red Hat, but customer has installed
on the host? Would it make sense to copy only openvswitch kernel
module?

Third, what `rpm` utility would do when our users would try to remove
old kernels? Would the /lib/modules/$i directory stay on filesystem
forever once we have created files there that rpm does not know
anything about? Would customer have to explicitly remove them with
`rm`?

Fourth. How would this "directory copying" work if major version
number changed? Would you copy incompatible kernel modules that can't
be loaded?

> +             found="true"
> +         fi
> +    done
> +    if [ "$found" != "true" ]; then

Can you tell me under what condition $found would not be equal to
true? I imagine that from our post installation scriptlet there always
should be at least one /lib/modules/X directory that has openvswitch
directory present?

> +        echo "Error in openvswitch kernel modules installation"
> +    fi
> +fi
> +
>  %post selinux-policy
>  /usr/sbin/semodule -i %{_datadir}/selinux/packages/%{name}/openvswitch-custom.pp &> /dev/null || :
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev