Missing python3 libvirt bindings

[NuxRo]

Missing python3 libvirt bindings on CentOS7 effectively break security groups.
There are 0 firewall rules added. The agent logs report:

2020-06-02 10:58:34,346 DEBUG [kvm.resource.LibvirtComputingResource] (main:null) (logid:) Traceback (most recent call last): File "/usr/share/cloudstack-common/scripts/vm/network/security_group.py", line 26, in import libvirtModuleNotFoundError: No module named 'libvirt'

Description

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

[andrijapanicsb]

Not sure how this was not tested during 4.14 👎

[wido]

Super weird that this was missed.

LGTM

[DaanHoogland]

in 4.14 we are still on python2 why are you expecting this to be tested?
@andrijapanicsb @wido

[wido]

@DaanHoogland The Security Grouping part has already moved to Python 3. The scripts inside the VR for example are still Py2.

security_group.py is a standalone script outside all the other scripts.

[yadvr]

@NuxRo can you rebase and edit the PR for 4.14?

[yadvr]

Change LGTM, but we should address this in 4.14.1.0

[NuxRo]

It'd be great if in such instances we'd get some sort of clue in the UI or somehow get an idea that SGs are not working, or fail adding the hypervisor altogether. It took me a while to catch the problem, idiotically assuming it works. Ended up with a stupid CentOS 5 VM IP spoofing the hell out of it. At least tc was doing its job, so the perpetrators only had 200 Mbps :-D

[weizhouapache]

@NuxRo
I do not use centos7. I have two questions
(1) is python36-libvirt valid in official centos7 repo or epel repo?
(2) is python3-libvirt a valid package in centos7 ? if so, I suggest to use python3-libvirt instead of python36-libvirt

[svenvogel]

@weizhouapache yes. python36-libvirt.x86_64 is a valid package from EPEL. :/ there is no python3-libvirt although it sounds better :)

[weizhouapache]

@weizhouapache yes. python36-libvirt.x86_64 is a valid package from EPEL. :/ there is no python3-libvirt although it sounds better :)

thanks @svenvogel
it seems epel repo is not suggested in installation guide, do we need to add it in cloudstack document ?

[andrijapanicsb]

installing this package will install python3-* (a few of these) - worth testing this thoroughly...

[DaanHoogland]

FWIW: just did an install of master on centos7;

Error: Package: cloudstack-common-4.15.0.0-SNAPSHOT.fc31.x86_64 (/cloudstack-common-4.15.0.0-SNAPSHOT.fc31.x86_64)
           Requires: python(abi) = 3.7
           Installed: python-2.7.5-77.el7_6.x86_64 (@updates)
               python(abi) = 2.7
               python(abi) = 2.7
           Installed: python3-3.6.8-13.el7.x86_64 (@base)
               python(abi) = 3.6
               python(abi) = 3.6
           Available: python-2.7.5-88.el7.x86_64 (base)
               python(abi) = 2.7
               python(abi) = 2.7
           Available: python34-3.4.10-4.el7.x86_64 (epel)
               python(abi) = 3.4

I don't know if it is significant, but it seems to ask for python 3.7. (not libvirt related management server install.
maybe due to build on fedora??? so probably nothing/red hering

[NuxRo]

Daan, it looks like those RPMs were built on Fedora 31 which probably comes with python 3.7.
The python version is not mentioned in the rpm spec file, but rpm is "smart" enough to add certain dependencies outside of what is in the spec file, this can be turned off in the spec with "AutoReqProv: no".
Ideally you should be building the RPMs in a CentOS7 environment, though...

[NuxRo]

@weizhouapache yes. python36-libvirt.x86_64 is a valid package from EPEL. :/ there is no python3-libvirt although it sounds better :)

thanks @svenvogel
it seems epel repo is not suggested in installation guide, do we need to add it in cloudstack document ?

We should definitely do that, EPEL is pretty safe.

[yadvr]

@NuxRo changing just the base branch is not enough, I've taken the liberty to fix it for you and do a force push. Now the PR can be merged for 4.14.1.0 and fwd merged to master.

@blueorangutan package

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[andrijapanicsb]

This requires a docs (also branch 4.14) update to add Epel as the pre-requisite on KVM hosts.

Potentially - this whole work here in this PR, could be moved to the docs, instead of package dependency - i.e. dependency will NOT work if Epel is not installed... and making epel-release a dependency is too "bold" on the user...

Opinions?

[blueorangutan]

Packaging result: ✔centos7 ✔debian. JID-1272

[NuxRo]

I don't see requiring epel as a problem, it's very widely used.

[wido]

Me neither. Adding this to the docs will only confuse people. RPM and DEB packages should depend on everything you need to run CloudStack.

[andrijapanicsb]

ok, then let's leave it as it is.

[yadvr]

Smoketests are not necessary as change is in the centos7 package. I tested in the bhaisaab/centos7-cloudstack-slave container the package is available in epel:

Installing:
 python36-libvirt                                 x86_64                           4.5.0-1.el7                                  epel                              284 k
I

Doc change requested @andrijapanicsb

[yadvr]

Doc PR raised apache/cloudstack-documentation#135 cc @andrijapanicsb