Setting to make sure we evaluate this bug now. This may be the reason for the
small leak Eldad found in the last scaling test.

(In reply to Milan Zamazal from comment #0)
> Steps to Reproduce:
> 
> 1. Run the following on a Vdsm host:
> 
>   vmid=$(uuidgen)
>   while true; do
>     vdsClient 0 create /dev/null memSize=256 vmId=$vmid
>     sleep 1
>     vdsClient 0 destroy $vmid
>   done


How are create and destroy calls relate to supervdsm?? did you see calls to supervdsmd here? can you attach supervdsm.log?

this does not check the rpc at all but create and destroy vm flow.. so I'm not sure about your conclusions here. I'd try to call something that only calls to supervdsmd and returns without additional logic (like getVdsHardwareInfo)

> 
> 2. Observe memory usage of `vdsmd' process.
> 
> Actual results:
> 
> The memory usage slowly grows.
> 
> Expected results:
> 
> The memory usage is approximately constant.
> 
> Additional info:
> 
> This is actually a Python bug, see http://bugs.python.org/issue26180 .
> 

I will investigate multiprocessing and check for alternatives , but I'm not sure you checked the right thing

> Nevertheless, using multiprocessing for communication between Vdsm and
> Supervdsm should probably be completely avoided and replaced with some
> simple RPC.  For at least the following reasons:
> 
> - It would fix the leak independently of the contingent Python fix and used
> Python version.

I don't understand what you mean here ..

> - Mixing multiprocessing and threading may be generally not a very good idea.

Why? this is the way to work with multiprocessing.managers

> - Multiprocessing is not needed for the communication and it can be replaced
> with a simpler, more robust and more transparent mechanism.

any simpler alternative that you have in mind?
please explain how the alternative will be simpler and "safer" to use, because currently multiprocessing makes the proxy code quite clean and easy to use.

After examination with:

while true; do
    vdsClient -s 0 getVdsHardwareInfo > /dev/null
    sleep 1
done

RSS is growing ..44392->44396->44400 4k each time (not each call) , we need to replace multiprocessing usage for sure.. I still wonder with what

(In reply to Yaniv Bronhaim from comment #2)

> did you see calls to supervdsmd here? can you attach supervdsm.log?

Yes, sample supervdsm.log excerpt -- on create:

supervdsmServer::116::SuperVdsm.ServerCallback::(wrapper) call prepareVmChannel with ('/var/lib/libvirt/qemu/channels/9a220556-e06c-45c3-948b-2e075f3d5db1.com.redhat.rhevm.vdsm',) {}
supervdsmServer::123::SuperVdsm.ServerCallback::(wrapper) return prepareVmChannel with None
supervdsmServer::116::SuperVdsm.ServerCallback::(wrapper) call getVmPid with ('n9a220556-e06c-45c3-948b-2e075f3d5db1',) {}
supervdsmServer::123::SuperVdsm.ServerCallback::(wrapper) return getVmPid with 2826

On destroy:

supervdsmServer::116::SuperVdsm.ServerCallback::(wrapper) call rmAppropriateMultipathRules with ('9a220556-e06c-45c3-948b-2e075f3d5db1',) {}
supervdsmServer::123::SuperVdsm.ServerCallback::(wrapper) return rmAppropriateMultipathRules with []

> > - It would fix the leak independently of the contingent Python fix and used
> > Python version.
> 
> I don't understand what you mean here ..

If the bug gets ever fixed in Python, it still doesn't fix Vdsm until the user
installs the fixed Python version.  If we avoid multiprocessing, we get rid of
the leak independently of the Python fix.

> > - Mixing multiprocessing and threading may be generally not a very good idea.
> 
> Why? this is the way to work with multiprocessing.managers

I'm not an expert on that but people say using both threading and
multiprocessing (forking) may expose one to tricky situations.  After all, the
Python multiprocessing leak is reproducible only when threading is used.

> > - Multiprocessing is not needed for the communication and it can be replaced
> > with a simpler, more robust and more transparent mechanism.
> 
> any simpler alternative that you have in mind?
> please explain how the alternative will be simpler and "safer" to use,
> because currently multiprocessing makes the proxy code quite clean and easy
> to use.

It may be easy to use but may not be that simple inside the underlying
implementation in Python etc. and thus can be error-prone.

As for possible alternatives, I think Nir can elaborate.

There are several approaches:
1. Open a bug on python to address the small memory leak.
2. Change the way we work between supervdsm and vdsm.

Piotr - yout thoughts?

(In reply to Oved Ourfali from comment #5)
> 1. Open a bug on python to address the small memory leak.

That has already been done: http://bugs.python.org/issue26180

I would solve this issue by using existing code that we have to communicate and added to it inter-process communication. Infra code would detect whether the client is talking to local process or remote and decide whether to use existing code we have or inter-process communication.

This type of optimization would be beneficial if we wanted to separate vdsm into multiple processes.

Restoring needinfo on Nir

For the special case of supervdsm, we need different solution than generic
way to send messages between components.

1. Only vdsm should be able to talk with supervdsm

We don't wan't to allow any process running as vdsm (e.g hooks) to be able
to perform operations as root.

2. We don't want to use pickle, as unpickling is not safe and can lead to 
arbitrary code execution:
https://lincolnloop.com/blog/playing-pickle-security/

The most secure way would be to start supervdsm from vdsm, and communicate 
with it using a pipes (e.g. supervdsm stdin and stdout).
This requires running vdsm master process as root (for starting supervdsm),
and child process for vdsm application.

Another solution is to let supervdsm connect to vdsm unix domain socket.
Vdsm can verify that the connection process is running as root. Once
supervdsm is connected, Vdsm can talk securely to supervdsm.
Using this method, we can manage supervdsm using systemd, and we don't need
to run vdsm as root.
See http://welz.org.za/notes/on-peer-cred.html

There are probably more options, lets not design this on bugzilla.

(In reply to Milan Zamazal from comment #6)
> (In reply to Oved Ourfali from comment #5)
> > 1. Open a bug on python to address the small memory leak.
> 
> That has already been done: http://bugs.python.org/issue26180

Did we get anyone from RH to look at this and potentially fix it for downstream Python?
Assuming the leak is solved on Python side, do we have a pressing issue on fixing this for 4.0? Changing a protocol is not something that can be done right away, I assume.

Right. I don't see any progress from python guys and this issue still open for improvement. As part of fixing this, we need to define ipc infra between services on host and use that instead

This request has been proposed for two releases. This is invalid flag usage. The ovirt-future release flag has been cleared. If you wish to change the release flag, you must clear one release flag and then set the other release flag to ?.

Oved - this does not look like a 4.1 fix (although the move to communicate over sockets makes a lot of sense to me). Shall we defer?

Need new numbers for 4.2.

Moving to 4.3 for now, we don't plan to change communication unless there are good reasons for that and we have no indication about the leak from customers

(In reply to Yaniv Kaul from comment #14)
> Need new numbers for 4.2.

Replacing needinfos

Resetting the need info as agreed

Start VM usage:
VmSize:	 1657264 kB

After approximately 3-4 minutes it tops on:
VmSize:	 1661616 kB

and it is not growing anymore ( I kept running it for 15 more minutes ).

while true; do vdsm-client -f vm.json VM create; sleep 1; vdsm-client -f vm.json VM destroy; done

# cat vm.json 
{
   "vmParams": {
     "memSize": 256,
     "vmID": "00000000-0000-0000-0000-000000000000"
   },
   "vmID": "00000000-0000-0000-0000-000000000000"
}

We didn't get to this bug for more than 2 years, and it's not being considered for the upcoming 4.4. It's unlikely that it will ever be addressed so I'm suggesting to close it.
If you feel this needs to be addressed and want to work on it please remove cond nack and target accordingly.

Closing old bug. Please reopen if still relevant/you want to work on it.