Bug 1303648 - Vdsm memory leak in Supervdsm communication [NEEDINFO]
Vdsm memory leak in Supervdsm communication
Status: NEW
Product: vdsm
Classification: oVirt
Component: SuperVDSM (Show other bugs)
4.17.19
Unspecified Unspecified
unspecified Severity high (vote)
: ovirt-4.3.0
: ---
Assigned To: Martin Perina
Jan Zmeskal
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-01 10:07 EST by Milan Zamazal
Modified: 2018-07-10 04:05 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: Infra
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ratamir: needinfo? (dagur)
mperina: ovirt‑4.3?
rule-engine: planning_ack?
rule-engine: devel_ack?
pstehlik: testing_ack+


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
Python 26180 None None None 2016-04-18 03:53 EDT

  None (edit)
Description Milan Zamazal 2016-02-01 10:07:53 EST
Description of problem:

When Vdsm talks to Supervdsm, memory is leaked in vdsmd process.

Version-Release number of selected component (if applicable):

Git master and 4.17.x.

How reproducible:

Always reproducible.

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

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 .

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.
- Mixing multiprocessing and threading may be generally not a very good idea.
- Multiprocessing is not needed for the communication and it can be replaced with a simpler, more robust and more transparent mechanism.
Comment 1 Nir Soffer 2016-02-01 10:20:01 EST
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.
Comment 2 Yaniv Bronhaim 2016-02-01 16:18:45 EST
(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.
Comment 3 Yaniv Bronhaim 2016-02-02 05:01:01 EST
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
Comment 4 Milan Zamazal 2016-02-02 05:53:42 EST
(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.
Comment 5 Oved Ourfali 2016-02-04 03:53:58 EST
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?
Comment 6 Milan Zamazal 2016-02-04 04:02:22 EST
(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
Comment 7 Piotr Kliczewski 2016-02-04 04:04:14 EST
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.
Comment 8 Piotr Kliczewski 2016-02-04 04:05:14 EST
Restoring needinfo on Nir
Comment 9 Nir Soffer 2016-02-20 15:39:51 EST
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.
Comment 10 Yaniv Kaul 2016-03-13 11:58:48 EDT
(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.
Comment 11 Yaniv Bronhaim 2016-03-28 10:29:57 EDT
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
Comment 12 Red Hat Bugzilla Rules Engine 2016-04-18 06:58:22 EDT
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 ?.
Comment 13 Yaniv Kaul 2016-11-30 03:44:53 EST
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?
Comment 14 Yaniv Kaul 2017-09-04 10:07:54 EDT
Need new numbers for 4.2.
Comment 15 Martin Perina 2017-11-21 10:48:35 EST
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
Comment 17 Raz Tamir 2018-07-10 04:05:43 EDT
(In reply to Yaniv Kaul from comment #14)
> Need new numbers for 4.2.

Replacing needinfos

Note You need to log in before you can comment on or make changes to this bug.