On 1/11/2018 7:27 PM, Ansis Atteka wrote:
On 11 January 2018 at 16:13, Greg Rose <[email protected]> wrote:
From: Greg Rose <[email protected]>
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?
Noted.
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.
That makes sense. I'll change the wording as you suggest.
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?
"""
That works for me.
Signed-off-by: Greg Rose <[email protected]>
---
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?
Noted.
+# 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/*"?
I wanted to start the loop with the most recent entry. In fact the
whole script depends on it.
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"?
yes, absolutely. That's a bug. I missed that case in my testing.
Again, the algorithm
depends on the finding the most recently installed directory.
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?
Yes, that's much safer.
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`?
I am not sure to be honest. I'd have to test those cases some more to
find out.
Fourth. How would this "directory copying" work if major version
number changed? Would you copy incompatible kernel modules that can't
be loaded?
The NSX-T installer uses the correct major version rpms for each major
version. There
are different rpms for each.
+ 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?
Not in extra or weak-updates though.
Thanks for the thorough review! I'll incorporate the changes we've
discussed and send a v2.
- Greg
+ 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
