Bug 1182092

Why is this needed?

This looks like invalid bug. Bug should be about user needs and not about "solutions".

I don't know about code duplication with current supervdsm code.

There are several reasons why this pattern is preferred:
- It allows modules/capabilities to be loaded independently, per availability (we have such a scenario now).
- It decouples modules and reduces dependency in the project.
- It is cleaner: Instead of a messy single flat file that includes everything, it is grouped and distributed to independent api files of each module.
- An addition or change in a module api does not need to collide with same changes of a different module (I guess it falls in the 'decoupled' category).

As I see it, the supervdsm api is an infrastructure service, modules that need to expose their api to supervdsm should locate their api under supervdsm_api folder.

(In reply to Edward Haas from comment #2)
> There are several reasons why this pattern is preferred:
> - It allows modules/capabilities to be loaded independently, per
> availability (we have such a scenario now).

You can load modules modules as needed without having a plugable api
for supervdsm.

> - It decouples modules and reduces dependency in the project.

We don't have a need to decouple supervdsm modules from the code implementing
it.

> - It is cleaner: Instead of a messy single flat file that includes
> everything, it is grouped and distributed to independent api files of each
> module.

There is nothing messy about the current supervdsm code; it is a single 
class exposed to vdsm via multiprocessing, that should contain one line
wrapper for each api.

Having independent apis is not needed currently, is more complex, and
needs more code that someone has to maintain.

> - An addition or change in a module api does not need to collide with same
> changes of a different module (I guess it falls in the 'decoupled' category).

supervdsm changes infrequently, this is a non-issue.

> As I see it, the supervdsm api is an infrastructure service, modules that
> need to expose their api to supervdsm should locate their api under
> supervdsm_api folder.

Or locate their api in the single supervdsm file, as we do today.

Having single location for defining the apis that allowed to run as root make
it easier to maintain and keep secure.

Currently vdsm always contain all subsystems, so all supervdsm apis are 
always needed. We may like a plugable api if we break vdsm to multiple
components, and only some of them may be installed on a host.

I suggest to close this bug for now.

As there are benefits (some outlined above, and I'm sure Yaniv can add more) and there is work we've done around that, we aren't gonna close it. 
Unless the patch has technical issues, which should be reviewed in Gerrit, I don't see a reason not to push this work. 

The infra team is the owner of this code, and will maintain it.

(In reply to Nir Soffer from comment #3)
> (In reply to Edward Haas from comment #2)
> > There are several reasons why this pattern is preferred:
> > - It allows modules/capabilities to be loaded independently, per
> > availability (we have such a scenario now).
> 
> You can load modules modules as needed without having a plugable api
> for supervdsm.
> 
How would you propose to do that? The trivial way is with an if statement for each module, that checks something (and that something is a module logic), but that pollutes the api with if/s.

> > - It decouples modules and reduces dependency in the project.
> 
> We don't have a need to decouple supervdsm modules from the code implementing
> it.
Why modularity is not a need here? I thought it is an universal need :)

> 
> > - It is cleaner: Instead of a messy single flat file that includes
> > everything, it is grouped and distributed to independent api files of each
> > module.
> 
> There is nothing messy about the current supervdsm code; it is a single 
> class exposed to vdsm via multiprocessing, that should contain one line
> wrapper for each api.

The api/s themselves are not flat, they are owned by different modules, some belong to the same module.
One file for such a thing is not scalable, it grows and is not clean.

> 
> Having independent apis is not needed currently, is more complex, and
> needs more code that someone has to maintain.
> 
> > - An addition or change in a module api does not need to collide with same
> > changes of a different module (I guess it falls in the 'decoupled' category).
> 
> supervdsm changes infrequently, this is a non-issue.
> 
> > As I see it, the supervdsm api is an infrastructure service, modules that
> > need to expose their api to supervdsm should locate their api under
> > supervdsm_api folder.
> 
> Or locate their api in the single supervdsm file, as we do today.
> 
> Having single location for defining the apis that allowed to run as root make
> it easier to maintain and keep secure.
>
Having it under a dedicated folder has the same result IMO.
 
> Currently vdsm always contain all subsystems, so all supervdsm apis are 
> always needed. We may like a plugable api if we break vdsm to multiple
> components, and only some of them may be installed on a host.
> 
> I suggest to close this bug for now.

There are VDSM features that may not be available on a specific host, therefore, its  corresponding apis better not be available.
We are focusing here on supervdsm, which is internal.. And I think it will be good to start breaking components at all layers.

Oved is current. same as splitting vm.py, same as changing names and clean code, same as changing to yaml scheme and more other improvements for readability and easier maintaining to the code - here we also provide such thing. instead of one package which contains the server logic and the api logic inside a huge object, with tons of imports above, we want to separate it, and allow others to add functions which supervdsm exposes without reading the server code - only by dropping new package.py with foo func, and you'll be able to run it with root privileges by supervdsm proxy. 
In addition, supervdsmServer code needs to include only server implementation, external logic should not be there. Dima suggested much nicer approach in the past which Dan found a bit risky (the proxy decorator - the patch is still attached), the api folder is another alternative which also provide nice solution for adding\removing logic to the service. unless you provide good explanation why not to do that, lets move on .. we (and other that requested for it) want to refine this code in that direction

For me, the greatest importance modularizing supervdsm is to simplify splitting the service based on verticals: network, storage, virt and gluster. gLuster has a @decorator trick of their own, so they are not clustered into supervdsmServer, but everybody else - does.

(In reply to Yaniv Bronhaim from comment #6)
Not clear what info is requested, removing needinfo.

Yaniv, please note that supervdsm.log must still include the name of the called function. I believe that as of dce403f222c this is no longer the case. Please fix it.

(In reply to Dan Kenigsberg from comment #9)
> Yaniv, please note that supervdsm.log must still include the name of the
> called function. I believe that as of dce403f222c this is no longer the
> case. Please fix it.

Indeed this is what I saw why testing my

MainProcess|Thread-119::DEBUG::2016-02-29 11:44:16,211::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with (u'/var/lib/libvirt/qemu/channels/b29c12fe-d0ce-4bed-b2f5-184488854d37.com.redhat.rhevm.vdsm',) {}
MainProcess|Thread-119::DEBUG::2016-02-29 11:44:16,212::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with None
MainProcess|Thread-119::DEBUG::2016-02-29 11:44:16,213::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with (u'/var/run/ovirt-vmconsole-console/b29c12fe-d0ce-4bed-b2f5-184488854d37.sock', 'ovirt-vmconsole') {}
MainProcess|Thread-119::DEBUG::2016-02-29 11:44:16,213::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with None
MainProcess|Thread-119::DEBUG::2016-02-29 11:44:16,215::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|Thread-119::DEBUG::2016-02-29 11:44:16,216::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with 7169
MainProcess|periodic/2::DEBUG::2016-02-29 11:44:19,249::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/2::DEBUG::2016-02-29 11:44:19,261::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/0::DEBUG::2016-02-29 11:44:34,240::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/0::DEBUG::2016-02-29 11:44:34,249::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/1::DEBUG::2016-02-29 11:44:49,244::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-29 11:44:49,252::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/0::DEBUG::2016-02-29 11:45:04,226::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/0::DEBUG::2016-02-29 11:45:04,234::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/2::DEBUG::2016-02-29 11:45:19,228::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/2::DEBUG::2016-02-29 11:45:19,237::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/2::DEBUG::2016-02-29 11:45:34,234::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/2::DEBUG::2016-02-29 11:45:34,242::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/3::DEBUG::2016-02-29 11:45:49,228::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/3::DEBUG::2016-02-29 11:45:49,236::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/2::DEBUG::2016-02-29 11:46:04,240::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/2::DEBUG::2016-02-29 11:46:04,248::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/0::DEBUG::2016-02-29 11:46:19,245::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/0::DEBUG::2016-02-29 11:46:19,254::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/3::DEBUG::2016-02-29 11:46:34,240::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/3::DEBUG::2016-02-29 11:46:34,248::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}
MainProcess|periodic/1::DEBUG::2016-02-29 11:46:49,240::supervdsmServer::111::SuperVdsm.ServerCallback::(wrapper) call wrapper with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-29 11:46:49,249::supervdsmServer::118::SuperVdsm.ServerCallback::(wrapper) return wrapper with {0: [0], 1: [0]}

https://gerrit.ovirt.org/#/c/54004/3

Previously it was something like

MainProcess|periodic/1::DEBUG::2016-02-25 02:02:15,769::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/2::DEBUG::2016-02-25 02:02:30,779::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/2::DEBUG::2016-02-25 02:02:30,780::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/2::DEBUG::2016-02-25 02:02:30,780::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331
MainProcess|periodic/2::DEBUG::2016-02-25 02:02:30,788::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/1::DEBUG::2016-02-25 02:02:45,762::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-25 02:02:45,762::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-25 02:02:45,762::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331
MainProcess|periodic/1::DEBUG::2016-02-25 02:02:45,770::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:00,766::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:00,766::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:00,767::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:00,774::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:15,763::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:15,763::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:15,763::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:15,771::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:30,768::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:30,768::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:30,769::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331
MainProcess|periodic/3::DEBUG::2016-02-25 02:03:30,776::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:45,765::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:45,765::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:45,765::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331
MainProcess|periodic/1::DEBUG::2016-02-25 02:03:45,773::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVcpuNumaMemoryMapping with {0: [0], 1: [0]}
MainProcess|periodic/0::DEBUG::2016-02-25 02:04:00,768::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVcpuNumaMemoryMapping with ('a_c7_1',) {}
MainProcess|periodic/0::DEBUG::2016-02-25 02:04:00,769::supervdsmServer::115::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('a_c7_1',) {}
MainProcess|periodic/0::DEBUG::2016-02-25 02:04:00,769::supervdsmServer::122::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 22331

oh sorry. apparently I checked only vdsm.log in that regards which prints this same output fine: 

Thread-191::DEBUG::2016-03-01 04:05:14,299::bindingxmlrpc::1253::vds::(wrapper) client [::1]::call getHardwareInfo with () {}

Thread-982::DEBUG::2016-03-01 06:48:09,922::bindingxmlrpc::1253::vds::(wrapper) client [::1]::call setupNetworks with ({}, {}, {'connectivityCheck': 'False'}) {}

fixing it is easy - posting https://gerrit.ovirt.org/54209

Moving bug to modified, as this issue is not related to the RFE itself and the patch for the plug-able API is already merged.

This bug was fixed and is slated to be in the upcoming version. As we
are focusing our testing at this phase on severe bugs, this bug was
closed without going through its verification step. If you think this
bug should be verified by QE, please set its severity to high and move
it back to ON_QA